Skip to content

Issue quick-fix batch 2: six small independent fixes (#415 #447 #413 #411 #451 #414) - #456

Merged
lmoresi merged 2 commits into
developmentfrom
bugfix/quickfix-batch2
Jul 28, 2026
Merged

Issue quick-fix batch 2: six small independent fixes (#415 #447 #413 #411 #451 #414)#456
lmoresi merged 2 commits into
developmentfrom
bugfix/quickfix-batch2

Conversation

@lmoresi

@lmoresi lmoresi commented Jul 28, 2026

Copy link
Copy Markdown
Member

Summary

Six small, independent fixes, each with fail-first evidence where demonstrable. Closes #415, closes #447, closes #413, closes #411, closes #451, closes #414 (manual close on merge — PRs target development).

Issue Fix
#415 UWexpression.is_comparableFalse (plain-Symbol semantics). sympy.Max/Min(expr, number) previously crashed on Float internals (_prec); now constructs symbolically, resolves on unwrap, stays live to constant ramps. Pre-fix AttributeError reproduced.
#447 Parameter assignment guards both self-reference cycles: read-back write is a no-op (was: container nested in itself → RecursionError on .value); composite P.x = P.x * 2 snapshots current contents. Pre-fix RecursionError reproduced.
#413 2D boundary-flux recovery detects trace order from the reaction data and assembles the matching P1 two-node / P2 three-node line mass (lumped + consistent); mixed traces raise legibly. Was a bare KeyError on P1.
#411 compute_volume_residual_fields + compute_boundary_residual_fields get the #407 DMPlexInsertBoundaryValues insert (with residual_time plumbing). Regression test pins exact agreement with _assemble_volume_reaction and fails on the pre-fix build.
#451 Spiegelman notch example builds gmsh geometry in ND units (S = 1.0) — was 30× too large in every direction.
#414 Curved-boundary warning (P2 vertices slow, midpoints superconvergent, with measured numbers) on the three consumer docstrings.

Validation

Adversarial review to follow on this thread before merge, per project policy.

Underworld development team with AI support from Claude Code

… P1 trace flux, residual-variant inserts, example scale, curved-boundary caveats

Six small, independent fixes:

- #415: UWexpression.is_comparable now returns False (plain-Symbol semantics).
  Advertising the wrapped contents' comparability made sympy.Max/Min attempt an
  immediate numeric comparison and crash on Float internals (_prec). Max/Min of
  an expression and a number now construct symbolically and resolve on unwrap,
  staying live to later constant ramps.

- #447: constitutive Parameter assignment guards the two self-reference cycles:
  a read-back write (p = Params.x; Params.x = p) is a no-op instead of nesting
  the container in itself (RecursionError on the next .value), and a composite
  self-reference (Params.x = Params.x * 2) snapshots the current contents.

- #413: 2D boundary-flux recovery detects the trace order from the reaction
  data (edge-midpoint DOFs present or not) and assembles the matching P1
  two-node or P2 three-node line mass, lumped and consistent; mixed traces
  raise legibly. Previously P1 traces died with a bare KeyError.

- #411: the two Stokes-only residual diagnostics (compute_volume_residual_fields
  and compute_boundary_residual_fields) get the same DMPlexInsertBoundaryValues
  insert as _assemble_volume_reaction (#407) — they still evaluated against
  zero-filled essential values and returned garbage on g != 0 walls. Regression
  test pins exact agreement with the validated core and fails on the pre-fix
  build.

- #451: the Spiegelman notch example builds its gmsh geometry in
  non-dimensional units of H (S = 1.0) — building in km made the solved domain
  30x too large in every direction.

- #414: curved-boundary warning (P2 vertex values converge slowly; midpoints
  superconvergent) on boundary_flux, boundary_normal_traction, and
  dynamic_topography docstrings, with the measured numbers.

Gate: level_1+tier_a green in the worktree; new tests shown to fail pre-fix
(#415 AttributeError, #447 RecursionError, #411 divergence) where demonstrable.

Underworld development team with AI support from Claude Code
Copilot AI review requested due to automatic review settings July 28, 2026 06:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@lmoresi

lmoresi commented Jul 28, 2026

Copy link
Copy Markdown
Member Author

Adversarial review — live probes on the batch worktree

Verdict: APPROVE with one scope caveat, addressed in a response commit. All six changes hold under attack. The caveat: the #447 guard as submitted covers the two direct cycles but not a self-reference nested inside a wrapper expression — and that hole's worst mode is silent garbage, not a crash. Hardened in the response commit (recursive cycle detection → legible ValueError), so #447 closes fully. One pre-existing (not-a-regression) P3 defect discovered en route is filed separately.

Findings

MAJOR (scope) — deeper #447 cycle was unguarded — PROBED, fixed in response commit. wrapper = uw.expression("w", Params.yield_stress * 2); Params.yield_stress = wrapper was accepted; afterwards constitutive_model.fluxRecursionError (the original crash site) and unwrap_expression(...)silently returns 2⁵⁰·τ_y — the cycle unrolled ~50 times against an internal depth cap and handed back as a live expression. The response commit adds recursive containment detection through nested UWexpression contents (visited-set bounded): any assignment that would create a cycle through wrappers now raises a ValueError telling the user to snapshot explicitly. Cross-container cycles (a→b→a) are caught by the same walk.

MINOR — Matrix-valued composite snapshot produced malformed sympy — PROBED, fixed in response commit. xreplace of a Matrix-valued container inside a Matrix entry stored nested-matrix garbage silently. Now raises for Matrix-shaped contents instead of substituting.

NOTE — pre-existing P3 silent-wrong boundary flux — PROBED both sides, filed as a separate issue. A degree-3 trace returns finite but wrong flux (−0.57…−0.74 vs exact −1.0) — byte-identical on origin/development: both P3 edge DOFs collapse onto the midpoint coordinate key and one reaction overwrites the other, on the old code and the new. Not a regression from this batch; the recovery should reject degree > 2 traces explicitly.

NOTE — doc touch-ups, fixed in response commit: stale "km" comment in the Spiegelman example (line ~123) contradicting the corrected framing; "Flat boundaries are pointwise exact" softened to "exact up to solver tolerance" (the tests assert 1e-3).

Attacks that failed (evidence)

Adversarial review per project policy — Underworld development team with AI support from Claude Code

…ix snapshot guard, doc touch-ups

From the adversarial review of this PR:

- The #447 guard now detects cycles RECURSIVELY through nested UWexpression
  contents (visited-set bounded walk): a self-reference hidden inside a wrapper
  expression — previously accepted, then either RecursionError at flux build or
  a silently ~2^50-scaled result from the depth-capped unwrap — is rejected with
  a legible ValueError, in both the reference branch and the plain-sympy branch.
  Cross-container cycles (a -> b -> a) are caught by the same walk.
- Matrix-valued composite snapshot raises instead of xreplace-ing a Matrix into
  a matrix entry (malformed sympy stored silently).
- Stale 'coordinate units (km)' comment in the Spiegelman example corrected;
  'pointwise exact' softened to 'exact up to solver tolerance' in the
  boundary_flux warning.
- New regression: wrapper-cycle assignment raises, parameter unmodified,
  flux still builds.

Underworld development team with AI support from Claude Code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants