refactor(readability): Wave D ddt — decompose SL update_pre_solve and dedupe the five DDt flavors (D-46..D-56)#343
Open
lmoresi wants to merge 10 commits into
Open
refactor(readability): Wave D ddt — decompose SL update_pre_solve and dedupe the five DDt flavors (D-46..D-56)#343lmoresi wants to merge 10 commits into
lmoresi wants to merge 10 commits into
Conversation
…e scope Hoist math, warnings, UnitAwareArray, RemeshPolicy and remap_var_set to module level and delete the repeated inline copies (sympy x3, math x2, warnings x2, RemeshPolicy x4, remap_var_set x1, UnitAwareArray x4, numpy-as-_np x1). Both underworld3.discretisation.remesh and underworld3.utilities.unit_aware_array are import-safe at ddt import time (verified: package import and DDt probe bit-identical). The IPython.display imports in the _object_viewer methods stay local by design (optional, notebook-only dependency) and now say so. The 'import underworld3 as _uw' copies inside the model-registration try/except blocks are left for D-49, which extracts that whole block. Verified: serial gate baseline 373 passed / 10 skipped / 2 xfailed / 1 xpassed; DDt numerical probe bit-identical to pre-change baseline. Underworld development team with AI support from Claude Code
…e unwrap dance The UnitAwareArray / .magnitude / non-dimensionalise unwrap block was copy-pasted six times (SemiLagrangian.initialise_history, the psi_star coords and the node/midpoint velocity blocks in update_pre_solve, and the coords + _eval_nd paths in update_forcing_history) with slightly different branch orderings that were not intentional. All six now call one module-level _to_nondim_ndarray(value, units=None), placed next to _as_float, whose docstring states the ND-space invariant (issue #267). Unified branch-order notes (all unreachable in practice, since uw.non_dimensionalise returns UnitAwareArray or plain arrays): - the velocity sites checked .value before falling through; the coords sites checked .magnitude; the helper checks UnitAwareArray, then .magnitude, then .value, then np.asarray. - the velocity sites passed the default model explicitly (non_dimensionalise(v, model)); the helper uses the default-model behaviour of non_dimensionalise(v), which is the same model. Verified bit-identical pre/post on both a no-units model (73-array DDt probe: SL scalar/vector order-2 variable-dt, Eulerian advective, Symbolic, Lagrangian, Lagrangian_Swarm) and a scaled units-active model (SL order-2 trace-back probe). Underworld development team with AI support from Claude Code
…ack velocity legs The node-point and mid-point velocity blocks in the SemiLagrangian RK2 characteristic trace-back were near-identical copies (evaluate vs global_evaluate routing, plus the same slice-rewrap and ND reduction), so unit-handling fixes on the hottest path had to be applied twice. Both legs now call one _velocity_nd_at(coords, use_global, evalf, subtract_v_mesh) method. Deliberate divergences between the two legs, now explicit parameters: - use_global: node points are rank-local (evaluate); mid-points may leave the partition (global_evaluate) - evalf is forwarded on the global path only (matching the original call signatures) - subtract_v_mesh carries the per-history-index ALE gating decided by the caller Verified bit-identical pre/post on the no-units DDt probe (73 arrays) and the scaled units-active SL trace-back probe. Underworld development team with AI support from Claude Code
…ve into phase helpers
The ~500-line method interleaved history-shift bookkeeping, the
current-field record step, unit reduction and the RK2 characteristic
trace — the mathematical core was buried under unit bookkeeping. Pure
code motion into intention-named helpers so the method now reads as its
phases: shift -> record -> trace -> sample:
- _shift_history_with_blend: psi*_i <- phi psi*_{i-1} + (1-phi) psi*_i
(phi = min(1, dt/dt_physical) time-lag blend)
- _centroid_shifted_node_coords: ND sample points nudged 0.1% toward
the owning cell centroid (point-location edge ambiguity)
- _record_current_field_into_history: direct-copy / evaluate /
projection dispatch for psi_star[0] (parallel band-aid and old-frame
carry comments move with the code)
- _nondim_timestep: dt reduced to plain ND model-time (#267)
- _trace_departure_points: x_mid = x - dt/2 v(x); x_dep = x - dt v(x_mid)
with the per-leg clamping rules (old-frame skips the departure clamp)
- _sample_history_at_departure: upstream sample + units re-wrap + store,
including the old-frame ephemeral-geometry sampling
No logic edits; all guard conditions, evaluator routing, exception
dispatch and explanatory comments move verbatim. Two dead diagnostic
locals deleted (coords_template / has_units — assigned, never read).
The per-iteration ALE gating comment and decision stay in the caller.
Verified bit-identical pre/post on the no-units DDt probe (73 arrays:
SL scalar/vector, variable dt, dt_physical blend, state round-trip)
and the scaled units-active SL trace-back probe.
Underworld development team with AI support from Claude Code
… flavor boilerplate ~250 lines of verbatim per-class boilerplate were repeated across the five DDt flavors (Symbolic, Eulerian, SemiLagrangian, Lagrangian, Lagrangian_Swarm): the model-registration try/except (including the identical 7-line '#195' comment x5), history-tracking init, BDF/AM coefficient creation + order-1 startup, effective_order, bdf_coefficients, bdf(), adams_moulton_flux(), the deprecated initiate_history_fn alias, and the state getter/setter validation. All now live once on a shared _DDtBase(uw_object); each flavor keeps only its storage and update_* sequencing. Net -244 lines. Deliberate per-flavor divergences are parameterized explicitly, never averaged away: - AM theta: Symbolic/Eulerian/SemiLagrangian pass self.theta; Lagrangian/Lagrangian_Swarm have no theta parameter and pass the fixed Crank-Nicolson 0.5 (call sites carry a comment) — both at construction (_init_coefficient_expressions) and on snapshot restore (_restore_core_state(am_theta=...)). - ETD-2 exp coefficients: created only for the Maxwell/viscoelastic flavors via with_exp=True (Symbolic, Eulerian, SemiLagrangian); the Lagrangian flavors never had them. - History symbols for bdf()/adams_moulton_flux(): storage-backed flavors contribute [ps.sym for ps in psi_star]; Symbolic overrides _history_syms() to return its raw sympy matrices. - Snapshot validation: the four storage-backed flavors check psi_star variable-name binding (_validate_psi_star_names); Symbolic instead checks psi_star list length and restores the expression list by value; SemiLagrangian additionally checks its with_forcing_history flag (kept in its own setter). - The inner 'import underworld3 as _uw' in the registration block is replaced by the module-level uw; the except (ImportError, AttributeError) narrowing and its #195 comment are preserved. - Symbolic's state setter originally restored psi_star before the coefficient re-derivation; it now restores psi_star first, then calls _restore_core_state — same effect, the re-derivation reads only dt/history metadata. update(), update_pre_solve/update_post_solve, initialise_history, psi_fn properties, _object_viewer and update_exp_coefficients remain per-flavor (genuinely divergent). uw_object's instance counter is global, so inserting the base does not perturb instance_number. Verified bit-identical pre/post on the no-units DDt probe (73 arrays + 47 symbolic reprs covering all five flavors, coefficient values, bdf/adams expressions and state round-trips) and the units-active SL probe. Underworld development team with AI support from Claude Code
…story_fn The copy -> evaluate -> projection dispatch was written as nested bare except: clauses with no types or intent comments, so a genuine evaluate() bug silently became an expensive projection solve. Now: - the mesh-variable direct-copy branch is an explicit 'if self._psi_meshVar is not None:' with the only realistic copy failure (ValueError layout/broadcast mismatch) narrowed and its fallthrough to pointwise evaluation stated; - the projection fallback swallows Exception from evaluate() only, with the sanctioned failure mode (derivative-bearing expressions) stated on the block (Charter section 4). Dispatch order and outcomes preserved: tracked-variable copy, else evaluate at psi_star nodes, else L2 projection. Underworld development team with AI support from Claude Code
… loop index
The SemiLagrangian working MeshVariable was registered as
'W_{instance}_{i}' where 'i' was a loop index leaked from the psi_star
construction loop — the suffix looked meaningful but was accidental,
and 'W' said nothing. Renamed to 'psi_work_sl_{instance}' and the
comment now states why the variable exists (potentially different
discretisation: swarm_degree / swarm_continuous).
Internal-only name: the variable is transient working storage, not part
of the snapshot state or any checkpoint contract; repo-wide grep shows
no other reference to the old name. Python attribute (_workVar) and
display symbol unchanged.
Underworld development team with AI support from Claude Code
… and state the tolerated failure The two 'try: register_remesh_hook(self) except Exception: pass' blocks (SemiLagrangian and Lagrangian __init__) swallowed everything with no stated tolerated failure — reading as fear, in contrast to the well-documented model-registration guard just above them. Narrowed to AttributeError (an older Mesh without the hook registry) with the sanctioned failure mode stated on the block (Charter section 4). Anything else now propagates instead of silently disabling adapt-time ALE for the history stack. Underworld development team with AI support from Claude Code
…meters Symbolic accepts and stores bcs and smoothing (and threads evalf through its update methods) although it has no projection solver or numerical evaluation to use them — they exist so callers can treat all DDt flavors uniformly. The class docstring's Parameters entries and a comment at the assignments now say so explicitly instead of implying they do something. Docstring/comment-only change (D-doc); signatures untouched. Underworld development team with AI support from Claude Code
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors src/underworld3/systems/ddt.py to improve readability and reduce duplication across the five DDt “flavors”, primarily by decomposing Semi-Lagrangian update_pre_solve() into phased helpers and consolidating shared history/coefficient/snapshot logic into a new _DDtBase.
Changes:
- Introduces
_DDtBaseto centralize shared BDF/Adams–Moulton bookkeeping, state validation, and restore logic across DDt implementations. - Refactors
SemiLagrangian.update_pre_solve()into smaller helpers and deduplicates unit/non-dimensional unwrapping and velocity evaluation paths. - Hoists repeated imports to module scope and tightens a few exception-handling cases (e.g., remesh hook registration, Eulerian history update fallback routing).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+213
to
+226
| carries_units = isinstance(value, UnitAwareArray) or hasattr(value, "magnitude") | ||
| if not carries_units and units and str(units) != "dimensionless": | ||
| value = UnitAwareArray(np.asarray(value), units=units) | ||
| carries_units = True | ||
| if not carries_units: | ||
| return value | ||
| nd = uw.non_dimensionalise(value) | ||
| if isinstance(nd, UnitAwareArray): | ||
| return np.array(nd) | ||
| if hasattr(nd, "magnitude"): | ||
| return nd.magnitude | ||
| if hasattr(nd, "value"): | ||
| return nd.value | ||
| return np.asarray(nd) |
Member
Author
There was a problem hiding this comment.
Verified — pre-existing, tracked as #328; the extraction did not widen exposure.
Probes at the branch head (amr-dev env, reference scales active):
uw.non_dimensionalise(pint.Quantity)does crash (TypeError: only 0-dimensional arrays can be converted to Python scalars) — that is the open issue uw.non_dimensionalise(pint.Quantity) crashes: invalid dimensionality= kwarg in units.py protocol 5 #328, whose defect is inunits.pyprotocol 5 (invaliddimensionality=kwarg; a TODO(BUG) marker already sits at that site).uw.function.evaluate/global_evaluatereturn onlyUnitAwareArrayor plain ndarray — never a rawpint.Quantity— andUnitAwareArray − UnitAwareArray(the ALE subtraction) staysUnitAwareArray. So no DDt data path actually feeds pint into the helper.
Base-vs-head semantics of the six unified sites:
- Four sites (both coords unwraps, the psi_star coords unwrap, the forcing-result unwrap) had the identical
hasattr(x, "magnitude") → uw.non_dimensionalise(x)gate at the base, so a pint input crashed there before this PR too. - The two velocity sites gated on
isinstance(UnitAwareArray)at the base, but their inputs come exclusively fromevaluate/global_evaluate(see probe above), so the broadened gate routes nothing new in practice. - The helper omitting the
modelargument is equivalent:non_dimensionalise(x, model=None)defaults touw.get_default_model(), which is exactly what the base sites passed.
Per the readability-wave contract the behaviour is preserved deliberately; the fix belongs in units.py under #328. Added a comment-only TODO(BUG): ... see #328 marker at the helper in 74d5169 (ddt test files 0857/1052/1053/1054: 24 passed).
Underworld development team with AI support from Claude Code
…ty non_dimensionalise crash (#328) Copilot review of #343 flagged that the unified unwrap helper passes anything with .magnitude into uw.non_dimensionalise(), which crashes for raw pint.Quantity when reference scales are active. Verified pre-existing: four of the six unified sites had the identical hasattr("magnitude") gate at the base, and the two velocity sites only ever receive UnitAwareArray / plain ndarray from uw.function evaluate/global_evaluate (probed). The defect itself is in units.py protocol 5 and is tracked as #328; comment- only marker here, no behaviour change. Underworld development team with AI support from 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.
Wave D readability rewrites for
src/underworld3/systems/ddt.py(group ddt, rows D-46..D-56 ofdocs/reviews/2026-07/REMEDIATION-WORKLIST.md; evidence indocs/reviews/2026-07/READABILITY-REVIEW.md). Behaviour-neutral, mechanically verified. Base: development @ 8a94f67.Per-row status
SemiLagrangian.update_pre_solve(~500 lines) decomposed into six phase helpers; now reads shift -> record -> trace -> sample (~145 lines incl. docs). Two dead diagnostic locals (coords_template/has_units— assigned, never read) deleted._to_nondim_ndarray(value, units=None)(docstring states the ND-space invariant, issue #267) replaces the x6 copy-pasted unwrap dance; unintentional branch-ordering differences unified (noted in the commit body)._velocity_nd_at(coords, use_global, evalf, subtract_v_mesh)replaces the near-identical node/midpoint velocity blocks on the trace-back hot path; the two legs' deliberate divergences (evaluate vs global_evaluate routing, evalf forwarded on the global path only, per-slot ALE gating) are the explicit parameters._DDtBase(uw_object)absorbs the ~250-line quintuplicated boilerplate (model-registration try/except incl. the 7-line #195 comment x5, history/coefficient init,effective_order,bdf_coefficients,bdf(),adams_moulton_flux(),initiate_history_fn, state getter/setter validation). Net -244 lines. Divergences parameterized, listed in the commit body: AM theta (self.theta vs fixed CN 0.5 on the Lagrangian flavors),with_exp(ETD-2 coeffs on Symbolic/Eulerian/SL only),_history_syms()(Symbolic's raw matrices vs.sym), snapshot validation shape (name-binding vs list-length; SL's forcing flag stays local).self.thetaon restore; nothing remained at the remediation base (verified by grep before acting). D-49 preserves it via_restore_core_state(s, am_theta=self.theta).Eulerian.update_history_fnnested bare-except dispatch replaced by explicitif self._psi_meshVar is not None:+ narrowed excepts (ValueErroron the direct copy,Exceptionfromevaluate()only), each swallow stating its sanctioned failure mode.W_{instance}_{i}->psi_work_sl_{instance}(theiwas a leaked loop index); comment states why the variable exists. Internal-only name, repo-wide grep clean, no deprecation alias needed.register_remesh_hookswallows narrowed toAttributeError+ one-line sanctioned-failure comment ("older Mesh without the hook registry").Symbolic'sbcs/smoothing(and threadedevalf) documented as accepted-for-interface-parity, unused; signatures untouched.Gates (exact counts)
pytest tests/ -m "level_1 and tier_a" -q:Bit-identical checks
Two probe scripts run after every refactor commit, outputs compared bit-for-bit (
np.array_equalon every array + exact string equality on sympysreprs):dt_physicalblend step, SL vector, Eulerian with the V_fn advection correction, Eulerian on an evaluable expression and on a derivative expression (all three D-51 dispatch legs), Symbolic, Lagrangian, Lagrangian_Swarm; per-step psi_star data, BDF/AM/exp coefficient values,bdf()/adams_moulton_flux()expression trees, and state snapshot/restore round-trips.Both BIT-IDENTICAL after every commit (D-53, D-47, D-48, D-46, D-49) and at the final state.
The single parallel failure is PRE-EXISTING at the remediation base (verified by rebuilding base 8a94f67's ddt.py and re-running):
test_0765_internal_boundary_integral_mpi.py::test_deformed_spherical_shell_boundary_area_parallelfails withRuntimeError: Mesh._deform_mesh() is an internal primitive — it moves nodes...— the test drivesMesh._deform_mesh()directly and trips a guard in mesh territory (wave-d-mesh's file); reported to the campaign rather than patched here (scope discipline, Charter section 9).TODO(BUG) markers placed
None — no out-of-scope defects found beyond BF-15 (already tracked in the worklist's own table).
References:
docs/reviews/2026-07/READABILITY-REVIEW.md(READ-17/18/19/20/45/47/87/89/90/91/92).Underworld development team with AI support from Claude Code