Repository navigation
Conversation
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
…ion-redeclaration-explicit
…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
…ion-redeclaration-explicit
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E87Q7KrpapxeQiLD1RiNXv
The refusal found a real defect: two untagged solver template namesMerging That test builds two Navier-Stokes solvers back to back, one per τ shape.
r"\mathbf{f}_0\left( \mathbf{u} \right)"
r"\mathbf{F}_1\left( \mathbf{u} \right)"with no instance tag, where every peer in the same file tags itself — Fixed in Why only this solverThe sibling does not have the bug. expr._sym = updated_value # preserves object identitySo it never re-declares and never meets the refusal. Only TestsRebuilt from clean after merging
Underworld development team with AI support from Claude Code |
Brings the branch up to current development (100 commits), including the test_1079 checkpoint-layout fix (#848). Underworld development team with AI support from Claude Code
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
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
Adversarial review, and what the three failures actually wereRebased onto The three CI failures were all this guard firing, not pre-existing breakageAn earlier note on this branch said they reproduce on
The fix, and why it is narrow
Left open deliberately, recorded in The review broke our own commitFour defects in
(1) and (2) are silent-data bugs that no amount of reading found; the probe did. Evidence
Local regression after the review fixes, since Issue adjudication in this territory
|
Correction: the
|
|
CI is green: The three failures this branch carried are gone, and they were the three the Nothing outstanding on our side. The local evidence stands as reported: 238 One note for whenever this merges: it ran 80 minutes because the branch |
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:The rule now
name.sym = valueto change contents,uw.expression(name)to fetch). The existing contents are left untouched.Blast radius, measured before choosing
Surveyed 576 tests (bands 00–03) with the refusal instrumented to record rather than raise:
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 heldfrom 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:
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_namesis a class attribute, so it outlives the model: afterreset_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