Skip to content

fix(mmpde): per-cell collapse floor — mover was a silent no-op on graded meshes (#419)#420

Open
lmoresi wants to merge 1 commit into
developmentfrom
bugfix/mmpde-graded-area-floor
Open

fix(mmpde): per-cell collapse floor — mover was a silent no-op on graded meshes (#419)#420
lmoresi wants to merge 1 commit into
developmentfrom
bugfix/mmpde-graded-area-floor

Conversation

@lmoresi

@lmoresi lmoresi commented Jul 25, 2026

Copy link
Copy Markdown
Member

Closes #419.

The bug

The MMPDE line-search accept test used a single absolute collapse floor derived from the median cell volume:

a0_own_med = _global_mean(float(np.median(np.abs(signed_vol(coords, cells_own)))))
a_min_floor = float(area_floor_frac) * a0_own_med
...
if np.all(np.isfinite(trial)) and _min_area(trial) > a_min_floor:

1. It disabled the mover on any graded mesh. The floor is 1% of the median cell volume. Anything out of mesh.adapt has finest cells orders of magnitude below the median, so they start already under the floor. Every trial step was rejected, the line search backtracked to scale=0, and the mover returned having moved nothing — silently:

mmpde outer 1/8: I=1.991126e+00 dI=+0.00e+00 scale=0.000 max|dx|=0.00e+00

2. It was partition-dependent. A _global_mean of per-rank medians is not the global median, so which cells fell foul of the floor depended on how cells were spread across ranks. The same mesh moved at np=1 and stood completely still at np=2.

This affected redistribute_nodes / node_redistribution generally, not just new code.

The fix

Floor each cell against its own starting volume: no cell may shrink below area_floor_frac of what it started as. Scale-free, grading-free, partition-independent, and still a strict no-fold certificate — a positive ratio certifies positive volume. area_floor_frac keeps its name and default; only its reference changes.

Tests

New tests/test_0751_mmpde_graded_area_floor.py (level_1, tier_a):

  • test_mover_moves_on_a_graded_mesh — asserts the mesh is genuinely graded first, so the test cannot pass without exercising the floor, then asserts the mover actually moves.
  • test_collapse_guard_still_rejects_folding — the guard must still do its job: no cell folds, and none collapses below the documented fraction of its own start.

54 existing tests pass across the mover, follow_metric, graded adapt and mesh smoothing.

Reviewer notes

  • The min(initial=np.inf) in _min_area_frac is deliberate: a rank owning no cells must contribute the identity for the global MIN rather than raising on an empty reduction.
  • Worth checking: area_floor_frac=0.01 now means something stricter in the fine region than it used to (1% of a small cell, not 1% of the median), so a previously-accepted step that shrank a fine cell hard will now be backtracked. That is the intended reading, but it is a behaviour change for well-graded meshes that were already moving.
  • Not addressed here: the mover still reports no warning when the line search backtracks to scale=0 for any other reason. A silent no-op is hard to diagnose — probably worth a follow-up warning.

Underworld development team with AI support from Claude Code

…ded meshes (#419)

The MMPDE line-search accept test used a single ABSOLUTE collapse floor,
`area_floor_frac x median cell volume`, with the median taken as a
`_global_mean` of per-rank medians. Two defects followed.

Any graded mesh disabled the mover outright. Anything out of `mesh.adapt`
has finest cells orders of magnitude below the median, so those cells
start already under the floor; every trial step failed the test, the line
search backtracked to `scale=0`, and the mover returned having moved
nothing — with no warning:

    mmpde outer 1/8: I=1.991126e+00 dI=+0.00e+00 scale=0.000 max|dx|=0.00e+00

And the behaviour depended on the partition, since a mean of per-rank
medians is not the global median: the same mesh moved at np=1 and stood
completely still at np=2.

Floor each cell against its OWN starting volume instead — no cell may
shrink below `area_floor_frac` of what it started as. Scale-free,
grading-free, partition-independent, and still a strict no-fold
certificate (a positive ratio certifies positive volume). The kwarg
keeps its name and default; only its reference changes.

This affected `redistribute_nodes` / `node_redistribution` generally,
not just new code. Found while building shape relaxation on the mover.

Underworld development team with AI support from Claude Code
Copilot AI review requested due to automatic review settings July 25, 2026 10:27

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 25, 2026

Copy link
Copy Markdown
Member Author

Adversarial review (author, pre-review)

Trying to break my own fix. Four things a reviewer should push on.

1. area_floor_frac silently changes meaning, and 0.01 may now be too strict. Previously it was 1% of the median cell volume — for a fine cell in a graded mesh that was a very loose bound (the cell could shrink to nothing and still clear it). Now it is 1% of that cell's own volume. Any workflow that legitimately compresses a cell by more than 100x within a single mover call will now backtrack where it previously did not. Free-surface collapse and strong re-grading are the plausible victims. I have not tested those paths. If someone is relying on large single-call compression, this is a regression for them and the default may need to be looser (1e-3?).

2. It caps how much redistribution one call can do. A cell cannot shrink below 1% of its entry volume in this call, no matter what the metric asks. Repeated calls compose fine, but a single redistribute_nodes onto a very aggressive metric is now bounded in a way it was not. Arguably correct (that is what a collapse guard is for) but it is a new constraint, not a restoration of intended behaviour.

3. Degenerate input cells. np.maximum(a0_own, 1e-300) means a cell that arrives with ~zero volume gets an essentially infinite ratio, so the floor never binds for it. Folding is still caught (the ratio goes negative), but a sliver that arrives degenerate is effectively exempt from the collapse half of the guard. That is probably the right call — you cannot floor against zero — but it should be a conscious decision, not an accident of the clamp.

4. The test does not actually reproduce the original parallel failure. The sharpest form of this bug was partition-dependent: np=1 moved, np=2 did not. My test is serial and only asserts "a graded mesh moves". It would catch a reintroduced median-based floor, but it would not catch a re-introduction of a partition-dependent reduction. A proper regression would compare max|dx| at np=1 vs np=2. I did verify that by hand (both now move, energies agree to ~3 digits) but did not automate it, and the repo has form for rank-local diagnostics slipping back in.

Also not fixed here: the mover still emits no warning when the line search backtracks to scale=0. That silence is what made this bug expensive to find — it presented as "adaptation does nothing" with no diagnostic. Worth a follow-up.

What I am confident about: the no-fold guarantee is preserved (a positive volume ratio certifies positive volume), the reduction is partition-independent by construction, and the empty-rank path is handled (min(initial=np.inf)).

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