Pack and index auxiliary fields by DM field, not by position in mesh.vars - #676
Closed
lmoresi wants to merge 1 commit into
Closed
Pack and index auxiliary fields by DM field, not by position in mesh.vars#676lmoresi wants to merge 1 commit into
lmoresi wants to merge 1 commit into
Conversation
…vars A MeshVariable that is dropped and garbage-collected (the default Model holds the only strong reference; uw.reset_default_model() releases it, and the statistics helpers delete temporaries deliberately) leaves its PETSc field in the DM. Mesh.update_lvec zipped mesh.vars.values() against the field decomposition by position, and the JIT's petsc_a[] offsets were a running count over the live variables, so every later variable was packed into, and read from, the wrong slots. Measured: a P0 cell-size field landing in a P2 slot as garbage, NaN residuals in one run and a subtly wrong answer in the next, depending on when the collector ran. update_lvec now packs by field name and zeroes an orphaned field; the JIT reads component offsets from the DM's own field list and patches each variable from its field_id. Regression test: 2 of its 3 checks fail without the fix. Underworld development team with AI support from Claude Code Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018T2VHUGaZiQVJ95qQ4DiSL (cherry picked from commit e84dea9)
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core mesh/JIT packing logic used broadly by solvers, so it warrants final human review despite strong test coverage.
Pull request overview
This PR fixes a correctness defect in how auxiliary mesh fields are packed/indexed for PETSc DMs when MeshVariable instances are dropped (garbage-collected) but their PETSc fields remain in the DM. The change makes auxiliary layout robust to “orphaned” DM fields, preventing stale/wrong data from being fed into assembled kernels and subsequent solver instability or silent wrong answers.
Changes:
- Pack
Mesh.update_lvec()by DM field name (and zero orphaned fields) instead of zipping bymesh.varsposition. - Derive JIT auxiliary-component offsets from the DM’s field list (via
field_id) rather than counting only livemesh.vars. - Add a level_1 / tier_a regression test that reproduces the orphan-field scenario and validates solve equivalence + packing correctness.
File summaries
| File | Description |
|---|---|
src/underworld3/discretisation/discretisation_mesh.py |
Updates update_lvec() to pack aux data by DM field name and zero orphan fields to avoid stale data propagation. |
src/underworld3/utilities/_jitextension.py |
Computes aux component offsets from the DM’s fields and patches each variable using its field_id-based offset. |
tests/test_1058_dropped_meshvariable_aux_layout.py |
Adds regression coverage for dropped-variable orphan fields affecting aux layout and downstream solves. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
863
to
867
| varlist: list | ||
| The variables to patch. Note that *all* the variables in the | ||
| corresponding `PetscDM` must be included. They must also be | ||
| ordered according to their `field_id`. | ||
| prefix_str: str |
Comment on lines
+26
to
+32
| import gc | ||
|
|
||
| import numpy as np | ||
| import pytest | ||
| import sympy | ||
|
|
||
| import underworld3 as uw |
Member
Author
|
Contained in #688, merged to development as 1d5e8c1: the by-name packing of auxiliary fields and the offsets from the DM field list, with 🤖 Generated with Claude Code |
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.
Extracted from #673 so it can land ahead of the solver work. Nothing here depends on that PR. The defect surfaced while testing the Eulerian SUPG solver that grew out of @NengLu's contribution in issue #657; the P0 cell-size field that solver reads is what made the mis-packing visible.
The defect
mesh.varsholds variables weakly. The only strong reference to a MeshVariable is the default Model's registry, and the mesh outlives the model it was created under, souw.reset_default_model()(the test suite does it between tests) releases every variable a script no longer names; the variable-statistics helpers also delete temporaries from the registry on purpose. A released variable is garbage-collected, but a DMPlex cannot shed a field, so its PETSc field stays in the DM.Two places assumed the registry and the DM's field list line up by position:
Mesh.update_lveczippedmesh.vars.values()againstdm.createFieldDecomposition(), so every variable after the orphan was packed into the wrong field and its own slot kept whatever it held;petsc_a[]/petsc_a_x[]offsets were a running count over the live variables, skipping the orphan's components.Measured: a DM with seven fields and a registry with three; a P0 cell-size field landing in a P2 slot as garbage (values of order 1.7); a solver giving
DIVERGED_FUNCTION_NANORINFat iteration zero in one run and a subtly wrong answer (1e-4) in the next, depending on when the collector ran. It surfaced as a flaky test that only failed after a test which dropped variables.The fix
update_lvecpacks by field name and zeroes an orphaned field so nothing stale reaches a kernel._jitextension._aux_component_offsets(mesh)reads component offsets from the DM's own field list andccode_patch_fnspatches each variable from itsfield_id.Test
tests/test_1058_dropped_meshvariable_aux_layout.py(level_1, tier_a, in thetest_105*CI batch): the premise (a dropped variable leaves an orphaned field), a Poisson solve with a field coefficient after two dropped variables matching a clean mesh to 1e-10, and the packed aux vector landing in the named fields. Two of the three checks fail with the fix reverted. Core batchestest_00xxtotest_02xxpass with the fix (398 tests).Not changed: the DM still grows by one field per dropped variable for the life of the mesh. That is the pre-existing memory behaviour, now merely correct.
Underworld development team with AI support from Claude Code
🤖 Generated with Claude Code
https://claude.ai/code/session_018T2VHUGaZiQVJ95qQ4DiSL