refactor(pyx-safe): naming, docs, dead code in the solver layer (D-14..22, WA-05..09, WA-18)#366
Merged
Conversation
…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
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
…ion 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
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
…es (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
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
…nstructors (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
… 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
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
…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
…m 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
… (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
…s (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
… 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
…x 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
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.
The last remediation group of the 2026-07 quality campaign that can run without the maintainer present: naming, comments/docstrings, dead code, and byte-identical dedents ONLY, in the Cython solver layer. The structural multi-solver unification (D2, rows D-23..D-28) remains OUT of scope per maintainer ruling and is untouched. Base:
development@ 045eac0.Per-row status
_maybe_install_snes_update->_attach_snes_update_hook— the exact name used on the unmergedfeature/snes-update-callbackstip (b82acea), so its rebase stays trivial. Rename only (2 sites); allowlist entry removed (see below).if u_Field is None: ... else: ...inSNES_Scalar.__init__;SNES_Vectorauto-create switched from truthiness tois None(WC-10 verified the auto-create already existed). Behaviour-identical: theUnknowns.usetter ignoresNone(verified in source). The suspiciousnum_components=mesh.dimon the default SCALAR var is kept and flaggedTODO(BUG).diffFIRST (14 lines each), then dedented to run once; the misleading# Now go back to the original plancomment replaced with one that states the control flow._convergence_reasons = {code: (NAME, explanation)}(contents identical to the fuller of the two old copies);get_snes_diagnosticsand_warn_on_divergenceboth format from it. No external consumers (grep)._nondimensional_timehelper; the six copies verified verbatim (5 at one indent, 1 deeper) before extraction; nothing parameterized.dim->cdimin the three scopes that bindmesh.cdim(add_nitsche_bc,SNES_Vector._setup_pointwise_functions— which bound both names to the same value — andSNES_MultiComponent._setup_pointwise_functions); no topologicalmesh.dimuse exists inside any of them (verified). Staleline ~173comment now refers to_setup_discretisationby name.TODO(DESIGN):(the Vector/Scalar-unification musing now points at the deferred D-23..D-28 rows).snes_max_it = 50clobber commented: hardcoded, silently overrides userpetsc_optionseach solve; respecting the user setting is deferred to the benchmarked D2 sub-wave per ruling D18. No behaviour change.raise("string")(a TypeError at raise time) ->TypeError/ValueErrorwith messages verbatim:add_conditionf_id (TypeError), c_type (ValueError), components fallback (TypeError),_get_dof_partition_by_field_idsection_type (ValueError).if True: # c_label and label_val != -1:constant guards deleted, bodies dedented (SNES_Vector + Stokes DS wiring).xmlrpc.client.Boolean,sympify(only qualifiedsympy.sympifyis used),TypeAlias,class_or_instance_method— each verified unreferenced, deleted.'robust'/'fast'passbranches deleted; unknown strategy ->ValueError; the kaskade-vs-additivepc_mg_typedivergence commented ("do not change without benchmarking"). No option VALUES changed. No internal/test code assigns.strategy(grep), so the ValueError has no reachable caller today.petsc_dm_get_periodicityblock + its## Todo !header deleted frompetsc_discretisation.pyx.uw.pprint(0, msg)inpetsc_dm_find_labeled_points_localpassed0as a message (procis keyword-only); dropped — same fix as the x8 in #344.Mechanical verification (dedup/dedent commits)
Every dedup/dedent commit was verified beyond the test gates:
__PYX_ERR/lineno numbers, goto labels, comments stripped): remaining hunks are exclusively code-object first-line metadata shifted by the two deleted guard lines and constant-pool repacking. The generated function bodies are identical (Cython constant-foldsif True:)...._line_3057->..._line_3033) plus metadata; bodies identical (comment-only change).SNES_Stokes_SaddlePt.solve(the duplicated call sequence collapsing to a single dedented copy, same operations in the same order).get_snes_diagnosticsoutput identical to the pre-change table for every reason code in -12..5;_warn_on_divergenceidentical except codes -9/-10/-11, which now printDIVERGED_DTOL/DIVERGED_JACOBIAN_DOMAIN/DIVERGED_TR_DELTAinstead ofUNKNOWN(...)— precisely the gap READ-72 flagged (code 0 remains unreachable there)._nondimensional_timeequals the old inline logic for float, int,np.float64, PintQuantity, andUWQuantityinputs.Gates (identical pre/post; env rebuilt
rm -rf build/ && ./uw buildbefore every gated run; suites strictly serial)pytest -m "level_1 and tier_a"mpirun -np 2 pytest --with-mpi(test_1017/1062/1064 parallel)scripts/check_deprecated_patterns.py* The six failures are pre-existing at base (confirmed by two independent base runs):
test_1051_VE_shear_box(order2_converges, order2_better_than_order1, steady_state_approach) andtest_1052_VEP_stability_regression(vep_yield_lock_fixed_dt, vep_snes_no_divergence_loading_through_yield, pure_ve_variable_dt_accuracy) — the known VE/VEP territory.Allowlist ratchet — first shrink
D-14 removes the
src/underworld3/cython/petsc_generic_snes_solvers.pyx:hedging-nameentry fromscripts/deprecated_pattern_allowlist.txt(87 -> 86 allowlisted hits); the scanner verified green with the entry gone.Note for maintainers
bugfix/yield-homotopy(KEEP_ACTIVE) edits this file and will need a shallow rebase — all changes here are local (renames, comment/dead-code deletion, two small data-motion extractions); no reordering or signature changes.feature/snes-update-callbacksrebases trivially by construction (D-14 chose its name).Underworld development team with AI support from Claude Code