refactor(readability): Wave D mesh — Mesh.__init__ split, unified coords callback, de-duplicated submesh/adapt machinery (D-29..D-45, WE-07) by lmoresi · Pull Request #344 · underworldcode/underworld3

lmoresi · 2026-07-07T06:10:24Z

Wave D readability rewrites, GROUP mesh — src/underworld3/discretisation/discretisation_mesh.py, rows D-29..D-45 plus ride-along WE-07. Base: development @ 8a94f67. One commit per row (D-31/D-32 share one — same two functions). Evidence: docs/reviews/2026-07/READABILITY-REVIEW.md; row table in docs/reviews/2026-07/REMEDIATION-WORKLIST.md.

Per-row status

Row	Finding	Status	Notes
D-29	READ-15	done	`Mesh.__init__` (~830 lines) split into 8 named construction-phase methods; pure code motion, one commit. Only extra text changes: two positional comment references now name methods; a few commented-out dead-code lines dropped (Charter §4).
D-30	READ-16	done	One module-level `_mesh_coords_update_callback` (the `__init__` version) + `Mesh._install_coords_array`, used at all 3 sites. Coordinated with `bugfix/deform-cache-invalidation`: its hunk lives inside `_deform_mesh` — no textual overlap; the callback's post-deform `_mesh_version` bump stays a harmless second increment if both land. Sanctioned behaviour delta: `_re_extract_from_parent` gains the identity gate, `adapt` gains both guards.
D-31	READ-36	done	`_surviving_labels` with the safe getValueIS-first idiom, used by `extract_region` (previously hard-abort-prone direct `getStratumIS`) and `extract_surface`. Candidate order preserved so enum member order is unchanged; `extract_surface` keeps its extra "Centre" exclusion.
D-32	READ-37	done	Stale inline KDTree vertex map + issue-#197 comment deleted; `_build_vertex_map()` called. Tuple ordering verified bit-identical (probe below).
D-33	READ-38	done	Shared `_destroy_variable_petsc_state` / `_reinit_variable_on_new_dm` / `_invalidate_caches_after_dm_change`; IDW transfer extracted as `_idw_transfer_to_var` so the IDW-vs-symbol-evaluate difference from `adapt()` stays visible. `_re_extract`'s per-variable destroy order kept with a `TODO(DESIGN)` referencing adapt's upfront-destroy (#48). adapt's solver-rebuild guard unified on `is_setup` (all solver classes carry both attributes); its per-solver verbose debug print removed.
D-34	READ-39	done	`_facet_outward_unit_normal` carries the four-way dimension dispatch + comments once; boundary marker fetches cell vertices lazily as before.
D-35	READ-40	done	view() level-0 dead collective gathers deleted; `_print_variable_table` / `_print_boundary_table(with_sizes=)` shared by view(0)/view(1)/view_parallel; tail print now `uw.pprint`. Level-1's leftover-size All_Boundaries gather quirk preserved + `TODO(DESIGN)`.
D-36	READ-35	done	Duplicate 12-line length-scale try/except collapsed to a keyed loop (priority order preserved); bare excepts narrowed to `(AttributeError, TypeError, ValueError)` (pint errors subclass these) with the fallback-not-raise rationale stated.
D-37	READ-41	done	Single guarded block; `vertex_stratum_is` rename (getStratumIS(0) fetches vertices, not edges); intent comment (Null_Boundary = every vertex, 666). Regression test added (level_1/tier_a, test_0001).
D-38	READ-80	done	quality() locals named (`edge_a/b/c`, `area`, `shape_q`, `largest_angle`, `edge_to_tris`, `size_jump`, ...); edge-to-tris build commented; formulas and dict keys unchanged.
D-39	READ-79	done	One loop computes the control-point discriminator; threshold selected per mode; strict/non-strict + tol precedence in one comment.
D-40	READ-82	done	`dict[str, numpy.ndarray]` builtin generic.
D-41	READ-83	done	`if not …: raise`, `abs_dir` hoisted once. Regression test added (level_1/tier_a, test_0003).
D-42	READ-84	done	Forward-compat `level` param dropped; all 3 in-module callers updated; name used nowhere else in src/tests/docs-code/skills.
D-43	READ-42	done (D-doc)	UW2 `FeMesh_Cartesian` example replaced with a runnable UW3 snippet.
D-44	READ-78	done (D-doc)	Stray contradictory `tol > 0` sentence deleted.
D-45	READ-81	done (D-doc)	Sanctioned reason + consequence stated at each refresh-path swallow (`_cell_size_var._refresh`, deform()'s normal/cell-size refresh, adapt()'s cell-size refresh and submesh re-extraction loop). Comment-only.
WE-07	DOC-08	done	`mesh-adaptation.md`: `mesh.data.shape[0]` → `mesh.X.coords.shape[0]`; `mesh.adapt()` docstring Examples rewritten to direct `metric.data[...]` + `mesh.X.coords`.

Gates

Serial level_1 and tier_a — baseline at 8a94f67: 373 passed, 10 skipped, 1118 deselected, 2 xfailed, 1 xpassed. Final: 375 passed, 10 skipped, 1118 deselected, 2 xfailed, 1 xpassed — identical plus exactly the 2 sanctioned regression tests (D-37, D-41; worklist gate 5).
Parallel np2 gate (mesh/deform-exercising, run serially, pre and post at identical counts):
- test_0770_submesh_extract_mpi: 6 passed (pre) / 6 passed (post)
- test_0855_mesh_smoothing_parallel: 4 passed / 4 passed
- test_0765_internal_boundary_integral_mpi: 1 failed + 2 passed / 1 failed + 2 passed — the failure (test_deformed_spherical_shell_boundary_area_parallel) is pre-existing on development and already carries an in-test TODO(BUG) (direct _deform_mesh call rejected by the PR fix: medium Track-0 misc — parallel BoxInternalBoundary, SL theta restore, viewer crash, projection double-count, units-boundary honesty (BF-10a/12/13/15/18, D9) #326 live-mesh guard)
- test_0005_xdmf_viz_topology_mpi: 1 passed / 1 passed

Bit-identical checks (gate 3)

D-32: (sub_rows, parent_rows) from _build_vertex_map() compared element-for-element against the old inline KDTree build on a box extract_surface — exactly equal.
D-34: faces_inner/outer_control_points compared against the verbatim pre-change inline computation on 2D-simplex (396 values), 3D-simplex (1200) and 2D-quad (72) meshes — numpy.array_equal exact.
D-35: view(0)/view(1)/view_parallel output on a quad box with one variable matches the pre-change tables line for line (including the pre-existing duplicated All_Boundaries row).

TODO markers placed

TODO(DESIGN) in _re_extract_from_parent (per-variable vec destroy order vs adapt's AMR error after Stokes solve. #48 upfront destroy).
TODO(DESIGN) in _print_boundary_table (level-1 All_Boundaries row re-gathers the final loop iteration's size).

Underworld development team with AI support from Claude Code

…ction phases Pure code motion: the ~830-line monolithic constructor now calls eight named private methods in sequence, one per construction phase: _derive_length_scale_from_model (reference-quantity length scale) _load_dm_from_file (DMPlex / .msh / .h5 dispatch + metadata) _patch_boundary_enum (Null_Boundary/All_Boundaries, labels) _splice_hierarchy_from_sidecar (FMG hierarchy reload) _build_refined_hierarchy (uniform refinement hierarchy) _build_coarsened_hierarchy (coarsening hierarchy) _install_coordinate_array (coords + update callback) _setup_symbolic_coordinates (sympy N/Gamma/t + JIT bindings) No logic edits, no reordering beyond what extraction requires. The only text changes beyond motion: two positional comment references ("below", "the fresh refinement branch below") now name the methods they refer to, and a handful of commented-out dead code lines (options.delValue block, refineHierarchy alternative, stray .view() calls) were dropped per Charter section 4 (git remembers). Verified: mesh-construction test batch (0001/0004/0008/0056/0057/0058/ 0771/0772/0815/0820/0825/0826/0830) 62 passed pre/post. Underworld development team with AI support from Claude Code

…l three install sites The mesh_update_callback closure existed in three copies (__init__, _re_extract_from_parent, adapt) and had silently diverged: only the __init__ copy carried both the teardown guard (owner gone during interpreter exit / mesh replacement) and the load-bearing 'array is not mesh._coords' identity gate that stops derived sub-arrays from triggering a full-mesh deform. Now there is ONE module-level _mesh_coords_update_callback (the __init__ version, comments included) attached by a single Mesh._install_coords_array helper called from all three sites. Verbosity is carried on the mesh (_coords_callback_verbose) instead of by closure capture. Behaviour deltas (sanctioned by READ-16): _re_extract_from_parent gains the identity gate; adapt gains both guards. Coordinated with bugfix/deform-cache-invalidation: that branch's _mesh_version bump lives inside _deform_mesh, so there is no textual overlap; the callback's post-deform bump remains a harmless second increment of a monotonic counter if both land. Underworld development team with AI support from Claude Code

…ion; narrow bare excepts The domain_depth and length branches of _derive_length_scale_from_model were byte-identical 12-line try/except blocks differing only in the reference-quantity key. Collapsed to one loop over the keys in priority order (first match wins — same semantics as the old if/elif). The bare 'except:' is narrowed to (AttributeError, TypeError, ValueError) — pint's errors subclass TypeError/AttributeError — and the fallback-not-raise rationale is stated at the swallow per Charter section 4. Underworld development team with AI support from Claude Code

…ll_edges misnomer all_edges_IS_dm was assigned only inside 'if all_edges_label_dm:' yet referenced afterwards — a NameError if the DM carried no 'depth' label. The name was also wrong: getStratumIS(0) on the depth label fetches depth-0 VERTICES, not edges. Now a single guarded block builds the stratum as vertex_stratum_is, with an intent comment (Null_Boundary = every vertex, reserved value 666). Adds a minimal regression test (level_1/tier_a in test_0001_meshes.py) pinning that Null_Boundary covers exactly the vertex stratum. The no-depth-label path itself is not constructible through the public API (every stratified DMPlex carries 'depth'), so the guard is defensive. Underworld development team with AI support from Claude Code

…of inverted no-op branches The output-directory existence and write-access checks were written as 'if ok: pass else: raise' — inverted no-op guards. Rewritten as direct 'if not ...: raise' with the absolute directory hoisted once (abs_dir). Same RuntimeError messages. Adds a minimal regression test (level_1/tier_a in test_0003_save_load.py) that write_timestep to a missing directory raises RuntimeError. Underworld development team with AI support from Claude Code

… drop stale inline vertex map D-31: surviving-boundary-label enum construction was duplicated between extract_region and extract_surface with DIFFERENT safety idioms — the extract_region copy used the direct getStratumIS pattern that can hard-abort PETSc on submesh DMs (probing a value outside the label's live set, cf. the 'Centre' pseudo-label). Extracted _surviving_labels implementing the safe idiom (enumerate submesh labels by index, getValueIS before any getStratumIS); both sites use it. Candidate order (boundaries then regions for extract_region) is preserved so the resulting Enum member order is unchanged; extract_surface keeps its extra 'Centre' exclusion as a deliberate per-site difference. D-32: extract_surface inlined a KDTree vertex-map build under a stale comment claiming _build_vertex_map was broken (issue #197) — the method was already fixed (071c563) with the same code. The inline block is deleted and _build_vertex_map is called, as extract_region does. Verified bit-identical: (sub_rows, parent_rows) from the method equal the old inline build exactly on a box surface probe; extract tests (0771/0772) 10 passed pre/post. Underworld development team with AI support from Claude Code

…s for re-extraction and adapt _re_extract_from_parent duplicated adapt()'s variable teardown, reinit and cache-invalidation machinery with small differences a reader could not tell were intentional. Extracted: _destroy_variable_petsc_state (vec destroy + cached-view drop, #48) _reinit_variable_on_new_dm (_setup_ds + _set_vec) _idw_transfer_to_var (submesh transfer strategy, NAMED so the IDW-vs-symbol-evaluate difference from adapt() stays visible) _invalidate_caches_after_dm_change (solver rebuild + eval/DMInterp caches; reason parameterised) Deliberate differences preserved and documented: the two transfer strategies stay separate; _re_extract's per-variable destroy order keeps a TODO(DESIGN) pointing at adapt()'s upfront-destroy (#48) fix. Unifications: adapt's solver-rebuild guard now tests 'is_setup' like every other site (all solver classes carry both attributes) and its per-solver verbose debug print is gone. Verified: adapt/extract test batch (0760/0771/0772/0830) 24 passed pre/post. Underworld development team with AI support from Claude Code

…he duplicated four-way dispatch The dimension/embedding-case facet-normal dispatch (1-manifold edge, 2-D volume, 2-manifold-in-3-space tangent plane, 3-D face) was duplicated with near-identical code and comments in _mark_faces_inside_and_out and _mark_local_boundary_faces_inside_and_out. One helper now carries the dispatch and the dimension-case commentary; the boundary marker fetches the owning cell's vertices lazily (only the tangent-plane case needs them), exactly as before. Bit-identical verified: faces_inner/outer_control_points equal the old inline computation exactly on 2D-simplex, 3D-simplex and 2D-quad probes (396/1200/72 values); deformed-domain-membership tests pass. Underworld development team with AI support from Claude Code

quality() computed its metrics in single-letter soup (a, b, cl_, A, q, et, jr, rel, ang, Lmax) beneath a thorough docstring. Locals now say what they are (edge_a/b/c, area, shape_q, largest_angle, longest_edge, edge_to_tris, size_jump, rel_area, ...), the law-of-cosines helper is _angle_deg(opposite, side1, side2), and the edge-to-triangles build carries a comment. Formulas and returned dict keys unchanged. Underworld development team with AI support from Claude Code

…loop _test_if_points_in_cells_internal repeated the per-face control-point loop three times with only the final comparison differing (tol-relative, on-boundary -1e-12 floor, strict > 0). One loop now computes the inner/outer squared-distance discriminator once and selects the acceptance test; the strict/non-strict and tol-precedence semantics are stated in one comment. Same arithmetic, same thresholds. Underworld development team with AI support from Claude Code

… annotation snapshot_payload annotated var_arrays with typing.Dict, which is never imported in this module (harmless at runtime because annotations are unevaluated, but a latent NameError for any tool that evaluates them). Use the builtin dict[str, ...] generic. Underworld development team with AI support from Claude Code

…ompat level parameter Every call site passed level=0 and the checkpoint design persists only the coarsest hierarchy level; the parameter existed 'for forward-compatibility' — speculative generality the Charter bans. The function now takes only the mesh filename and always produces the .hierarchy.L0 sidecar name (the docstring says why L0 is the only level). All three in-module callers updated; the name appears nowhere else in src/tests/docs-code. Underworld development team with AI support from Claude Code

…shared table printers view() level 0 performed three collective gather_data calls whose results were never printed (one per boundary row, one on the loop-leftover size, one for UW_Boundaries) — dead results costing a collective each. Deleted; level 0 needs no gathers. The variable and boundary tables were triplicated across view(0), view(1) and view_parallel with drifting print/pprint styles. Extracted _print_variable_table and _print_boundary_table(with_sizes=...); all sites now print through uw.pprint (rank 0), and the level-0 tail 'Use view(1)...' print is uw.pprint too. view(1)'s quirk of re-gathering the final loop iteration's size for its All_Boundaries row is preserved and marked TODO(DESIGN); the duplicated All_Boundaries row (the enum member plus the hardcoded row) also predates this change and is preserved. Verified: view(0)/view(1)/view_parallel output on a quad box with a variable matches the pre-change tables. Underworld development team with AI support from Claude Code

…mple The example was UW2 copy-paste that cannot run (FeMesh_Cartesian, and 'with someMesh._deform_mesh():' used as a context manager — the exact pattern the surrounding code forbids). Replaced with a runnable UW3 snippet and a pointer to the current data-access guide. Docstring-only. Underworld development team with AI support from Claude Code

…tence _get_closest_local_cells_internal's docstring stated two opposite semantics for 'tol > 0' in consecutive sentences; the first was a stray duplicate of the on_boundary=True description (already given one sentence earlier). Deleted the stray sentence — 'tol > 0' now has one meaning: a face-relative tolerance taking precedence over on_boundary. Docstring-only. Underworld development team with AI support from Claude Code

…path exception swallow Charter section 4: every intentional swallow states its failure mode. The blanket 'except Exception' swallows in the geometry-refresh paths — the _cell_size_var reinit callback, deform()'s boundary-normal and cell-size re-assembly, and adapt()'s cell-size refresh and submesh re-extraction loop — now carry comments stating why fallback-not-raise is sanctioned there (a completed deform/adapt must not be rolled back for a refresh aid) and what the consequence of skipping is (the BC aid lags one geometry until the next refresh). Comment-only. Underworld development team with AI support from Claude Code

…aptation docs docs/advanced/mesh-adaptation.md's worked example printed mesh.data.shape[0] (mesh.data raises a DeprecationWarning); now mesh.X.coords.shape[0]. The mesh.adapt() docstring's own Examples demonstrated both deprecated patterns ('with mesh.access(metric):' and fault.distance_from(mesh.data)); rewritten to direct metric.data[...] assignment and mesh.X.coords per the current pattern table. Docs and docstring only. Underworld development team with AI support from Claude Code

Copilot

Pull request overview

This PR refactors the core Mesh implementation for readability and maintainability by splitting Mesh.__init__ into named construction phases, centralising coordinate-array callback installation, and de-duplicating repeated submesh/adaptation DM-replacement logic. It also updates documentation/examples to the modern coordinate access path (mesh.X.coords) and adds regression tests for two previously fragile behaviours.

Changes:

Split the monolithic Mesh.__init__ into multiple internal construction-phase methods and extracted several shared helpers (hierarchy splice/build, coordinate wrapping/callback install, DM-change invalidation).
Unified mesh-coordinate update callback into a single module-level implementation and routed all coordinate-array rewrap sites through Mesh._install_coords_array.
Added regression tests for Null_Boundary vertex coverage and write_timestep() failing loudly when the output directory is missing; updated mesh-adaptation docs accordingly.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.

File	Description
`src/underworld3/discretisation/discretisation_mesh.py`	Large readability refactor: constructor split, unified coords callback installer, shared DM-replacement helpers, safer label-survival probing, and minor doc/example adjustments.
`tests/test_0001_meshes.py`	Adds regression test that `Null_Boundary` marks every vertex (depth-0 stratum).
`tests/test_0003_save_load.py`	Adds regression test ensuring `write_timestep()` raises when output directory is missing.
`docs/advanced/mesh-adaptation.md`	Updates example to use `mesh.X.coords.shape[0]` instead of the legacy `mesh.data`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

lmoresi · 2026-07-07T08:53:10Z

+    verbose = getattr(mesh, "_coords_callback_verbose", False)
+    if verbose:
+        uw.pprint(0, f"Mesh update callback - mesh deform")
+


Verified — correct. uw.pprint's signature is pprint(*args, proc=0, prefix=None, ...) (src/underworld3/mpi.py), so proc is keyword-only and the positional 0 was printed as a stray leading 0 (probed: uw.pprint(0, 'msg') → 0 msg). The pattern is pre-existing at the base commit (D-29 code motion moved it here unchanged), but it's an output bug, so fixed in 7ce688e: dropped the positional 0 at this site and at the six identical pre-existing sites in this file (proc=0 is already the default, so rank-0-only printing is preserved). All affected prints are verbose-gated debug output — view() output is unchanged. Serial level_1+tier_a gate re-run green (375 passed / 10 skipped / 2 xfailed / 1 xpassed).

Underworld development team with AI support from Claude Code

lmoresi · 2026-07-07T08:53:15Z

+        mesh._mesh_version += 1
+        if verbose:
+            uw.pprint(0, f"Mesh version incremented to {mesh._mesh_version}")


Verified — same issue as the sibling comment above: proc is keyword-only in pprint(*args, proc=0, ...), so the positional 0 printed as a stray leading 0 . Pre-existing at base (moved by the D-29 code motion), fixed in 7ce688e along with the other six identical call sites in this file by dropping the positional 0 (default proc=0 preserves rank-0-only printing). Gate re-run green.

Underworld development team with AI support from Claude Code

uw.pprint's signature is pprint(*args, proc=0, ...) — proc is keyword-only, so uw.pprint(0, msg) printed a stray leading "0 " instead of selecting rank 0. Probed and confirmed. The default proc=0 already gives rank-0-only printing, so the positional 0 is simply dropped at all 8 sites in this file (the two flagged by review at lines 244/253 plus six identical pre-existing instances on the submesh re-extraction path). All are verbose-gated debug prints; view() output is untouched, so the wave's line-identical view() check is unaffected. Underworld development team with AI support from Claude Code

…igation (#360) Every moving-mesh / manifold workflow crashed at np>1 in _build_kd_tree_index with `IndexError: index N is out of bounds` (both negative and past-end). The kd-tree and the two mark-faces routines mapped vertex plex-points to coordinate rows with the affine shift `nav_coords[points - pStart]`, valid only when vertex numbering is serial-contiguous. On the 1-cell-overlap navigation clone used for manifold (codim-1) meshes the local coordinate array is laid out by the coordinate PetscSection, so the affine shift runs out of bounds across ghost vertices. Root cause has two parts, both fixed here: 1. Vertex->row mapping. New `_coord_rows_for_points(nav_dm, points)` maps each vertex via the coordinate PetscSection offset (`getOffset(p)//cdim`), the authoritative map for the overlapped array. In serial the section offset equals `(plex_point - pStart)*cdim`, so the rows are bit-identical to the old affine indexing (verified: max row diff 0, evaluate error 0) and serial navigation is unchanged. Applied at all five sites in _build_kd_tree_index, _mark_faces_inside_and_out (x2) and _mark_local_boundary_faces_inside_and_out (x2). 2. Incomplete ghost coordinates. distributeOverlap grows the nav clone's coordinate section to cover ghost vertices but never scatters their coordinates into the local vector, so getCoordinatesLocal() is short by the ghost rows and even the section offsets overrun it. New `_navigation_coords_from_dm(nav_dm)` rebuilds the full local coordinate vector via the coordinate DM global->local scatter. Used at construction and, for the deform/adapt refresh, together with setCoordinates(main global) so the overlapped clone tracks the moved geometry. Regression test tests/parallel/test_0775_deform_kdtree_parallel.py: a box deform navigation check plus a manifold surface navigation check that fails pre-fix (IndexError at _build_kd_tree_index, np2 and np4) and passes after. The pre-existing manifold test test_0773 (which failed pre-fix at np2/np4) also goes green. Closes #360 Underworld development team with AI support from Claude Code

… other DM scatters INSERT and INSERT_VALUES are aliases in petsc4py (both enum value 1); this matches the spelling used elsewhere (Copilot review). Underworld development team with AI support from Claude Code

…360 fix through the D-29/D-34 refactor Conflict resolution: the Wave D extraction moved the coordinate-array installation into _install_coordinate_array and the facet-normal dispatch into _facet_outward_unit_normal; the #360 section-offset row mapping and ghost-coordinate completion are applied inside the extracted methods. Underworld development team with AI support from Claude Code

lmoresi · 2026-07-14T23:41:00Z

Merged bugfix/moving-mesh-kdtree-np (#363) into this branch and resolved the two conflicts: the #360 section-offset row mapping and ghost-coordinate completion are now applied inside the extracted methods (_install_coordinate_array, _facet_outward_unit_normal call site). Verified on the merged branch: #363's regression test test_0775 passes at np2 and np4 through the refactored code; serial level_1 and tier_a = 375 passed / 12 skipped / 2 xfailed / 1 xpassed (this PR's baseline + the environment's 2 extra parallel-only skips).

Merge order: #363 first, then this PR — once #363 lands, GitHub recomputes this diff and the shared content drops out.

Underworld development team with AI support from Claude Code

…ady embeds the #360 fix ported through D-29/D-34 Underworld development team with AI support from Claude Code

…..22, WA-05..09, WA-18) (#366) * refactor(solvers): rename _maybe_install_snes_update -> _attach_snes_update_hook (D-14) Charter S3 bans hedging prefixes (maybe_/try_/do_). The name chosen is the one already used on the unmerged feature/snes-update-callbacks tip (b82acea), so that branch's rebase stays trivial. Rename only — the method body, its docstring, and the single call site in _snes_solve_with_retries are otherwise untouched. Also removes the now-satisfied hedging-name allowlist entry for this file from scripts/deprecated_pattern_allowlist.txt — the ratchet's first shrink (87 -> 86 allowlisted hits; scanner verified green with the entry gone). Underworld development team with AI support from Claude Code * chore(solvers): delete four unused imports (WA-08) Line 1 of the flagship solver file was 'from xmlrpc.client import Boolean'. Also removes the unused 'sympify' (the one live use is the qualified sympy.sympify), 'TypeAlias', and 'class_or_instance_method' imports — each verified unreferenced in the file (READ-70). Underworld development team with AI support from Claude Code * fix(solvers): raise('string') -> TypeError/ValueError in four BC/section error paths (WA-05) raise('...') is itself a TypeError at raise time ('exceptions must derive from BaseException'), swallowing the diagnostic. Each site now raises a real exception with the message text kept verbatim (READ-21): - add_condition f_id type check -> TypeError - add_condition c_type value check -> ValueError - add_condition components fallback -> TypeError - _get_dof_partition_by_field_id section_type check -> ValueError Error-path only; no reachable behaviour change for valid arguments. Underworld development team with AI support from Claude Code * fix(solvers): F1 guard property error message said F0 (D-17) Copy-paste of the F0 base-class guard pointed developers at the wrong term (READ-71). Error-path message only. Underworld development team with AI support from Claude Code * refactor(solvers): delete two 'if True:' constant guards, dedent bodies (WA-06) 'if True: # c_label and label_val != -1:' wrapped the natural-BC DS-wiring blocks in SNES_Vector._setup_discretisation and SNES_Stokes_SaddlePt's equivalent — a constant-literal guard adding a spurious indent level and implying a condition that no longer exists (READ-14). Cython constant-folds the guard, so this is byte-identical. Mechanical verification: cythonized the file pre/post and diffed the generated C with line-number bookkeeping normalized (__PYX_ERR/lineno, goto labels, comments stripped). Remaining hunks are exclusively code-object first-line metadata shifted by the two deleted lines and the compressed constant-pool repacking; the generated function bodies are identical. Underworld development team with AI support from Claude Code * chore(solvers): delete fossil commented-out code blocks (WA-07) Comment-only deletions per LE-13 / READ-24 / LE-15 (git keeps the history): - SNES_Vector.solve: 13-line dead rebuild path; dead clearDS()/createDS() code (its 'destroys field registrations' rationale KEPT, reworded to be self-contained) - whole commented-out add_essential_p_bc method (Stokes) - SNES_Scalar natural-BC flux-Jacobian sketch (the one-line 'different user-interface for flux-like bcs' intent note KEPT) - old f0/F1 Array-form constructions in Scalar/Vector/Stokes _setup_pointwise_functions; FP1 pu_G2/pu_G3 fossils (one-line intent note kept); Stokes __init__ zero-matrix fossils - commented debug prints, 'with mesh.access' one-liners (7), bc_label/getStratumIS alternates, coarse_dm.createDS() one-liner, dead attribute assignments (# self.u = u_Field triplet, # self._constitutive_model = None x3) Mechanical verification: cythonized pre/post; normalized diff contains only docstring-table identifiers that embed source line numbers, code-object first-line metadata, and constant-pool repacking — the generated function bodies are identical (comment-only change). Underworld development team with AI support from Claude Code * refactor(solvers): explicit u_Field=None branches in Scalar/Vector constructors (D-15) SNES_Scalar.__init__ created a default unknown then unconditionally re-assigned 'self.Unknowns.u = u_Field' — correct only because the Unknowns.u setter silently ignores None (READ-22). Now an explicit if/else. SNES_Vector's auto-create used truthiness ('if not u_Field:'); now the explicit 'is None' contract, with a comment noting the supplied field is assigned earlier in the constructor (WC-10 confirmed the auto-create already existed — this row is readability only). The suspicious num_components=mesh.dim on the default SCALAR variable is kept as-is (load-bearing until reviewed) and flagged with a TODO(BUG). Underworld development team with AI support from Claude Code * refactor(solvers): dedent the byte-identical Picard/else Stokes solve tail (D-16) The picard and else branches of SNES_Stokes_SaddlePt.solve ended with a byte-identical 14-line tail (verified with diff before the change); the '# Now go back to the original plan' comment sat at the wrong indent, misleading about control flow (READ-29). The common tail now runs once after the optional Picard warmup — verbatim code motion — and the comment states the actual control flow. Mechanical verification: cythonized pre/post; mapped every C diff hunk to its enclosing generated function — all code-body changes fall inside SNES_Stokes_SaddlePt.solve (the duplicated call sequence collapsing to a single copy at outer indentation, same operations in the same order); remaining hunks are constant-pool/code-object line metadata. Underworld development team with AI support from Claude Code * docs(solvers): document the hardcoded snes_max_it=50 clobber (D-22) Comment-only (D-doc). The Stokes solve path pushes a hardcoded snes_max_it=50 to petsc_options each solve, overriding user settings (READ-30). The behaviour change (respect the user's option) was deferred with the D2 benchmarked sub-wave per maintainer ruling D18; the comment records that and points at the worklist rows. Underworld development team with AI support from Claude Code * refactor(solvers): one class-level SNES convergence-reason table for both consumers (D-18) Two divergent tables lived in SolverBaseClass (READ-72): the 16-entry 'NAME - explanation' local map in get_convergence_diagnostics and the 12-entry compact class map used by _warn_on_divergence (-9/-10/-11 and 0 missing). Now a single class-level table code -> (NAME, explanation), contents identical to the fuller copy; get_convergence_diagnostics formats 'NAME - explanation' from it and _warn_on_divergence uses the NAME. No external consumers (verified by grep). Only reachable output delta: _warn_on_divergence now names codes -9/-10/-11 (DIVERGED_DTOL / DIVERGED_JACOBIAN_DOMAIN / DIVERGED_TR_DELTA) instead of printing UNKNOWN(...) — the gap the review flagged. Code 0 remains unreachable there (early return for reason >= 0). Runtime probe of both consumers across all reason codes runs post-build (recorded in the PR). Underworld development team with AI support from Claude Code * refactor(solvers): one _nondimensional_time helper for the x6 verbatim copies (D-19) The Pint/UWQuantity -> float time conversion snippet was copy-pasted six times across the solve/residual paths (READ-73); all six verified verbatim (modulo indentation) before extraction. The helper body is the snippet unchanged — nothing parameterized. Call sites now read 't_nd = self._nondimensional_time(time)'. Runtime probe (post-build, recorded in the PR): helper output equals the old inline logic for float, int, Pint Quantity, and UWQuantity inputs. Underworld development team with AI support from Claude Code * refactor(solvers): rename dim -> cdim where the local holds mesh.cdim (D-20) Three method scopes bound a local named 'dim' to the EMBEDDING dimension mesh.cdim (READ-76): SNES_Vector.add_nitsche_bc, SNES_Vector._setup_pointwise_functions (which bound both 'dim' and 'cdim' to the same value), and SNES_MultiComponent._setup_pointwise_functions. Purely lexical rename within those scopes (no topological mesh.dim use exists inside any of them — verified); the duplicate binding is gone and shape-check error messages now say cdim, which is the accurate term on manifold meshes. Also fixes the stale '...FE element construction at line ~173' comment to refer to _setup_discretisation by name (Charter S4: no line-number references). Underworld development team with AI support from Claude Code * docs(solvers): TODO(DESIGN)-ify or delete unresolved editorial musings (D-21) Charter S4 bans editorial musings in comments (READ-74): - 'LM: this is probably not something we need...' above SNES_Vector -> TODO(DESIGN) pointing at the deferred unification rows D-23..D-28 - 'Why is this here - this is not generic at all ??' -> TODO(DESIGN) on _setup_history_terms placement - 'F0, F1 should be f0 and F1... uf0, uF1 are redundant' -> TODO(DESIGN) on residual-term naming - trailing 'probably just needs to boot the DM...' musing deleted Comment-only. Underworld development team with AI support from Claude Code * refactor(solvers): remove reserved 'robust'/'fast' pass branches from the Stokes strategy setter (WA-09) The strategy setter carried empty '(Reserved)' branches ('robust'/'fast' -> pass) — speculative generality banned by Charter S5 (READ-23). The names remain accepted (all three configure the identical bundle, now stated in a comment); an unknown name raises ValueError instead of silently configuring 'default'. No option VALUES changed anywhere. Also comments the long-standing kaskade-vs-additive pc_mg_type divergence between this setter and __init__'s velocity block: kept as-is, not to be changed without benchmarking. No internal or test code assigns .strategy (verified by grep), so the new ValueError has no reachable caller today. Underworld development team with AI support from Claude Code * chore(discretisation): delete dead petsc_dm_get_periodicity block; fix stray pprint positional proc (WA-18 + #344 follow-up) - Deletes the module-level triple-quoted string holding the never-runnable petsc_dm_get_periodicity sketch and its content-free '## Todo !' introduction (LE-16). Git keeps the code if DM periodicity reading is ever wanted. - petsc_dm_find_labeled_points_local: uw.pprint(0, msg) passed 0 as a *message* (proc is keyword-only, default 0), printing a stray leading '0'. Same fix as applied x8 elsewhere in the #344 Copilot pass. Underworld development team with AI support from Claude Code

lmoresi added 17 commits July 7, 2026 06:06

Copilot AI review requested due to automatic review settings July 7, 2026 06:10

Copilot started reviewing on behalf of lmoresi July 7, 2026 06:10 View session

Copilot AI reviewed Jul 7, 2026

View reviewed changes

lmoresi added 2 commits July 7, 2026 09:52

lmoresi mentioned this pull request Jul 14, 2026

fix(mesh): parallel plex-point→coord-row mapping in kd-tree/face navigation (#360) #363

Merged

lmoresi added 2 commits July 14, 2026 13:14

style(mesh): InsertMode.INSERT_VALUES for consistency with the repo's…

ed43804

… other DM scatters INSERT and INSERT_VALUES are aliases in petsc4py (both enum value 1); this matches the spelling used elsewhere (Copilot review). Underworld development team with AI support from Claude Code

Merge development (squashed #363): keep the refactored file — it alre…

53153d3

…ady embeds the #360 fix ported through D-29/D-34 Underworld development team with AI support from Claude Code

lmoresi merged commit 58d9482 into development Jul 15, 2026
1 check passed

lmoresi mentioned this pull request Jul 15, 2026

refactor(pyx-safe): naming, docs, dead code in the solver layer (D-14..22, WA-05..09, WA-18) #366

Merged

lmoresi mentioned this pull request Jul 16, 2026

feat(mg/adapt): land custom-MG parallel support + NVB adapt-on-top (stranded branch, closes #365) #369

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(readability): Wave D mesh — Mesh.init split, unified coords callback, de-duplicated submesh/adapt machinery (D-29..D-45, WE-07) - #344

refactor(readability): Wave D mesh — Mesh.init split, unified coords callback, de-duplicated submesh/adapt machinery (D-29..D-45, WE-07)#344
lmoresi merged 22 commits into
developmentfrom
feature/wave-d-mesh

lmoresi commented Jul 7, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

lmoresi Jul 7, 2026

Uh oh!

lmoresi Jul 7, 2026

Uh oh!

lmoresi commented Jul 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

lmoresi commented Jul 7, 2026

Per-row status

Gates

Bit-identical checks (gate 3)

TODO markers placed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

lmoresi Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

lmoresi Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

lmoresi commented Jul 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants