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 b119f98c2..d6fa57084 100644 --- a/src/underworld3/function/expressions.py +++ b/src/underworld3/function/expressions.py @@ -683,6 +683,49 @@ 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 _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. @@ -780,9 +823,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 @@ -931,6 +984,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, @@ -940,6 +1014,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 @@ -951,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. @@ -1992,6 +2075,40 @@ 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. + + 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) + + # 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 + + # ============================================================================ # Backward Compatibility Aliases # ============================================================================ diff --git a/src/underworld3/model.py b/src/underworld3/model.py index 06603c751..80221e415 100644 --- a/src/underworld3/model.py +++ b/src/underworld3/model.py @@ -5966,6 +5966,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/src/underworld3/systems/navier_stokes_eulerian.py b/src/underworld3/systems/navier_stokes_eulerian.py index 3e9715c90..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 @@ -473,8 +474,21 @@ def _stabilisation_flux(self): @property def F0(self): """Pointwise momentum residual: strong residual of the time scheme.""" - f0 = public_expression( - r"\mathbf{f}_0\left( \mathbf{u} \right)", + # 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. + # + # 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)", ) @@ -487,8 +501,9 @@ 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( - r"\mathbf{F}_1\left( \mathbf{u} \right)", + # 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(), "Navier-Stokes SUPG: viscous flux of the time scheme, pressure, tau R (x) a", diff --git a/tests/test_0007_snapshot_inmemory.py b/tests/test_0007_snapshot_inmemory.py index dc92b4f13..8b633bba8 100644 --- a/tests/test_0007_snapshot_inmemory.py +++ b/tests/test_0007_snapshot_inmemory.py @@ -919,15 +919,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_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" 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") 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 bd9dd67a7..27a0a6280 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 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): """