refactor(readability): Wave D rotated_bc — helper reuse, line-search extraction, lifecycle dedent (D-69..D-78)#347
refactor(readability): Wave D rotated_bc — helper reuse, line-search extraction, lifecycle dedent (D-69..D-78)#347lmoresi wants to merge 12 commits into
Conversation
…module-docstring opening The module IS underworld3.utilities.rotated_bc on the integration branch; the 'productizes the validated prototypes' provenance framing misled about status. Docstring-only change. Underworld development team with AI support from Claude Code
Five inline 'from underworld3 import mpi' copies (all just for mpi.pprint) replaced by a single module-top import; underworld3.mpi is a leaf loaded before underworld3.utilities in the package __init__, so the top-level import cannot cycle. Underworld development team with AI support from Claude Code
…ible-state guard sympy is a hard dependency: the try/except around its import guarded a state that cannot occur (Charter §5, false guards lie), and its 'except Exception: sym_fn = None' fallback would silently misread a lambdify/unwrap failure as a constant-array normal. A genuine lambdify error now raises at the call site. Underworld development team with AI support from Claude Code
… field ids _velocity_field_id(solver) ignored its argument and returned 0 while the pressure id was a bare inline PRE = 1 — inconsistent ceremony for two fixed constants. Both become module constants (_VELOCITY_FIELD/_PRESSURE_FIELD) with one comment; used at every call site. No external callers of the deleted private helper (repo-wide grep). Underworld development team with AI support from Claude Code
…oing through _zero_rows_local The helper encapsulating the np>1 ownership-relative RHS zeroing was hand- inlined at three more sites (constrained-RHS write, iterative-solution exactification, null-space compatibility loop) plus the LU pressure-datum write, which is the same operation on [pin]. All now call the helper; its definition moves above first textual use. The datum write's ownership guard 'pin is not None and brs <= pin < bre' becomes 'pin is not None' + the helper's own ownership filter — identical effect. Underworld development team with AI support from Claude Code
…o the LU branch; TODO(BUG) the parallel-unsafety The datum search ran unconditionally in solve_rotated_freeslip but 'pin' is consumed only by the opt-in direct-LU branch — pure code motion into that branch. The parallel-unsafety (per-rank chart scan pins a different or no DOF at np>1) was noted only in prose; it is now a TODO(BUG) marker per the worklist (marker, NOT a fix — scope discipline). Underworld development team with AI support from Claude Code
…lizer
Boundary-spec normalization ('name' vs '(name, normal)') was written two
different ways: a loop destructure in build_rotation and a nested-
comprehension dict one-liner in boundary_normal_traction. One helper,
used at both sites; identical semantics.
Underworld development team with AI support from Claude Code
…c lifecycle chains Mechanical split of every semicolon-chained create/use/destroy line (the lifecycle the file's own comments call trickiest); ownership comments stay attached to the create/destroy they describe. No statement added, removed or reordered. Underworld development team with AI support from Claude Code
…arch; one destroy point per Newton iteration The 177-line nonlinear driver interleaved the ‖F̂‖ backtracking line search with manual destroy() bookkeeping across three exit paths. Pure code motion: * _backtracking_line_search owns its temporaries' destroys and returns (u, improved), destroying/replacing the input iterate on acceptance; * _project_out_normal_component is the u → v_n=0 exact projection, shared by the warm-start feasibility step and the line search (2nd occurrence, Charter §5 DRY); * the four per-iteration vecs (dhat, d, bhat, Fhat) now die at ONE point reached on every exit path — the same set the two previous paths freed. Underworld development team with AI support from Claude Code
… the result-dict keys The untyped 'info' dict's contract was learnable only by diffing the two solve functions' return statements. Both solve functions now carry a NumPy Returns section listing every key (and which path omits A/b vs reaction); the consumers' parameter is renamed to solve_result. All external callers (pyx solver methods) pass it positionally — repo-wide grep found no info= keyword call — so no alias is needed. Underworld development team with AI support from Claude Code
There was a problem hiding this comment.
Pull request overview
Refactors src/underworld3/utilities/rotated_bc.py for readability and reduced duplication in the rotated strong free-slip implementation backing solver.add_rotated_freeslip_bc, including helper extraction and clearer resource-lifecycle structure.
Changes:
- Extracts reusable helpers (
_boundary_spec,_zero_rows_local,_project_out_normal_component,_backtracking_line_search) and reuses them across linear/nonlinear solve paths. - Dedents and linearizes PETSc create/use/destroy sequences (one statement per line) and removes repeated inline
mpiimports via a module-top import. - Updates module and function docstrings, including documenting result-dict keys and renaming
info→solve_resultin postprocessing APIs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * ``"rotation_gauge_removed"`` — whether a rigid-rotation gauge was projected out; | ||
| * ``"ksp_reason"`` — converged-reason of the LAST Newton increment's KSP; | ||
| * ``"ksp_its"`` — list of linear iteration counts, one per Newton iteration; | ||
| * ``"nonlinear_iterations"``, ``"converged"`` — outer-loop count and status; | ||
| * ``"continuation_switched"`` — whether the Picard→Newton tangent switch fired. |
There was a problem hiding this comment.
Verified — the claim is correct, and slightly understated. The loop returns the 0-based index iters, which undercounts on three of the four exit paths: on the step-norm exit, the line-search-stall exit and the max_it exit, the final pass has already solved a Newton increment before the break, so the true count is iters + 1; only on the top-of-loop residual exit is iters the correct count. Returning iters + 1 unconditionally would therefore overcount the residual-converged path instead.
The quantity that is correct on every exit path is len(lin_its) — one linear solve per Newton increment — which equals iters + 1 on all the warning paths and iters on the residual exit. Fixed in 26481bd: the dict now returns that count (so nonlinear_iterations == len(ksp_its) always), the non-convergence warning uses the same variable, and the docstring states the invariant. Grepped repo-wide: the key is diagnostic-only (test assertions and skill docs; nothing in src/ branches on it). Added the invariant check to test_1018_rotated_freeslip.py and re-ran the serial rotated-BC batch (test_1018 + test_1064): 17 passed, 2 pre-existing xfails.
Underworld development team with AI support from Claude Code
There was a problem hiding this comment.
Follow-up: the corrected count surfaced one golden-value consumer — GOLDEN_BOX_NONLINEAR in tests/parallel/test_1064_rotated_freeslip_parallel.py asserted an iteration count of 7, which was the old loop-index report of a solve that actually performs 8 Newton increments (it exits on the step-norm test after the 8th). Updated to 8 in 7503af5 and re-verified the partition-independence test passes at np=2 and np=4 (velocity L2 unchanged).
Underworld development team with AI support from Claude Code
…ult dict The nonlinear rotated free-slip result dict returned the 0-based loop index 'iters' as "nonlinear_iterations", undercounting by one on the step-norm / line-search-stall / max_it exits and disagreeing with the non-convergence warning (which printed iters + 1). Return the number of Newton increments actually solved (len(ksp_its) — one linear solve per increment), which is correct on every exit path, and use the same count in the warning. Diagnostic-only: nothing in src/ branches on the value. Adds the count == len(ksp_its) invariant to test_1018. Addresses Copilot review comment on PR #347. Underworld development team with AI support from Claude Code
… iteration report The power-law box solve exits on the step-norm test after its 8th Newton increment; the old report (loop index) said 7, the corrected count (number of increments solved, == len(ksp_its)) says 8. L2 unchanged. Verified partition-independent at np=2 and np=4. Underworld development team with AI support from Claude Code
Wave D readability rewrites for
src/underworld3/utilities/rotated_bc.py(rows D-69..D-78 ofdocs/reviews/2026-07/REMEDIATION-WORKLIST.md; evidence indocs/reviews/2026-07/READABILITY-REVIEW.md).One commit per row, behaviour-neutral code motion / rename / dedent per the Wave D contract.
All rows re-verified at remediation base
8a94f678before acting (audit line anchors haddrifted; every target still existed).
Per-row status
_zero_rows_localcalled at the 3 hand-inlined sites (+ the LU datum write, same operation); definition moved above first textual use72c0df68_boundary_specnormalizer extracted, used inbuild_rotationandboundary_normal_tractiona26e7c80_backtracking_line_searchextracted (owns its temporaries' destroys);_project_out_normal_componentshared with the warm-start feasibility step; the four per-iteration Vecs die at ONE point on all exit pathsbd89ed63d3cf219a_VELOCITY_FIELD = 0/_PRESSURE_FIELD = 1module constants with one comment;_velocity_field_id(ignored its argument) deleted, no external callers (repo-wide grep)43d57ef4# TODO(BUG):marker, NOT fixed (scope discipline per the worklist)30d5d2ecimport sympy; the impossible-state try/except (hard dependency) deleted. Sanctioned behaviour delta: a genuine lambdify/unwrap failure now raises instead of being silently misread as a constant-array normalc5cd74c1b0abeae4from underworld3 import mpi(leaf module, loads beforeunderworld3.utilitiesin the package__init__); five inline copies removedf0e23f29info→solve_resultinboundary_normal_traction/dynamic_topography_field; both solve functions gain a NumPy Returns section listing every result-dict key (and which path omitsA/bvsreaction). All external callers (pyx solver methods) pass positionally — noinfo=keyword call repo-wide, so no alias needed57d374caGates
All runs from the group worktree env (
amr-dev), suites strictly serial, pre = base8a94f678build, post = this branch's build.pytest tests/ -m "level_1 and tier_a" -qmpirun -n 2 python -m pytest --with-mpi tests/parallel/test_1017/1062/1063/1064/1065Bit-identical checks: no row in D-69..D-78 prescribes a mover/remesh bit-identical probe.
The np2 suite itself performs the quantitative comparison for this file's machinery —
test_1064_rotated_freeslip_parallelchecks the parallel velocity L2 and per-arc radialleakage against the serial reference for both the box (GAMG) and annulus
(rotation-nullspace + gauge-removal) geometries; both identical pre/post.
TODO(BUG) markers placed
solve_rotated_freeslipLU branch: the pressure-datum search is a per-rankglobal-section-chart scan — at np>1 each rank pins a different pressure DOF (or none,
pin=None→zeroRows([None])fails). Tolerated only because LU is opt-in viasolver._rotated_use_lu(D-74/READ-101).Underworld development team with AI support from Claude Code