Skip to content

WIP: Namespaced AppContext#16

Draft
JHopeCollins wants to merge 17 commits intomainfrom
JHopeCollins/appctx
Draft

WIP: Namespaced AppContext#16
JHopeCollins wants to merge 17 commits intomainfrom
JHopeCollins/appctx

Conversation

@JHopeCollins
Copy link
Copy Markdown
Member

@JHopeCollins JHopeCollins commented Aug 19, 2025

Corresponding Firedrake PR: firedrakeproject/firedrake#4526

@JHopeCollins JHopeCollins self-assigned this Aug 19, 2025
@JHopeCollins JHopeCollins added the enhancement New feature or request label Aug 19, 2025
Comment thread petsctools/__init__.py Outdated
Comment thread petsctools/appctx.py
Comment thread petsctools/appctx.py Outdated
Comment thread petsctools/appctx.py Outdated
Comment thread tests/test_options.py
Copy link
Copy Markdown
Collaborator

@connorjward connorjward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should expose the autogenerated key to the user at all. Why not just deal in the string options?

Comment thread petsctools/appctx.py Outdated
Comment thread tests/test_appctx.py Outdated
Comment thread petsctools/appctx.py
Comment thread petsctools/appctx.py Outdated
@JHopeCollins
Copy link
Copy Markdown
Member Author

I wonder if we should expose the autogenerated key to the user at all. Why not just deal in the string options?

I'd be fine with that. I can take out that part of the docstring so the only way we tell users to use the AppContext is by calling appctx['prefix_obj_option'].

Comment thread petsctools/appctx.py

with opts.inserted_options():
default = MyCustomData(10)
data = appctx.get('solver_custompc_somedata', default)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

split this docstring.

Comment thread petsctools/appctx.py Outdated
Comment thread petsctools/appctx.py Outdated
Comment thread petsctools/appctx.py
Comment thread tests/test_appctx.py Outdated
Comment thread tests/test_appctx.py
xcheck = x.duplicate()
xcheck.pointwiseMult(b, diag)

with petsctools.inserted_options(ksp), petsctools.push_appctx(appctx):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we even have to do any appctx manipulation here? It could safely live in global scope.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This is the same point as #16 (comment))

Comment thread petsctools/appctx.py


def get_appctx():
return _global_appctx_stack[-1]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing this could we maybe do a WeakKeyDictionary mapping between, say, ksp and AppContext? Or alternatively just do setattr(ksp, "appctx", AppContext)?

I don't recall the motivation behind having a stack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants