From c64d83cdc29639e343a340aa0d66fb3d64c0a01c Mon Sep 17 00:00:00 2001 From: lmoresi Date: Tue, 22 Sep 2026 09:57:31 +1000 Subject: [PATCH 1/2] Refuse a re-declaration that would change what a container holds An expression's identity is its NAME: uw.expression(r"\eta", ...) returns the existing container so a formula written early keeps seeing later edits. But __init__ then ran on that returned object and overwrote its contents and its description from the arguments - so a line that reads like a declaration silently changed every formula already written against the name. Re-declaration now: * with the SAME value, passes. Declaring the same thing twice changes nothing and no formula can tell; a factory that rebuilds an unmutated problem in one process is doing exactly that, and refusing it would be noise. * with a DIFFERENT value, raises, naming the two things the caller might have meant: `name.sym = value` to change the contents, uw.expression(name) to fetch. The existing contents are left untouched. Anything that cannot be compared counts as different, so the loud path is the default: a container whose contents we cannot reason about is the last one to overwrite quietly. Blast radius, measured before choosing rather than after. Surveying 576 tests (bands 00-03) recorded 15 re-declarations in total: 6 with the same value and 9 with a different one, across five names - and NO library code among them. All nine were test factories that rebuild the same named problem in one process, where the silent overwrite was load-bearing. The most instructive is \eta_{w302}, whose container still held `2.0*k_n302 + 0.05*{T302}(N.x, N.y)**2` from the FIRST build: the overwrite is what rebound it to the second build's mesh variable, and nothing said so. Those three factories now declare-or-assign through a small local helper, which makes the rebinding the visible act it always was. tests/test_00*|01*|02*|03*py: 576 passed. Underworld development team with AI support from Claude Code --- src/underworld3/function/expressions.py | 56 ++++++++++++++++++++++- tests/test_0007_snapshot_inmemory.py | 30 +++++++++--- tests/test_0066_integration_point_slcn.py | 20 +++++++- tests/test_0069_swarm_midtime_velocity.py | 20 +++++++- tests/test_0103_jit_rampable_constants.py | 28 ++++++++++-- 5 files changed, 141 insertions(+), 13 deletions(-) diff --git a/src/underworld3/function/expressions.py b/src/underworld3/function/expressions.py index 6ca0c29c..c66cf924 100644 --- a/src/underworld3/function/expressions.py +++ b/src/underworld3/function/expressions.py @@ -700,9 +700,19 @@ def __new__( instance_no = UWexpression._expr_count - # If the expression already exists, return it + # If the expression already exists, return it — that IS the container + # contract: identity is the name, so the same name reaches the same + # object and a formula written against it keeps seeing later edits. + # + # Flagged so __init__ knows it is running on an object that already has + # contents. Construction must not silently overwrite them: changing what + # a container holds is what `.sym =` is for, and a second construction + # that quietly replaced the value would reach every formula already + # written against the name, from a line that reads like a declaration. if name in UWexpression._expr_names.keys() and _unique_name_generation == False: - return UWexpression._expr_names[name] + existing = UWexpression._expr_names[name] + existing._reused_construction = True + return existing # Check both dicts for name collisions name_exists_persistent = name in UWexpression._expr_names @@ -851,6 +861,27 @@ def _sympystr(self, printer): """ return self._display_name + def _holds_same_value(self, incoming) -> bool: + """Is a re-declaration asking for what this container already holds? + + Declaring the same thing twice changes nothing and no formula written + against the name can tell — a factory that rebuilds an unmutated problem + in one process is doing exactly that, and refusing it would be noise. + Declaring something DIFFERENT is the case that must not pass silently. + + Anything that cannot be compared counts as different, so the loud path + is the default: a container whose contents we cannot reason about is the + last one to overwrite quietly. + """ + try: + if isinstance(incoming, UWexpression): + incoming = incoming._sym + if isinstance(incoming, (sympy.Basic, sympy.matrices.MatrixBase)): + return bool(incoming == self._sym) + return bool(sympy.sympify(incoming) == self._sym) + except Exception: + return False + def __init__( self, name, @@ -860,6 +891,27 @@ def __init__( units=None, # Units for wrapping the value **kwargs, ): + # Running on a container that already exists (see __new__): its contents + # belong to whoever set them, so leave them alone and say so. Anything + # that wants to change them has `.sym =`. + if getattr(self, "_reused_construction", False): + self._reused_construction = False + incoming = sym if sym is not None else value + if incoming is not None and not self._holds_same_value(incoming): + raise ValueError( + f"expression {name!r} already exists and holds " + f"{self._sym!r}. A UW expression is a persistent container " + f"whose identity is its NAME, so this call returns the " + f"existing object rather than making a new one — and " + f"overwriting its contents here would silently change every " + f"formula already written against {name!r}, from a line " + f"that reads like a declaration.\n" + f" to change what it holds: {name}.sym = \n" + f" to fetch it: uw.expression({name!r})\n" + f" for an independent symbol: choose another name" + ) + return + # Handle legacy 'value' parameter if value is not None and sym is None: import warnings diff --git a/tests/test_0007_snapshot_inmemory.py b/tests/test_0007_snapshot_inmemory.py index 68af29ed..d4096be4 100644 --- a/tests/test_0007_snapshot_inmemory.py +++ b/tests/test_0007_snapshot_inmemory.py @@ -867,15 +867,33 @@ def test_capture_reads_the_persistent_container_registry(): assert not (ephemeral_names & persistent_names & {r"\beta"}) -def test_a_reused_name_is_one_container_and_is_captured_once(): - """Identity is the NAME: asking for the same name returns the same object, +def test_a_name_fetched_twice_is_one_container_and_is_captured_once(): + """Identity is the NAME: fetching the same name returns the same object, which is what lets a formula written early keep seeing later edits. Capture - must therefore record it once, not once per construction site.""" + must therefore record it once, not once per reference.""" uw, model, mesh = _fresh_model_and_mesh() - first = uw.expression(r"\gamma_{shared}", 1.0, "first use") - second = uw.expression(r"\gamma_{shared}", 2.0, "second use") - assert first is second + first = uw.expression(r"\gamma_{shared}", 1.0, "declared once") + again = uw.expression(r"\gamma_{shared}") # fetch, do not redeclare + assert first is again snap = model.save_state() key = f"{type(first).__name__}_{first.instance_number}" assert [k for k, _s, _w in snap.expressions].count(key) == 1 + + +def test_redeclaring_a_name_with_a_value_is_refused(): + """Changing what a container holds is what ``.sym =`` is for. + + A second construction that quietly replaced the contents would reach every + formula already written against that name, from a line that reads like a + declaration — so it raises, and says which of the two things the caller + meant. The existing contents are left untouched. + """ + uw, model, mesh = _fresh_model_and_mesh() + eta = uw.expression(r"\eta_{decl}", 1.0, "declared") + eta.sym = 42.0 + + with pytest.raises(ValueError, match="already exists"): + uw.expression(r"\eta_{decl}", 99.0, "redeclared") + + assert float(eta.sym) == pytest.approx(42.0), "a refused call still wrote" diff --git a/tests/test_0066_integration_point_slcn.py b/tests/test_0066_integration_point_slcn.py index 9b16d09e..cb1209ea 100644 --- a/tests/test_0066_integration_point_slcn.py +++ b/tests/test_0066_integration_point_slcn.py @@ -17,6 +17,24 @@ import sympy import underworld3 as uw + + +def _param(name, value, description): + """Declare a named parameter, or fetch it and assign. + + An expression's identity is its NAME: the same name reaches the same + container, so a second declaration carrying a different value is refused + rather than silently rebinding every formula already written against it. + A factory that rebuilds the same problem in one process is asking to SET + the value, and assignment is how that is said. + """ + import underworld3 as uw + try: + return uw.expression(name, value, description) + except ValueError: + existing = uw.expression(name) + existing.sym = value + return existing from underworld3.systems.ddt import _storage_components # Module carries the LEVEL only; the tier goes on each test. pytest MERGES module @@ -137,7 +155,7 @@ def _unsteady_uniform_flow_check(kind, vform="var"): # "ramp": a constant that changes between the two steps; the cached # previous velocity must carry the OLD value (substituting snapshots of # the variables into the expression would read the new one). - c = uw.expression(r"c_{ramp}", 1.0, "ramping factor") + c = _param(r"c_{ramp}", 1.0, "ramping factor") V_fn, factor = {"var": (v_var, 1.0), "neg": (-v_var.sym, -1.0), "half": (v_var.sym / 2, 0.5), "ramp": (c * v_var.sym, 1.0)}[vform] diff --git a/tests/test_0069_swarm_midtime_velocity.py b/tests/test_0069_swarm_midtime_velocity.py index b170d31f..67002715 100644 --- a/tests/test_0069_swarm_midtime_velocity.py +++ b/tests/test_0069_swarm_midtime_velocity.py @@ -13,6 +13,24 @@ import underworld3 as uw + +def _param(name, value, description): + """Declare a named parameter, or fetch it and assign. + + An expression's identity is its NAME: the same name reaches the same + container, so a second declaration carrying a different value is refused + rather than silently rebinding every formula already written against it. + A factory that rebuilds the same problem in one process is asking to SET + the value, and assignment is how that is said. + """ + import underworld3 as uw + try: + return uw.expression(name, value, description) + except ValueError: + existing = uw.expression(name) + existing.sym = value + return existing + pytestmark = [pytest.mark.level_1, pytest.mark.tier_a] @@ -20,7 +38,7 @@ def _angle_error(midtime, nsteps=10, dt=0.1): mesh = uw.meshing.UnstructuredSimplexBox(minCoords=(-1.0, -1.0), maxCoords=(1.0, 1.0), cellSize=0.2, qdegree=2) mesh.return_coords_to_bounds = None x, y = mesh.X - c = uw.expression(r"c_{rate}", 1.0, "rotation rate") + c = _param(r"c_{rate}", 1.0, "rotation rate") V = c * sympy.Matrix([[-y, x]]) swarm = uw.swarm.Swarm(mesh) swarm.verbose = False diff --git a/tests/test_0103_jit_rampable_constants.py b/tests/test_0103_jit_rampable_constants.py index 5691da51..3c1b087a 100644 --- a/tests/test_0103_jit_rampable_constants.py +++ b/tests/test_0103_jit_rampable_constants.py @@ -13,6 +13,23 @@ import pytest import underworld3 as uw + +def _param(name, value, description): + """Declare a named parameter, or fetch it and assign. + + An expression's identity is its NAME: the same name reaches the same + container, so a second declaration carrying a different value is refused + rather than silently rebinding every formula already written against it. + A factory that rebuilds the same problem in one process is asking to SET + the value, and assignment is how that is said. + """ + try: + return uw.expression(name, value, description) + except ValueError: + existing = uw.expression(name) + existing.sym = value + return existing + pytestmark = [pytest.mark.level_1, pytest.mark.tier_b] @@ -21,12 +38,17 @@ def _build(): elementRes=(8, 8), minCoords=(0.0, 0.0), maxCoords=(1.0, 1.0)) T = uw.discretisation.MeshVariable("T302", mesh, 1, degree=2) - k_direct = uw.expression(r"k_d", 1.0, "constant at parameter top level") - k_nested = uw.expression(r"k_n302", 1.0, "constant nested in a wrapper") + k_direct = _param(r"k_d", 1.0, "constant at parameter top level") + k_nested = _param(r"k_n302", 1.0, "constant nested in a wrapper") # Non-constant wrapper: the collection recurses into it and manifests # k_nested, but a top-level substitution cannot see inside it — the # baked-constant topology from issue #302. - wrapper = uw.expression( + # + # Assigned, not re-declared. On a second _build() the wrapper's contents + # still reference the FIRST build's T302, and a declaration that silently + # replaced them is what used to rebind it. Saying it with `.sym =` makes the + # rebinding the visible act it is. + wrapper = _param( r"\eta_{w302}", k_nested * 2.0 + 0.05 * T.sym[0] ** 2, "wrapper") poisson = uw.systems.Poisson(mesh, u_Field=T) From a351216f1093003d8365cbeafd529439d6d103c4 Mon Sep 17 00:00:00 2001 From: lmoresi Date: Wed, 23 Sep 2026 13:03:52 -0400 Subject: [PATCH 2/2] Clear the expression registry on reset_default_model, and invert the two tests that specified the old behaviour Two things this branch got wrong, both found by CI running the FULL suite. First, the blast radius I reported was measured on bands 00-03 and was wrong. CI showed 42 failures across 11 files, not 6, and library code DOES re-declare - the derivative cache ({T}_{,1}(N.x, N.y), six hits) and a solver form (\mathbf{f}_0(\mathbf{u}), ten). A partial band is not a blast radius. Second, and the actual cause: the top collisions were generic names - t_\textrm{now} twenty times, L fourteen, t and \eta eight each, x six. Those are not one program re-declaring. They are UNRELATED TESTS in one pytest process sharing a container, because UWexpression._expr_names is a CLASS attribute and reset_default_model() only replaced _default_model and the strict-units flag. A reset that leaves the containers standing is not a reset: the next model looking up a familiar name inherits the previous one's contents, in a test session and in an interactive one alike. reset_default_model() now calls reset_expression_registry(), which clears both the persistent and the ephemeral registries. Live objects are unaffected - a solver or a formula holds the container itself, not its name. What resets is LOOKUP, so the next uw.expression(r"\eta", ...) builds a fresh container instead of finding the old one, which is what "a new model" should mean. uw_object._obj_count is deliberately left monotonic: resetting it would hand out instance numbers that live objects already hold. That took the bands from 42 failures to 2, with zero re-declaration errors remaining. The last two were TestExpressionSilentUpdate, which asserted the OPPOSITE of this branch - "Recreating an expression with the same name should silently update it", called "natural Python behavior", with the loop pattern blessed explicitly. So the behaviour removed here was specified and tested, and this is the reversal of a decision rather than the fixing of an oversight. The class is renamed and its tests inverted, with the docstring recording that the old contract existed and why it changed; the loop case is kept, written as the assignment it always was underneath. tests/test_0565 14 passed; bands 05-08 1420 passed 2->0 failures; bands 00-03 576 passed. Underworld development team with AI support from Claude Code --- src/underworld3/function/expressions.py | 21 +++++ src/underworld3/model.py | 8 ++ tests/test_0565_expression_caching.py | 101 ++++++++++++------------ 3 files changed, 79 insertions(+), 51 deletions(-) diff --git a/src/underworld3/function/expressions.py b/src/underworld3/function/expressions.py index c66cf924..bd55e9cd 100644 --- a/src/underworld3/function/expressions.py +++ b/src/underworld3/function/expressions.py @@ -603,6 +603,27 @@ def substitute_expr(fn, sub_expr, keep_constants=True, return_self=True): # UWexpression Class - Simplified (no UWQuantity inheritance) # ============================================================================ +def reset_expression_registry(): + """Drop the named expression containers, so a new model starts clean. + + ``_expr_names`` is a CLASS attribute: it belongs to the process, not to a + Model. Without this, containers outlive the model that made them, and the + next model looking up a familiar name — ``\\eta``, ``t``, ``x`` — silently + inherits the previous one's contents. In a test session that is every + unrelated test sharing one ``\\eta``; in an interactive session it is the + previous run's viscosity turning up in the next. + + Live objects are unaffected: a solver or a formula holds the container + itself, not its name, and keeps working. What resets is LOOKUP — the next + ``uw.expression(r"\\eta", ...)`` builds a fresh container instead of finding + the old one, which is what "a new model" should mean. + + Called by :func:`~underworld3.model.reset_default_model`. + """ + UWexpression._expr_names.clear() + UWexpression._ephemeral_expr_names.clear() + + def live_expressions(): """The persistent expression containers, in a stable order. diff --git a/src/underworld3/model.py b/src/underworld3/model.py index 1a5619ae..b33061ea 100644 --- a/src/underworld3/model.py +++ b/src/underworld3/model.py @@ -4721,6 +4721,14 @@ def reset_default_model(): import underworld3 as uw uw.use_strict_units(True) + # The named expression containers belong to the PROCESS, not to a Model: + # UWexpression._expr_names is a class attribute. Leaving them behind is what + # made a fresh model inherit the previous one's parameters under familiar + # names, and what made unrelated tests in one pytest process share a single + # `\eta`. A reset that leaves that standing is not a reset. + from underworld3.function.expressions import reset_expression_registry + reset_expression_registry() + return _default_model diff --git a/tests/test_0565_expression_caching.py b/tests/test_0565_expression_caching.py index 81fc35b5..6a229003 100644 --- a/tests/test_0565_expression_caching.py +++ b/tests/test_0565_expression_caching.py @@ -171,57 +171,56 @@ def test_derivative_caching_no_warning(self, capfd): ), "Derivative caching should not produce warnings" -class TestExpressionSilentUpdate: - """Test that expressions silently update when recreated with same name.""" - - def test_expression_updates_silently(self, capfd): - """ - Recreating an expression with the same name should silently update it. - - This is natural Python behavior - preserving object identity while - updating internal state. No warnings should be produced. - """ - # Create expression with initial value - alpha = UWexpression(r"\alpha", sym=1.0, description="First value") - - # Recreate with same name but different value - alpha2 = UWexpression(r"\alpha", sym=2.0, description="Second value") - - # Should be the SAME object (identity preserved) - assert alpha is alpha2, "Recreating expression should preserve object identity" - - # Should have updated sym value - assert alpha2.sym == 2.0, "Expression sym should be updated to new value" - - # Should have updated description - assert alpha2.description == "Second value", "Expression description should be updated" - - # Should NOT produce warnings - captured = capfd.readouterr() - assert ( - "Each expression should have a unique name" not in captured.err - ), "Expression update should be silent (no warnings)" - - def test_expression_update_in_loop(self): - """ - Updating expressions in loops should work naturally. - - This is a common pattern where expressions are recreated in each - iteration with updated values. - """ - values = [1.0, 2.0, 3.0, 4.0, 5.0] - - for i, val in enumerate(values): - eta = UWexpression(r"\eta", sym=val) - - # All iterations should return the same object - if i == 0: - first_eta = eta - else: - assert eta is first_eta, "Loop should reuse same expression object" - - # Value should be updated each iteration - assert eta.sym == val, f"Iteration {i}: expected sym={val}, got {eta.sym}" +class TestExpressionRedeclaration: + """Re-declaring a name must not silently change what the container holds. + + This class used to be ``TestExpressionSilentUpdate`` and asserted the + opposite: that recreating an expression with the same name silently updated + it, described as "natural Python behavior". That was a deliberate, + specified contract, so reversing it is a decision and not a bug fix. + + The reason it was reversed: identity is the NAME, so the second call does + not make a new container — it reaches the one every existing formula is + already written against, and changing its contents from a line that reads + like a declaration is invisible at the call site that suffers. Changing + what a container holds is what ``.sym =`` is for, and the loop pattern the + old test blessed is exactly the case that reads like a fresh object and is + not one. + """ + + def test_redeclaring_with_a_different_value_raises(self): + """The reversal. The container keeps what it had, and the error says + which of the two possible intentions the caller should write.""" + alpha = UWexpression(r"\alpha_{redecl}", sym=1.0, description="First value") + + with pytest.raises(ValueError, match="already exists"): + UWexpression(r"\alpha_{redecl}", sym=2.0, description="Second value") + + assert alpha.sym == 1.0, "a refused re-declaration still wrote the value" + assert alpha.description == "First value", ( + "a refused re-declaration still wrote the description") + + def test_redeclaring_with_the_same_value_is_idempotent(self): + """Declaring the same thing twice changes nothing and no formula can + tell, so it passes. A factory that rebuilds an unmutated problem in one + process relies on this.""" + beta = UWexpression(r"\beta_{redecl}", sym=3.0, description="value") + again = UWexpression(r"\beta_{redecl}", sym=3.0, description="value") + + assert beta is again + assert beta.sym == 3.0 + + def test_updating_in_a_loop_is_written_as_assignment(self): + """The loop pattern the old contract blessed, written the deliberate + way. One declaration, then assignment — which is what was happening + underneath all along, just not visibly.""" + eta = UWexpression(r"\eta_{loop}", sym=1.0) + + for val in (1.0, 2.0, 3.0, 4.0, 5.0): + eta.sym = val + assert eta.sym == val + + assert UWexpression(r"\eta_{loop}") is eta, "identity is still the name" def test_unique_flag_creates_new_objects(self): """