Use PETSc's options parser instead of our own (#642)

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

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")