Capture expression contents in the snapshot, so parameters rewind with the fields by lmoresi · Pull Request #775 · underworldcode/underworld3

lmoresi · 2026-09-21T20:56:28Z

Fixes #774.

snapshot() captured each registered mesh's coordinates and mesh-variable DOFs, each swarm's particles, and every registered state-bearer. A uw.expression is none of those, so it was not captured at all.

A restore put the fields back and left every parameter wherever the run had since moved it. Nothing raised. A replayed step solved a different problem from the one the transcript recorded.

after rewind to step 1:   kappa=7.0 (should be 1.0)    T=1.0 (correct)

Ramping a parameter between steps (kappa.sym = 7.0) is the ordinary way to write such a run, so this is not an exotic path.

Scope is wider than `rewind()`

checkpoint/snapshot.py is on development; only rewind() is confined to the timestepping branch. Anyone calling model.save_state() / model.load_state() today has the same gap — the field half of the state round-trips and the parameter half does not.

It also reaches the discrete adjoint, whose driver restores a step's snapshot and replays the solve. Replaying at the wrong parameter values gives a gradient for a problem the run never solved — and a Taylor test would not catch it, because both sides move together.

Two details that decide whether this is correct

Stored by reference, not deep-copied. _sym is a sympy object and sympy objects are immutable, so a later sym = assignment replaces rather than mutates, and the reference cannot go stale. Deep-copying would be actively wrong here: an expression's _sym can carry mesh-variable symbols, and cloning that graph detaches the restored parameter from the live mesh. A test asserts the restored value keeps its coordinate symbols.

Missing on either side is quiet, not fatal. Captured-but-collected has nothing to write to; alive-but-not-captured was created after the snapshot, so the snapshot has no opinion about it. That differs from the state-bearer rule immediately above, which raises — a missing state-bearer means the snapshot came from a different Model, whereas expressions are made and dropped freely during a normal run.

Tests

Four added to tests/test_0007_snapshot_inmemory.py:

a field and a parameter are both scribbled and both come back — the field half is asserted alongside so a regression that breaks both cannot pass by breaking them symmetrically;
a symbolic value (not just a float) round-trips, keeping its coordinate symbols;
an expression created after the snapshot is left alone;
capture covers expressions made anywhere, including the ones UW3 builds for itself.

Negative control: with the restore disabled, the two contract tests fail and the other 25 pass — so they discriminate rather than passing by construction.

tests/test_0007_snapshot_inmemory.py 27 passed; tests/test_00[0-4]*py 145 passed.

Capture reads the existing container registry

An earlier revision of this PR added a WeakSet of every UWexpression, and described the strong hold on expressions as a leak. Both were wrong, and the second commit corrects them.

UWexpression._expr_names is the definition of a UW expression: a container with identity by name, looked up rather than rebuilt, so uw.expression(r"\eta", ...) reaches the same object twice and a formula written early keeps seeing later edits to its contents. The strong hold is the design. That registry is exactly the set whose contents a snapshot must capture, so capture now reads it directly instead of maintaining a parallel one.

_ephemeral_expr_names is deliberately excluded — the _unique_name_generation=True expressions built for derivative lowering and template substitution. Their contents are derived, they are held weakly and keyed (name, uw_id), and they are rebuilt from the persistent ones; restoring them would write over a value the machinery is about to recompute.

Underworld development team with AI support from Claude Code

Scope note: the expression re-declaration rule that was briefly on this branch has been split out to #780. This PR is now only the snapshot fix: capture and restore expression contents. #780 stacks on it.

…d with the fields (#774) snapshot() captured each mesh's coordinates and mesh-variable DOFs, each swarm's particles, and every registered state-bearer. A uw.expression is none of those, so it was not captured at all. A restore therefore put the FIELDS back and left every parameter wherever the run had since moved it, with nothing raised. A replayed step then solved a different problem from the one the transcript recorded. Ramping a parameter between steps - kappa.sym = 7.0 - is the ordinary way to write such a run, so this was not an exotic path: after rewind to step 1: kappa=7.0 (should be 1.0) T=1.0 (correct) Every live expression's contents are now captured alongside the fields and put back on restore. Two details that decide whether this is correct: * stored by REFERENCE, not deep-copied. _sym is a sympy object and sympy objects are immutable, so a later `sym =` assignment REPLACES rather than mutates and the reference cannot go stale. Deep-copying would be actively wrong: an expression's _sym can carry mesh-variable symbols, and cloning that graph detaches the restored parameter from the live mesh. A test asserts the restored value keeps its coordinate symbols. * missing on either side is quiet, not fatal. Captured-but-collected has nothing to write to; alive-but-not-captured was created after the snapshot, so the snapshot has no opinion about it. That differs from the state-bearer rule directly above, which raises - a missing state-bearer means the snapshot came from a different Model, whereas expressions are made and dropped freely during a normal run. Scope is wider than rewind(): checkpoint/snapshot.py is on development and only rewind() is confined to the timestepping branch, so save_state()/load_state() users have the same gap today. It also reaches the discrete adjoint, whose driver restores a step's snapshot and replays the solve - replaying at the wrong parameter values gives a gradient for a problem the run never solved, and a Taylor test would not catch it because both sides move together. Verified by negative control: with the restore disabled, the two contract tests fail and the other 25 pass. Noted, not fixed: expressions are already held STRONGLY by a name-keyed cache elsewhere in UW3, so dropping the last user reference does not collect one. The capture registry is a WeakSet and so adds no lifetime, but it is not what bounds the set today - the docstring says so rather than claiming a weakness the code does not currently have. Worth its own issue. tests/test_0007_snapshot_inmemory.py 27 passed; tests/test_00[0-4]*py 145 passed. Underworld development team with AI support from Claude Code

Copilot

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

The first version added a WeakSet of every UWexpression. That was a second registry for a job the first one already does, and it described expression lifetime wrongly in the bargain. UWexpression._expr_names IS the definition of a UW expression: a container with identity by NAME, looked up rather than rebuilt, so uw.expression(r"\eta", ...) reaches the same object twice and a formula written early keeps seeing later edits to its contents. The strong hold is the point, not a leak. That registry is exactly the set whose contents a snapshot must capture - the parameters of the run - so capture now reads it directly. _ephemeral_expr_names is deliberately excluded. Those are the _unique_name_generation=True expressions built for derivative lowering and template substitution: contents derived, held weakly, keyed (name, uw_id), and rebuilt from the persistent ones. Restoring them would write over a value the machinery is about to recompute. Two tests follow the corrected model: capture reads the container registry and skips the ephemerals, and a name reused twice is ONE container captured ONCE - which is the identity rule the design rests on. tests/test_0007_snapshot_inmemory.py 28 passed; tests/test_00[0-4]*py 146 passed. Underworld development team with AI support from Claude Code

…nscripts #697 and #775 landed while this branch was open. expressions.py and test_0007 conflicted because this branch carried a SUPERSEDED draft of the expression-capture fix: it was developed in this worktree (rewind lives here, so the round trip could be exercised end to end) and then ported to a branch off development, where review replaced the parallel WeakSet with a read of UWexpression._expr_names - the registry that already defines the container. The draft was never taken back out here, and `git add -A` on the Charter commit swept it in. Resolved by taking development's side wholesale: the reviewed version is the one that merged as #775. The same `git add -A` also committed run OUTPUT - two transcripts/ directories written by the repro script while the bug was being reproduced, with their launch.json and transcript.jsonl. Removed, and .gitignore now covers transcripts/ anywhere rather than only under docs/examples: a run writes its transcript beside itself, so ANY directory a script is run from collects one. tests/test_00[0-4]*py 254 passed; deprecated-pattern scan clean (87 allowlisted, down from 88 - the removed scratch file took one with it). Underworld development team with AI support from Claude Code

…s, and the expression snapshot 64 commits of development — #697, #716, #753 and #775 among them. Two conflicts, both the same shape as #716's, because this branch forked from the timestepping branch before that merge happened. swarm.py — kept BOTH sides. Two unrelated additions land at the same point: this branch's _adjoint_support (the particle-set rule: an advection is adjointable exactly when the count is unchanged across it) and development's _characteristics_for. Neither supersedes the other. CLAUDE.md — took development's trimmed version. This branch still carried the pre-#725 file, so taking its side wholesale would have reverted a 22.9 KB -> 8.3 KB cut. The one ruling it genuinely adds — the consistent_jacobian tangent policy, and that the rotated constraint is transparent to it — is carried over, rewritten in the trimmed file's voice and attached to the free-slip ruling it belongs with. Also annotated one except-pass the Charter scan flagged in adjoint.py: the _sync_lvec_to_gvec call is an optimisation, not the write. The values are already in out_var.vec; a variable class without that method syncs on demand instead, which is correct and merely later. Caught AttributeError specifically, so a failure inside the sync still propagates. Adjoint suite (test_0018-0026) 38 passed, including the nonlinear-continuation gradient that CI had been failing on. Core band tests/test_00[0-4]*py 292 passed. Deprecated-pattern scan clean. Underworld development team with AI support from Claude Code

Copilot AI lite review requested due to automatic review settings September 21, 2026 20:56

Copilot AI reviewed Sep 21, 2026

lmoresi force-pushed the bugfix/snapshot-captures-expressions branch from c64d83c to 30b2130 Compare September 22, 2026 20:00

lmoresi mentioned this pull request Sep 22, 2026

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

Open

lmoresi merged commit e1a50d3 into development Sep 23, 2026
3 of 4 checks passed

Navigation Menu

Capture expression contents in the snapshot, so parameters rewind with the fields - #775

Capture expression contents in the snapshot, so parameters rewind with the fields#775
lmoresi merged 2 commits into
developmentfrom
bugfix/snapshot-captures-expressions

lmoresi commented Sep 21, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

lmoresi commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Scope is wider than rewind()

Two details that decide whether this is correct

Tests

Capture reads the existing container registry

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lmoresi commented Sep 21, 2026 •

edited

Loading

Scope is wider than `rewind()`