From c64d83cdc29639e343a340aa0d66fb3d64c0a01c Mon Sep 17 00:00:00 2001 From: lmoresi Date: Tue, 22 Sep 2026 09:57:31 +1000 Subject: [PATCH 1/5] 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 6ca0c29c1..c66cf9245 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 68af29ed4..d4096be43 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 9b16d09e9..cb1209ea1 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 b170d31fb..670027154 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 5691da513..3c1b087a6 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/5] 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 c66cf9245..bd55e9cd4 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 1a5619ae8..b33061ea3 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 81fc35b5b..6a2290033 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): """ From fa9da96414912c040d31c0ada4527ac9418fd7f9 Mon Sep 17 00:00:00 2001 From: lmoresi Date: Mon, 12 Oct 2026 16:01:36 -0700 Subject: [PATCH 3/5] Tag the Navier-Stokes residual templates with their instance Merging development turned this branch's refusal red on test_1056_navier_stokes_supg_api::test_tau_shapes_construct_and_step, which builds two Navier-Stokes solvers back to back, one per tau shape. The second construction raised: expression '\mathbf{f}_0\left( \mathbf{u} \right)' already exists and holds ... The refusal is right and the defect is the name. F0 and F1 declared '\mathbf{f}_0\left( \mathbf{u} \right)' and '\mathbf{F}_1\left( \mathbf{u} \right)' with no instance tag, where every peer in the same file tags: \rho_{tag}, \mathbf{a}_{tag}, \mathbf{u}^{n-1}_{tag}. A UWexpression's identity is its NAME, so one container was shared by every solver of the class, and a second solver rebound it to its own instance's symbols under any formula already written against the first solver's residual. Before this branch that happened silently; the refusal is what made it visible. Both names now carry self.instance_number. The sibling solver does not have this: advection_diffusion_eulerian declares F0/F1 through the Template descriptor, whose __get__ updates .sym IN PLACE on an existing container and so never re-declares. Only this solver hand-rolls the declaration in a property, which is why only it collided. Converting it to Template is the larger tidy-up and is left alone here -- #800 rewrites much of this file. test_1056 7 passed; with test_0610, test_0016 and test_0017, 24 passed. Underworld development team with AI support from Claude Code Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01E87Q7KrpapxeQiLD1RiNXv --- src/underworld3/systems/navier_stokes_eulerian.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/underworld3/systems/navier_stokes_eulerian.py b/src/underworld3/systems/navier_stokes_eulerian.py index 3e9715c90..98bbe217b 100644 --- a/src/underworld3/systems/navier_stokes_eulerian.py +++ b/src/underworld3/systems/navier_stokes_eulerian.py @@ -473,8 +473,14 @@ def _stabilisation_flux(self): @property def F0(self): """Pointwise momentum residual: strong residual of the time scheme.""" + # Tagged with the instance number, as \rho, \mathbf{a} and + # \mathbf{u}^{n-1} are above. A UWexpression's identity is its NAME, so + # an untagged name is ONE container shared by every solver of this class: + # a second Navier-Stokes solver in the same process re-declared it with + # its own instance's symbols, which silently rebound the name under any + # formula already written against the first solver's residual. f0 = public_expression( - r"\mathbf{f}_0\left( \mathbf{u} \right)", + rf"\mathbf{{f}}_0\left( \mathbf{{u}}_{{{self.instance_number}}} \right)", self._strong_residual(), "Navier-Stokes SUPG: strong residual (time derivative, advection, body force)", ) @@ -488,7 +494,7 @@ def F1(self): mechanical_pressure = ( self.p.sym[0] - self.penalty * self.constitutive_model.K * self.div_u) F1 = public_expression( - r"\mathbf{F}_1\left( \mathbf{u} \right)", + rf"\mathbf{{F}}_1\left( \mathbf{{u}}_{{{self.instance_number}}} \right)", self._viscous_flux() + self._devss_flux() - sympy.eye(dim) * mechanical_pressure + self._stabilisation_flux(), "Navier-Stokes SUPG: viscous flux of the time scheme, pressure, tau R (x) a", From 06c6db0a9cafb8749bc3e7033fdc983c8719c4fb Mon Sep 17 00:00:00 2001 From: lmoresi Date: Fri, 23 Oct 2026 21:33:35 +1100 Subject: [PATCH 4/5] A solver refreshing its own template says so, rather than re-declaring The re-declaration guard refuses a second construction whose contents differ, which is right for the case it was written for: two unrelated owners reaching the same name. But a solver's F0 and F1 are templates, re-derived on every access from the parameters as they stand, so their contents legitimately move as \Delta t and the Adams-Moulton coefficients do. Construction was doing that work: __init__ writes _sym unconditionally, so a second call silently updated the container in place, and these properties have always relied on it. template_expression() makes that explicit -- it writes through .sym, the sanctioned path for changing what a container holds, and declares only when the container is absent. Behaviour at these sites is what it was; what changes is that the intent is now stated, and the guard stays strict for every caller where a repeated name is an accident. test_0024 built the same Poisson problem on two meshes under one set of names, so g_{0024f} genuinely held different contents the second time. It now frees the names between the two builds. Only lookup resets -- _expr_count keeps counting, so the shifted-counter premise the test turns on is untouched, and the first problem's live objects are unaffected. tests/test_0024_jit_graph_lowering.py and tests/test_1059_stress_transport.py: 44 passed, where the two files gave 3 failed / 26 passed before. Leaves open: every F0/F1 name in solvers.py is untagged, and \mathbf{F}_1\left( \mathbf{u} \right) is declared by four classes (SNES_Tensor_Projection, SNES_AdvectionDiffusion, SNES_Diffusion, SNES_NavierStokes), f_0 \left( \mathbf{u} \right) by three. That is one container shared between solvers, which is the hazard this guard exists to catch, and it wants its own branch and a probe. Underworld development team with AI support from Claude Code --- src/underworld3/function/__init__.py | 1 + src/underworld3/function/expressions.py | 26 +++++++++++++++++++ .../systems/navier_stokes_eulerian.py | 13 ++++++++-- tests/test_0024_jit_graph_lowering.py | 10 +++++++ 4 files changed, 48 insertions(+), 2 deletions(-) diff --git a/src/underworld3/function/__init__.py b/src/underworld3/function/__init__.py index eacf0f356..5ce8034e4 100644 --- a/src/underworld3/function/__init__.py +++ b/src/underworld3/function/__init__.py @@ -64,6 +64,7 @@ from .expressions import UWexpression as expression +from .expressions import template_expression from .expressions import UWDerivativeExpression as _derivative_expression from .quantities import quantity, UWQuantity from .expressions import substitute as fn_substitute_expressions diff --git a/src/underworld3/function/expressions.py b/src/underworld3/function/expressions.py index 0b8937256..b4911b84b 100644 --- a/src/underworld3/function/expressions.py +++ b/src/underworld3/function/expressions.py @@ -2065,6 +2065,32 @@ def unpack_var_fns(exp): return mesh, varfns, derivfns +def template_expression(name, sym, description="No description provided", units=None): + """Declare a derived quantity, or refresh what it already holds. + + A solver's ``F0``/``F1`` are templates: each access re-derives them from the + parameters as they stand now, so the second access within a step carries a + different ``\\Delta t`` and different Adams-Moulton coefficients from the + first. Construction is the wrong verb for that. A plain + ``expression(name, ...)`` call reads as a declaration, and a declaration + that silently replaced the contents is exactly what the re-declaration + guard refuses -- rightly, because for any other caller a repeated name is + an accident. + + So the owner says what it means: write through ``.sym``, the sanctioned + path for changing what a container holds, and declare only when the + container does not exist yet. Formulae already written against the name go + on seeing the refreshed contents, which is the container contract and the + behaviour these properties have always relied on. + """ + existing = UWexpression._expr_names.get(name) + if existing is None: + return UWexpression(name, sym, description, units=units) + + existing.sym = sym + return existing + + # ============================================================================ # Backward Compatibility Aliases # ============================================================================ diff --git a/src/underworld3/systems/navier_stokes_eulerian.py b/src/underworld3/systems/navier_stokes_eulerian.py index 98bbe217b..25c553c84 100644 --- a/src/underworld3/systems/navier_stokes_eulerian.py +++ b/src/underworld3/systems/navier_stokes_eulerian.py @@ -39,6 +39,7 @@ from underworld3.systems.ddt import _step_ratio import underworld3.timing as timing from underworld3.function import expression as public_expression +from underworld3.function import template_expression from underworld3.systems.ddt import _DDtBase from underworld3.systems.ddt import EulerianSUPG as EulerianSUPG_DDt from underworld3.systems.advection_diffusion_eulerian import _check_supplied_manager @@ -479,7 +480,14 @@ def F0(self): # a second Navier-Stokes solver in the same process re-declared it with # its own instance's symbols, which silently rebound the name under any # formula already written against the first solver's residual. - f0 = public_expression( + # + # A template, not a declaration: each access re-derives the residual + # from the parameters as they stand, so the contents legitimately + # change from one access to the next as \Delta t and the Adams-Moulton + # coefficients move. template_expression writes through .sym, which is + # the sanctioned path for that; constructing again would read as a + # declaration and is what the re-declaration guard refuses. + f0 = template_expression( rf"\mathbf{{f}}_0\left( \mathbf{{u}}_{{{self.instance_number}}} \right)", self._strong_residual(), "Navier-Stokes SUPG: strong residual (time derivative, advection, body force)", @@ -493,7 +501,8 @@ def F1(self): dim = self.mesh.dim mechanical_pressure = ( self.p.sym[0] - self.penalty * self.constitutive_model.K * self.div_u) - F1 = public_expression( + # A template, refreshed on every access -- see F0. + F1 = template_expression( rf"\mathbf{{F}}_1\left( \mathbf{{u}}_{{{self.instance_number}}} \right)", self._viscous_flux() + self._devss_flux() - sympy.eye(dim) * mechanical_pressure + self._stabilisation_flux(), diff --git a/tests/test_0024_jit_graph_lowering.py b/tests/test_0024_jit_graph_lowering.py index e289a95dd..5b54165d2 100644 --- a/tests/test_0024_jit_graph_lowering.py +++ b/tests/test_0024_jit_graph_lowering.py @@ -14,6 +14,7 @@ from sympy.vector.scalar import BaseScalar import underworld3 as uw +from underworld3.function.expressions import reset_expression_registry import underworld3.function.expressions as ex from underworld3.function import diff_wrt_field from underworld3.utilities import _jit_graph as jg @@ -268,6 +269,15 @@ def test_the_emitted_source_does_not_depend_on_what_came_before(monkeypatch): uw.discretisation.MeshVariable("J0024f", other, 1, degree=1) mesh2 = uw.meshing.UnstructuredSimplexBox( minCoords=(0.0, 0.0), maxCoords=(1.0, 1.0), cellSize=0.5) + # A second, independent problem that happens to reuse the names: _poisson + # declares k_{0024f} and g_{0024f} against whichever mesh it is given, and + # g's contents name that mesh's variable, so on mesh2 they are genuinely + # different contents under the same name. Freeing the names says that + # outright, rather than leaning on a re-declaration quietly overwriting + # them. Only LOOKUP resets -- _expr_count keeps counting, so the counter + # shift this test turns on is untouched, and the first problem's live + # objects hold their containers directly and are unaffected. + reset_expression_registry() second = _header(_poisson("0024f", mesh2), monkeypatch) assert first == second assert "uwt_0" in first, "the kernel has no temporaries: nothing was lowered" From 927133cbbec59219f294d048edaaa98a6439b74b Mon Sep 17 00:00:00 2001 From: lmoresi Date: Sat, 24 Oct 2026 09:17:48 +1100 Subject: [PATCH 5/5] Review of 06c6db0a: a refresh keeps the units and the description Our own adversarial pass on the previous commit, probed rather than read. A refresh dropped the units the container was declared with. Writing straight to .sym does not wrap, and only __init__ did: template_expression turned 1.0 [metre] into a bare 2.0 with .units None, silently, since nothing downstream asks whether a length is still a length. The wrapping is now _wrap_with_units(), one definition called by both construction and refresh -- a second copy is how the two drift apart again. A refresh also discarded the description it was given, keeping the first wording. describe() and the transcript print that string, so they would have quoted a stale line. template_expression cannot detect the case it is unsafe for: two owners passing one untagged name silently rebind each other, which is the bug the guard refuses. Stated as a precondition on the function, with the remedy (tag the name with the owner). tests/test_0027_expression_redeclaration_guard.py covers the guard's own contract, which nothing did -- the refusal, the allowed identical re-declaration, the in-place refresh, the units, the description, and what a registry reset does and does not touch. level_1/tier_b, inside the test_00[0-4]*py batch glob. 7 passed; the units case was shown failing on the previous commit first. Not fixed, recorded: template_expression consults only the persistent registry, so a name held ephemerally would get a persistent twin. Unreachable today -- templates are never ephemeral. Adjudicated: #767 (the name-interning path overwrites the cached object's value and description) is fixed by this branch, on its own reproducer -- both mechanisms refused, a.sym stays 0.01. Wants the fixed-in-PR label. Still live and adjacent: #843, #771, #641, #765, #760. Underworld development team with AI support from Claude Code --- src/underworld3/function/expressions.py | 46 +++++-- ...est_0027_expression_redeclaration_guard.py | 125 ++++++++++++++++++ 2 files changed, 157 insertions(+), 14 deletions(-) create mode 100644 tests/test_0027_expression_redeclaration_guard.py diff --git a/src/underworld3/function/expressions.py b/src/underworld3/function/expressions.py index b4911b84b..d6fa57084 100644 --- a/src/underworld3/function/expressions.py +++ b/src/underworld3/function/expressions.py @@ -704,6 +704,28 @@ def reset_expression_registry(): UWexpression._ephemeral_expr_names.clear() +def _wrap_with_units(sym, units): + """Wrap a plain number in a UWQuantity, leaving anything symbolic alone. + + One definition, because two callers need it: construction, and + :func:`template_expression` refreshing a container that already exists. A + second copy of this is how a refresh ends up dropping the units that + construction applied. + """ + if units is None or isinstance(sym, UWQuantity): + return sym + if isinstance(sym, (int, float, np.integer, np.floating)): + return UWQuantity(sym, units) + if sym is not None and not isinstance(sym, (sympy.Basic, UWexpression)): + # numeric-like, e.g. a numpy scalar; anything that will not convert + # keeps the units parameter unapplied rather than failing the call + try: + return UWQuantity(float(sym), units) + except (TypeError, ValueError): + pass + return sym + + def live_expressions(): """The persistent expression containers, in a stable order. @@ -1024,19 +1046,7 @@ def __init__( if value is not None and sym is not None: raise ValueError("Both 'sym' and 'value' provided - use only one") - # If units are provided and sym is a plain numeric value, wrap it in UWQuantity - # Don't wrap if sym is already a UWQuantity, UWexpression, or SymPy expression - if units is not None and not isinstance(sym, UWQuantity): - # Only wrap plain numeric values - not expressions or other complex types - if isinstance(sym, (int, float, np.integer, np.floating)): - sym = UWQuantity(sym, units) - elif sym is not None and not isinstance(sym, (sympy.Basic, UWexpression)): - # Try to wrap other numeric-like things (e.g., numpy scalars) - try: - sym = UWQuantity(float(sym), units) - except (TypeError, ValueError): - # Can't convert to float - ignore units parameter - pass + sym = _wrap_with_units(sym, units) # TRANSPARENT CONTAINER PRINCIPLE (2025-11-27): # Store the wrapped object directly - don't extract or decompose it. @@ -2082,12 +2092,20 @@ def template_expression(name, sym, description="No description provided", units= container does not exist yet. Formulae already written against the name go on seeing the refreshed contents, which is the container contract and the behaviour these properties have always relied on. + + The name must be unique to the owner. This function does deliberately what + the guard refuses, so a name two owners both reach is silently rebound by + it, which is the bug the guard exists to catch -- tag the name with the + owner (``self.instance_number``) and it cannot happen. """ existing = UWexpression._expr_names.get(name) if existing is None: return UWexpression(name, sym, description, units=units) - existing.sym = sym + # Through the same wrapping construction uses: a refresh that skipped it + # would quietly strip the units the container was declared with. + existing.sym = _wrap_with_units(sym, units) + existing._description = description return existing diff --git a/tests/test_0027_expression_redeclaration_guard.py b/tests/test_0027_expression_redeclaration_guard.py new file mode 100644 index 000000000..2437cc7cd --- /dev/null +++ b/tests/test_0027_expression_redeclaration_guard.py @@ -0,0 +1,125 @@ +#!/usr/bin/env python3 +"""The re-declaration guard, and the one path that is allowed to refresh. + +An expression container's identity is its NAME, so a second construction under +a name reaches the object the first one made. Construction used to write the +contents anyway, which meant a line that reads like a declaration could rebind +a name two unrelated owners both used, under every formula already written +against it. The guard refuses that. + +A solver's own templates are the case that must still work: ``F0`` and ``F1`` +are re-derived on every access, so their contents move as the timestep and the +Adams-Moulton coefficients do. ``template_expression`` is the path that says +so, and these tests pin both halves -- the refusal and the refresh -- because +weakening either one restores a silent overwrite. +""" + +import numpy as np +import pytest +import sympy + +import underworld3 as uw +from underworld3.function.expressions import ( + UWexpression, + reset_expression_registry, + template_expression, +) + +pytestmark = [pytest.mark.level_1, pytest.mark.tier_b] + + +@pytest.fixture(autouse=True) +def _clean_registry(): + """Names are process-wide, so a test that leaves one behind fails its neighbour.""" + reset_expression_registry() + yield + reset_expression_registry() + + +def test_a_second_declaration_with_different_contents_is_refused(): + """The guard's whole point: two owners reaching one name must not pass quietly.""" + UWexpression(r"guard_0027_a", sympy.Symbol("first"), "owner one") + with pytest.raises(ValueError, match="already exists and holds"): + UWexpression(r"guard_0027_a", sympy.Symbol("second"), "owner two") + + +def test_a_second_declaration_of_the_same_contents_is_allowed(): + """A factory rebuilding an unmutated problem changes nothing, and no formula + written against the name can tell. Refusing it would be noise.""" + first = UWexpression(r"guard_0027_b", sympy.Symbol("same"), "owner") + again = UWexpression(r"guard_0027_b", sympy.Symbol("same"), "owner") + assert again is first + assert again.sym == sympy.Symbol("same") + + +def test_a_template_refresh_updates_the_container_in_place(): + """What the solver properties rely on: the same object, new contents, so a + formula already written against the name sees the refreshed value.""" + first = template_expression(r"guard_0027_c", sympy.Symbol("dt_1"), "a template") + formula = 2 * first + + second = template_expression(r"guard_0027_c", sympy.Symbol("dt_2"), "a template") + + assert second is first, "a refresh must not make a second container" + assert first.sym == sympy.Symbol("dt_2") + # the formula holds the container, not a copy of its contents + assert sympy.Symbol("dt_2") in formula.subs(first, first.sym).atoms(sympy.Symbol) + + +def test_a_template_refresh_keeps_the_units_it_was_declared_with(): + """A refresh goes through the same wrapping construction uses. + + Writing straight to ``.sym`` does not wrap, so a refresh that skipped the + wrapping replaced ``1.0 [metre]`` with a bare ``2.0`` and the container + lost the units it was declared with -- silently, since nothing downstream + asks whether a length is still a length. + """ + declared = template_expression(r"guard_0027_d", 1.0, "a length", units="m") + assert declared.units is not None + + refreshed = template_expression(r"guard_0027_d", 2.0, "a length", units="m") + + assert refreshed is declared + assert refreshed.units is not None, "the refresh dropped the units" + assert str(refreshed.units) == str(declared.units) + + +def test_a_template_refresh_carries_the_description_it_was_given(): + """The description is what ``describe()`` and the transcript print, so a + refresh that kept the first one would make them quote a stale line.""" + template_expression(r"guard_0027_e", sympy.Symbol("q"), "first wording") + refreshed = template_expression(r"guard_0027_e", sympy.Symbol("r"), "second wording") + assert refreshed._description == "second wording" + + +def test_resetting_the_registry_frees_lookup_and_leaves_live_objects_alone(): + """``_expr_names`` belongs to the process, not to a model. + + A reset is about LOOKUP: the next declaration under a freed name builds a + fresh container. Objects already holding a container keep working, and the + creation counter keeps counting, so a reset cannot be used to make two runs + look alike by rewinding the numbering. + """ + held = UWexpression(r"guard_0027_f", sympy.Symbol("kept"), "held by a formula") + formula = held + 1 + count_before = UWexpression._expr_count + + reset_expression_registry() + + assert r"guard_0027_f" not in UWexpression._expr_names + assert held.sym == sympy.Symbol("kept"), "a live container must be untouched" + assert formula.subs(held, held.sym) == sympy.Symbol("kept") + 1 + assert UWexpression._expr_count >= count_before, "the counter must not rewind" + + # the freed name is now available, and declaring it is not a collision + fresh = UWexpression(r"guard_0027_f", sympy.Symbol("new"), "a different owner") + assert fresh is not held + assert held.sym == sympy.Symbol("kept") + + +def test_the_guard_does_not_fire_on_a_fetch_with_no_value(): + """Asking for a container by name is a lookup, not a declaration.""" + made = UWexpression(r"guard_0027_g", sympy.Symbol("v"), "owner") + fetched = uw.expression(r"guard_0027_g") + assert fetched is made + assert fetched.sym == sympy.Symbol("v")