refactor(readability): Wave D solvers — dedup estimate_dt/delta_t/smoothing machinery, action-named gauge install (D-57..D-68)#342
Conversation
…ed def with docstring Behaviour-neutral: same call signature and semantics; a def gives real tracebacks and documents why _unique_name_generation is required. Underworld development team with AI support from Claude Code
…e -> _install_auto_pressure_gauge Charter §3 bans the maybe_ hedging prefix. The new name states the action (install the automatic pressure gauge); the eligibility conditions are early-return guards stated in the method body and docstring. Private method, all four references in-file updated; no external callers (repo-wide grep: only docs/docstrings/review_queue.md, a generated list). Underworld development team with AI support from Claude Code
…or the triplicated projection smoothing accessors The smoothing setter and the unit-aware smoothing_length getter/setter (including the _smoothing_is_dimensional round-trip flag) were verbatim copies across SNES_Projection, SNES_Vector_Projection and SNES_MultiComponent_Projection (SNES_Tensor_Projection inherits the scalar copy). The alpha <-> L bookkeeping now lives once in _SmoothingLengthMixin; each class keeps its own property docstrings as thin wrappers, per the worklist prescription. Also removes the three in-method 'import sympy' shadow imports (part of READ-98 / D-66). Pure code motion — no logic changes. Underworld development team with AI support from Claude Code
…step; narrow the bare excepts The 55-line Pint-time -> nondimensional conversion in the delta_t setters of SNES_AdvectionDiffusion and SNES_Diffusion was a verbatim duplicate; it now lives once as module helper _nondimensionalise_timestep, with the Pint to_reduced_units() rationale stated once. The setters' opening bare 'except: pass' (which swallowed KeyboardInterrupt) is narrowed to (TypeError, ValueError) — the two exceptions float() coercion of a non-numeric timestep can raise — per the worklist prescription. The helper's terminal fallback keeps the legacy use-value-as-is behaviour and states the sanctioned swallow. Underworld development team with AI support from Claude Code
… and _centroid_velocities_nd The diffusivity-max block in estimate_dt was a verbatim x4 copy (TransientDarcy, AdvectionDiffusion, Diffusion, NavierStokes) and the centroid-velocity sampling block a x3 copy (Stokes, AdvectionDiffusion, NavierStokes). Both now live once as module helpers; the '.magnitude keeps physical units — nondimensionalise instead' rationale is stated once. Deliberate divergences parameterized, not averaged away: - Navier-Stokes passes basis=mesh.N to evaluate (the others use the default), preserved via the helper's basis parameter. - Navier-Stokes historically reduces the UN-squeezed evaluate result; preserved via ensure_2d=False and flagged with a TODO(DESIGN) note at the call site (axis-1 norm on a packed (N,1,dim) array is a max |component|, not a max vector magnitude). Pure code motion otherwise; bit-identical estimate_dt checked before/after (see PR body). Underworld development team with AI support from Claude Code
…ing to its single effective path The field-units inspection branch and the fallback branch both multiplied by fundamental_scales['time'], so the inspection changed nothing; the function now has one path. The unused 'except Exception as e' binding is gone and the remaining swallow states its sanctioned failure mode (units machinery unavailable -> return the nondimensional estimate). The redundant in-function 'import sympy' is dropped (part of READ-98 / D-66). Signature (dt, field, mesh) is frozen: cython/petsc_generic_snes_solvers.pyx imports and calls this helper with (dt_nd, self.u, self.mesh), and .pyx edits are out of scope for Wave D — documented in the docstring. Underworld development team with AI support from Claude Code
…che from the transient solve() trio The 3-line PETSc-replaced-buffers cache-invalidation idiom was verbatim in SNES_TransientDarcy.solve, SNES_AdvectionDiffusion.solve and SNES_Diffusion.solve; it now lives once as a module helper with the rationale stated in its docstring. The worklist's optional transient-solve template was considered and declined: the three solve() choreographies carry deliberate divergences (TransientDarcy bypasses SNES_Darcy.solve and runs a post-solve velocity projection; the evalf/_evalf keyword plumbing differs; NavierStokes adds flux-order overrides), so a hook-based template would obscure more than it deduplicates. Pure code motion — no logic changes. Underworld development team with AI support from Claude Code
…r in SNES_Stokes.__init__ The VE/VEP effective-order rewire tracker was created lazily behind a hasattr guard inside solve(), leaving solver state invisible from __init__. It is now initialised to None in __init__ with the guard removed. Ships the minimal regression test the worklist requires (tests/test_1016_snes_update_callbacks.py, level_1/tier_a). Underworld development team with AI support from Claude Code
…orts Removes the in-method re-imports of module-scope names: underworld3-as-uw (x2, SNES_Poisson.f setter) and numpy-as-np (SNES_Stokes.solve). The three in-method 'import sympy' shadows fell out with D-58, and the one in _apply_unit_aware_scaling with D-61. While touching the f setter, the local 'model' is renamed 'orchestration_model' per Charter naming (never name a variable 'model'). Underworld development team with AI support from Claude Code
…eadiness flag once CM_is_setup was defined verbatim on both SNES_Poisson and SNES_Stokes and raised AttributeError before a constitutive model was assigned. The flag now lives once on _ConstitutiveModelStateMixin (the shared .pyx base classes are out of Wave D scope) under the unabbreviated name constitutive_model_is_setup, returning False in the not-yet-assigned case as the worklist prescribes; CM_is_setup is kept as an alias for existing user code. Repo-wide grep found no callers outside solver_template.py (which defines its own copy — noted in the report, not touched: Charter scope discipline). Underworld development team with AI support from Claude Code
…f AdvectionDiffusion.estimate_dt D-doc: docstring-only. Lifts the inline explanation of the per-element dt reduction (strict global minimum vs Nth-percentile, sliver-cell rationale) into the Parameters section where help()/Sphinx can see it. Underworld development team with AI support from Claude Code
There was a problem hiding this comment.
Pull request overview
Readability-focused refactor of Wave D solver code in src/underworld3/systems/solvers.py, primarily deduplicating timestep / smoothing / cache-invalidation machinery and renaming the auto pressure gauge installer to an action-named method, plus adding a small regression test for Stokes solver state initialization.
Changes:
- Extract helper functions for timestep nondimensionalisation, global max diffusivity sampling, centroid velocity sampling, and solution-cache invalidation.
- Introduce
_SmoothingLengthMixinto centralizesmoothing/smoothing_lengthbookkeeping across projection solvers. - Add regression test ensuring
SNES_Stokesdeclares_prev_effective_orderduring__init__.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/underworld3/systems/solvers.py |
Refactors solver utilities (dt conversion, CFL estimate helpers, smoothing mixin, gauge installer rename) and removes duplicated code blocks. |
tests/test_1016_snes_update_callbacks.py |
Adds regression test asserting _prev_effective_order is initialized in SNES_Stokes.__init__. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| from ..scaling import units as ureg | ||
|
|
||
| if value.dimensionality != ureg.second.dimensionality: | ||
| return value |
There was a problem hiding this comment.
Verified with a runtime probe — the comparison does match. UWQuantity.dimensionality is indeed a plain dict ({'[time]': 1}), but Pint's UnitsContainer.__eq__ handles comparison with dicts (via the reflected comparison), so uq.dimensionality == ureg.second.dimensionality evaluates True (probed on pint 0.24.4, the pinned range). End-to-end probe with fundamental scales configured: solver.delta_t = uw.quantity(1000, 'year') stores _delta_t = 3.15576e-05 (= 1000 yr / time scale) — identical on this branch and on base 8a94f67. The comparison is also character-identical to the pre-refactor duplicated blocks, so no behaviour change either way.
Underworld development team with AI support from Claude Code
| try: | ||
| old_dt = float(self._delta_t.data) | ||
| new_dt = float(value.data) if hasattr(value, 'data') else float(value) | ||
| if np.isclose(old_dt, new_dt, rtol=1e-12, atol=1e-15): | ||
| return |
There was a problem hiding this comment.
Verified with a runtime probe — the narrowed except still catches this. Since pint 0.21, DimensionalityError subclasses TypeError (MRO: DimensionalityError → PintTypeError → TypeError; probed on pint 0.24.4, and the project pins pint >=0.24,<0.25), so except (TypeError, ValueError) catches float(1000*ureg.year) exactly as the old bare except: did — the caught set is unchanged at this site. UWQuantity inputs don't even reach float(value): they have .data and take that branch (which returns the magnitude without raising). End-to-end probe: adv.delta_t = 1000*ureg.year and adv.delta_t = uw.quantity(1000, 'year') both succeed and store the nondimensional value 3.15576e-05, byte-identical between this branch and base 8a94f67.
Underworld development team with AI support from Claude Code
| try: | ||
| old_dt = float(self._delta_t.data) | ||
| new_dt = float(value.data) if hasattr(value, 'data') else float(value) | ||
| if np.isclose(old_dt, new_dt, rtol=1e-12, atol=1e-15): | ||
| return |
There was a problem hiding this comment.
Same verification as the AdvectionDiffusion setter: pint.DimensionalityError subclasses TypeError (via PintTypeError, since pint 0.21; project pins pint >=0.24,<0.25), so except (TypeError, ValueError) catches float() on a unitful Quantity exactly as the old bare except: did. Runtime probe on the Diffusion solver: diff.delta_t = 1000*ureg.year and diff.delta_t = uw.quantity(1000, 'year') both succeed and nondimensionalise to 3.15576e-05, identical on this branch and on base 8a94f67. No behaviour change.
Underworld development team with AI support from Claude Code
Wave D readability rewrites for
src/underworld3/systems/solvers.py(rows D-57..D-68 ofdocs/reviews/2026-07/REMEDIATION-WORKLIST.md; evidence indocs/reviews/2026-07/READABILITY-REVIEW.md). Base: development @ 8a94f67. Behaviour-neutral code motion / rename / dedup, except the two changes the worklist explicitly prescribes (D-59 except-narrowing, D-67 pre-assignment case).Per-row status
_maybe_install_auto_gauge->_install_auto_pressure_gauge(names the action; eligibility guards stay in the body). All 4 in-file references updated; repo-wide grep found no external callers (only the generateddocs/docstrings/review_queue.mdmention, left for the next sweep regeneration)._SmoothingLengthMixinholds the alpha<->L bookkeeping once (incl._smoothing_is_dimensional); the three projection classes keep their per-class property docstrings as thin wrappers.SNES_Tensor_Projectioninherits viaSNES_Projection._nondimensionalise_timestep; the two setters' bareexcept: passnarrowed to(TypeError, ValueError)per the prescription. Helper's terminal fallback keeps legacy use-value-as-is behaviour with the swallow stated._global_max_diffusivity(x4 copies) and_centroid_velocities_nd(x3 copies) extracted;.magnituderationale stated once. Deliberate divergences parameterized, not averaged: NS passesbasis=mesh.Nandensure_2d=False(its historical reduction runs on the un-squeezed array — flagged# TODO(DESIGN)at the call site, and confirmed numerically real:ns_adv0.06031 vs 0.06007 for the squeezed reduction on the probe)._apply_unit_aware_scalingcollapsed to its single effective path (both branches multiplied by the samefundamental_scales['time']); unusedas eand shadowimport sympyremoved; the remaining swallow states its sanctioned failure mode. Signature(dt, field, mesh)is frozen:cython/petsc_generic_snes_solvers.pyx:8388imports and calls it, and.pyxis out of Wave D scope — documented in the docstring.SNES_Vector_Projection.projection_problem_description. Repo-wide grep at 8a94f67: no definition, no callers. Nothing remains of this row._invalidate_solution_cacheextracted from the x3 verbatim idiom. The optional transient-solve template was considered and declined: the threesolve()choreographies carry deliberate divergences (TransientDarcy bypassesSNES_Darcy.solve+ post-solve velocity projection;evalf/_evalfplumbing differs) that a hook template would obscure.expressionlambda -> nameddefwith docstring citing SYMBOL_DISAMBIGUATION_2025-12.md._prev_effective_order = NoneinSNES_Stokes.__init__; hasattr guard dropped fromsolve(). Minimal regression test added (worklist rule 5):test_stokes_init_declares_effective_order_stateintests/test_1016_snes_update_callbacks.py(level_1/tier_a).sympyx3 via D-58, plus the one in_apply_unit_aware_scalingvia D-61;uwx2 andnpx1 deleted here. A localmodelrenamedorchestration_modelin the touched setter (Charter naming)._ConstitutiveModelStateMixin(the shared solver bases live in the out-of-scope.pyx) asconstitutive_model_is_setup, returning False before a constitutive model is assigned (previously AttributeError — prescribed by the row);CM_is_setupkept as alias. Note:systems/solver_template.pycarries its own copy (template/example file, other group's territory) — left untouched.percentiledocumented inSNES_AdvectionDiffusion.estimate_dtParameters (lifted from the inline comment).Gates
pytest tests/ -m "level_1 and tier_a" -q:mpirun -n 2 python -m pytest --with-mpiontests/parallel/test_1017_custom_mg_parallel_mpi.py,test_1062_constrained_stratum_guard_parallel.py,test_1064_rotated_freeslip_parallel.py:estimate_dt()(incl.percentile=50anddirection_aware=Truepaths),dt_adv/dt_diff, and thedelta_tsetter round-trip before vs after at full float repr precision — identical to the last digit (12 quantities).TODO markers placed
# TODO(DESIGN)at the NavierStokesestimate_dtreduction: the historical un-squeezedaxis=1norm makesmax_magvela max |component| rather than a max vector magnitude (measured: 0.06031 vs 0.06007 on the probe). Preserved as-is; Wave D does not redesign.Underworld development team with AI support from Claude Code