Use PETSc's options parser instead of our own (#642) - #652
Open
lmoresi wants to merge 2 commits into
Open
Conversation
`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
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
lmoresi
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the silent half of #642. Two commits: the first patches our parser, the second deletes it in favour of PETSc's. The second is the one that matters — read the diff, not just the first commit.
Root cause — ours, not PETSc's
parse_cmd_line_optionsdecided what was an option name withwhich accepts
-2. So in-uw_sense -2the parser read-2as the next key, storeduw_sensewith no value, and registered a stray option2. The negative never arrived,Paramsfell back to its default, and nothing was said. It once ran half of a 26-run parameter ladder at the default sign while reporting it under the requested label.PETSc does not have this problem.
PetscOptionsValidKeyrequires a hyphen followed by a letter, which is exactly what separates-uw_sensefrom-2.Why the second commit
The first commit copied PETSc's rule into our parser. That repaired the one instance and left the duplication — and the duplication was the actual defect. UW parameters are namespaced
-uw_*precisely so they can sit in the PETSc options database alongside PETSc's own, anduw.optionsis already aPETSc.Options("uw_")view onto it. Parsing was the single part being re-done by hand, and the only part that was wrong.So the body is now a hand-off to
PetscOptionsInsertString(petsc4py'sOptions.insertString), which applies the same rules PETSc applies to its own arguments.The function still exists and is still called per
Paramsconstruction, because petsc4py does not populate the database fromsys.argvon every platform (#111, Gadi). Arguments carrying whitespace are re-quoted, sinceinsertStringtakes one string and PETSc reads double quotes.Measured
-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'End to end,
-uw_sense -2now yields-2.0from sourcecli. A value containing a space survives intact, and a positional argument alongside the options does not disturb them.Tests
Negative integer, negative float, and negative exponent — the last because its inner hyphen is the case a naive fix would miss. The test fails on the old predicate (verified by reverting the installed copy) and passes now. A positive-value test sits beside it as the negative control, since this narrows what counts as an option name.
level_1 and tier_a: 1074 passed, 0 failed.Deliberately not fixed here
getIntraises on a float-valued option, and the legacy branch of_get_petsc_optionswallows it with a bareexcept: return default— souw.Params(uw_steps=3)with-uw_steps 2.5still falls back to 3 silently. Same failure class, different route. Documented on #642; #642 should stay open for it.Underworld development team with AI support from Claude Code