Extend derivative block to derive residuals from functionals#4118
Extend derivative block to derive residuals from functionals#4118Joseabcd wants to merge 21 commits intoFEniCS:mainfrom
Conversation
|
Looks like a good start.
|
|
thanks @schnellerhase and @jorgensd I found no While adding tests, I realized there were more situations where the user can supply bad inputs and get obscure errors (or unintended results silently), so I have been more comprehensive when checking the inputs. For example, before you could wrongly pass trial functions to derive functionals, and it would still return a form (or For consistency, I’ve also replaced the Could you let me know how it looks, if the new checks do indeed belong here, or any other feedback or thoughts? |
python/test/unit/fem/test_forms.py
Outdated
| R = derivative_block(F, f0) | ||
| assert isinstance(R, ufl_form) and len(R.arguments()) == 1 | ||
|
|
||
| R = derivative_block(F, f0, v0) |
There was a problem hiding this comment.
In this case, where u and du are not sequences (what I called "univariate" functional), there is a subtle choice I made worth mentioning.
In the odd event that the user passes v0 from a mixed space, applying ufl.extract_blocks to the output of ufl.derivative will be a sequence containing one ufl.Form. In other words, the result can be a ufl.Form or a Sequence depending on whether v0 is part or not of a mixed space (!)
I found this potentially confusing, so I avoided applying ufl.extract_blocks in derivative_block when the input du is just a function
There was a problem hiding this comment.
This should be added to the documentation.
There was a problem hiding this comment.
@schnellerhase I'll update the docstring, but could you tell me if I made the right choice? I'm still exploring the rest of the project and I lack the big picture of how this fits in the rest of the python layer.
I think that in essence, my dilemma is captured in the following question:
Given:
mesh = dolfinx.mesh.create_unit_interval(MPI.COMM_WORLD, 10)
V0 = functionspace(mesh, ("Lagrange", 1))
V1 = functionspace(mesh, ("Lagrange", 2))
V = MixedFunctionSpace(V0, V1)
f0, f1 = dolfinx.fem.Function(V0), dolfinx.fem.Function(V1)
M = f0**2 * dx
which of the following is the more accurate statement?
Mis a functional defined over a single function. We don't know about "blocks".Mis a functional defined overNfunctions (whereNhappens to be 1). We work with "blocks".
There was a problem hiding this comment.
Going along the argument in #4118 (comment) - I would say the first option is the favourable perspective. Going with the perspective: the block interpretation is at the symbolic level one of argument counting, not one of the setup of the argument's function spaces. What do you think @jorgensd - does that make any sense?
There was a problem hiding this comment.
This should be added to the documentation.
I implemented in the docstring a more structured format that I observed in other functions from the project. I hope the detail about the type of returned output depending on the input is clearer now
There was a problem hiding this comment.
I also think the first one is the correct thing to consider here. As f1 does not enter the functional, the construct of blocking them is of no interest to the functional itself.
| raise ValueError("When F is a rank-one UFL form, u must be a ufl.Function") | ||
| if du is None: | ||
| du = ufl.TrialFunction(u.function_space) | ||
| elif not isinstance(du, ufl.Argument) or du.number() != 1: |
There was a problem hiding this comment.
Shouldn't this be raised within ufl.derivative already?
There was a problem hiding this comment.
It is not raised, ref:
from ufl import (
FunctionSpace,
Mesh,
Coefficient,
dx,
TrialFunction,
derivative
)
import basix.ufl
from mpi4py import MPI
import dolfinx
cell = "interval"
P1 = basix.ufl.element("Lagrange", cell, 1, shape=())
domain = Mesh(basix.ufl.element("Lagrange", cell, 1, shape=(1,)))
V = FunctionSpace(domain, P1)
u = Coefficient(V)
J = u ** 2 * dx
v = TrialFunction(V)
F = derivative(J, u, v)
F_compiled = dolfinx.fem.compile_form(MPI.COMM_WORLD, F)which runs with no problem, but becomes a problem if you want to get the jacobian
There was a problem hiding this comment.
Should we then keep/remove this check? In my view, if the concept of deriving a rank-zero form w.r.t. a “trial” function never ever makes sense (that is, if it’s conceptually wrong at the UFL layer), then it’d be UFL’s responsibility to complain
There was a problem hiding this comment.
ufl strictly speaking actually handles this correctly.
If we consider this minimal example using the Lagrange-element from the UFL test suite:
from ufl import (
FunctionSpace,
Mesh,
Coefficient,
dx,
TrialFunction,
derivative,
interval,
TestFunction
)
from utils import LagrangeElement
cell = interval
P1 = LagrangeElement(cell, 1, shape=())
domain = Mesh(LagrangeElement(cell, 1, shape=(1,)))
V = FunctionSpace(domain, P1)
u = Coefficient(V)
J = u ** 2 * dx
v = TestFunction(V)
F = derivative(J, u, v)
N = derivative(F, u)
for arg in F.arguments():
print("Test F", arg.number(), arg.part())
for arg in N.arguments():
print("Test N", arg.number(), arg.part())
v = TrialFunction(V)
F_t = derivative(J, u, v)
N_t = derivative(F_t, u)
for arg in F_t.arguments():
print("Trial F", arg.number(), arg.part())
for arg in N_t.arguments():
print("Trial N", arg.number(), arg.part())this returns:
Test F 0 None
Test N 0 None
Test N 1 None
Trial F 1 None
Trial N 1 None
Trial N 2 Nonewhich means that if you do not supply the derivative direction of your second variation, you get an argument that has its number incremented by one, which is then correct in UFL terms, as the first variation is the one with the lowest numbering.
python/dolfinx/fem/forms.py
Outdated
| raise ValueError("u contains some non ufl.Function elements") | ||
| if du is None: | ||
| du = ufl.TestFunctions(ufl.MixedFunctionSpace(*(u_i.function_space for u_i in u))) | ||
| elif (not isinstance(du, Sequence) |
There was a problem hiding this comment.
Shouldn't this be raised within ufl.derivative or ufl.extract_blocks already?
There was a problem hiding this comment.
The UFL functions aren’t raising here either. For example, ufl.derivative(F, [u0, u1, u2], [du0]) silently returns a sequence with just one form. But I agree this is UFL’s job, so I have for now removed the check len(u) == len(du).
I will wait to remove the other checks that remain in this if, depending on the answer to question in the previous comment.
| u: Sequence[ufl.Form], | ||
| du: Sequence[ufl.Argument] | None = None, | ||
| ) -> Sequence[Sequence[ufl.Form]]: | ||
| if not all(isinstance(F_i, ufl.Form) and len(F_i.arguments()) == 1 for F_i in F): |
There was a problem hiding this comment.
Before removing it, we need to answer the question in the comment above: should we allow derivative_block to derive a functional using a “trial” function?
| raise ValueError("u contains some non ufl.Function elements") | ||
| if du is None: | ||
| du = [ufl.TrialFunction(u_i.function_space) for u_i in u] | ||
| elif (not isinstance(du, Sequence) |
There was a problem hiding this comment.
The tests isinstance(du, Sequence)and len(u) == len(du) I believe are appropriate. Given that the concept of “component-wise” Jacobian is created here in the Python layer, isn't this layer responsible for making sure the result is a square “matrix”? UFL will never get to see anything other than single pairs of (u_i, du_i), so will never get a chance to complain.
About the third check on du_i.number(), I will wait to remove it depending on answer to question in this comment above
| if du is not None: | ||
| assert len(F) == len(du), "Number of forms and du must be equal" | ||
|
|
||
| if isinstance(F, ufl.Form) and not F.arguments(): |
There was a problem hiding this comment.
What does not F.arguments() do/why does it work?
There was a problem hiding this comment.
F.arguments() returns a tuple of ufl.Argument. if a tuple is empty, its boolean state is False, thus the check says, if we have no arguments (rank=0) go there.
|
This looks pretty good but I think it's a bit 'strongly typed/overly checked' with if statements - shouldn't a lot of these already be picked up lower down in UFL? |
…, and rename to 'F' variables holding residuals.
… and derive_multivariate_jacobian to derive_block_jacobian.
My bad - the
I agree this is a problematic interface. The underlying reason for this is that the only difference between a trial and test function in UFL is an index denoting the placement https://github.com/FEniCS/ufl/blob/61ea6299409a61d9e90e94d0b34f14ed269fa745/ufl/argument.py#L297-L312, however no input checks are performed on 2 only showing up if 1 is provided. This causes also other surprising behaviour #3721. |
…ence of them. Remove the corresponding tests from test_derivative_block..
… Remove the corresponding test from test_derivative_block.
…s than the test intended.
I've removed now some of the checks as suggested above. I'm happy to remove the rest of those suggested, but I left a question in an inline comment that I think needs to be resolved first I marked as "resolved" the comments that I believe require no further action, but please let me know if anything doesn't look good |
…s in test_derivative_block.
… values in each case that it supports.
14b082d to
30ef1a9
Compare
Make
fem.forms.derivative_blockwork on functionals to derive residuals, as described in #3766.The following notes could be useful for the review:
ValueErrorexceptions as opposed toasserts, assuming that the checks are on user input (as opposed to checks against bugs). The original code however has someasserts, so I’m not sure which is their right intentFis a list of functionalsufl.TrialFunctions(ufl.MixedFunctionSpace(…))?duis the correct type of argument (test vs trial), but possibly this might be helpful