-
Notifications
You must be signed in to change notification settings - Fork 169
Use petsctools.AppContext
#4526
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?
Conversation
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.
Quick skim. I hope this is useful and not just annoying!
A, P = pc.getOperators() | ||
|
||
if pc.type != "python": | ||
raise ValueError("Expecting PC type python") | ||
opc = pc | ||
appctx = self.get_appctx(pc) | ||
fcp = appctx.get("form_compiler_parameters") | ||
fcp = get_appctx(pc).fcp |
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.
Did you set fcp
? It's not a clear name.
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.
No, it is already attached to the _SNESContext
. I'm just switching to getting the compiler parameters from there instead of the appctx because it is a "global" rather than a namespaced (prefixed) thing.
ctx = get_appctx(pc) | ||
fcp = ctx.fcp | ||
|
||
appctx = ctx.appctx |
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 weird. This is saying appctx = get_appctx(pc).appctx
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 agree, this is really annoying. There is the _SNESContext
object and the dict
object, both of which are called appctx
in various places.
_SNESContext.appctx
is thedict
we call theappctx
in user-code.dmhooks.get_appctx
returns the_SNESContext
.PCSNESBase.get_appctx
returnsdmhooks.get_appctx(pc_or_snes).appctx
which is thedict
.
I would be in favour of cleaning up this naming, but it'd touch a lot of code and I don't want to do it unilaterally.
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.
Sure. I'd be happy for now to just open an issue about it, plus perhaps an explanatory comment.
@@ -194,14 +193,13 @@ def __init__(self, problem, mat_type, pmat_type, appctx=None, | |||
self._x = problem.u_restrict | |||
|
|||
if appctx is None: | |||
appctx = {} | |||
from petsctools import AppContext |
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.
Do import at the top level
Switch from an un-namespaced python
dict
to the namespacedpetsctools.AppContext
in this PR: firedrakeproject/petsctools#16