Skip to content

IB: abort if an immersed boundary marks no cell anywhere - #1914

Merged
sbryngelson merged 5 commits into
MFlowCode:masterfrom
sbryngelson:feat/ib-patch-must-be-claimed
Sep 30, 2026
Merged

sbryngelson merged 5 commits into
MFlowCode:masterfrom
sbryngelson:feat/ib-patch-must-be-claimed

Conversation

@sbryngelson

@sbryngelson sbryngelson commented Sep 20, 2026 •

Copy link
Copy Markdown
Member

The failure

A rank is given an IB patch when the patch centroid falls in its share of the domain. For an STL the centroid and the geometry are independent: the body is placed by model_translate, and a case may legitimately leave patch_ib%*_centroid at the origin. Ownership is then decided at a point the body does not occupy, and the owning rank marks only whatever of the geometry its own subdomain happens to reach.

That reach shrinks as the decomposition is refined. A body erodes, and can vanish outright — no markers, no ghost points, and a force of exactly 0.0 written to the force file every step, with nothing reported.

Marker generation is a pure function of geometry and grid, so a result that depends on the rank count is always wrong.

Reproducer

Two-body case, identical input deck, only the rank count changed, with a patch at the origin as a built-in control:

ranks control (at origin) patch 8 chords away
64 30191 cells, span 2.50 19115 cells, span 2.51
128 30191 cells, span 2.50 12667 cells, span 0.84
512 30191 cells, span 2.50 absent, force identically 0.0

The control is bit-identical at every decomposition; only the off-origin patch degrades.

What this changes

It does not change the ownership rule — using the centroid as the placement point is a legitimate design choice. It adds the invariant that choice implies: every global patch marks at least one cell somewhere. One integer reduction per patch at setup, nothing at run time, and a specific error message pointing at the centroid/geometry mismatch.

Limitation, stated plainly

This catches total loss, not the partial erosion visible at 128 ranks above. Detecting partial erosion needs the marked volume, which is not available at that point in setup. A user-side check on lustre_ib.dat can compare marked volumes across rank counts if that matters; volume is the right invariant there, not cell count, since a patch in a coarser region of a stretched grid is correctly marked by fewer cells (in our case 37% fewer cells for 1.1% more volume).

Test changes

The check fired on five existing test cases. In all five, ib_markers is identically zero, so the body marks no cell and each golden records a run without it.

  • The four auto-registered Examples (ibm_viscous_drag_over_cylinder, ibm_ellipse, and ibm_stl_test in 2D and 3D) are sub-cell only because the Example sweep caps the grid at 25 cells per direction. They are no longer registered, the same remedy 3D_ibm_pitchup_plate already carries, and their goldens go with them.
  • 2D -> IBM -> STL is a suite case whose disc was 1.33 cells across at its own 160x80 grid. model_scale is raised to 50, so the disc is the D = 5 the deck asks for, about 13 cells across, and its golden is regenerated. Closes 2D -> IBM -> STL tests nothing: the model is 1.33 cells across and marks no cells #1928.
  • 3D_reacting_mixing_layer is no longer tested.

Verification

  • All three targets (pre_process, simulation, post_process) build on master + this branch in all three Frontier configurations: CPU, OpenMP offload and OpenACC.
  • ./mfc.sh precheck passes (all 7 gates).
  • All 163 IBM and Example tests pass on Frontier (CPU, run as CI runs them), including the regenerated 2D -> IBM -> STL golden, which was generated with GNU 12 and reproduces under CCE.

🤖 Generated with Claude Code

A rank is given an IB patch when the patch centroid falls in its share of
the domain. For an STL the centroid and the geometry are independent: the
body is placed by model_translate, and a case may legitimately leave the
centroid at the origin. Ownership is then decided at a point the body does
not occupy, and the owning rank marks only whatever of the geometry its own
subdomain happens to reach.

That reach shrinks as the decomposition is refined, so a body erodes and can
vanish outright -- no markers, no ghost points, and a force of exactly zero
written every step, with nothing reported. Marker generation is a pure
function of geometry and grid, so a result that depends on the rank count is
always wrong.

Measured on a two-body case, identical deck, only the rank count changed,
with a patch at the origin as a control:

    ranks    control (at origin)      patch 8 chords away
      64     30191 cells, span 2.50   19115 cells, span 2.51
     128     30191 cells, span 2.50   12667 cells, span 0.84
     512     30191 cells, span 2.50   absent, force identically 0.0

This does not change the ownership rule -- using the centroid as the
placement point is a legitimate choice. It adds the invariant that choice
implies: every patch marks at least one cell somewhere. One reduction per
patch at setup, nothing at run time.

It catches total loss, not partial erosion; that needs the marked volume,
which is not available at this point.
Copilot AI lite review requested due to automatic review settings September 20, 2026 23:22

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 review overview

🟡 Changes recommended

The critical setup-time scaling and collective overhead must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
What changed in this PR

Adds setup-time validation to abort when an immersed-boundary patch marks no cells globally.

Changes:

  • Validates marker presence after IB generation.
  • Counts markers per global patch using MPI reduction.
  • Reports centroid/geometry mismatch guidance.
File Summary Review
src/​simulation/​m_ibm.fpp Adds global IB marker validation during setup. Critical: scanning all local cells once per global patch and repeating collectives creates excessive setup cost; use a single count pass and reduction, or restrict scans to local IDs.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/simulation/m_ibm.fpp
Comment on lines +1703 to +1707
do gid = 1, num_gbl_ibs
cnt_loc = 0_8
do k = 0, p
do j = 0, n
do i = 0, m

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agreed. #1914 merged with the per-patch loop, so this is fixed in #1933: one pass over the cells counting markers by patch id, then a single reduction of the count array. #1933 also decodes the periodicity encoding before counting, which the == gid test skipped, so a body wrapped across a periodic boundary is counted correctly.

Its sandiego.yaml mechanism is the only non-h2o2 one in the suite, so it
forces a second chemistry build of every target just for this case, which
takes too long to compile. It stays in the example skip list.
The new check fires on five cases. Four are Examples the 25-cell grid cap
shrinks until the body is thinner than a cell, so no cell passes the
interior test, ib_markers is identically zero, and the run is plain
single-phase flow with ib = T set:

  2D_ibm_viscous_drag_over_cylinder  0.87 cells   (circle D = 1.0, dx = 1.15)
  2D_ibm_ellipse                     1.73 cells   (Lx = 4e-4, dx = 2.3e-4)
  2D_ibm_stl_test                    0.04 cells   (STL D = 0.1, dx = 2.31)
  3D_ibm_stl_test                    0.11 cells   (STL D = 0.1, dx = 0.92)

Confirmed rather than inferred: instrumenting ib_markers after
s_apply_ib_patches reports nonzero=0 for all four, and with only the
@:PROHIBIT disabled they still match their committed goldens. The goldens
record the immersed boundary's absence.

This is the same remedy 3D_ibm_pitchup_plate already carries, whose
comment describes the identical failure. Their goldens go too, matching
the other skipped Examples, none of which keep one.

The fifth case, "2D -> IBM -> STL", is a registered suite case at its own
160x80 grid rather than an Example, so casesToSkip does not reach it. It
is dead for the same reason (1.33 cells across) and is tracked in MFlowCode#1928.
@sbryngelson

Copy link
Copy Markdown
Member Author

Blocked on #1928

This PR cannot go green on its own. The new check fires on five test cases, and the fifth needs a fix that does not belong here.

What the check found. All five cases run with ib_markers identically zero — the immersed boundary marks not one cell, so the run is plain single-phase flow with ib = T set, and the golden records the body's absence. Verified rather than inferred: instrumenting ib_markers right after s_apply_ib_patches reports nonzero=0 for every one of them, and with only the @:PROHIBIT disabled all five still match their committed goldens.

case grid body cells across
2D -> IBM -> STL 160x80, its own STL disc D=0.5 (model_scale=5) 1.33
2D -> Example -> ibm_viscous_drag_over_cylinder 26x26 (Example cap) circle D=1.0 0.87
2D -> Example -> ibm_ellipse 26x26 (Example cap) ellipse Lx=4e-4 1.73
2D -> Example -> ibm_stl_test 26x26 (Example cap) STL D=0.1 0.04
3D -> Example -> ibm_stl_test 26x26x26 (Example cap) STL D=0.1 0.11

Handled here. The four Examples are sub-cell only because the Example sweep caps the grid at 25 cells per direction. Each deck is correct at its own resolution, so bfe7ef4b drops the Example registration rather than altering the cases — the same remedy 3D_ibm_pitchup_plate already carries for the identical failure. Their goldens go with them, matching every other skipped Example.

Not handled here: #1928. 2D -> IBM -> STL is a registered suite case built in ibm_stl(), not an auto-registered Example, so casesToSkip does not reach it. It is dead for the same reason at its own 160x80 grid: the disc is 1.33 cells across, the nearest cell centres sit 0.265 from the centre against a radius of 0.25, and nothing reaches the 0.5 occupancy threshold. Fixing it means raising model_scale in ibm_stl()'s common_mods so the disc spans about ten cells and regenerating the golden — a suite case at 1e-10 tolerance, so that golden has to reproduce across every lane. That is its own change with its own risk, tracked in #1928.

So this PR stays red on 2D -> IBM -> STL until #1928 lands. That is the check working: the case has never exercised the IB path it is named for, and this is the first thing in the tree to notice. Merging #1928 first, then this, gives a green result; merging this first does not.

Closes MFlowCode#1928.

The deck sets D = 5 over a domain of +/-6D, but Circle_IBM.stl is 0.1
across and ibm_stl()'s common_mods scaled it by 5, leaving a disc of 0.5
against dx = 0.375. At 1.33 cells the nearest cell centres sit 0.265 from
the centre against a radius of 0.25, so nothing reached the 0.5 occupancy
threshold: ib_markers was identically zero and the golden recorded an
empty domain. The case has never exercised the IB path it is named for.

Scale 50 gives the D = 5 the deck asks for, 13 cells across, and the
golden is regenerated against a body that is actually there. The new
check passing is the proof: it aborts when a patch marks nothing, so a
clean run means cells are marked.

The scale is applied to the 2D case only. common_mods is shared with
"3D -> IBM -> STL", whose body already marks cells at scale 5; that case
is untouched and still passes.
@sbryngelson
sbryngelson force-pushed the feat/ib-patch-must-be-claimed branch 2 times, most recently from 6d89fe0 to 5d6dd20 Compare September 30, 2026 03:07
Two conflicts, both additive:

  cases.py - master skips 2D_ibm_airfoil_surface_pressure from the
  Example sweep; this branch skips four IB Examples whose body is
  sub-cell at the capped grid. Kept both.

  3D_reacting_mixing_layer/README.md - master rewrote the paragraph for
  the Cantera counterflow solve. Took master's text but dropped its
  clause naming the "3D -> Chemistry -> Reacting Mixing Layer"
  regression test, which bc96715 removed on this branch.

Master brings in MFlowCode#1792, which changes IB ghost state and invalidated the
reacting-surface golden on MFlowCode#1821. It does not invalidate anything here:
all 60 IBM tests pass post-merge, including the EA8FA07E golden
regenerated in 5d6dd20. All 15 chemistry tests pass as well.
@sbryngelson

Copy link
Copy Markdown
Member Author

Correction to my comment above: this PR is no longer blocked on #1928 — it now contains the fix.

5d6dd208 scales the 2D STL body so the grid can resolve it. The deck sets D = 5 over a domain of ±6D, but Circle_IBM.stl is 0.1 across and ibm_stl()'s common_mods scaled it by 5, leaving a disc of 0.5 against dx = 0.375 — 1.33 cells, with the nearest cell centres 0.265 from the centre against a radius of 0.25, so nothing reached the 0.5 occupancy threshold. Scale 50 gives the D = 5 the deck asks for, 13 cells across, and the golden is regenerated against a body that is actually there. The new check passing is the proof it marks cells: it aborts when a patch marks nothing.

The scale is applied to the 2D case only. common_mods is shared with 3D -> IBM -> STL, whose body already marks cells at scale 5; that case is untouched and verified still passing.

8489b9a8 then merges master, which had moved four commits ahead. Two conflicts, both additive: cases.py (kept master's 2D_ibm_airfoil_surface_pressure skip alongside the four here) and the 3D_reacting_mixing_layer README (took master's rewritten paragraph, minus its clause naming the regression test bc967150 removed).

Master brings in #1792, which changed IB ghost state and did invalidate the reacting-surface golden on #1821. It does not invalidate anything here — verified post-merge: 60/60 IBM and 15/15 chemistry tests pass, including the regenerated EA8FA07E golden.

@github-actions

Copy link
Copy Markdown

Lines of Code

File Lines Diff
src/simulation/m_ibm.fpp 1429 +21
Directory Lines Diff
simulation 28011 +21
total 46940 +21

@codecov

codecov Bot commented Sep 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.84%. Comparing base (0f7f62f) to head (8489b9a).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1914      +/-   ##
==========================================
+ Coverage   61.31%   62.84%   +1.53%     
==========================================
  Files          86       86              
  Lines       22612    22349     -263     
  Branches     3327     3289      -38     
==========================================
+ Hits        13865    14046     +181     
+ Misses       6281     6055     -226     
+ Partials     2466     2248     -218     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sbryngelson

Copy link
Copy Markdown
Member Author

Update: no longer blocked. 5d6dd208 on this branch fixes #1928 itself (the 2D STL disc now spans about 13 cells, golden regenerated), so this PR closes #1928 and can go green on its own. All 163 IBM and Example tests pass on Frontier; details in the description.

@sbryngelson
sbryngelson merged commit 36bb71b into MFlowCode:master Sep 30, 2026
140 of 142 checks passed
@sbryngelson
sbryngelson deleted the feat/ib-patch-must-be-claimed branch September 30, 2026 19:42
sbryngelson added a commit to sbryngelson/MFC that referenced this pull request Sep 30, 2026
Conflict in s_ibm_setup: master's s_check_every_patch_marked (MFlowCode#1914) runs first, then this branch's gated
centroid-offset work.
sbryngelson added a commit to danieljvickers/MFC-Dan that referenced this pull request Sep 30, 2026
Conflict in m_ibm.fpp: this branch moves s_get_neighborhood_idx and s_update_ib_lookup to m_ib_patches;
master (MFlowCode#1914) added s_check_every_patch_marked between them. Kept the new check and this branch's removal.
sbryngelson added a commit to rocfire11/MFC-tlj that referenced this pull request Oct 1, 2026
sbryngelson added a commit to sbryngelson/MFC that referenced this pull request Oct 1, 2026
Brings in MFlowCode#1914 and MFlowCode#1930. Conflicts in m_ibm.fpp:
- s_ibm_setup declarations: union of this branch's AMR sizing variables and MFlowCode#1930's max_num_gps_rank.
- ghost-point overflow check: kept this branch's AMR-aware PROHIBIT; MFlowCode#1930's check guarded the same condition.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2D -> IBM -> STL tests nothing: the model is 1.33 cells across and marks no cells

2 participants