Read split-mesh checkpoints through the section, not the coordinates (#640) - #714
Open
lmoresi wants to merge 1 commit into
Open
Read split-mesh checkpoints through the section, not the coordinates (#640)#714lmoresi wants to merge 1 commit into
lmoresi wants to merge 1 commit into
Conversation
…640) A cut duplicates a node at exactly the same coordinate, one copy per side of the fault. `read_timestep` matches saved values to live DOFs by nearest neighbour, which cannot choose between them: both sides were handed whichever saved point the tree returned first, and part of the slip discontinuity was smeared into the first element ring. Measured on a 2-D fault box, every one of the 15 coincident groups collapsed onto a single value; the near-fault stress recovered from a reloaded checkpoint ran ~200x the in-memory answer, which is how this was found. The PETSc-native payload written by `write_timestep(..., petsc_reload= True)` carries the section, so it restores the two sides exactly. When the saved cloud contains coincident coordinates, `read_timestep` now reads through that payload instead. Without one — or when the saved and live DOF counts differ, a cross-mesh remap no coordinate can disambiguate at a cut — it raises rather than return a quietly wrong field. `allow_ambiguous_duplicates=True` restores the old behaviour for far-field quantities. The ambiguity is measured on the SAVED cloud, not on the live mesh, and that distinction is load-bearing in parallel: the partitioner routinely puts the two sides of a cut node on different ranks, so a rank-local duplicate test sees nothing to guard against (np=2: 15 coincident groups in serial, 0 seen rank-locally) and the guard would never fire where it is needed most. The file is a single global object, so one rank-0 read answers it for every rank. Verified across write and read rank counts (np=1, 2, 3 in every combination): all 15 cut pairs keep their two distinct values. Tests: test_0864 (serial, 5 cases: the round trip, the refusal, the escape hatch, and an unsplit mesh to show the remap path is untouched) and ptest_0864 (parallel, 4 cases including a full-coverage stamp that reaches pairs straddling the partition). Underworld development team with AI support from Claude Code
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new guard appears to treat v1.1 snapshot wrappers as lacking a PETSc-native payload (raising on split meshes even though sidecar checkpoints exist), and one new parallel premise test assertion is currently non-testing/tautological.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes split-mesh checkpoint reload corruption (#640) by detecting when the coordinate-based nearest-neighbour remap is ambiguous (coincident duplicate DOF coordinates from add_fault) and, when possible, reloading through the PETSc-native section payload written by Mesh.write_timestep(..., petsc_reload=True); otherwise it raises unless explicitly overridden.
Changes:
- Add coincident-DOF detection and a guarded fallback to
read_checkpoint()insideMeshVariable.read_timestep(), plus anallow_ambiguous_duplicatesescape hatch. - Add serial and parallel regression tests that stamp the two sides of cut-node duplicates to make collapses exact/visible without a solve.
- Extend
read_timestep()docstring and error messaging to describe split-mesh semantics and required writer settings.
File summaries
| File | Description |
|---|---|
src/underworld3/discretisation/discretisation_mesh_variables.py |
Adds duplicate-coordinate guard + PETSc-native payload reload path to make split-mesh reload unambiguous. |
tests/test_0864_split_checkpoint_roundtrip.py |
Serial regression coverage for round-trip correctness, refusal without payload, and escape hatch behavior. |
tests/parallel/ptest_0864_split_checkpoint_parallel.py |
Parallel regression coverage ensuring cut-node distinctions survive reload across rank counts / partitions. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if self._lvec is None: | ||
| self._set_vec(available=True) | ||
| live_rows = self._gvec.getSize() // self.num_components | ||
| payload = None if is_v1_1 else self._petsc_payload_name( |
Comment on lines
+1202
to
+1205
| """The DM group in ``data_file`` holding ``data_name``'s section. | ||
|
|
||
| ``None`` when the file was written without ``petsc_reload=True``. | ||
| """ |
| # the cut is in the topology whatever the partition did with it ... | ||
| assert global_dofs == serial_dofs | ||
| # ... but a rank need not see both copies of any given node | ||
| assert uw.mpi.comm.allreduce(local_pairs, op=MPI.SUM) >= 0 |
Comment on lines
+152
to
+158
| def _side_stamped(mesh, name): | ||
| """Stamp each side of a cut differently wherever a rank sees both. | ||
|
|
||
| This reaches the pairs an arbitrary stamp misses — including the ones | ||
| whose two copies straddle the partition, which is where the remaining | ||
| defect lives. | ||
| """ |
Comment on lines
+173
to
+179
| def test_every_cut_pair_survives_a_parallel_write(tmp_path): | ||
| """Full-coverage version of the reload check — the writer's turn. | ||
|
|
||
| The stamp above leaves a few pairs holding the same value by chance; | ||
| this one reaches every pair a rank can see both copies of, so it | ||
| covers the ones that straddle the partition too. | ||
| """ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #640.
The defect
A cut duplicates a node at exactly the same coordinate, one copy per side of the fault.
read_timestepmatches saved values to live DOFs by nearest neighbour, which cannot choose between them — both sides were handed whichever saved point the tree returned first, and part of the slip discontinuity was smeared into the first element ring.Reproduced without a solve, by stamping each side of every cut pair with a value that differs by construction:
That matches the field symptom in #640: near-fault stress recovered from a reloaded checkpoint ran ~200x the in-memory answer.
The fix
The PETSc-native payload written by
write_timestep(..., petsc_reload=True)carries the section, so it restores the two sides exactly. When the saved cloud contains coincident coordinates,read_timestepreads through that payload instead. Without one — or when the saved and live DOF counts differ, a cross-mesh remap that no coordinate can disambiguate at a cut — it raises rather than return a quietly wrong field.allow_ambiguous_duplicates=Truerestores the old behaviour, which stays valid for far-field quantities.The parallel twist
The ambiguity is measured on the saved cloud, not on the live mesh, and that distinction is load-bearing: the partitioner routinely puts the two sides of a cut node on different ranks, so a rank-local duplicate test sees nothing to guard against — measured at np=2, 15 coincident groups in serial, 0 seen rank-locally. The guard would never fire where it is needed most. The file is a single global object, so one rank-0 read answers it for every rank.
Verified across write and read rank counts, every combination of np=1, 2, 3: all 15 cut pairs keep their two distinct values.
Tests
tests/test_0864_split_checkpoint_roundtrip.py— 5 serial cases: the round trip, the refusal without a payload, the escape hatch, and an unsplit mesh showing the remap path is untouched.tests/parallel/ptest_0864_split_checkpoint_parallel.py— 4 cases at np=2 and np=3, including a full-coverage stamp that reaches the pairs straddling the partition.Regression:
test_0003/0005/0010/0102/0114(109 passed), the fault suitetest_0845-0848(30 passed), and the parallelread_timesteptests (4 passed at np=2).One trap worth recording
write_timestepreuses<name>.mesh.00000.h5when it already exists (meshUpdates=False). Writing successive runs into one output directory therefore reads a stale mesh file against fresh variable files, which silently corrupts values at a cut — it cost an hour here posing as a writer defect. Worth a guard later; it is not one this PR adds.Underworld development team with AI support from Claude Code