Skip to content

Proxy mesh variables inherit the swarm variable's units (#439) - #450

Merged
lmoresi merged 1 commit into
developmentfrom
bugfix/proxy-units
Jul 28, 2026
Merged

Proxy mesh variables inherit the swarm variable's units (#439)#450
lmoresi merged 1 commit into
developmentfrom
bugfix/proxy-units

Conversation

@lmoresi

@lmoresi lmoresi commented Jul 28, 2026

Copy link
Copy Markdown
Member

Fixes #439.

The defect

SwarmVariable._create_proxy_variable did not forward units= to the proxy MeshVariable. Since var.sym resolves to the proxy, reading a proxied variable through the symbolic path returned the non-dimensional number with no units attached, presented as though it were the answer.

Measured, with density=3300 kg/m³ as the reference quantity and a stored non-dimensional value of 1.0:

before after
var.units kg/m³ kg/m³
var._meshVar.units None kg/m³
var.array 3300.0 3300.0
var._meshVar.array 1.0 3300.0
evaluate(var.sym) ndarray 1.0, no units UnitAwareArray 3300.0 kg/m³

The two rows in bold are the bug: the swarm variable and its own proxy disagreed about the value, and evaluate handed back the raw non-dimensional number silently.

Stored data stays non-dimensional on both sides — only what the proxy advertises changes, so the transfer itself is untouched (asserted by test).

Context

This is the half of #426 that #434 deliberately left out. It was split off at the time because attaching units to the proxy makes var.sym unit-aware everywhere proxies are composed into expressions, which is a units-boundary decision rather than a coordinate fix. Maintainer decision was to forward the units and measure what moves.

Blast radius: none

  • level_1 tier_a/tier_b: 571 passed (567 baseline + the 4 new tests), 0 failed
  • level_2 tier_a/tier_b: 319 passed, 0 failed
  • np=2 parallel swarm/proxy: 4 passed

No existing test both declares units on a swarm variable and reads it symbolically. That is why the change moves nothing — and also why the defect survived. Worth recording as a coverage gap in its own right, not just as a convenient result.

Tests added

To tests/test_0116_swarm_proxy_units.py (created by #434):

  • proxy .units equals the variable's .units
  • .array on both sides reaches the same dimensional number, while .data stays non-dimensional on both
  • evaluate(var.sym) returns a UnitAwareArray with the right units and value — the silent-wrong-value case
  • a variable declaring no units keeps a dimensionless proxy

Underworld development team with AI support from Claude Code

`SwarmVariable._create_proxy_variable` did not forward `units=` to the
proxy MeshVariable. Since `var.sym` resolves to the proxy, reading a
proxied variable through the symbolic path returned the NON-DIMENSIONAL
number with no units attached, presented as though it were the answer.

Measured before, with density=3300 kg/m^3 as the reference quantity and a
stored non-dimensional value of 1.0:

    var.units          kilogram / meter ** 3
    var._meshVar.units None
    var.array          3300.0
    var._meshVar.array 1.0                      <- disagree
    evaluate(var.sym)  ndarray 1.0, no units    <- silently wrong

after:

    var._meshVar.units kilogram / meter ** 3
    var.array          3300.0
    var._meshVar.array 3300.0
    evaluate(var.sym)  UnitAwareArray 3300.0 kg/m^3

Stored data stays non-dimensional on both sides; only what the proxy
advertises changes, so the transfer itself is untouched.

This is the half of #426 that PR #434 deliberately left out, split off at
the time as #439 because it needed a units-boundary decision rather than a
coordinate fix. Maintainer decision was to forward the units and measure
the blast radius.

Blast radius: none. level_1 571 passed (567 + the 4 new tests), level_2
319 passed, np=2 parallel 4 passed, zero failures. No existing test both
declares units on a swarm variable and reads it symbolically -- which is
also why the defect survived, and is itself worth recording.

Variables that declare no units keep a dimensionless proxy, asserted by
test.

Underworld development team with AI support from Claude Code
Copilot AI review requested due to automatic review settings July 28, 2026 01:08

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.

@lmoresi

lmoresi commented Jul 28, 2026

Copy link
Copy Markdown
Member Author

Adversarial review

"Blast radius: none" is a weaker result than it reads

571/319/4 passing means no existing test exercises the changed path at all, not that the change is safe. The condition to trip it is: a swarm variable that declares units=, with a proxy, read symbolically, under an active units model. Nothing in the suite does all four.

So the honest claim is "nothing regressed that is tested". Anything downstream that composed a proxied symbol and relied on it being dimensionless — arithmetic against a bare float, a constitutive term assembled without unit checking — now sees a unit-aware operand. The suite cannot tell us whether such code exists, and the docs/examples tree is not run by CI.

The place I would expect trouble is multi-material constitutive assembly, where a density proxy multiplies a level-set fraction. IndexSwarmVariable level sets carry no units, so that product is now unit-aware x dimensionless. That composes fine in principle; it is not exercised.

The type of _units being forwarded is not pinned

self._units is a pint.Unit by the time it reaches the constructor, not the string the user passed. MeshVariable accepts both — but the test asserts proxy.units == var.units, which would pass even if both were wrong in the same way. A test pinning the string ("kilogram / meter ** 3") exists only in the evaluate test, indirectly.

The vector/tensor case is untested

Every new test uses size=1. A vector swarm variable with units gets a proxy with self.shape components and one units= for all of them, which is presumably right, but "presumably" is doing work there. Same for dtype=int variables, where units are meaningless and now get forwarded anyway if declared.

What holds up

  • The before/after is measured on both sides of the change rather than asserted, including the .array disagreement that is the concrete symptom.
  • The dimensionless case is pinned, so the change cannot silently start attaching units to variables that never asked for them.
  • The fix is one keyword at the single site that constructs the proxy; there is no second path to keep in step.

@lmoresi
lmoresi merged commit 6cd75a6 into development Jul 28, 2026
2 checks passed
@lmoresi
lmoresi deleted the bugfix/proxy-units branch July 28, 2026 01:28
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