Skip to content

An owned option's user latch dies with the option - #597

Open
lmoresi wants to merge 1 commit into
developmentfrom
bugfix/solver-review-followups
Open

An owned option's user latch dies with the option#597
lmoresi wants to merge 1 commit into
developmentfrom
bugfix/solver-review-followups

Conversation

@lmoresi

@lmoresi lmoresi commented Aug 17, 2026

Copy link
Copy Markdown
Member

Closes #490.

Five of the six follow-ups from the #475 review. One is a live defect; the rest
are a swallowed exception, two documentation corrections, a test rename and a
setter that accepted a value with no reading.

The latch outlived the option

_snes_max_it_user, which the issue names, no longer exists — it became the
general _resolve_owned_option. The defect survived the refactor.

solve() re-pushes the keys the solver owns, so the resolver has to tell its own
previous push from a value the user set, and latches the latter. Deleting the key
made the next solve fall back correctly, but that solve pushed the default; the
one after read the default back, found current == pushed, and returned the
latched value instead. Measured on development:

resolved
user sets snes_max_it = 200 200
user deletes the key 50
next solve 200 — resurrected
and every solve after 200

and on this branch, 200 / 50 / 50 / 50.

The latch is now cleared when the key is absent. test_0204_owned_option_latch.py
drives the resolve-then-push pair directly rather than through solve() — that is
the pair solve() calls, it needs no solve, and the test then says what it means.
It carries the control (a value still present keeps being honoured, three solves
running) and the case where the user moves the value rather than deleting it.

The rest

  • Item 3. The bare except Exception: return default at that read now says
    what it swallows — a stored value that will not convert to the option's type,
    such as a key set as a bare flag and holding None — and why taking the
    default is the right response (Charter § on swallowed exceptions).
  • Item 2. The homotopy_options docstring attributed the side of Min to
    the smoother family: "powermean (default, approaches the yield surface from
    below) or sqrt (from above)"
    . The side belongs to yield_anchor — under the
    default onset anchor both families sit below Min near yield. anchor is also
    added to the documented key list, which it was forwardable through and missing
    from.
  • Item 5. test_yield_anchor_keeps_onset_stress_exact drives
    anchor="yield"; "onset" is the other anchor, the one the test shows missing.
    Renamed to test_the_yield_anchor_keeps_stress_exact_at_nominal_yield, with
    the reason in the docstring. No other reference to the old name exists.
  • Item 6. viscosity_min_rounding is a rounding WIDTH — zero is the exact
    hard Max, positive rounds the corner — so a negative value has no reading. It
    reached the tangent only through the compiled expression, so the symptom of
    setting one was a solver that would not converge rather than anything naming
    the property. Now rejected at the setter, sympy values passed through
    untouched.
  • Item 4. The yield_anchor setter discards the softness atom and so orphans
    the δ held by a YieldHomotopyControl built before the call. Documented rather
    than rebound, following the issue's own reading: yield_continuation refuses
    anchor= together with a ready-made control, so the only route to it is
    building a control by hand, setting the anchor after, and reading δ back for
    diagnostics. The comment says to set the anchor first.

Not in this PR

#546 (the rotated workspace cache follow-ups) was scoped alongside this one and
is deliberately left out. Its two items change the cache key and the coefficient
enumeration in a subsystem where a wrong answer is quiet, and they want their own
measurement rather than a place at the end of a docs-and-latch batch.

Verified

Full ./uw test: 1512 passed, 32 skipped, 2 xfailed.

Underworld development team with AI support from Claude Code

Five follow-ups from the #475 review of yield_anchor.

The latch (#490 item 1). _snes_max_it_user no longer exists — it became
_resolve_owned_option — but the defect survived the refactor. solve() re-pushes
the owned keys, so the resolver latches a value it did not push. Deleting the
key made the next solve fall back correctly, but that solve pushed the default,
and the one after read the default back, found it equal to what it had pushed,
and returned the latched value instead. Measured on development:

    user sets 200        -> 200
    user deletes the key -> 50
    next solve           -> 200      resurrected, and permanent

The latch is now cleared when the key is absent from the options DB.
test_0204 drives the resolve-then-push pair directly rather than through
solve(), with a control that a value still present keeps being honoured and a
case where the user moves it.

Item 3: the bare `except Exception: return default` at that read says what it
swallows — a value that will not convert to the option's type — and why the
default is the right answer.

Item 2: the homotopy docstring attributed the side of Min to the smoother
family. The side belongs to yield_anchor; under the default onset anchor both
families sit below Min near yield. `anchor` is also documented in the
homotopy_options key list, which it was forwardable through but missing from.

Item 5: test_yield_anchor_keeps_onset_stress_exact drives anchor="yield" and
"onset" is the other anchor. Renamed for what it exercises.

Item 6: viscosity_min_rounding is a rounding WIDTH, so a negative value has no
reading. It reached the tangent only through the compiled expression, so the
symptom was a solver that would not converge rather than anything naming the
property. Rejected at the setter.

Item 4: the yield_anchor setter discards the softness atom and so orphans the
delta held by a YieldHomotopyControl built before it. Documented rather than
rebound, per the issue: yield_continuation refuses anchor= together with a
ready-made control, so the only route to it is building one by hand and
reading delta back for diagnostics.

Closes #490.

Underworld development team with AI support from Claude Code
Copilot AI lite review requested due to automatic review settings August 17, 2026 08:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@lmoresi

lmoresi commented Aug 17, 2026

Copy link
Copy Markdown
Member Author

Adversarial review

Reviewed at 3e96cc5. Three findings.

1. The latch fix is tested through the private pair, so it does not pin the
route users take.
test_0204 calls _resolve_snes_max_it / _push_snes_max_it
directly. That is what solve() calls, and it makes the test a second rather
than three solves — but it assumes the call pattern, and a refactor that changed
where solve() resolves owned options would leave the test green over a
reintroduced defect. The end-to-end version costs three Stokes solves and would
pin snes.getIterationNumber() instead; we judged the exchange worth it, and are
flagging it because the judgement is the reviewable part.

2. Item 6's guard admits sympy values without checking them. A sympy
expression is passed through on the grounds that its sign is not knowable at set
time. That is right for a free expression, but sympy.Float(-1.0) has a knowable
sign and slips through — is_negative would catch the constant case. Left as is
because the property is documented as taking a number and the sympy path exists
for δ-style atoms, but it is a hole in an otherwise complete check.

3. Item 4 is documented, not fixed, and the comment is the only thing stopping
it.
Setting yield_anchor after building a YieldHomotopyControl still leaves
the control ramping an atom the model has discarded. The reasoning that this is
unreachable rests on yield_continuation refusing anchor= alongside a
ready-made control — a guard in a different module. If that guard is relaxed, the
orphaning becomes reachable with nothing to signal it. A _yield_softness_expr
rebind, or the setter raising when a control is attached, would close it by
construction.

Checked and clean: the negative control for the latch was run against
development's build in the main checkout rather than argued — 200 / 50 / 200 /
200 there, 200 / 50 / 50 / 50 here. No other reference to the renamed test exists
in tests/, docs/, scripts/ or src/.

Underworld development team with AI support from Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants