Refactor /common and beta buffer routines for future particle inclusion by joshgillis · Pull Request #1672 · MFlowCode/MFC

joshgillis · 2026-07-24T04:48:01Z

Description

The beta buffer communication routines were generalized to support both bubble and particle data. Previously, these routines were hard-coded to use beta_vars, which defines the communication mapping for bubble variables. Particle communication will require a different mapping (vars_send). Instead of duplicating the communication routines for each case, a generic communication variable list (vars_comm) is now passed through the call chain. Bubble routines pass beta_vars, while particle routines will pass vars_send, allowing the same communication infrastructure to be reused for both implementations.

Type of change (delete unused ones)

Refactor

Testing

How did you test your changes?

Test suite ran on personal machine cpu, and tuolumne gpu

Checklist

Check these like this [x] to indicate which of the below applies.

I added or updated tests for new behavior
I updated documentation if user-facing behavior changed

See the developer guide for full coding standards.

GPU changes (expand if you modified src/simulation/)

GPU results match CPU results
Tested on NVIDIA GPU or AMD GPU

AI code reviews

Reviews are not retriggered automatically. To request a review, comment on the PR:

@claude full review — Claude full review (also triggers on PR open/reopen/ready)
Or add label claude-full-review — Claude full review via label

into refactor/particle-arrays

github-actions · 2026-07-24T14:11:21Z

Claude Code Review

Head SHA: df0a969

Files changed:

4
src/common/m_boundary_common.fpp
src/common/m_boundary_primitives.fpp
src/common/m_mpi_common.fpp
src/simulation/m_bubbles_EL.fpp

Findings:

src/common/m_boundary_primitives.fpp: s_beta_extrapolation gains a new mandatory vars_comm dummy argument (and its body now indexes via idx = vars_comm(i) instead of beta_vars(i)), but no hunk in this diff updates any call site to pass the new argument — s_beta_periodic and s_beta_reflective had their call sites updated in m_boundary_common.fpp, s_beta_extrapolation did not. If this routine is invoked anywhere with the old 6-argument form, the build breaks; please confirm every caller was updated.
src/common/m_boundary_common.fpp, src/common/m_boundary_primitives.fpp, src/common/m_mpi_common.fpp: the new vars_comm parameter is threaded through s_populate_beta_buffers → s_populate_beta_bc_direction → s_beta_periodic/s_beta_reflective → s_mpi_reduce_beta_variables_buffers, but both call sites in m_bubbles_EL.fpp pass the same beta_vars array that was previously hardcoded inside these routines. As written, this is generalization with no current caller variation — per CLAUDE.md's guidance against "abstractions for one call site," consider whether this parameter is actually needed yet or should be added alongside the code that first varies it.

joshgillis · 2026-07-24T15:57:56Z

I'm not sure what the review means about the updated call site for s_beta_extrapolation, I'm actually not even sure if where that subroutine is called. I can remove the changes there if that subroutine is not in use.

As for the PR as a whole, if it is preferred that this structure is not added until particles are functional I can wait and make other adjustments.

wilfonba · 2026-07-24T20:45:01Z

I believe s_beta_extrapolation can just be removed. It must have slipped through at some point. Extrapolation boundary conditions for beta don't make any physical sense.

codecov · 2026-07-28T00:20:20Z

Codecov Report

❌ Patch coverage is 14.01869% with 92 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.08%. Comparing base (782a1fd) to head (9fba977).

Files with missing lines	Patch %	Lines
src/common/m_boundary_primitives.fpp	0.00%	68 Missing ⚠️
src/common/m_mpi_common.fpp	23.07%	17 Missing and 3 partials ⚠️
src/common/m_boundary_common.fpp	72.72%	3 Missing ⚠️
src/simulation/m_bubbles_EL.fpp	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1672      +/-   ##
==========================================
+ Coverage   61.04%   61.08%   +0.03%     
==========================================
  Files          83       83              
  Lines       20978    20975       -3     
  Branches     3099     3098       -1     
==========================================
+ Hits        12807    12813       +6     
+ Misses       6126     6114      -12     
- Partials     2045     2048       +3

☔ 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

Following up on my approval with some things I want answered before this lands. The mechanical part of the refactor checks out: v_size = nVar is reassigned locally at the top of s_mpi_reduce_beta_variables_buffers, so the pack loop bounds stay consistent with vars_comm; mapCells is a parameter in m_constants.fpp, so the new sizing block has no init-order hazard; the max() lands before the @:ALLOCATE; and the s_beta_extrapolation removal is clean (no references left anywhere in src/).

My concerns are about coverage and about the halo-size change being more than a refactor. Two of the inline comments are pre-existing issues rather than things this PR introduces — I have marked those, and I am fine with them going in a separate PR, but this change rewrites every line involved so it seemed worth raising now.

Leaving this as a comment rather than re-requesting changes; the approval stands unless the answers to items 1-3 turn up something.

sbryngelson · 2026-07-28T16:29:03Z

    !! @param mpi_dir MPI communication coordinate direction
    !! @param pbc_loc Processor boundary condition (PBC) location
-    subroutine s_mpi_reduce_beta_variables_buffers(q_comm, kahan_comp, mpi_dir, pbc_loc, nVar)
+    subroutine s_mpi_reduce_beta_variables_buffers(q_comm, kahan_comp, mpi_dir, pbc_loc, nVar, vars_comm)


This routine has no CI coverage. alter_lag_bubbles in toolchain/mfc/test/cases.py sets no ppn, so every Lagrange golden runs on a single rank. The call site in s_populate_beta_bc_direction is guarded by if (bc_bounds%beg >= 0 .or. bc_bounds%end >= 0), which requires an actual MPI neighbor, so nothing in the suite enters this routine at all.

That means roughly 120 changed lines here, plus the new halo sizing, are exercised by no test. Was the Tuolumne run multi-rank? If not, this refactor is currently unverified end to end.

Please add a ppn=2 Lagrange golden. It is the only thing that would actually cover the pack/unpack rewrite.

After making the other changes I will add in the ppn=2 test and verify the multi-rank results. I see in a few of the test cases that cases.append(define_case_d(stack, "", {}, ppn=2)) is added to the end to create a multi-rank test, does adding that to the end of alter_lag_bubbles produce a second test with mpi?

sbryngelson · 2026-07-28T16:29:03Z

+            else
+                beta_halo_size = 2*(mapCells + 1)*beta_v_size - 1
+            end if
+            halo_size = max(halo_size, beta_halo_size)


This is a behavior change, not a refactor. Before this PR the beta exchange reused a buff_send/buff_recv sized only from buff_size*v_size*(...). The beta path needs 2*(mapCells + 1)*3*max(...), i.e. 24*max(...) with mapCells = 3. For 2D Lagrange cases with small sys_size that exceeds the conservative-variable sizing, which means the old code was overrunning the send buffer.

If that reading is right, this is a latent buffer overflow fix arriving under "Type of change: Refactor" with the tests checkbox unchecked and the commit message "Adding integers for halo expansion". Please call it out explicitly in the PR description, and say which configurations actually overflowed before — that determines whether this needs to be backported.

If my reading is wrong and the old sizing was always sufficient, I would like to understand why this block is needed at all.

This change was made in preparation for overflows from the additional particle variables that were reported by Jose, but is preemptive since this version has not been tested with particles, and improperly included given this PR's main focus. I will remove this and note it potentially for later.

sbryngelson · 2026-07-28T16:29:03Z

        end if

+        if (bubbles_lagrange) then
+            beta_v_size = size(beta_vars)


The generalization stops short of the buffer sizing. The call chain now takes a generic vars_comm, but the sizing here is still hard-wired to size(beta_vars) and the whole block is gated on if (bubbles_lagrange).

The stated purpose of the PR is that particle routines will pass vars_send instead. If size(vars_send) > 3, or if particles can be active without bubbles_lagrange, halo_size will not cover the particle exchange and you get exactly the overflow the block above is fixing.

Since this PR exists specifically to prepare for that, the sizing should be driven by whatever mapping is actually registered rather than by beta_vars. Otherwise the follow-up PR inherits a silent overflow that will be much harder to spot once particles are in the mix.

Connected to the other halo change, this will be removed and I'll wait to add it in until a more appropriate time

sbryngelson · 2026-07-28T16:29:04Z

            end if

-            $:GPU_PARALLEL_LOOP(private='[l, k, bc_code]', collapse=2)
+            $:GPU_PARALLEL_LOOP(private='[l, k, bc_code]', collapse=2, copyin = '[vars_comm]')


beta_vars is already device-resident — $:GPU_DECLARE(create='[comm_coords, comm_size, beta_vars]') at m_mpi_common.fpp:34 and $:GPU_UPDATE(device='[beta_vars]') at m_mpi_common.fpp:102. Routing it through an assumed-shape dummy and adding copyin here replaces a device-resident read with a host-to-device transfer on every launch of this loop: six launches per timestep (two locations times three directions) in a hot BC path.

The payload is three integers, so this is not about bandwidth — it is the per-launch transfer and the descriptor handling for an assumed-shape dummy. Did the Tuolumne runs show any cost here?

A device-resident module-level array, or passing an integer selector instead of the array, would avoid it entirely.

Minor style note while here: copyin = '[vars_comm]' has spaces around = where every other clause in the file is written copyin='[...]'.

I'm considering introducing a device-resident communication mapping (comm_vars) in m_mpi_common. The bubble routines would register beta_vars with this mapping, while the particle routines would register vars_send. The communication routines would then operate on comm_vars, avoiding the per-kernel copyin while also allowing the halo sizing to be driven by the active communication mapping. Do you think that this would be an improvement?

sbryngelson · 2026-07-28T16:29:04Z

        integer, intent(in)                                        :: k, l
        integer, intent(in)                                        :: nvar
-        integer                                                    :: j, i
+        integer, dimension(:), intent(in)                          :: vars_comm


Pre-existing, but this PR rewrites every line that depends on it. q_beta and kahan_comp are declared dimension(num_dims + 1) just above — 3 in 2D, 4 in 3D. With nvar = 3 this routine indexes vars_comm(3), which is beta_vars(3) = 5. Indexing element 5 of a dummy declared with 3 or 4 elements is not conforming and is fatal under -fcheck=bounds.

The parent s_populate_beta_buffers already declares these as dimension(:). Please make these two match — it is a one-line change per declaration and it removes the discrepancy at the point where you are already touching every use. Same applies to s_beta_reflective below.

See my comment on the nvar = 3 call site in m_bubbles_EL.fpp for the underlying problem.

I will make those changes

sbryngelson · 2026-07-28T16:29:04Z

        call nvtxStartRange("BUBBLES-LAGRANGE-BETA-COMM")
        if (lag_params%cluster_type >= 4) then
-            call s_populate_beta_buffers(q_beta, kahan_comp, bc_type, 3)
+            call s_populate_beta_buffers(q_beta, kahan_comp, bc_type, 3, beta_vars)


Pre-existing, flagging because this line is being touched. nvar = 3 means the comm routines reach beta_vars(3) = 5, so q_beta(5) must exist. It only does when q_beta_idx = 6, which requires lag_params%solver_approach == 2 .and. p == 0 (see lines 92-103).

For 3D two-way coupling q_beta_idx = 4, so q_beta(5) is written past the end of the allocation. Neither m_checker.fpp nor case_validator.py prevents cluster_type >= 4 from being combined with a 3D two-way setup, and s_deltafunc / s_smearfunction in m_bubbles_EL_kernels.fpp write updatedvar(5) under the same condition.

No example or test in the repo sets cluster_type >= 4, so this entire path is unexercised — which is presumably why it has survived.

Worth adding the constraint (cluster_type >= 4 requires solver_approach == 2 and p == 0), either here or in a follow-up.

I will add this constraint

sbryngelson · 2026-07-28T16:29:04Z

    !> Initialize the module.
    impure subroutine s_initialize_mpi_common_module

+        integer :: beta_v_size, beta_comm_size_1, beta_comm_size_2, beta_comm_size_3, beta_halo_size


Minor: these are declared outside #ifdef MFC_MPI but only used inside it, so non-MPI builds will warn about five unused variables. Moving the declaration inside the guard fixes it.

joshgillis and others added 11 commits July 23, 2026 14:26

Modifying beta storage in common to account for particle arrays.

109ef34

Merge branch 'MFlowCode:master' into add-particles

106324a

Adding integers for halo expansion

1e166c8

Merge branch 'refactor/particle-arrays' of https://github.com/UF-MFC/MFC

371d885

into refactor/particle-arrays

typos and minor changes to the MPI common module

5b5fe33

removing particles call until implementation of particles_lagrange

1787700

removing particles lagrange here as well

eb5f415

adding beta_vars to bubble boundary routine calls

9be23da

Merge branch 'MFlowCode:master' into refactor/particle-arrays

19411ad

Merge branch 'MFlowCode:master' into refactor/particle-arrays

ca611ea

formatting

df0a969

sbryngelson added claude-full-review Trigger Claude Code review and removed claude-full-review Trigger Claude Code review labels Jul 24, 2026

joshgillis added 2 commits July 27, 2026 13:08

Merge branch 'master' into refactor/particle-arrays

8d48346

remove s_beta_extrapolation (dead code)

9fba977

joshgillis marked this pull request as ready for review July 27, 2026 22:23

joshgillis requested a review from sbryngelson as a code owner July 27, 2026 22:23

sbryngelson approved these changes Jul 28, 2026

View reviewed changes

sbryngelson reviewed Jul 28, 2026

View reviewed changes

joshgillis marked this pull request as draft July 29, 2026 16:19

Uh oh!

Conversation

joshgillis commented Jul 24, 2026

Description

Type of change (delete unused ones)

Testing

Checklist

AI code reviews

Uh oh!

github-actions Bot commented Jul 24, 2026

Uh oh!

joshgillis commented Jul 24, 2026

Uh oh!

wilfonba commented Jul 24, 2026

Uh oh!

codecov Bot commented Jul 28, 2026

Codecov Report

Uh oh!

sbryngelson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants