Use mapreduce_neighbor and remove Ref values#1211
Draft
efaulhaber wants to merge 3 commits into
Draft
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors neighbor-interaction kernels to avoid Ref-based accumulation and to use mapreduce_neighbor, improving CPU SIMD/vectorization opportunities while keeping the “accumulate then write once” pattern for GPU performance.
Changes:
- Replace
foreach_neighbor+Refaccumulators withmapreduce_neighborreductions in selected RHS/structure kernels. - Convert several previously
Ref-mutating helpers (e.g., viscosity, shifting, surface tension, continuity/density diffusion) into pure functions returning the updated value. - Update affected call sites and viscosity tests to match the new return-value APIs.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/schemes/fluid/viscosity.jl | Updates viscosity tests to use return-value API instead of Ref mutation. |
| src/schemes/structure/total_lagrangian_sph/system.jl | Switches neighbor accumulation to mapreduce_neighbor for deformation gradient assembly. |
| src/schemes/structure/total_lagrangian_sph/rhs.jl | Refactors TLSPH structure-structure RHS accumulation to mapreduce_neighbor. |
| src/schemes/structure/structure.jl | Updates structure–fluid interaction helpers to use return-value APIs (viscosity/adhesion/continuity). |
| src/schemes/structure/rigid_body/system.jl | Converts rigid-body adhesion force helper to return-value style. |
| src/schemes/fluid/weakly_compressible_sph/rhs.jl | Uses mapreduce_neighbor to accumulate (dv, drho) contributions without Refs. |
| src/schemes/fluid/weakly_compressible_sph/density_diffusion.jl | Converts density diffusion kernel to return updated drho rather than mutating a Ref. |
| src/schemes/fluid/viscosity.jl | Converts viscosity dispatch and implementations to return updated dv_particle. |
| src/schemes/fluid/surface_tension.jl | Converts surface tension + adhesion force helpers to return updated dv_particle. |
| src/schemes/fluid/shifting_techniques.jl | Converts shifting term helpers to return updated dv_particle. |
| src/schemes/fluid/implicit_incompressible_sph/system.jl | Updates viscosity usage to return-value API. |
| src/schemes/fluid/implicit_incompressible_sph/rhs.jl | Updates viscosity usage to return-value API. |
| src/schemes/fluid/fluid.jl | Converts continuity equation + density diffusion application to return-value API. |
| src/schemes/fluid/entropically_damped_sph/rhs.jl | Updates viscosity/shifting/surface tension/adhesion/continuity usage to return-value APIs. |
| src/schemes/boundary/wall_boundary/rhs.jl | Converts boundary continuity equation helpers to return-value API. |
| src/schemes/boundary/open_boundary/rhs.jl | Updates viscosity/continuity usage to return-value APIs. |
| src/io/write_vtk.jl | Updates surface tension postprocessing to use return-value API. |
| src/general/neighborhood_search.jl | Adds TrixiParticles wrapper(s) for mapreduce_neighbor, including a GPU unsafe fast path. |
Comments suppressed due to low confidence (1)
src/schemes/structure/total_lagrangian_sph/rhs.jl:86
mapreduce_neighboris used to avoidRef-based accumulation, but this closure still allocates/uses aRefper neighbor to calldv_penalty_force!/dv_viscosity_tlsph!. This undermines the stated goal of removingRefboxing and may add per-neighbor overhead. Consider providing non-mutating variants (e.g.,dv_penalty_force/dv_viscosity_tlsphreturning the updated value) so the closure can stay allocation-free and justreturn dv_particle.
dv_particle = Ref(m_b * (pk1_rho2_a + pk1_rho2_b) * grad_kernel)
@inbounds dv_penalty_force!(dv_particle, penalty_force, particle, neighbor,
initial_pos_diff, initial_distance,
current_pos_diff, current_distance,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Based on trixi-framework/PointNeighbors.jl#158.
This PR improves two patterns I introduced in #1124, #1125, #1130, #1116 and #1139.
I changed the RHS code to work on
Refvalues, but this is suboptimal both aesthetically (is ugly) and functionally (prevents SIMD vectorization on CPUs). This PR changed many lines, but most of these are boilerplate changes. The actual change is very simple as explained below.Pattern 1
main:
Since this prevents SIMD vectorization, I introduced a clean mapreduce function in trixi-framework/PointNeighbors.jl#158.
Pattern 2
main:
This pattern was consistent with pattern 1 on main and successfully removed adds of zero
SVectors to improve GPU performance, but now with pattern 1 changed, it becamewhich is ugly and now boxes the value in a
Reffor no reason.All of these I changed to
So all functions that previously inplace modified
Refvalues now just return the new value.