From 3e96cc556a2de86cfa7ec68e461cc29840c6c301 Mon Sep 17 00:00:00 2001 From: lmoresi Date: Mon, 17 Aug 2026 18:32:17 +1000 Subject: [PATCH] An owned option's user latch dies with the option MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/underworld3/constitutive_models.py | 21 +++++ .../cython/petsc_generic_snes_solvers.pyx | 63 +++++++++----- tests/test_0204_owned_option_latch.py | 84 +++++++++++++++++++ tests/test_1059_yield_anchor.py | 8 +- 4 files changed, 154 insertions(+), 22 deletions(-) create mode 100644 tests/test_0204_owned_option_latch.py diff --git a/src/underworld3/constitutive_models.py b/src/underworld3/constitutive_models.py index 3829776f..1c807d4a 100644 --- a/src/underworld3/constitutive_models.py +++ b/src/underworld3/constitutive_models.py @@ -1034,6 +1034,15 @@ def yield_anchor(self, value): # rebuilt, not merely revalued — and _get_yield_softness only builds it # alongside δ. (The power mean carries the anchor in _combine_yield itself, so # for that family the _reset is the whole of the update.) + # + # Discarding the softness atom ORPHANS the δ held by any + # YieldHomotopyControl built before this call: the control still ramps + # the atom it captured, and the model now reads a fresh one. That cannot + # bite a march, because `yield_continuation` refuses `anchor=` together + # with a ready-made control — so the only way to reach it is to build a + # control by hand, set the anchor afterwards, and read δ back off the + # control for diagnostics. Set the anchor BEFORE building the control + # (#490). self._yield_offset_expr = None self._yield_softness_expr = None self._reset() @@ -1191,6 +1200,18 @@ def viscosity_min_rounding(self): @viscosity_min_rounding.setter def viscosity_min_rounding(self, value): + # A rounding scale is a width, so it is non-negative by construction: + # zero is the exact hard Max, positive rounds the corner. A negative + # value has no reading — it would sharpen the corner past Max — and + # since it reaches the tangent only through the compiled expression, + # the symptom of setting one is a solver that will not converge rather + # than anything naming this property (#490). + if value is not None and not isinstance(value, sympy.Basic): + if float(value) < 0.0: + raise ValueError( + f"viscosity_min_rounding is a rounding WIDTH and cannot be " + f"negative; got {value}. Use 0 for the exact Max.") + self._viscosity_min_rounding = value self._reset() diff --git a/src/underworld3/cython/petsc_generic_snes_solvers.pyx b/src/underworld3/cython/petsc_generic_snes_solvers.pyx index dcadfe23..f3df7cf6 100644 --- a/src/underworld3/cython/petsc_generic_snes_solvers.pyx +++ b/src/underworld3/cython/petsc_generic_snes_solvers.pyx @@ -6783,19 +6783,36 @@ class SNES_Stokes_SaddlePt(SolverBaseClass): """ pushed = self._owned_option_pushes.get(key) user = self._owned_option_user.get(key) - if self.petsc_options.hasName(key): - try: - current = type(default)(self.petsc_options.getString(key)) - except Exception: - return default - if pushed is None or current != pushed: - # Never pushed by us, or the user has moved it since. Latch: from here on - # the option is theirs, because the next solve will read back OUR push of - # THEIR value and would otherwise mistake it for our own default. - self._owned_option_user[key] = current - return current - if user is not None: - return user + if not self.petsc_options.hasName(key): + # The key is gone from the options DB, so any value latched from a + # previous solve is gone with it. Without this the latch outlives + # the option: set snes_max_it=200, delete it, and the next solve + # correctly uses the default — but the one after that reads back + # OUR push of the default, finds `current == pushed`, falls through + # to the latched 200 and resurrects it (#490). + self._owned_option_user.pop(key, None) + return default + + try: + current = type(default)(self.petsc_options.getString(key)) + except Exception: + # The stored value will not convert to the option's type: a user who + # wrote `snes_max_it = "lots"`, or a key set as a bare flag and so + # holding None. Neither is a number this solve can use, so it takes + # its own default and leaves the value in the DB for PETSc to object + # to in its own terms (Charter: say what is swallowed and why). + return default + + if pushed is None or current != pushed: + # Never pushed by us, or the user has moved it since. Latch: from here on + # the option is theirs, because the next solve will read back OUR push of + # THEIR value and would otherwise mistake it for our own default. + self._owned_option_user[key] = current + return current + + if user is not None: + return user + return default def _push_owned_option(self, key, value): @@ -9350,13 +9367,19 @@ class SNES_Stokes_SaddlePt(SolverBaseClass): homotopy_options : dict, optional March settings passed to :func:`~underworld3.systems.yield_continuation.yield_continuation` — - ``smoother``, ``delta0``, ``down``, ``dmin``, ``entry_maxit``, - ``step_maxit``, ``retries``. All are defaulted; tuning them is optional. - ``smoother`` picks the soft-min family — ``"powermean"`` (default, - approaches the yield surface from below) or ``"sqrt"`` (from above). Which - gives the better cold entry is problem-dependent, so it is worth trying both - when a march will not start. Leave ``delta0`` unset unless you have a - reason: each family supplies its own entry, and the two δ are not the same + ``smoother``, ``anchor``, ``delta0``, ``down``, ``dmin``, + ``entry_maxit``, ``step_maxit``, ``retries``. All are defaulted; tuning + them is optional. + + ``smoother`` picks the soft-min family — ``"powermean"`` (default) or + ``"sqrt"``. Which gives the better cold entry is problem-dependent, so + it is worth trying both when a march will not start. + + Which SIDE of the exact ``Min`` the softened yield sits on belongs to + ``anchor``, not to the family: under the default onset anchor both + families sit below ``Min`` near yield. See ``yield_anchor`` on the + constitutive model. Leave ``delta0`` unset unless you have a reason: + each family supplies its own entry, and the two δ are not the same parameter. Returns diff --git a/tests/test_0204_owned_option_latch.py b/tests/test_0204_owned_option_latch.py new file mode 100644 index 00000000..9f81fe1b --- /dev/null +++ b/tests/test_0204_owned_option_latch.py @@ -0,0 +1,84 @@ +"""An owned option's user latch dies with the option (#490). + +`solve()` re-pushes the options the solver owns, so `_resolve_owned_option` has +to tell its own previous push from a value the user set. It does that by +latching the user's value the first time it sees one it did not push. + +The latch outlived the option. 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 + and the next -> 200 + +The sequence is driven through `_resolve_snes_max_it` / `_push_snes_max_it` +rather than through `solve()`: that is the pair `solve()` itself calls, and it +takes no solve to exercise, so the test says what it means and runs in a second. +""" + +import pytest + +import underworld3 as uw + +pytestmark = [pytest.mark.level_1, pytest.mark.tier_a] + +DEFAULT = 50 +USER = 200 + + +@pytest.fixture +def solver(): + mesh = uw.meshing.StructuredQuadBox(elementRes=(3, 3)) + v = uw.discretisation.MeshVariable("Vlatch", mesh, mesh.dim, degree=2) + p = uw.discretisation.MeshVariable("Platch", mesh, 1, degree=1) + + return uw.systems.Stokes(mesh, velocityField=v, pressureField=p) + + +def _solve_cycle(solver): + """One solve's worth of the resolve-then-push pair.""" + + resolved = solver._resolve_snes_max_it(DEFAULT) + solver._push_snes_max_it(resolved) + + return resolved + + +def test_a_user_value_is_honoured_and_keeps_being_honoured(solver): + """The control: the latch does its job while the option is there.""" + + solver.petsc_options["snes_max_it"] = USER + + assert _solve_cycle(solver) == USER + assert _solve_cycle(solver) == USER + assert _solve_cycle(solver) == USER + + +def test_deleting_the_option_does_not_leave_the_value_behind(solver): + """Deleting the key gives the default back, and it stays given back.""" + + solver.petsc_options["snes_max_it"] = USER + assert _solve_cycle(solver) == USER + + del solver.petsc_options["snes_max_it"] + + assert _solve_cycle(solver) == DEFAULT + assert _solve_cycle(solver) == DEFAULT, ( + "the deleted user value came back on the second solve after deletion — " + "the latch outlived the option it was latched from") + assert _solve_cycle(solver) == DEFAULT + + +def test_a_second_user_value_replaces_the_first(solver): + """A moved value is picked up rather than shadowed by the latched one.""" + + solver.petsc_options["snes_max_it"] = USER + assert _solve_cycle(solver) == USER + + solver.petsc_options["snes_max_it"] = 7 + + assert _solve_cycle(solver) == 7 + assert _solve_cycle(solver) == 7 diff --git a/tests/test_1059_yield_anchor.py b/tests/test_1059_yield_anchor.py index 16765d85..7747d12d 100644 --- a/tests/test_1059_yield_anchor.py +++ b/tests/test_1059_yield_anchor.py @@ -69,8 +69,12 @@ def test_delta_zero_is_exact_min_for_both_anchors(): @pytest.mark.level_1 @pytest.mark.tier_a @pytest.mark.parametrize("smoother", ("sqrt", "powermean")) -def test_yield_anchor_keeps_onset_stress_exact(smoother): - """tau/tau_y = f*eta/eta_ve = 1 at f=1 for every delta. The onset anchor does NOT.""" +def test_the_yield_anchor_keeps_stress_exact_at_nominal_yield(smoother): + """tau/tau_y = f*eta/eta_ve = 1 at f=1 for every delta. The onset anchor does NOT. + + Named for the anchor it exercises: this drives ``yield_anchor="yield"``, and + "onset" is the OTHER anchor — the one shown here to miss (#490). + """ c = _model(smoother); c.yield_anchor = "yield" for d in DELTAS[smoother]: assert abs(eta(c, 1.0, d) - 1.0) < 1e-10, d