uw.expression: the name-interning path runs __init__ on the cached object and silently overwrites its value AND description

What

UWexpression.__new__ returns the existing object when the name is already registered
(expressions.py:679, "shared identity by name" — deliberate, and needed for cross-mesh
symbol disambiguation). But __init__ then runs on that cached object with the new
arguments, so the second construction overwrites the first's value:

a = uw.expression(r"\eta_{\min}", 0.01,  "first")
b = uw.expression(r"\eta_{\min}", 0.009, "second")
a is b      # True
a.sym       # 0.00900000000000000   <-- a's value changed under it

Why this is more than a surprise

The ordinary way to write a parameter study is a build() function that constructs its
expressions and returns a model. Call it twice in one process — a finite-difference check,
a parameter sweep, an inversion, a continuation ladder — and both models share one set of
parameter expressions. The first model's residual holds those same objects, so the
first model's physics silently changes when the second is built. Nothing warns.

Two concrete ways it bit a sensitivity driver today:

A reported parameter value was the wrong one. A run at floor = 0.01 reported
p_nd = 0.009, because the minus-step FD build had already reset the shared expression
by the time the table printed. The relative sensitivity was 10% wrong.
A finite-difference interval was silently wrong. Perturbing by reading the current
value and scaling it compounds across builds: the plus step sets p*(1+d), then the
minus step scales that by (1-d) and gets p*(1-d**2) instead of p*(1-d). The
difference is then taken over the wrong interval — and for small d it looks entirely
plausible.

Both are avoidable once you know (capture the baseline once, set absolute values), but the
failure mode is a plausible number rather than an error, and the knowledge is not
discoverable from the API.

Note

_unique_name_generation=True exists and gives an unshared expression, so the mechanism
for the other behaviour is already there — it is the default and the silence that are the
problem.

Options

Leave identity shared but make a conflicting value on re-construction an error, or at
least a warning naming both call sites. Re-constructing with the same value stays
silent, which covers the benign repeat case.
Or ignore the new value when returning a cached expression (identity wins, value is
set once) — quieter, but then a caller who thinks they set a value silently has not.

The first seems right: the current behaviour has both a silent read hazard and a silent
write hazard, and an error on conflict removes both.

Either way it wants documenting in
docs/developer/design/SYMBOL_DISAMBIGUATION_2025-12.md, which covers shared identity for
coordinates but not the value-overwrite consequence for parameters.

Correcting the framing of this issue: shared identity by name is intended and should stay. It is not a regression, and it is not the name-mangling that SYMBOL_DISAMBIGUATION_2025-12.md replaced.

Two separate mechanisms were being conflated:

SYMBOL_DISAMBIGUATION_2025-12.md removed the invisible \hspace{} name-mangling and replaced it with _hashable_content() / _uw_id. That work was scoped to MeshVariables and coordinates — v on mesh1 versus v on mesh2.
uw.expression's identity-by-name predates it, from 41665b79 (2025-05-07), whose message states the intent explicitly:
1. Symbols have unique names
2. Where name clashes occur, no expression is built unless
3. A unique identifier is explicitly requested
UW expressions are essentially immutable containers — only their contents can change but not the name or description.

_unique_name_generation=True is the documented escape hatch and works correctly: distinct objects, distinct _uw_id, c == a False.

So the defect is narrower than first reported, and it violates that commit's own contract

__new__ returns the cached object, and then Python calls __init__ on it with the new arguments:

a = uw.expression(r"\theta_q", 1.0, "FIRST description")
b = uw.expression(r"\theta_q", 2.0, "SECOND description")

a is b           # True   -- intended
a.sym            # 2.0                    <-- a's value silently overwritten
a.description    # 'SECOND description'   <-- a's DESCRIPTION silently overwritten

The description being overwritten is flatly against "not the name or description". The value being overwritten silently is what makes two build() calls share their parameters unnoticed, with the two consequences already described above (a reported p_nd of 0.009 for a run at 0.01, and a finite-difference interval of p*(1-d**2) instead of p*(1-d)).

Revised recommendation

Not "reconsider shared identity" — keep it. Instead: the interning path must not run __init__ on the object it returns. Either

ignore the new arguments entirely when returning a cached expression (identity wins, and the "immutable container" contract is honoured as written), or
raise on a conflicting value or description, staying silent when the arguments are identical — which keeps the benign repeat-construction case working while making the hazardous case loud.

The second is preferable: silently ignoring a value the caller passed is its own trap, just a quieter one.

Escalating this: the consequence is worse than the documentation I added, and it constrains how experiments can be structured, not just how values should be read.

Two fully independent models share their parameter expressions. Separate meshes, separate solvers, separate material fields — and one floor:

S1 = build(..., floor_scalar=1e-2)
S2 = build(..., floor_scalar=3e-1)

S1["stokes"] is not S2["stokes"]      # True
S1["mesh"]   is not S2["mesh"]        # True
S1["mat"]    is not S2["mat"]         # True
S1["floor_expr"] is S2["floor_expr"]  # True   <-- shared

S1["floor_expr"].sym   # 0.300000000000000   <-- S1 was built at 1e-2

Building S2 retroactively changed S1's physics. There is only ever one floor in the process, and S1's residual now contains S2's value.

Why this matters beyond "read your values early"

The guidance I added says to read reportable values before the next build and to perturb from a captured baseline. That is necessary but not sufficient. The stronger consequence:

An experiment that needs two models at different parameter values cannot be run in one process at all. Not a sequencing problem — the two models cannot coexist. Concretely, this defeated an attempt to hold two converged states at different viscosity floors in order to compare them: the second build silently redefined the first, and a probe that should have shown two distinct states reported them as identical to 1 part in 1e8.

Anything of this shape is affected: a resolution study that builds two models, a two-branch continuation, a bifurcation probe, an A/B comparison of two configurations, a regression harness constructing several models in one pytest session.

Suggested addition to whatever fix is chosen

Whether the resolution is "raise on a conflicting re-construction" or "ignore the new value", the docs need to state the structural rule plainly: one model per process when parameter values differ, unless every parameter is created with _unique_name_generation=True.

It is also worth asking whether a model-scoped registry is the right long-term answer — expressions interned per uw.Model rather than globally — so that two models in one process are genuinely independent. That would make the shared-identity behaviour (which is correct and load-bearing within a model) stop leaking between models.

Fixed by PR #780, which is open. Labelling fixed-in-PR.

Probed with this issue's own reproducer, run against #780's branch rather than
taken on the PR's description:

=== #767 reproducer ===
  REFUSED: expression '\eta_{\min}' already exists and holds 0
  a.sym unchanged: 0.0100000000000000   (#767: silently became 0.009)

=== #767's second mechanism: compounding FD interval ===
  REFUSED: the second build cannot silently rescale the first's value

Both halves of the report are covered. The second construction under an
existing name no longer runs __init__ on the interned object and overwrite
_sym; it raises instead, so a.sym keeps the 0.01 it was declared with
and the finite-difference interval cannot compound to p*(1-d^2) across two
builds.

#780 also adds tests/test_0027_expression_redeclaration_guard.py, which pins
the refusal and the one sanctioned refresh path, so this cannot regress
silently.

Relabel to fixed-in-development when #780 merges; the issue stays open until
development is released to main.

Review of 06c6db0: a refresh keeps the units and the description

Navigation Menu

Repository navigation

uw.expression: the name-interning path runs init on the cached object and silently overwrites its value AND description #767

What

Why this is more than a surprise

Note

Options

So the defect is narrower than first reported, and it violates that commit's own contract

Revised recommendation

Why this matters beyond "read your values early"

Suggested addition to whatever fix is chosen

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

uw.expression: the name-interning path runs __init__ on the cached object and silently overwrites its value AND description #767

Description

What

Why this is more than a surprise

Note

Options

Activity

lmoresi commented on Sep 20, 2026

So the defect is narrower than first reported, and it violates that commit's own contract

Revised recommendation

lmoresi commented on Sep 21, 2026

Why this matters beyond "read your values early"

Suggested addition to whatever fix is chosen

lmoresi commented on Oct 28, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

uw.expression: the name-interning path runs init on the cached object and silently overwrites its value AND description #767