Handle MeshGeometry in cached solvers (-> #4638)#5188
Conversation
angus-g
force-pushed
the
angus-g/4638-meshgeometry
branch
from
dc82f03 to
3be5af2
Compare
|
I’m currently on leave. I can look into this next Thursday.
…On Thu, 18 Jun 2026 at 06:35 Angus Gibson ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In firedrake/adjoint_utils/blocks/solving.py
<#5188 (comment)>
:
> + def __init__(
+ self, forward_cache, tangent_cache, adjoint_cache, hessian_cache, ad_block_tag=None
+ ):
Oops I was going to type-annotate these but that introduces a circular
dependency, I should probably go back to an if TYPE_CHECKING import.
—
Reply to this email directly, view it on GitHub
<#5188?email_source=notifications&email_token=AFAFSPXODRBPRWTNPMCCZ6T5AN5RTA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJSGE4DQNRRGIY2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#pullrequestreview-4521886121>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFAFSPTFE6ZQHI5NIAJTRNL5AN5RTAVCNFSNUABEKJSXA33TNF2G64TZHMYTGMZYGQYDKMR3JFZXG5LFHM2DMOBYHAZDIOBQGKQXMAQ>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/AFAFSPW5LSFTM2KPZTUIXWT5AN5RTA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJSGE4DQNRRGIY2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/AFAFSPUZ4GW7BQXUUB3N4LD5AN5RTA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJSGE4DQNRRGIY2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
angus-g
force-pushed
the
JHopeCollins/nlvs-hessian-fix
branch
from
442f29e to
62f3a75
Compare
This test relies on moving geometry, so it does exercise the changes in this branch. test_shape_derivatives probably does too, but implicitly through interpolate so it might not fail until we route all solves through the cached solver block.
For whatever reason, derivative(-F, m, dm) has very different behaviour when passed through the adjoint -> action -> expand_derivatives chain compared to the non-negated variant. We just push subtraction to the dFdm_adj and dFdm_adj2 forms instead.
angus-g
force-pushed
the
angus-g/4638-meshgeometry
branch
from
3be5af2 to
923d2f4
Compare
| self._mesh_dependencies()): | ||
| if dep.tlm_value is None: | ||
| continue | ||
| replaced_dep.assign(dep.tlm_value) |
There was a problem hiding this comment.
Do we ever hit this line for a mesh dependency in the test suite?
I see that the replaced_dep is a Function matching the mesh.coordinates. Is the tlm_value for a mesh also a coordinate Function?
| for m in self._ad_forward_cache.meshes: | ||
| X = SpatialCoordinate(m) | ||
| # we can't take the CoordinateDerivative of an Action, so we have | ||
| # to invert this form compared to the expression above | ||
| dFdm = derivative(action(-F, adj_sol), X, TestFunction(m._ad_function_space())) | ||
| dFdm_adj_forms.append(dFdm) |
There was a problem hiding this comment.
Could you explain this comment a bit? It looks like we are in fact taking the derivative of an action wrt the coordinates here. Does it not resolve to a CoordinateDerivative?
| for m, dm in zip(self._ad_forward_cache.meshes, | ||
| self._ad_tangent_cache.mesh_tlms): | ||
| X = SpatialCoordinate(m) | ||
| d2Fdmdu = expand_derivatives( | ||
| derivative(dFdu_adj, X, dm) | ||
| ) | ||
|
|
||
| d2Fdmdu_forms.append(d2Fdmdu) |
There was a problem hiding this comment.
This is quite nit-picky, feel free to ignore if you don't agree.
The only difference in the body of this loop from the one above is the m_ = SpatialCoordinate(m) if (m is mesh) else m logic for what to take the derivative with respect to.
Would it be clearer to combine them into a single loop and have an if-block to set m and make it clear that the form calculation is identical? Or does it just make it too messy?
| solve(lhs(F) == rhs(F), u1) | ||
| lvp1 = LinearVariationalProblem(lhs(F), rhs(F), u1) | ||
| lvs1 = LinearVariationalSolver(lvp1) | ||
| lvs1.solve() |
There was a problem hiding this comment.
Could we parameterise these tests to run with either solve free function or a Solver?
JHopeCollins
left a comment
JHopeCollins
left a comment
There was a problem hiding this comment.
In general this looks fine, thanks for working through all the cases. I have a few very very minor comments/questions.
I see that there are failures here related to the solve free function, and failures in the solve PR you think will be fixed by this PR. Which order would you like to merge in?
Merge this mesh PR into the main PR first, then update the solve PR and hopefully the test failures will be fixed, or the other way around?
3 participants
This threads
MeshGeometrydependencies through theCachedSolverBlock. BecauseCoordinateDerivativeis a bit strange, we do need to special case forms and flip signs here and there. Particularly for the Hessian forms it goes back to the action -> derivative approach (restrained byCoordinateDerivative) rather than derivative -> action -> adjoint.I did also test Fireshape here (it passes, whereas before this PR it would fail on
test_L2tracking[FeMultiGridControlSpace]andtest_TimeTracking). @APaganini do you have anything that we should ensure we check related to this?