-
Notifications
You must be signed in to change notification settings - Fork 167
Clement interpolant #4244
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?
Clement interpolant #4244
Changes from all commits
560bad2
88d2748
b4dfbd9
651b01f
34ceb67
7ec5197
6e67568
63b5cb7
9323bcf
db99911
cd20a5e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2415,6 +2415,37 @@ def clear_cell_sizes(self): | |
except AttributeError: | ||
pass | ||
|
||
@utils.cached_property | ||
def cell_volume(self): | ||
""" | ||
A :class:`~.Function` in the :math:`P^0` space containing the local mesh volume. | ||
|
||
This is computed by interpolating the UFL :class:`~.CellVolume` for the current | ||
mesh. | ||
""" | ||
from firedrake.function import Function | ||
from firedrake.functionspace import FunctionSpace | ||
DG0 = FunctionSpace(self, "Discontinuous Lagrange", 0) | ||
volume = Function(DG0) | ||
return volume.interpolate(ufl.CellVolume(self)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this actually mathematically correct for higher order meshes? Shouldn't it be a projection? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As long as |
||
|
||
@utils.cached_property | ||
def patch_volume(self): | ||
""" | ||
A :class:`~.Function` in the :math:`P^1` space containing the sum of the volumes | ||
of cells neighbouring a vertex. | ||
""" | ||
from firedrake.function import Function | ||
from firedrake.functionspace import FunctionSpace | ||
from firedrake.parloops import par_loop, READ, RW | ||
CG1 = FunctionSpace(self, "Lagrange", 1) | ||
patch_vol = Function(CG1) | ||
domain = "{[i]: 0 <= i < patch.dofs}" | ||
instructions = "patch[i] = patch[i] + vol[0]" | ||
keys = {"vol": (self.cell_volume, READ), "patch": (patch_vol, RW)} | ||
par_loop((domain, instructions), ufl.dx(domain=self), keys) | ||
return patch_vol | ||
|
||
@property | ||
def tolerance(self): | ||
"""The relative tolerance (i.e. as defined on the reference cell) for | ||
|
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 sort of thing makes me wonder whether this is the best place to implement this functionality. Might it be better to simply have a free function called
interpolate_clement
that does the parloop?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.
As in instead of having the
Interpolator
at all?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.
Yes. I think we need @dham to weigh in here. Is this the right abstraction?
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 doesn't look great.
Interpolator
objects should be dying out of the public API of Firedrake. They are really just the implementation backend tointerpolate
. Having users directly instantiateInterpolator
objects will, for example, break differentiability.I think the actually correct way to do this is to inherit from
AbstractExternalOperator
so that you get all the right symbolic properties for free. The syntax would then beassemble(interpolate_clement(source, target))
. If you haven't yet implemented the operator (i.e. the matrix) or the adjoint then these will raiseNotImplementedError
and at least the right interface will be there for the future.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.
It might be nice to have
assemble(interpolate(source, target, variant="clement"))
but that might be tricky/unwieldy.Uh oh!
There was an error while loading. Please reload this page.
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 think we'd have to think harder about whether that is actually the Right Thing. For a finite element space with a known dual basis, there is a single canonical interpolation operation. Other interpolation operations are meaningfully different: they are just other projections that don't directly arise from the definition of the space.
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 variant option at least requires heavier surgery because
interpolate
is defined in UFL.