IB: check every patch is marked in one pass and one reduction - #1933
sbryngelson wants to merge 3 commits into
Conversation
s_check_every_patch_marked swept every local cell and ran a reduction once per global patch, and num_gbl_ibs counts every particle-cloud particle. Count markers by decoded patch id in one pass and reduce the counts in one collective. Decoding also counts the cells of a body wrapped across a periodic boundary, which the raw == gid test missed. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 4
Open (8)
real(wp), then compared via exact equality to… · Newreal(wp), then compared via exact equality to… · Newreal(wp), then compared via exact equality to… · Newreal(wp), then compared via exact equality to… · New1, which… · New1, which… · New1, which… · New
What changed in this PR
Optimizes s_check_every_patch_marked to avoid per-patch grid sweeps and collectives by counting marked cells per patch in a single local pass and performing a single global reduction, while correctly handling periodic-encoded markers.
Changes:
- Replace
num_gbl_ibsrepeated sweeps/reductions with one local sweep that bins counts by patch id. - Perform a single collective reduction over the full per-patch count array.
- Decode marker periodicity before counting so wrapped images count toward their owning patch.
| File | Description |
|---|---|
src/simulation/m_ibm.fpp |
Refactors “every patch marked” validation to single-pass counting + single reduction and adds periodicity-aware marker decoding. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| integer(kind=8) :: cnt_loc, cnt_glb | ||
| integer :: gid, i, j, k | ||
| real(wp), allocatable :: cnt_loc(:,:), cnt_glb(:,:) |
There was a problem hiding this comment.
Done in 97d7106: counts are now 1-D integer(kind=8), reduced with a new s_mpi_allreduce_integer_sum_vec, and tested with == 0_8. (The real-valued version could not misreport, since a sum of non-negative counts is zero only if all of them are, but integers are the right type.)
| allocate (cnt_loc(num_gbl_ibs, 1), cnt_glb(num_gbl_ibs, 1)) | ||
| cnt_loc = 0._wp |
| do i = 0, m | ||
| if (ib_markers%sf(i, j, k) /= 0) then | ||
| call s_decode_patch_periodicity(ib_markers%sf(i, j, k), gid) | ||
| cnt_loc(gid, 1) = cnt_loc(gid, 1) + 1._wp |
| & "An immersed boundary marked no cell anywhere: its centroid decides which rank "// "owns it, so a body placed elsewhere with model_translate is handed to a rank that "// "does not hold it. Set patch_ib%x/y/z_centroid to where the body actually is.") | ||
| end do | ||
| call s_mpi_allreduce_vectors_sum(cnt_loc, cnt_glb, num_gbl_ibs, 1) | ||
| @:PROHIBIT(any(cnt_glb(:, 1) == 0._wp), & |
| @:PROHIBIT(cnt_glb == 0_8, & | ||
| & "An immersed boundary marked no cell anywhere: its centroid decides which rank "// "owns it, so a body placed elsewhere with model_translate is handed to a rank that "// "does not hold it. Set patch_ib%x/y/z_centroid to where the body actually is.") | ||
| end do | ||
| call s_mpi_allreduce_vectors_sum(cnt_loc, cnt_glb, num_gbl_ibs, 1) |
There was a problem hiding this comment.
No change here. s_mpi_allreduce_integer_sum_vec, like the other m_mpi_common reductions, has an #else branch that copies the input when MFC_MPI is undefined, so non-MPI builds work. This check runs once, in s_ibm_setup, so one collective on a single rank costs nothing.
| if (ib_markers%sf(i, j, k) == gid) cnt_loc = cnt_loc + 1_8 | ||
| end do | ||
| if (num_gbl_ibs == 0) return | ||
| allocate (cnt_loc(num_gbl_ibs, 1), cnt_glb(num_gbl_ibs, 1)) |
There was a problem hiding this comment.
Done in 97d7106: the counts are 1-D now. The trailing dimension came from the 2-D real-valued helper, which the new integer reduction replaces.
| do i = 0, m | ||
| if (ib_markers%sf(i, j, k) /= 0) then | ||
| call s_decode_patch_periodicity(ib_markers%sf(i, j, k), gid) | ||
| cnt_loc(gid, 1) = cnt_loc(gid, 1) + 1._wp |
| & "An immersed boundary marked no cell anywhere: its centroid decides which rank "// "owns it, so a body placed elsewhere with model_translate is handed to a rank that "// "does not hold it. Set patch_ib%x/y/z_centroid to where the body actually is.") | ||
| end do | ||
| call s_mpi_allreduce_vectors_sum(cnt_loc, cnt_glb, num_gbl_ibs, 1) | ||
| @:PROHIBIT(any(cnt_glb(:, 1) == 0._wp), & |
Use 1-D integer(kind=8) counts and a new s_mpi_allreduce_integer_sum_vec (shaped like s_mpi_allreduce_min_vec, with the same non-MPI fallback) instead of real(wp) counts in a (num_gbl_ibs, 1) array, as review suggested. Co-Authored-By: Claude <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1933 +/- ##
==========================================
- Coverage 62.82% 62.82% -0.01%
==========================================
Files 86 86
Lines 22394 22398 +4
Branches 3305 3305
==========================================
+ Hits 14070 14072 +2
- Misses 6071 6072 +1
- Partials 2253 2254 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Lines of Code
|
Summary
Follow-up to #1914, addressing its open Copilot review finding.
s_check_every_patch_markedlooped over every global patch, and for each one swept every local cell and ran an MPI reduction. That isnum_gbl_ibsgrid sweeps plusnum_gbl_ibscollectives at setup, andnum_gbl_ibscounts every particle-cloud particle (up to 54,000 with the configured limit). The check now:It also decodes each marker with
s_decode_patch_periodicitybefore counting. The old check compared the raw marker with== gid, so the cells of a body wrapped across a periodic boundary did not count toward their patch, and a fully wrapped image could wrongly abort a valid run.Counts are
integer(kind=8), reduced with a news_mpi_allreduce_integer_sum_vecinm_mpi_common(shaped likes_mpi_allreduce_min_vec, with the same non-MPI fallback). The abort and its message are unchanged, and the 16-line comment is cut to three.The check runs once, from
s_ibm_setup, not per time step. So the old cost was a setup-time cost, and the check catches a body that marks no cell at startup, not one that later leaves the domain.Verification
pre_process,simulationandpost_processbuild on masterd87a5feawith this change on Frontier, in all three configurations: CPU (--no-gpu), OpenMP offload (--gpu mp) and OpenACC (--gpu acc)../mfc.sh precheckpasses (all 7 gates).AI disclosure
Written with Claude Code (Anthropic).
Acknowledgement