Repository navigation
Fix missing MPI broadcasts and serial collective results - #2024
Draft
sbryngelson wants to merge 3 commits into
Draft
sbryngelson wants to merge 3 commits into
sbryngelson wants to merge 3 commits into
Conversation
(cherry picked from commit c9478e6)
pre_process reads particle_cloud(i)%shell_axis in s_sample_cloud_candidate but never gave it a default or broadcast it; only simulation, which does not use it, did both. Under file_per_process every rank samples the cloud itself, so non-root ranks placed hemisphere-shell particles with an uninitialized axis, and without a namelist value even rank 0 read garbage. Default it to 3 (+z) and broadcast it with the other hand-written particle_cloud members, mirroring simulation. Verified with a 3D hemisphere-shell cloud (shell_axis = 1, file_per_process, 20 particles): the IB state from 2 ranks now matches the 1-rank run exactly and every particle lies on the +x side. (cherry picked from commit 0a3d2d7)
periodic_bc is set from the global bc_[xyz] values in s_read_input_file, which only rank 0 runs, and was never broadcast. s_mpi_sendrecv_particles reads it on every rank to decide whether to wrap Lagrangian bubble positions across periodic boundaries, so on every other rank it stayed .false. and bubbles leaving through a periodic face were never wrapped. Broadcast it next to bc_io. Computing it after the broadcast is not an option because domain decomposition overwrites the interior ranks' bc values with neighbor ids. pre_process and post_process have no periodic_bc. Verified on a 2-rank 2D Lagrangian bubble case with periodic x: rank 1 now holds periodic_bc = T F F (it held F F F before). Lagrange bubble tests pass. (cherry picked from commit 1c86584)
This was referenced Oct 10, 2026
This branch has not been deployed
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.
Summary
Several serial collective helpers left their outputs undefined, and non-root MPI ranks did not receive particle_cloud shell_axis or periodic_bc. This draft combines defined serial collective results with the missing initialization and broadcasts.
Consolidation
Each source change is retained as a separate commit, with its original author, message, and a
cherry-pick -xreference. The complete original PR descriptions are reproduced below, including their verification records and limitations. Original review discussions remain available through the source links. Those original verification claims are historical records, not fresh runs on this combined branch.Verification of this combined branch
git diff --check origin/master...HEADpasses../mfc.sh precheckpasses all seven gates: formatting, spelling, toolchain lint/tests, source lint, documentation references, parameter documentation, and example case validation.This consolidation was performed with OpenAI Codex.
Contribution Policy
We do not accept pull requests generated primarily by AI without genuine understanding or real-world usage context.
All contributions are expected to demonstrate:
If these expectations are not met, we would prefer to implement the changes ourselves rather than spend time reviewing low-effort submissions.
Acknowledgement
PR template credit: junegunn
Original PR documentation
#1971: Fix undefined allreduce min/max/sum results in non-MPI builds
Source: #1971
Original head:
c9478e6da5e1a44ea02aaf5a2170e68a0a6859ecComplete original PR description
Contribution Policy
We do not accept pull requests generated primarily by AI without genuine understanding or real-world usage context.
All contributions are expected to demonstrate:
If these expectations are not met, we would prefer to implement the changes ourselves rather than spend time reviewing low-effort submissions.
Acknowledgement
Fixes #1966
Change
In non-MPI builds,
s_mpi_allreduce_minands_mpi_allreduce_maxinsrc/common/m_mpi_common.fppleft theirintent(out)var_glbundefined. Added#else var_glb = var_loc.s_mpi_allreduce_sumhad the same gap. It is also fixed in the unmerged Keep species mass fractions in [0, 1] by construction instead of clipping #1963; repeated here so this PR stands alone (trivial overlap, same one-line branch).s_mpi_gather_dataleft its allocatablegathered_vectorunallocated without MPI. Added#else gathered_vector = my_vector(single process: the gathered vector is the local one).s_mpi_allreduce_min_vec,s_mpi_allreduce_vectors_sum,s_mpi_allreduce_integer_sum(_vec),s_mpi_reduce_int_sum,s_mpi_reduce_stability_criteria_extrema.s_mpi_reduce_minands_mpi_reduce_maxlocareintent(inout), so the local value is already the result.Affected callers on single-process builds:
s_mpi_allreduce_mininm_boundary_common.fpp(global bounds), ands_mpi_allreduce_max/sumands_mpi_gather_datain post_processm_data_output.fpp(probe/integral output). These now receive the local value instead of garbage.Evidence
./mfc.sh test --no-mpi --no-gpu -j 24 --only IBMon Tuolumne (login node): 61 passed, 0 failed, 708 skipped../mfc.sh formatand./mfc.sh precheckpass.Not verified: no dedicated test exercises these routines in a no-MPI build (the garbage read is not caught by the golden-file tests); the claim that callers now get the local value is from reading the code. MPI builds are unaffected (the change is inside
#else), but I did not run MPI tests.#2004: Default and broadcast particle_cloud shell_axis in pre_process
Source: #2004
Original head:
0a3d2d712f714f0cf1c5f59b80a8ff450da65cb5Complete original PR description
pre_process reads particle_cloud(i)%shell_axis in s_sample_cloud_candidate
but never gave it a default or broadcast it; only simulation, which does
not use it, did both. Under file_per_process every rank samples the
cloud itself, so non-root ranks placed hemisphere-shell particles with an
uninitialized axis, and without a namelist value even rank 0 read
garbage.
Default it to 3 (+z) and broadcast it with the other hand-written
particle_cloud members, mirroring simulation.
Verified with a 3D hemisphere-shell cloud (shell_axis = 1,
file_per_process, 20 particles): the IB state from 2 ranks now matches
the 1-rank run exactly and every particle lies on the +x side.
Acknowledgement
#2005: Broadcast periodic_bc to all simulation ranks
Source: #2005
Original head:
1c8658469e2a28c31d038a1d648c7928be2aad7fComplete original PR description
periodic_bc is set from the global bc_[xyz] values in s_read_input_file,
which only rank 0 runs, and was never broadcast. s_mpi_sendrecv_particles
reads it on every rank to decide whether to wrap Lagrangian bubble
positions across periodic boundaries, so on every other rank it stayed
.false. and bubbles leaving through a periodic face were never wrapped.
Broadcast it next to bc_io. Computing it after the broadcast is not an
option because domain decomposition overwrites the interior ranks' bc
values with neighbor ids. pre_process and post_process have no
periodic_bc.
Verified on a 2-rank 2D Lagrangian bubble case with periodic x: rank 1
now holds periodic_bc = T F F (it held F F F before). Lagrange bubble
tests pass.
Acknowledgement