Repository navigation
Conversation
OptionsManager built to_delete before it merged the default options into parameters. inserted_options() therefore inserted the inherited options under the prefix of the manager and never deleted them. Build to_delete from the merged parameters instead. The loop that follows still keeps every option that the database already held. Fixes #58. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
JHopeCollins
left a comment
JHopeCollins
left a comment
There was a problem hiding this comment.
Thanks for spotting this. The default options are useful but not widely used so I'm not surprised this slipped through!
Please can we use a different word than "inherited" for the default options though? "inherited" has too much implied meaning in a language with object inheritance that doesn't apply here.
| options["parent_ksp_type"] = "cg" | ||
| options["parent_0_pc_type"] = "jacobi" | ||
| opts_before = dict(options.getAll()) |
There was a problem hiding this comment.
Please can you test:
- having multiple prefixes in the set (i.e. at least
parent_0andparent_1), and callinginserted_optionson both in turn (not nested). - The case where a default option gets overridden? e.g.
-parent_ksp_type cg -parent_1_ksp_type gmres
| # Keep existing options, except inherited defaults, which | ||
| # are temporary copies under this prefix. | ||
| if key not in inherited_options: | ||
| to_delete.discard(key) |
There was a problem hiding this comment.
This comment is not clear, please can you rewrite it? I would suggest two sentences, one explaining what happens for non-default and one for default.
| # Start building parameters from the defaults so | ||
| # that they will overwritten by any other source. | ||
| parameters = default_options | parameters | ||
| inherited_options = set(default_options) |
There was a problem hiding this comment.
If default_options isn't used as a dictionary again then I think its fine just to rebind it to the set.
| inherited_options = set(default_options) | |
| default_options = set(default_options) |
| to_delete.discard(key) | ||
| # Keep existing options, except inherited defaults, which | ||
| # are temporary copies under this prefix. | ||
| if key not in inherited_options: |
There was a problem hiding this comment.
| if key not in inherited_options: | |
| if key not in default_options: |
pbrubeck
changed the title
| # Start building parameters from the defaults so | ||
| # that they will overwritten by any other source. | ||
| parameters = default_options | parameters | ||
| default_options = set(default_options) |
There was a problem hiding this comment.
I don't think this line is needed
| # Existing keys outside the shared defaults are kept after | ||
| # this context. Shared-default keys stay in to_delete and are | ||
| # removed on exit. | ||
| if key not in default_options: | ||
| to_delete.discard(key) |
There was a problem hiding this comment.
This doesn't seem right to me. We should simply be removing anything that isn't already in the database
| @pytest.mark.skipnopetsc4py | ||
| def test_inserted_options_deletes_defaults_for_multiple_prefixes(): | ||
| """Check shared defaults are removed for sequential child managers.""" | ||
| from petsc4py import PETSc |
There was a problem hiding this comment.
What is the reasoning for having these as three separate tests? It'd be simpler just to have one test that covers all these cases.
There was a problem hiding this comment.
The advantage to keeping separate tests is that one can easily tell when individual features break. But we also want to be covering the interaction between these features, so it makes sense to merge them into a single test, so I did this.
Co-authored-by: Josh Hope-Collins <joshua.hope-collins13@imperial.ac.uk>
Co-authored-by: Josh Hope-Collins <joshua.hope-collins13@imperial.ac.uk>
Fixes #58.
On
main, anOptionsManagerwith adefault_options_setwrites shared defaults under its own prefix.inserted_options()leaves those entries in PETSc's global options database, so a later manager with the same prefix can mistake them for options the user supplied.This change includes shared-default keys in
to_deletebefore scanning options under the manager's prefix. At context exit, those keys are removed; existing prefixed options outside the shared defaults remain. A child-prefixed value for a shared default takes precedence while options are inserted.Tests cover cleanup for sequential managers with multiple prefixes and the case where a child-prefixed value overrides a shared default.
This PR was written with Claude Code.
🤖 Generated with Claude Code