Skip to content

Refuse a re-declaration that would change what an expression container holds - #780

Open
lmoresi wants to merge 3 commits into
developmentfrom
bugfix/expression-redeclaration-explicit
Open

lmoresi wants to merge 3 commits into
developmentfrom
bugfix/expression-redeclaration-explicit

Conversation

@lmoresi

@lmoresi lmoresi commented Sep 22, 2026

Copy link
Copy Markdown
Member

Stacked on #775 — merge that first, then this retargets to development. Split out of #775 because it is a behaviour change users will notice, and it deserves its own review rather than riding along with a snapshot fix.

What was happening

An expression's identity is its name: uw.expression(r"\eta", ...) returns the existing container, which is what lets a formula written early keep seeing later edits to its contents. That much is deliberate.

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:

a = uw.expression(r'\eta', 1.0, 'first use')
b = uw.expression(r'\eta', 99.0, 'second, unrelated use')
a is b        # True
a.sym         # 99.0  — and the description is replaced too

The rule now

  • 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.
  • Different value → raises, naming the two things the caller might have meant (name.sym = value to change 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

Surveyed 576 tests (bands 00–03) with the refusal instrumented to record rather than raise:

re-declarations total 15
with the same value 6
with a different value 9, across 5 names
in library code 0

All nine were test factories rebuilding 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 — referencing the first build's mesh variable. The overwrite is what rebound it to the second build's variable, and nothing said so. That is the hazard in one line: not a stale value, a stale reference, invisible.

Those three factories now declare-or-assign through a small local helper, which makes the rebinding the visible act it always was:

def _param(name, value, description):
    try:
        return uw.expression(name, value, description)
    except ValueError:
        existing = uw.expression(name)
        existing.sym = value
        return existing

Tests

tests/test_00*|01*|02*|03*py: 576 passed. Two contract tests added — a name fetched twice is one container captured once, and re-declaring with a value raises while leaving the contents untouched.

Still open, not addressed here

_expr_names is a class attribute, so it outlives the model: after reset_default_model() a fresh model re-uses containers from the previous one, carrying their values until something sets them. Separate decision — clear the registry on reset, or namespace it per model.

Underworld development team with AI support from Claude Code

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
@lmoresi
lmoresi changed the base branch from bugfix/snapshot-captures-expressions to development September 23, 2026 13:33
…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
Copilot AI lite review requested due to automatic review settings September 23, 2026 17:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants