Conversation
connorjward
left a comment
connorjward
left a comment
There was a problem hiding this comment.
Please add some tests
connorjward
left a comment
connorjward
left a comment
There was a problem hiding this comment.
Please add some tests
| if isconstant(c): | ||
| mesh = as_domain(self.form) | ||
| space = c._ad_function_space(mesh) | ||
| space = c.function_space() |
There was a problem hiding this comment.
How come this needs to change? c.function_space() returns a Firedrake type but c._ad_function_space returns the ufl_function_space.
There was a problem hiding this comment.
I wonder if this is a holder from previous Firedrake/FEniCS compatibility. But I have no idea really.
There was a problem hiding this comment.
If it is then we should address that in a separate PR
There was a problem hiding this comment.
In which case someone should open an issue. This is a non-core dev submission and saying "leave that for another PR" makes it very likely to never be done.
There was a problem hiding this comment.
There's two questions here:
- Does it need changing for the fix in this PR to work? @KarsKnook does it work if you don't change how you get the function space? If not then you can revert back to the original.
- Is this a holdover that we could update? I don't know either, but I'm just saying that unless its essential for this PR then this isn't the place to start changing it, lets keep this one as simple as possible.
3 participants
To make the adjoint compatible with
Submesh, both the mesh and submesh are added as dependencies to an adjoint block. This is accomplished by replacingufl_domainandas_domainwithextract_domainsin the appropriate files.Perhaps this implementation is too naive and not all meshes need to be added as dependencies?
Does this account for all adjoint + submesh cases?
To do:
Disclaimer:
ChatGPT came up with the initial implementation which was restructured by me