-
Notifications
You must be signed in to change notification settings - Fork 176
refactor interpolation #4595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
refactor interpolation #4595
Conversation
|
@leo-collins this looks like it includes changes from the other ongoing interpolation PRs. This will be much easier to review if you point this PR at that branch instead of |
deb0a8c to
4097207
Compare
This is fine, but we might want to preserve a user-facing interface for reusable interpolators. |
210bae0 to
8d631f8
Compare
| else: | ||
| return self._interpolate(*cofunctions, output=tensor, adjoint=needs_adjoint, | ||
| default_missing_val=default_missing_val) | ||
| assert isinstance(tensor, Function | Cofunction | None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the 0-form interpolate we should also allow Constant here, it also supports assign.
a964f80 to
514f24d
Compare
|
The commit history includes commits from other PRs. Would it be much of a problem to drop them? |
|
|
||
| if isinstance(target_topology, VertexOnlyMeshTopology): | ||
| if isinstance(source_topology, VertexOnlyMeshTopology): | ||
| return VomOntoVomInterpolator(expr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does VomOntoVomInterpolator support MixedFunctionSpace, or are we never expecting to build one for this case? There should be a ValueError in the constructor if we go for the latter.
firedrake/interpolation.py
Outdated
| Parameters | ||
| ---------- | ||
| expr : ufl.Interpolate | ufl.ZeroBaseForm | ||
| The symbolic interpolation expression, or a zero form. Zero forms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"zero form" is too close to 0-form, which is not necesarily equal to 0.
| The symbolic interpolation expression, or a zero form. Zero forms | |
| The symbolic interpolation expression, or a ZeroBaseForm. ZeroBaseForms |
| if access == op2.INC: | ||
| copyin += (tensor.zero,) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the 1-form adjoint on a mixed space, we might be adding different sub-interpolates onto the same tensor. The zeroing should be handled outside the for loop of line 633
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tensor is already the indexed sub-tensor in this case. Can you write a test for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do that
89550eb to
9fd2344
Compare
connorjward
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't reviewed this in great detail but this seems really good!
firedrake/interpolation.py
Outdated
| ) | ||
|
|
||
|
|
||
| class Interpolate(ufl.Interpolate): | ||
| @dataclass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @dataclass | |
| @dataclass(kw_only=True) |
I think we should disallow positional arguments for this.
| """ | ||
| from firedrake.assemble import assemble | ||
| # If assembling the operator, we need the concrete permutation matrix | ||
| matfree = False if self.rank == 2 else self.matfree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems bizarre. A user could pass a matfree option that gets ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't assemble the cross-mesh interpolation matrix if matfree=True. By doing this the user can do assemble(interpolate(TrialFunction(V), U)) and get the operator without having to pass an additional matfree=False.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which case I think we should change (and document) the interface to
def interpolate(expr, ..., *, matfree=None):
if matfree is None:
if some_condition:
matfree = False
else:
matfree = Truebecause currently the user could run
assemble(interpolate(TrialFunction(V), U, matfree=True))and the option would be silently ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same could be said about the allow_missing_dofs and default_missing_val parameters if the user is doing same-mesh interpolation. I think it's fine if the docstring says something like "this parameter only applies if interpolating between a VertexOnlyMesh and its input_ordering".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well the right answer there is to disallow those options for same-mesh interpolation. I don't want to force you to make big changes here though so I'd be happy provided that this behaviour is made clear in the docstring and you open an issue about it.
| for indices, form in firedrake.formmanipulation.split_form(expr): | ||
| if isinstance(form, ufl.ZeroBaseForm): | ||
| # Get sub-interpolators and sub-bcs for each block | ||
| Isub: dict[tuple[int, int], tuple[Interpolator, list[DirichletBC]]] = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great idea. Big fan.
connorjward
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
dham
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subject to checking that adjoint interpolation on mixed spaces is really correct, this is great. thanks.
Simplifies interpolation code and introduces new features. So far:
InterpolateOptionsdataclass. We get type hinting, better IDE support, single source of truth for these options.__new__method and dispatch instead with aget_interpolatorfunction.