diff --git a/src/underworld3/function/expressions.py b/src/underworld3/function/expressions.py index 6ca0c29c..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. @@ -700,9 +721,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 +882,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 +912,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/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_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) 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): """