Remove Interpolator from public API and other interpolation refactoring
#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 |
leo-collins
force-pushed
the
leo/refactor_interpolate
branch
from
deb0a8c to
4097207
Compare
pbrubeck
changed the base branch from
main
to
pbrubeck/interp-adjoint-explicit
This is fine, but we might want to preserve a user-facing interface for reusable interpolators. |
pbrubeck
force-pushed
the
pbrubeck/interp-adjoint-explicit
branch
7 times, most recently
from
210bae0 to
8d631f8
Compare
connorjward
left a comment
connorjward
left a comment
There was a problem hiding this comment.
I haven't reviewed this in great detail but this seems really good!
firedrake/interpolation.py
Outdated
| 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.
This is a great idea. Big fan.
There was a problem hiding this comment.
I'm not a fan. Why can we not have the BCs passed on to the Interpolator?
There was a problem hiding this comment.
I guess that's just a matter of taste. We don't generate code for the boundary conditions so stashing things on the Interpolator is less important.
I was specifically praising the use of type hints to make an extremely complex data structure clear.
There was a problem hiding this comment.
We can't change the BCs on matrices on repeated assembly calls, or can we?
There was a problem hiding this comment.
Also the tuple[int, int] could in some cases just be tuple[int]
There was a problem hiding this comment.
We can't change the BCs on matrices on repeated assembly calls, or can we?
Good point. I'm not sure. We switch the lgmaps out at runtime so feasibly it's OK to do.
There was a problem hiding this comment.
My thought was that BCs should be passed to assemble, like we do for all other forms
dham
left a comment
dham
left a comment
There was a problem hiding this comment.
subject to checking that adjoint interpolation on mixed spaces is really correct, this is great. thanks.
| sub_tensor.assign(sum(self[i]._interpolate(*function, adjoint=adjoint, **kwargs) | ||
| for i in self if i[0] == k)) | ||
| return output | ||
| Isub[indices] = (get_interpolator(form), sub_bcs) |
There was a problem hiding this comment.
Can we not do this instead?
| Isub[indices] = (get_interpolator(form), sub_bcs) | |
| Isub[indices] = get_interpolator(form, bcs=sub_bcs) |
There was a problem hiding this comment.
Or if that is not your intended design, I suggest we enable something like this
| Isub[indices] = (get_interpolator(form), sub_bcs) | |
| Isub[indices] = get_assembler(form, bcs=sub_bcs) |
There was a problem hiding this comment.
A third option is to do something like
| Isub[indices] = (get_interpolator(form), sub_bcs) | |
| Isub[indices] = partial(get_interpolator(form).assemble, bcs=sub_bcs) |
fixes
removed GlobalWrapper class typo
leo-collins
force-pushed
the
leo/refactor_interpolate
branch
from
b7368d7 to
b5f8c0e
Compare
leo-collins
changed the title
Interpolator from public API and other interpolation refactoring
Simplifies interpolation code and introduces new features. So far:
__new__method and dispatch instead with aget_interpolatorfunction. The public API is to callassembleon a symbolicinterpolateas detailed in the manual.interpolateare no longer silently renumbered. See Stop renumbering arguments in the expression tointerpolate#4582 for details.frozen_interpolator.New features:
InterpolateOptionsdataclass. We get type hinting, better IDE support, single source of truth for these options.