TransferManager: Re-injection of coefficients; no-op for unmodified dat
#3159
Conversation
pbrubeck
changed the title
dat
| if isinstance(V, firedrake.functionspaceimpl.FiredrakeDualSpace): | ||
| V = firedrake.functionspace.DualSpace(mesh, V.ufl_element()) | ||
| else: | ||
| V = firedrake.FunctionSpace(mesh, V.ufl_element()) |
There was a problem hiding this comment.
We should add an interface to reconstruct on a new mesh.
pbrubeck
force-pushed
the
pbrubeck/feature/cache-transfer
branch
from
0dc8083 to
02ba9d0
Compare
Co-authored-by: Connor Ward <[email protected]>
connorjward
left a comment
connorjward
left a comment
There was a problem hiding this comment.
This basically looks fine to me.
firedrake/variational_solver.py
Outdated
| if problem.Jp is not None: | ||
| coefficients = coefficients + problem.Jp.coefficients() | ||
| for val in chain(coefficients, (bc.function_arg for bc in problem.bcs)): | ||
| if isinstance(val, (function.Function, cofunction.Cofunction)): |
There was a problem hiding this comment.
There's a UFL superclass for this that we can use instead right?
There was a problem hiding this comment.
Yes, ufl.coefficient.BaseCoefficient, but we opted to be more descriptive.
firedrake/variational_solver.py
Outdated
| # work right) | ||
| for ctx in chain((self.inserted_options(), dmhooks.add_hooks(dm, self, appctx=self._ctx)), | ||
| for ctx in chain((self.inserted_options(), | ||
| *(dmhooks.add_hooks(dm, self, appctx=self._ctx) for dm in reversed(problem_dms))), |
There was a problem hiding this comment.
@wence- Apparently the order of the contexts matters, we need to visit the solution DM after all other coefficients. Does this look good?
There was a problem hiding this comment.
🤷 Probably? Hooks are torn down in LIFO order, but I don't know why that is important in this scenario. What happens if you don't do it this way round?
There was a problem hiding this comment.
Minor, no need to splat the generator since you're in a chain, so:
chain([self.inserted_options()], (dmhooks.add_hooks(...) for dm in ...), self._transfer_operators)
I suppose.
There was a problem hiding this comment.
I can't put in a generator, either I pass options and hooks together in the same tuple as I had before, or I use your suggestion with tuple in front of the generator.
There was a problem hiding this comment.
🤷 Probably? Hooks are torn down in LIFO order, but I don't know why that is important in this scenario. What happens if you don't do it this way round?
If I don't use reverse then the appctx does not get attached properly and I get the warning "Creating new TransferManager".
There was a problem hiding this comment.
Huh, odd, anyway, I think that is fine.
name: "TransferManager: Re-injection of coefficients; no-op for unmodified
dat"description: ''
title: ''
labels: ''
assignees: ''
Description
I'm adding a cache on the arguments of
tm.prolong, tm.restrict, tm.injectthat holds thedat_versionsfrom the last time the operator was applied on those arguments. The transfer operator will behave as a no-op if the current versions coincide with the ones in the cache.I also add a coarsen hook at restriction with the injection of all coefficients of the problem at hand. Here we avoid the unnecessary re-injection of coefficients that remain constant.
Associated Pull Requests:
Need to merge this PR first
Fixes Issues:
If issues are fixed by this PR, prepend each of them with the word
"fixes", so they are automatically closed when this PR is merged. For
example "fixes #123, fixes #456".
Checklist for author:
make lintin thefiredrakesource directory).make linkcheck; make html; make latexpdfinfiredrake/docsdirectory)pytest testsin thefiredrakesource directory) (useful, but not essential if you don't have suitable hardware).Checklist for reviewer:
Feel free to add reviewers if you know there is someone who is already aware of this work.
Please open this PR initially as a draft and mark as ready for review once CI tests are passing.