diff --git a/src/underworld3/utilities/_petsc_tools.py b/src/underworld3/utilities/_petsc_tools.py index 60d76fcf..344b8aff 100644 --- a/src/underworld3/utilities/_petsc_tools.py +++ b/src/underworld3/utilities/_petsc_tools.py @@ -17,31 +17,42 @@ def require_dirs(ListOfDirs): def parse_cmd_line_options(): - """ - This function will parse all PETSc type command line options - and pass them through via `petsc4py`. + """Hand the command line to PETSc's own options parser. + + UW parameters are namespaced `-uw_*` precisely so they can sit in the PETSc + options database alongside PETSc's own without clashing, and `uw.options` is + a `PETSc.Options("uw_")` view onto it. So there is nothing here that PETSc + does not already do: `PetscOptionsInsertString` (exposed by petsc4py as + `Options.insertString`) applies the same parsing rules PETSc applies to its + own arguments. + + This used to re-implement that parsing, and got it wrong. Its test for an + option NAME was `item[0] == "-" and item[1] != "-"`, which accepts `-2`, so + `-uw_sense -2` stored `uw_sense` with no value and registered a stray option + `2` -- the negative silently never arrived (#642). PETSc's own rule + (`PetscOptionsValidKey`) requires a hyphen followed by a letter, which is + exactly what distinguishes an option from a negative number. Deferring to it + fixes that class of bug rather than the one instance of it. + + It exists at all because petsc4py does NOT populate the options database + from `sys.argv` on every platform (Gadi being the case in #111), so + something has to do the insertion explicitly. It is idempotent -- re-inserting + the same arguments rewrites the same values -- so it is safe to call on every + `Params` construction. """ from petsc4py import PETSc import sys - options = PETSc.Options() - - def is_petsc_key(item): - # petsc options have single hyphen prefix - return len(item) >= 2 and item[0] == "-" and item[1] != "-" - - for index, opt in enumerate(sys.argv): - if is_petsc_key(opt): - key = opt[1:] - # if it's the last item, set to None - if len(sys.argv) == index + 1: - options[key] = None - # if the next item is a different key, set to None - elif is_petsc_key(sys.argv[index + 1]): - options[key] = None - # else set next item to the option value - else: - options[key] = sys.argv[index + 1] + arguments = sys.argv[1:] + if not arguments: + return + + # PetscOptionsInsertString reads a single string, so an argument carrying + # whitespace has to be quoted back up; PETSc understands double quotes. + def requote(argument): + return f'"{argument}"' if any(c.isspace() for c in argument) else argument + + PETSc.Options().insertString(" ".join(requote(a) for a in arguments)) import os as _os diff --git a/tests/test_0821_params_cli_override.py b/tests/test_0821_params_cli_override.py index 7f46e2a1..ba9d32ae 100644 --- a/tests/test_0821_params_cli_override.py +++ b/tests/test_0821_params_cli_override.py @@ -51,3 +51,49 @@ def test_params_uses_default_without_cli(): assert params.uw_testparam_111 == "default_value" finally: sys.argv = saved + + +def test_a_negative_value_reaches_the_parameter(): + """A negative CLI value must arrive, not fall back to the default (#642). + + `parse_cmd_line_options` decided what was an option NAME with + `item[0] == "-" and item[1] != "-"`, which accepts `-2`. So `-uw_sense -2` + stored `sense` with no value and registered a stray option `2`, and Params + then used its default — silently. It once ran half of a 26-run parameter + ladder at the wrong sign while reporting it under the requested label. + + PETSc's own rule (`PetscOptionsValidKey`) is a hyphen followed by a LETTER, + which is exactly what distinguishes a key from a negative number. + """ + saved = sys.argv + try: + for name, given, expected in ( + ("uw_sense_642", "-2", -2.0), # negative integer + ("uw_scale_642", "-2.5", -2.5), # negative float + ("uw_tiny_642", "-1e-5", -1.0e-5), # negative exponent, inner hyphen + ): + _clear(name[3:]) + sys.argv = ["prog", f"-{name}", given] + params = uw.Params(**{name: uw.Param(1.0, "probe")}) + actual = float(getattr(params, name)) + assert actual == pytest.approx(expected), ( + f"-{name} {given} gave {actual}, not {expected} — a negative " + "value was read as the next option name again" + ) + _clear(name[3:]) + finally: + sys.argv = saved + + +def test_a_positive_value_still_reaches_the_parameter(): + """Negative control: the fix narrows what counts as an option name, so the + ordinary positive path has to be shown still working.""" + saved = sys.argv + try: + _clear("sense_642_pos") + sys.argv = ["prog", "-uw_sense_642_pos", "2.5"] + params = uw.Params(uw_sense_642_pos=uw.Param(1.0, "probe")) + assert float(params.uw_sense_642_pos) == pytest.approx(2.5) + finally: + sys.argv = saved + _clear("sense_642_pos")