From 0da13d2bcb722941c176a6e50d06f34204b09a60 Mon Sep 17 00:00:00 2001 From: lmoresi Date: Wed, 26 Aug 2026 18:47:49 +1000 Subject: [PATCH 1/2] Let a negative number be a value, not an option name (#642) `parse_cmd_line_options` decided what was an option NAME with item[0] == "-" and item[1] != "-" which accepts `-2`. So in `-uw_sense -2` the parser read `-2` as the next key, stored `uw_sense` with no value, and registered a stray option `2`. The negative never arrived, `Params` fell back to its default, and said nothing. It once ran half of a 26-run parameter ladder at the default sign while reporting it under the requested label; it was caught only because the partition numbers came out identical to the +1 runs. PETSc does not have this problem. `PetscOptionsValidKey` requires a hyphen followed by a LETTER, which is precisely what separates `-uw_sense` from `-2`. This adopts the same rule. `--long` stays excluded exactly as before, and a leading underscore is allowed for symmetry with the names PETSc accepts. Measured at the options database, before -> after: -uw_sense 2 '2' -> '2' -uw_sense -2 '' -> '-2' -uw_sense 2.5 '2.5' -> '2.5' -uw_sense -2.5 '' -> '-2.5' -uw_sense -1e-5 '' -> '-1e-5' and end to end through Params, `-uw_sense -2` now gives -2.0 from source 'cli' where it gave the default. The regression covers a negative integer, a negative float and a negative exponent (whose inner hyphen is the case a naive fix would miss). It FAILS on the old predicate and passes on the new one, and a positive-value test sits beside it as the negative control, since this change narrows what counts as an option name and the ordinary path has to be shown still working. Not fixed here, and left on #642: `getInt` RAISES on a float-valued option, and the legacy branch of `_get_petsc_option` swallows it with a bare `except: return default` -- so an int parameter given a float on the command line still falls back silently. Same failure class, different route, and it needs its own decision about whether to raise. Underworld development team with AI support from Claude Code --- src/underworld3/utilities/_petsc_tools.py | 12 +++++- tests/test_0821_params_cli_override.py | 46 +++++++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/src/underworld3/utilities/_petsc_tools.py b/src/underworld3/utilities/_petsc_tools.py index 60d76fcfd..22b010250 100644 --- a/src/underworld3/utilities/_petsc_tools.py +++ b/src/underworld3/utilities/_petsc_tools.py @@ -27,8 +27,16 @@ def parse_cmd_line_options(): options = PETSc.Options() def is_petsc_key(item): - # petsc options have single hyphen prefix - return len(item) >= 2 and item[0] == "-" and item[1] != "-" + # PETSc options have a single hyphen prefix followed by a LETTER. The + # letter matters: `PetscOptionsValidKey` requires it, which is how PETSc + # itself tells `-uw_sense` (a key) from `-2` (a negative value). Testing + # only for `item[1] != "-"` accepted `-2` as a key, so `-uw_sense -2` + # stored `uw_sense` with no value and registered a stray option `2` -- + # the negative never arrived and Params fell back to its default in + # silence (#642). Leading `_` is allowed for symmetry with the names + # PETSc accepts; `--long` stays excluded, as before. + return (len(item) >= 2 and item[0] == "-" + and (item[1].isalpha() or item[1] == "_")) for index, opt in enumerate(sys.argv): if is_petsc_key(opt): diff --git a/tests/test_0821_params_cli_override.py b/tests/test_0821_params_cli_override.py index 7f46e2a1b..ba9d32aeb 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") From 7f36e1d8212dc96ac6835666932e58fd9ff97122 Mon Sep 17 00:00:00 2001 From: lmoresi Date: Wed, 26 Aug 2026 20:26:16 +1000 Subject: [PATCH 2/2] Use PETSc's options parser instead of our own (#642) The previous commit fixed our hand-rolled argv parser by copying PETSc's valid-key rule into it. That was the wrong shape of fix: UW parameters are namespaced `-uw_*` precisely so they can live in the PETSc options database next to PETSc's own, and `uw.options` is already a `PETSc.Options("uw_")` view onto it. The parsing was the one part being re-implemented, and it was the one part that was wrong. `PetscOptionsInsertString` -- petsc4py's `Options.insertString` -- applies the same rules PETSc applies to its own arguments, negative numbers included. So the body becomes a hand-off, and the whole class of divergence-from-PETSc goes with it rather than the single instance we happened to hit. Kept: the function still exists and is still called on every `Params` construction, because petsc4py does not populate the database from `sys.argv` on every platform (#111, Gadi). It is still idempotent. Arguments carrying whitespace are re-quoted, since `insertString` takes one string and PETSc reads double quotes. Verified after the change: -uw_sense 2 -> '2' -uw_sense -2 -> '-2' -uw_sense 2.5 -> '2.5' -uw_sense -2.5 -> '-2.5' -uw_sense -1e-5 -> '-1e-5' end to end through Params (-2.0 from source 'cli'), a value containing a space survives intact, and a positional argument sitting alongside the options does not disturb them. Underworld development team with AI support from Claude Code --- src/underworld3/utilities/_petsc_tools.py | 61 ++++++++++++----------- 1 file changed, 32 insertions(+), 29 deletions(-) diff --git a/src/underworld3/utilities/_petsc_tools.py b/src/underworld3/utilities/_petsc_tools.py index 22b010250..344b8aff2 100644 --- a/src/underworld3/utilities/_petsc_tools.py +++ b/src/underworld3/utilities/_petsc_tools.py @@ -17,39 +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 a single hyphen prefix followed by a LETTER. The - # letter matters: `PetscOptionsValidKey` requires it, which is how PETSc - # itself tells `-uw_sense` (a key) from `-2` (a negative value). Testing - # only for `item[1] != "-"` accepted `-2` as a key, so `-uw_sense -2` - # stored `uw_sense` with no value and registered a stray option `2` -- - # the negative never arrived and Params fell back to its default in - # silence (#642). Leading `_` is allowed for symmetry with the names - # PETSc accepts; `--long` stays excluded, as before. - return (len(item) >= 2 and item[0] == "-" - and (item[1].isalpha() or 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