Skip to content

Fix missing MPI broadcasts and serial collective results - #2024

Draft
sbryngelson wants to merge 3 commits into
MFlowCode:masterfrom
sbryngelson:consolidate/mpi-fixes
Draft

sbryngelson wants to merge 3 commits into
MFlowCode:masterfrom
sbryngelson:consolidate/mpi-fixes

Conversation

@sbryngelson

Copy link
Copy Markdown
Member

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 -x reference. 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

  • Every source commit retains exactly the same added and deleted lines, verified against the original commit.
  • git diff --check origin/master...HEAD passes.
  • ./mfc.sh precheck passes all seven gates: formatting, spelling, toolchain lint/tests, source lint, documentation references, parameter documentation, and example case validation.
  • Full solver regressions and multi-rank numerical runs were not repeated for this consolidation; their original records are preserved below, and CI should validate the new head.

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:

  • A clear understanding of the codebase
  • Alignment with product direction
  • Thoughtful reasoning behind changes
  • Evidence of real-world usage or hands-on experience with the problem

If these expectations are not met, we would prefer to implement the changes ourselves rather than spend time reviewing low-effort submissions.


Acknowledgement

  • I confirm this PR meets the above expectations and reflects my own understanding and real-world context.

PR template credit: junegunn

Original PR documentation

#1971: Fix undefined allreduce min/max/sum results in non-MPI builds

Source: #1971

Original head: c9478e6da5e1a44ea02aaf5a2170e68a0a6859ec

Complete 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:

  • A clear understanding of the codebase
  • Alignment with product direction
  • Thoughtful reasoning behind changes
  • Evidence of real-world usage or hands-on experience with the problem

If these expectations are not met, we would prefer to implement the changes ourselves rather than spend time reviewing low-effort submissions.


Acknowledgement

  • I confirm this PR meets the above expectations and reflects my own understanding and real-world context.

Fixes #1966

Change

In non-MPI builds, s_mpi_allreduce_min and s_mpi_allreduce_max in src/common/m_mpi_common.fpp left their intent(out) var_glb undefined. Added #else var_glb = var_loc.

  • s_mpi_allreduce_sum had 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).
  • Audit of the rest of the file found one more: s_mpi_gather_data left its allocatable gathered_vector unallocated without MPI. Added #else gathered_vector = my_vector (single process: the gathered vector is the local one).
  • Already correct: 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_min and s_mpi_reduce_maxloc are intent(inout), so the local value is already the result.

Affected callers on single-process builds: s_mpi_allreduce_min in m_boundary_common.fpp (global bounds), and s_mpi_allreduce_max/sum and s_mpi_gather_data in post_process m_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 IBM on Tuolumne (login node): 61 passed, 0 failed, 708 skipped. ./mfc.sh format and ./mfc.sh precheck pass.

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: 0a3d2d712f714f0cf1c5f59b80a8ff450da65cb5

Complete 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

  • I confirm this PR meets the above expectations and reflects my own understanding and real-world context.

#2005: Broadcast periodic_bc to all simulation ranks

Source: #2005

Original head: 1c8658469e2a28c31d038a1d648c7928be2aad7f

Complete 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

  • I confirm this PR meets the above expectations and reflects my own understanding and real-world context.

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 branch has not been deployed

No deployments
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.

s_mpi_allreduce_min/max leave var_glb undefined in non-MPI builds

1 participant