Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions petsctools/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -499,14 +499,15 @@ def __init__(self, parameters: dict[str, Any],
else:
default_options = {}

# Start building parameters from the defaults so
# that they will overwritten by any other source.
parameters = default_options | parameters
Comment thread
pbrubeck marked this conversation as resolved.
Outdated
inherited_options = set(default_options)

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.

If default_options isn't used as a dictionary again then I think its fine just to rebind it to the set.

Suggested change
inherited_options = set(default_options)
default_options = set(default_options)


# The parameters to drop from the global options when we leave the
# inserted_options context. This is everything except for options
# passed on the command line.
to_delete = set(parameters.keys())
Comment thread
pbrubeck marked this conversation as resolved.

# Start building parameters from the defaults so
# that they will overwritten by any other source.
parameters = default_options | parameters
unsafe_options = []
for full_key, v in self.options_object.getAll().items():
if full_key.startswith(options_prefix):
Expand All @@ -516,9 +517,10 @@ def __init__(self, parameters: dict[str, Any],
key = full_key[len(options_prefix):]
parameters[key] = v

# option is set globally, don't drop when we exit the
# context manager
to_delete.discard(key)
# Keep existing options, except inherited defaults, which
# are temporary copies under this prefix.
if key not in inherited_options:

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.

Suggested change
if key not in inherited_options:
if key not in default_options:

to_delete.discard(key)

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.

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.

if unsafe_options:
unsafe_options_str = "\n".join(
f" {opt}" for opt in unsafe_options
Expand Down
25 changes: 25 additions & 0 deletions tests/test_options.py
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,31 @@ def test_default_options():
assert options2.parameters["opt4"] == "6"


@pytest.mark.skipnopetsc4py
def test_inserted_options_deletes_default_options():
"""Check that inserted_options removes the options that the
OptionsManager inherits from its default options set, and keeps
the options that the database already held."""
from petsc4py import PETSc

options = PETSc.Options()
options["parent_ksp_type"] = "cg"
options["parent_0_pc_type"] = "jacobi"
opts_before = dict(options.getAll())
Comment on lines +189 to +192

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.

Please can you test:

  1. having multiple prefixes in the set (i.e. at least parent_0 and parent_1), and calling inserted_options on both in turn (not nested).
  2. The case where a default option gets overridden? e.g. -parent_ksp_type cg -parent_1_ksp_type gmres


child = petsctools.OptionsManager(
parameters={},
options_prefix="parent_0",
default_options_set=petsctools.DefaultOptionSet("parent", [0]))
assert child.parameters == {"ksp_type": "cg", "pc_type": "jacobi"}

with child.inserted_options():
pass

assert options.getAll() == opts_before
assert "parent_0_ksp_type" not in options


@pytest.mark.skipnopetsc4py
def test_python_options_with_manager():
petsctools.init()
Expand Down
Loading