Implement 2-D and 3-D MPM particle-grid transfers - #174
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds 2-D and 3-D MPM system data with cubic B-spline interpolation, particle-grid transfers, PIC/FLIP blending, state validation, Python bindings, and tests. ChangesMPM System Data and Transfers
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MPMSystemData
participant CubicBSplineKernel
participant ParticleState
participant GridState
MPMSystemData->>CubicBSplineKernel: Build weighted stencil
MPMSystemData->>ParticleState: Read particle mass and velocity
MPMSystemData->>GridState: Deposit mass and momentum
GridState-->>MPMSystemData: Provide grid velocity and pre-update velocity
MPMSystemData->>ParticleState: Apply PIC/FLIP blended velocity
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 125 |
| Duplication | 4 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
Tests/UnitTests/MPMSystemDataTests.cpp (1)
263-299: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd 3-D invalid-input coverage.
RejectsInvalidInputtests onlyMPMSystemData2. The validation paths use dimension-dependent vectors and grid shapes. A 3-D regression can pass without detection. Extract this test into anN-parameterized helper and invoke it for both<2>and<3>.As per coding guidelines, “Update or add tests for both 2-D and 3-D counterparts when behavior applies to both.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/UnitTests/MPMSystemDataTests.cpp` around lines 263 - 299, Refactor RejectsInvalidInput into an N-parameterized test helper using dimension-generic vectors and grid data, then invoke it for both MPMSystemData2 and MPMSystemData3. Preserve all existing invalid-input assertions while adapting dimension-dependent values such as zero, negative-spacing, resolution, positions, velocities, and grid velocities to work for both dimensions.Source: Coding guidelines
Includes/Core/Particle/MPM/MPMSystemData-Impl.hpp (5)
372-387: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
FoldIndextoClampIndex.The function clamps. "Fold" suggests periodic wrapping or mirroring, which is a different boundary policy with different physics. The name matters here because the clamping behavior is the boundary contract for both transfer directions.
The underflow guard is sound:
ValidateGridParametersrejects resolution 0 and rejectsnumeric_limits<size_t>::max(), sodataSize[axis] - 1cannot underflow andresolution + 1cannot overflow.♻️ Proposed rename
-Vector<size_t, N> MPMSystemData<N>::FoldIndex(const Vector<ssize_t, N>& index, - const Vector<size_t, N>& dataSize) +Vector<size_t, N> MPMSystemData<N>::ClampIndex( + const Vector<ssize_t, N>& index, const Vector<size_t, N>& dataSize)Update the declaration at Includes/Core/Particle/MPM/MPMSystemData.hpp lines 131-132 and the two call sites at lines 299 and 352.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Includes/Core/Particle/MPM/MPMSystemData-Impl.hpp` around lines 372 - 387, Rename MPMSystemData<N>::FoldIndex to ClampIndex, updating its declaration in MPMSystemData.hpp and both call sites while preserving the existing clamping behavior.
307-315: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider the project parallel-iteration utilities for the normalization pass.
The normalization pass at lines 307-313 and the snapshot at line 315 have no write conflicts, so both can run under the parallel execution policy that the grid classes already accept. The scatter loop at lines 287-305 does have write conflicts, so keep it serial until a proper scatter strategy is in place.
This is a follow-up performance item, not a blocker for this PR.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Includes/Core/Particle/MPM/MPMSystemData-Impl.hpp` around lines 307 - 315, Update the normalization loop using m_gridMass.ForEachDataPointIndex and the subsequent m_gridVelocitiesBeforeUpdate.Set operation to use the grid classes’ parallel execution policy, while keeping the preceding scatter loop serial because it has write conflicts.
246-261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGetter name does not match the surrounding API style.
The base class uses
Radius(),Mass(),Positions(), andNeighborSearcher()without aGetprefix. This class usesGetFLIPBlendingFactor(). Rename it toFLIPBlendingFactor()while the type is still new and unreleased. KeepSetFLIPBlendingFactoras is, which also matchesSetRadiusandSetMass.The validation logic itself is correct and covers the non-finite case.
As per coding guidelines: "Use existing patterns and dependencies before adding abstractions, libraries, templates, builders, numerical helpers, parallel loops, serialization code, or tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Includes/Core/Particle/MPM/MPMSystemData-Impl.hpp` around lines 246 - 261, Rename the MPMSystemData getter GetFLIPBlendingFactor() to FLIPBlendingFactor() to match the surrounding accessor naming style, and update all call sites and declarations accordingly. Keep SetFLIPBlendingFactor() and its existing validation logic unchanged.Source: Coding guidelines
69-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBounds derivation is correct.
The check reserves headroom for
floor(normalized) - 1and for the+3offset, soentry.indexcannot overflowssize_t. Thenextafterguards handle the case wherestatic_cast<double>roundsssize_tlimits away from the representable integer.One small cleanup:
lowestIndexandhighestIndexdo not depend onaxis. Hoist them above the loop asconstexpr-style locals.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Includes/Core/Particle/MPM/MPMSystemData-Impl.hpp` around lines 69 - 97, Hoist the axis-independent lowestIndex and highestIndex calculations out of the per-axis loop, declaring them once as constexpr-style locals before iterating. Keep the existing bounds checks and index calculations in the loop unchanged.
99-133: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffBoth transfer paths pay for gradients they never read.
GetStencilalways fillsentry.gradient.TransferFromParticlesToGridandTransferFromGridToParticlesread onlyentry.weightandentry.index. In 3-D that costs 3 divisions and 6 multiplications per entry across 64 entries, for every particle, on every transfer call.Consider a template or runtime flag that skips the gradient loop, or a separate
GetWeightsentry point for transfer-only callers. Keep the currentGetStencilfor the force computation that will need gradients.Also note that
flatIndexcouples entry ordering to the traversal order ofForEachIndex. The transfer loops treat the stencil as an unordered set, but the test at Tests/UnitTests/MPMSystemDataTests.cpp lines 96-99 comparesstencil[i]withshiftedStencil[i]positionally. IfForEachIndexis ever switched to a parallel policy,flatIndex++also becomes a data race. Deriving the flat index fromoffsetremoves both couplings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Includes/Core/Particle/MPM/MPMSystemData-Impl.hpp` around lines 99 - 133, Optimize transfer-only stencil generation by adding a gradient-free path, such as GetWeights, and update TransferFromParticlesToGrid and TransferFromGridToParticles to use it while keeping GetStencil gradients for force computation. Replace flatIndex++ in GetStencil with a deterministic index derived from offset, avoiding dependence on traversal order and making indexing safe under parallel iteration. Preserve the existing stencil ordering expected by positional tests.Includes/Core/Particle/MPM/MPMSystemData.hpp (2)
55-62: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy lift
MPMSystemDatainherits serialization that drops MPM state.
ParticleSystemData<N>declaresSerializeandDeserializeas overrides ofSerializable.MPMSystemDataaddsm_particleMasses,m_initialVolumes,m_deformationStates, and three grids, but does not override either method. A caller that serializes anMPMSystemDatathrough theSerializableinterface silently loses all MPM state, and deserialization leaves the derived arrays at their previous sizes while the base particle count changes.If full serialization is out of scope for this PR, document the limitation in the class Doxygen comment and track it in a follow-up issue. Do you want me to open that issue?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Includes/Core/Particle/MPM/MPMSystemData.hpp` around lines 55 - 62, Update MPMSystemData to override Serialize and Deserialize so its MPM-specific state—m_particleMasses, m_initialVolumes, m_deformationStates, and the three grids—is persisted and restored when accessed through Serializable, keeping derived array sizes synchronized with the deserialized base particle count. If full serialization cannot be implemented in this change, document the limitation in the MPMSystemData class Doxygen comment and record a follow-up issue.
118-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpose
MPMSystemData2andMPMSystemData3in the Python APIThe Python API exposes
ParticleSystemDataandSPHSystemData, but no MPM types. Add matching bindings and register them inSources/API/Python/main.cpp, or document that the bindings are deferred.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Includes/Core/Particle/MPM/MPMSystemData.hpp` around lines 118 - 128, Expose MPMSystemData2 and MPMSystemData3 through the Python API alongside ParticleSystemData and SPHSystemData, adding matching bindings and registering both types in Sources/API/Python/main.cpp; if this cannot be completed, explicitly document that these bindings are deferred.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Includes/Core/Particle/MPM/MPMSystemData-Impl.hpp`:
- Around line 287-305: The TransferFromParticlesToGrid path must define an
explicit policy for particles outside the grid: either extend the validation
loop to reject out-of-domain positions like invalid masses, or document
FoldIndex boundary clamping in the TransferFromParticlesToGrid Doxygen comment
as intentional. Update the relevant validation or documentation symbol so
callers can reliably determine the expected behavior.
- Around line 149-157: Ensure derived MPM arrays stay synchronized whenever
particle counts change: in
Includes/Core/Particle/MPM/MPMSystemData-Impl.hpp:149-157, update
MPMSystemData<N>::Resize or add the requested internal synchronization path, and
cover nonvirtual AddParticle/AddParticles entry points. In
Includes/Core/Particle/ParticleSystemData.hpp:89-89, verify AddParticles, Set,
and Deserialize update counts through Resize; if not, route them through Resize
so derived dispatch occurs. Run the attached verification script before
selecting the implementation.
- Around line 358-369: Update TransferFromGridToParticles to validate
m_gridVelocities and m_gridVelocitiesBeforeUpdate completely before entering the
particle transfer loop, matching the upfront particle-state validation pattern
used in TransferFromParticlesToGrid. Remove or downgrade the per-particle
IsFinite(result) exception check so the transfer cannot throw after partially
updating velocities, while preserving the existing result calculation and
assignments.
In `@Includes/Core/Particle/MPM/MPMSystemData.hpp`:
- Around line 151-155: Add shared-pointer aliases alongside MPMSystemData2 and
MPMSystemData3, matching the ParticleSystemData2Ptr and ParticleSystemData3Ptr
naming and ownership conventions. Keep the existing dimensional value aliases
unchanged and define each pointer alias to reference its corresponding
MPMSystemData type.
---
Nitpick comments:
In `@Includes/Core/Particle/MPM/MPMSystemData-Impl.hpp`:
- Around line 372-387: Rename MPMSystemData<N>::FoldIndex to ClampIndex,
updating its declaration in MPMSystemData.hpp and both call sites while
preserving the existing clamping behavior.
- Around line 307-315: Update the normalization loop using
m_gridMass.ForEachDataPointIndex and the subsequent
m_gridVelocitiesBeforeUpdate.Set operation to use the grid classes’ parallel
execution policy, while keeping the preceding scatter loop serial because it has
write conflicts.
- Around line 246-261: Rename the MPMSystemData getter GetFLIPBlendingFactor()
to FLIPBlendingFactor() to match the surrounding accessor naming style, and
update all call sites and declarations accordingly. Keep SetFLIPBlendingFactor()
and its existing validation logic unchanged.
- Around line 69-97: Hoist the axis-independent lowestIndex and highestIndex
calculations out of the per-axis loop, declaring them once as constexpr-style
locals before iterating. Keep the existing bounds checks and index calculations
in the loop unchanged.
- Around line 99-133: Optimize transfer-only stencil generation by adding a
gradient-free path, such as GetWeights, and update TransferFromParticlesToGrid
and TransferFromGridToParticles to use it while keeping GetStencil gradients for
force computation. Replace flatIndex++ in GetStencil with a deterministic index
derived from offset, avoiding dependence on traversal order and making indexing
safe under parallel iteration. Preserve the existing stencil ordering expected
by positional tests.
In `@Includes/Core/Particle/MPM/MPMSystemData.hpp`:
- Around line 55-62: Update MPMSystemData to override Serialize and Deserialize
so its MPM-specific state—m_particleMasses, m_initialVolumes,
m_deformationStates, and the three grids—is persisted and restored when accessed
through Serializable, keeping derived array sizes synchronized with the
deserialized base particle count. If full serialization cannot be implemented in
this change, document the limitation in the MPMSystemData class Doxygen comment
and record a follow-up issue.
- Around line 118-128: Expose MPMSystemData2 and MPMSystemData3 through the
Python API alongside ParticleSystemData and SPHSystemData, adding matching
bindings and registering both types in Sources/API/Python/main.cpp; if this
cannot be completed, explicitly document that these bindings are deferred.
In `@Tests/UnitTests/MPMSystemDataTests.cpp`:
- Around line 263-299: Refactor RejectsInvalidInput into an N-parameterized test
helper using dimension-generic vectors and grid data, then invoke it for both
MPMSystemData2 and MPMSystemData3. Preserve all existing invalid-input
assertions while adapting dimension-dependent values such as zero,
negative-spacing, resolution, positions, velocities, and grid velocities to work
for both dimensions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2bd63b64-a9fc-490d-93b9-30897e35e6ac
📒 Files selected for processing (4)
Includes/Core/Particle/MPM/MPMSystemData-Impl.hppIncludes/Core/Particle/MPM/MPMSystemData.hppIncludes/Core/Particle/ParticleSystemData.hppTests/UnitTests/MPMSystemDataTests.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (16)
- GitHub Check: 🪟 CUDA Build - Windows Server 2025 + Visual Studio 2026 + CUDA 13.2.0 (Release)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: 🪟 CUDA Build - Windows Server 2022 + Visual Studio 2022 + CUDA 12.6.3 (Release)
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-16
- GitHub Check: 🐧 Build - Ubuntu 24.04 + gcc-14
- GitHub Check: 🪟 Build - Windows Server 2022 + Visual Studio 2022
- GitHub Check: 🪟 Build - Windows Server 2025 + Visual Studio 2026
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-18
- GitHub Check: 🐧 Build - Ubuntu 24.04 + gcc-12
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-17
- GitHub Check: 🧪 Code Coverage - Codecov (Ubuntu 24.04 + gcc-14, ubuntu-24.04, gcc, 14)
- GitHub Check: 🐧 Build - Ubuntu 24.04 + gcc-13
- GitHub Check: 🍎 Build - macOS 26.3 + Xcode 26.3
- GitHub Check: 🐧 CUDA Build - Ubuntu 24.04 + gcc-12 + CUDA 12.6.3
- GitHub Check: 🍎 Build - macOS 15.7.4 + Xcode 16.4
- GitHub Check: 🌞 Static Analysis - SonarCloud (Ubuntu 24.04 + gcc-14, ubuntu-24.04, gcc, 14)
🧰 Additional context used
📓 Path-based instructions (5)
Includes/Core/**/*.hpp
📄 CodeRabbit inference engine (AGENTS.md)
Keep public C++ declarations and Doxygen comments under
Includes/Core/; use project includes such as<Core/...>and theCubbyFlownamespace.
Files:
Includes/Core/Particle/MPM/MPMSystemData.hppIncludes/Core/Particle/ParticleSystemData.hppIncludes/Core/Particle/MPM/MPMSystemData-Impl.hpp
Includes/Core/**/*.{hpp,h}
📄 CodeRabbit inference engine (AGENTS.md)
For dimensional templates, keep dimension-independent logic shared, preserve
Foo2/Foo3and pointer aliases, and follow existing builder and ownership APIs.
Files:
Includes/Core/Particle/MPM/MPMSystemData.hppIncludes/Core/Particle/ParticleSystemData.hppIncludes/Core/Particle/MPM/MPMSystemData-Impl.hpp
**/*.{cpp,cc,cxx,h,hpp,cu,cuh}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{cpp,cc,cxx,h,hpp,cu,cuh}: Treat warnings as errors under the defaultCUBBYFLOW_WARNINGS_AS_ERRORS=ON; fix project warnings instead of globally suppressing them.
Follow.clang-format: four-space indentation, 80-column C++ limit, sorted includes, project brace style, and format only touched C++/CUDA files.
Preserve existing copyright headers and keep comments focused on intent, invariants, numerical reasoning, or non-obvious constraints.
Files:
Includes/Core/Particle/MPM/MPMSystemData.hppIncludes/Core/Particle/ParticleSystemData.hppTests/UnitTests/MPMSystemDataTests.cppIncludes/Core/Particle/MPM/MPMSystemData-Impl.hpp
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Keep 2-D and 3-D behavior aligned; inspect sibling implementations, aliases, explicit instantiations, bindings, and tests before changing one dimensional side.
Keep the public C++ API and Python-visible behavior synchronized when changing public types, methods, defaults, enums, or solver behavior.
Fix shared behavior at the shared layer; search callers, overrides, bindings, tests, and dimensional specializations before editing, and avoid one-off caller guards when an invariant belongs in a common utility or base class.
Use existing patterns and dependencies before adding abstractions, libraries, templates, builders, numerical helpers, parallel loops, serialization code, or tests.
Run the smallest relevant validation: focused C++ tests for core behavior, both dimensions for shared templates, focused pytest for Python APIs, CUDA compilation/tests for CUDA changes, fresh configure/build for build changes, and round-trip tests for serialization.
Do not commit build output, test logs, caches, IDE state, generated build files, or unrelated formatting; keep commits focused and use the appropriate conventional prefix.
Before handoff, confirm the fix is at the shared behavior source, dimensional/Python/CUDA surfaces are synchronized when relevant, focused checks pass, touched C++/CUDA files are formatted, and skipped platform/GPU/performance validation is explicitly reported.
Files:
Includes/Core/Particle/MPM/MPMSystemData.hppIncludes/Core/Particle/ParticleSystemData.hppTests/UnitTests/MPMSystemDataTests.cppIncludes/Core/Particle/MPM/MPMSystemData-Impl.hpp
Tests/UnitTests/**/*.{cpp,hpp}
📄 CodeRabbit inference engine (AGENTS.md)
Tests/UnitTests/**/*.{cpp,hpp}: Update or add tests for both 2-D and 3-D counterparts when behavior applies to both; use GoogleTest/GMock macros and focused regression scenarios.
Use the existingRESOURCES_DIRcompile definition for C++ fixtures rather than relying on the current working directory.
Files:
Tests/UnitTests/MPMSystemDataTests.cpp
🔇 Additional comments (13)
Tests/UnitTests/MPMSystemDataTests.cpp (1)
1-261: LGTM!Includes/Core/Particle/MPM/MPMSystemData.hpp (2)
64-79: LGTM!
34-41: 📐 Maintainability & Code Quality
ssize_tis defined byCore/Utils/Macros.hpp, whichMatrix.hppincludes throughSnowConstitutiveModel.hpp. The MSVC path usesBaseTsd.h, so this code is supported.> Likely an incorrect or invalid review comment.Includes/Core/Particle/MPM/MPMSystemData-Impl.hpp (10)
23-40: LGTM!
42-59: LGTM!
138-147: LGTM!
159-169: LGTM!
171-244: LGTM!
271-278: Validate-then-mutate ordering is correct.The validation loop runs before
Fill, so a throw leaves the grids untouched. Keep this ordering if the loop is later parallelized.
338-357: Blending accumulation is correct.The weights sum to one across the stencil, so
picVelocityis a convex combination even whenFoldIndexmaps several entries onto the same boundary node. TheflipDeltaaccumulation uses the same weights, which keeps the pure-PIC and pure-FLIP limits exact.
389-401: LGTM!
403-418: LGTM!
420-442: Geometry cross-check is well placed.
GridMass(),GridVelocities(), andGridVelocitiesBeforeUpdate()all return mutable references, so a caller can resize one grid alone. This check catches that desynchronization before either transfer reads the grids. Exact comparison is the correct choice here, because the three grids otherwise receive bit-identical values fromResizeGrid.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #174 +/- ##
==========================================
+ Coverage 83.18% 83.43% +0.24%
==========================================
Files 414 416 +2
Lines 23286 23568 +282
==========================================
+ Hits 19370 19663 +293
+ Misses 3916 3905 -11 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Includes/Core/Particle/MPM/MPMSystemData.hpp`:
- Around line 64-70: Synchronize the public MPMSystemData API with the supported
Python boundary: expose its new constructors, state accessors, and transfer
methods through Python bindings and add focused Python coverage, or explicitly
mark the type and API as C++-only using the project’s established mechanism.
Remove the stale deferred-bindings note once the chosen boundary is implemented.
In `@Sources/Core/Particle/ParticleSystemData.cpp`:
- Around line 425-432: After the Resize calls in ParticleSystemData::Deserialize
and ParticleSystemData::Set, add a shared post-restore/copy invalidation path
that clears or resets all MPM-specific state: m_gridMass, m_gridVelocities, and
m_gridVelocitiesBeforeUpdate, along with related preserved particle-derived
masses, initial volumes, and deformation states. Ensure reused MPMSystemData<2>
and MPMSystemData<3> objects cannot retain stale grid or particle state, and add
tests covering deserialize and copy reuse.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64969fbc-7573-4d7e-86d0-4bf7f68f0b1e
📒 Files selected for processing (4)
Includes/Core/Particle/MPM/MPMSystemData-Impl.hppIncludes/Core/Particle/MPM/MPMSystemData.hppSources/Core/Particle/ParticleSystemData.cppTests/UnitTests/MPMSystemDataTests.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- Includes/Core/Particle/MPM/MPMSystemData-Impl.hpp
- Tests/UnitTests/MPMSystemDataTests.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (16)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: 🐧 CUDA Build - Ubuntu 24.04 + gcc-12 + CUDA 12.6.3
- GitHub Check: 🪟 Build - Windows Server 2025 + Visual Studio 2026
- GitHub Check: 🐧 Build - Ubuntu 24.04 + gcc-14
- GitHub Check: 🪟 Build - Windows Server 2022 + Visual Studio 2022
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-16
- GitHub Check: 🐧 Build - Ubuntu 24.04 + gcc-12
- GitHub Check: 🐧 Build - Ubuntu 24.04 + gcc-13
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-17
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-18
- GitHub Check: 🍎 Build - macOS 15.7.4 + Xcode 16.4
- GitHub Check: 🧪 Code Coverage - Codecov (Ubuntu 24.04 + gcc-14, ubuntu-24.04, gcc, 14)
- GitHub Check: 🪟 CUDA Build - Windows Server 2025 + Visual Studio 2026 + CUDA 13.2.0 (Release)
- GitHub Check: 🪟 CUDA Build - Windows Server 2022 + Visual Studio 2022 + CUDA 12.6.3 (Release)
- GitHub Check: 🍎 Build - macOS 26.3 + Xcode 26.3
- GitHub Check: 🌞 Static Analysis - SonarCloud (Ubuntu 24.04 + gcc-14, ubuntu-24.04, gcc, 14)
🧰 Additional context used
📓 Path-based instructions (6)
Sources/Core/**/*.{cpp,hpp}
📄 CodeRabbit inference engine (AGENTS.md)
Keep non-inline implementations in the matching
Sources/Core/domain; use-Impl.hppfor template definitions that must be visible to callers.
Files:
Sources/Core/Particle/ParticleSystemData.cpp
Sources/Core/**/*.cpp
📄 CodeRabbit inference engine (AGENTS.md)
When both dimensions are supported, preserve explicit template instantiations for dimensions 2 and 3 and keep corresponding implementations aligned.
Files:
Sources/Core/Particle/ParticleSystemData.cpp
**/*.{cpp,cc,cxx,h,hpp,cu,cuh}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{cpp,cc,cxx,h,hpp,cu,cuh}: Treat warnings as errors under the defaultCUBBYFLOW_WARNINGS_AS_ERRORS=ON; fix project warnings instead of globally suppressing them.
Follow.clang-format: four-space indentation, 80-column C++ limit, sorted includes, project brace style, and format only touched C++/CUDA files.
Preserve existing copyright headers and keep comments focused on intent, invariants, numerical reasoning, or non-obvious constraints.
Files:
Sources/Core/Particle/ParticleSystemData.cppIncludes/Core/Particle/MPM/MPMSystemData.hpp
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Keep 2-D and 3-D behavior aligned; inspect sibling implementations, aliases, explicit instantiations, bindings, and tests before changing one dimensional side.
Keep the public C++ API and Python-visible behavior synchronized when changing public types, methods, defaults, enums, or solver behavior.
Fix shared behavior at the shared layer; search callers, overrides, bindings, tests, and dimensional specializations before editing, and avoid one-off caller guards when an invariant belongs in a common utility or base class.
Use existing patterns and dependencies before adding abstractions, libraries, templates, builders, numerical helpers, parallel loops, serialization code, or tests.
Run the smallest relevant validation: focused C++ tests for core behavior, both dimensions for shared templates, focused pytest for Python APIs, CUDA compilation/tests for CUDA changes, fresh configure/build for build changes, and round-trip tests for serialization.
Do not commit build output, test logs, caches, IDE state, generated build files, or unrelated formatting; keep commits focused and use the appropriate conventional prefix.
Before handoff, confirm the fix is at the shared behavior source, dimensional/Python/CUDA surfaces are synchronized when relevant, focused checks pass, touched C++/CUDA files are formatted, and skipped platform/GPU/performance validation is explicitly reported.
Files:
Sources/Core/Particle/ParticleSystemData.cppIncludes/Core/Particle/MPM/MPMSystemData.hpp
Includes/Core/**/*.hpp
📄 CodeRabbit inference engine (AGENTS.md)
Keep public C++ declarations and Doxygen comments under
Includes/Core/; use project includes such as<Core/...>and theCubbyFlownamespace.
Files:
Includes/Core/Particle/MPM/MPMSystemData.hpp
Includes/Core/**/*.{hpp,h}
📄 CodeRabbit inference engine (AGENTS.md)
For dimensional templates, keep dimension-independent logic shared, preserve
Foo2/Foo3and pointer aliases, and follow existing builder and ownership APIs.
Files:
Includes/Core/Particle/MPM/MPMSystemData.hpp
🪛 Cppcheck (2.21.0)
Sources/Core/Particle/ParticleSystemData.cpp
[warning] 431-431: The address of variable 'x' might be accessed at non-zero index.
(objectIndex)
🔇 Additional comments (3)
Includes/Core/Particle/MPM/MPMSystemData.hpp (3)
21-62: LGTM!
71-167: LGTM!
169-184: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Tests/PythonTests/test_mpm_system_data.py`:
- Around line 31-36: Update the test around particleMasses and initialVolumes to
reread both properties from data after assigning through the NumPy views, and
assert they contain 2.0 and 3.0 respectively before the transfer call. Preserve
the existing shape assertions and mutation flow.
- Line 54: Update the test around ResizeGrid to call it with different valid
resolution, spacing, and origin values instead of the original construction
parameters, then assert the grid properties reflect all three updated values so
a no-op ResizeGrid implementation cannot pass.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5eb17130-bf31-4b86-b072-f7e9111b7df1
📒 Files selected for processing (8)
Includes/API/Python/Particle/MPM/MPMSystemData.hppIncludes/Core/Particle/MPM/MPMSystemData-Impl.hppIncludes/Core/Particle/MPM/MPMSystemData.hppIncludes/Core/Particle/ParticleSystemData.hppSources/API/Python/Particle/MPM/MPMSystemData.cppSources/API/Python/main.cppTests/PythonTests/test_mpm_system_data.pyTests/UnitTests/MPMSystemDataTests.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- Includes/Core/Particle/MPM/MPMSystemData-Impl.hpp
- Tests/UnitTests/MPMSystemDataTests.cpp
- Includes/Core/Particle/MPM/MPMSystemData.hpp
📜 Review details
⏰ Context from checks skipped due to timeout. (16)
- GitHub Check: 🧪 Code Coverage - Codecov (Ubuntu 24.04 + gcc-14, ubuntu-24.04, gcc, 14)
- GitHub Check: 🌞 Static Analysis - SonarCloud (Ubuntu 24.04 + gcc-14, ubuntu-24.04, gcc, 14)
- GitHub Check: 🪟 Build - Windows Server 2022 + Visual Studio 2022
- GitHub Check: 🪟 Build - Windows Server 2025 + Visual Studio 2026
- GitHub Check: 🐧 CUDA Build - Ubuntu 24.04 + gcc-12 + CUDA 12.6.3
- GitHub Check: 🐧 Build - Ubuntu 24.04 + gcc-14
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-18
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-16
- GitHub Check: 🐧 Build - Ubuntu 24.04 + gcc-12
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-17
- GitHub Check: 🐧 Build - Ubuntu 24.04 + gcc-13
- GitHub Check: 🪟 CUDA Build - Windows Server 2022 + Visual Studio 2022 + CUDA 12.6.3 (Release)
- GitHub Check: 🪟 CUDA Build - Windows Server 2025 + Visual Studio 2026 + CUDA 13.2.0 (Release)
- GitHub Check: 🍎 Build - macOS 26.3 + Xcode 26.3
- GitHub Check: 🍎 Build - macOS 15.7.4 + Xcode 16.4
- GitHub Check: Codacy Static Code Analysis
🧰 Additional context used
📓 Path-based instructions (8)
Tests/PythonTests/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Tests/PythonTests/test_*.py: Add or preserve focused pytest coverage for every new or changed Python-visible API.
Name Python teststest_type.pyand run focused pytest files for Python API changes.
Files:
Tests/PythonTests/test_mpm_system_data.py
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Keep 2-D and 3-D behavior aligned; inspect sibling implementations, aliases, explicit instantiations, bindings, and tests before changing one dimensional side.
Keep the public C++ API and Python-visible behavior synchronized when changing public types, methods, defaults, enums, or solver behavior.
Fix shared behavior at the shared layer; search callers, overrides, bindings, tests, and dimensional specializations before editing, and avoid one-off caller guards when an invariant belongs in a common utility or base class.
Use existing patterns and dependencies before adding abstractions, libraries, templates, builders, numerical helpers, parallel loops, serialization code, or tests.
Run the smallest relevant validation: focused C++ tests for core behavior, both dimensions for shared templates, focused pytest for Python APIs, CUDA compilation/tests for CUDA changes, fresh configure/build for build changes, and round-trip tests for serialization.
Do not commit build output, test logs, caches, IDE state, generated build files, or unrelated formatting; keep commits focused and use the appropriate conventional prefix.
Before handoff, confirm the fix is at the shared behavior source, dimensional/Python/CUDA surfaces are synchronized when relevant, focused checks pass, touched C++/CUDA files are formatted, and skipped platform/GPU/performance validation is explicitly reported.
Files:
Tests/PythonTests/test_mpm_system_data.pySources/API/Python/main.cppIncludes/API/Python/Particle/MPM/MPMSystemData.hppSources/API/Python/Particle/MPM/MPMSystemData.cppIncludes/Core/Particle/ParticleSystemData.hpp
Sources/API/Python/**/*.{cpp,hpp}
📄 CodeRabbit inference engine (AGENTS.md)
Keep Python binding implementations synchronized with core APIs, use existing Python names and camelCase property conventions, and do not mechanically expose C++ spelling.
Files:
Sources/API/Python/main.cppSources/API/Python/Particle/MPM/MPMSystemData.cpp
Sources/API/Python/main.cpp
📄 CodeRabbit inference engine (AGENTS.md)
Register every exposed binding in
main.cppand preserve dependency order.
Files:
Sources/API/Python/main.cpp
**/*.{cpp,cc,cxx,h,hpp,cu,cuh}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{cpp,cc,cxx,h,hpp,cu,cuh}: Treat warnings as errors under the defaultCUBBYFLOW_WARNINGS_AS_ERRORS=ON; fix project warnings instead of globally suppressing them.
Follow.clang-format: four-space indentation, 80-column C++ limit, sorted includes, project brace style, and format only touched C++/CUDA files.
Preserve existing copyright headers and keep comments focused on intent, invariants, numerical reasoning, or non-obvious constraints.
Files:
Sources/API/Python/main.cppIncludes/API/Python/Particle/MPM/MPMSystemData.hppSources/API/Python/Particle/MPM/MPMSystemData.cppIncludes/Core/Particle/ParticleSystemData.hpp
Includes/API/Python/**/*.{hpp,h}
📄 CodeRabbit inference engine (AGENTS.md)
Keep Python binding declarations synchronized with public Python-visible C++ APIs.
Files:
Includes/API/Python/Particle/MPM/MPMSystemData.hpp
Includes/Core/**/*.hpp
📄 CodeRabbit inference engine (AGENTS.md)
Keep public C++ declarations and Doxygen comments under
Includes/Core/; use project includes such as<Core/...>and theCubbyFlownamespace.
Files:
Includes/Core/Particle/ParticleSystemData.hpp
Includes/Core/**/*.{hpp,h}
📄 CodeRabbit inference engine (AGENTS.md)
For dimensional templates, keep dimension-independent logic shared, preserve
Foo2/Foo3and pointer aliases, and follow existing builder and ownership APIs.
Files:
Includes/Core/Particle/ParticleSystemData.hpp
🔇 Additional comments (6)
Includes/Core/Particle/ParticleSystemData.hpp (1)
89-89: LGTM!Also applies to: 234-234
Includes/API/Python/Particle/MPM/MPMSystemData.hpp (1)
11-19: LGTM!Sources/API/Python/Particle/MPM/MPMSystemData.cpp (2)
22-87: LGTM!Also applies to: 120-145
92-117: 🎯 Functional CorrectnessConfirm the established Python names before changing these bindings.
The available API references do not identify the required camelCase names for these methods.
Sources/API/Python/main.cpp (1)
57-57: LGTM!Also applies to: 222-223
Tests/PythonTests/test_mpm_system_data.py (1)
1-30: LGTM!Also applies to: 38-53
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Tests/PythonTests/test_mpm_system_data.py`:
- Around line 56-60: Expand the ResizeGrid assertions in the test around
resized_grid to validate resolution, gridSpacing, and gridOrigin for
data.gridVelocities and data.gridVelocitiesBeforeUpdate as well as
data.gridMass. Keep the existing expected resized_grid values and focused pytest
coverage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be7fe077-ceee-4a39-b9fc-1334180e2d62
📒 Files selected for processing (1)
Tests/PythonTests/test_mpm_system_data.py
📜 Review details
⏰ Context from checks skipped due to timeout. (16)
- GitHub Check: 🍎 Build - macOS 26.3 + Xcode 26.3
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-17
- GitHub Check: 🍎 Build - macOS 15.7.4 + Xcode 16.4
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-16
- GitHub Check: 🐧 Build - Ubuntu 24.04 + gcc-14
- GitHub Check: 🐧 Build - Ubuntu 24.04 + gcc-12
- GitHub Check: 🐧 Build - Ubuntu 24.04 + gcc-13
- GitHub Check: 🪟 Build - Windows Server 2025 + Visual Studio 2026
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-18
- GitHub Check: 🐧 CUDA Build - Ubuntu 24.04 + gcc-12 + CUDA 12.6.3
- GitHub Check: 🪟 Build - Windows Server 2022 + Visual Studio 2022
- GitHub Check: 🧪 Code Coverage - Codecov (Ubuntu 24.04 + gcc-14, ubuntu-24.04, gcc, 14)
- GitHub Check: 🌞 Static Analysis - SonarCloud (Ubuntu 24.04 + gcc-14, ubuntu-24.04, gcc, 14)
- GitHub Check: 🪟 CUDA Build - Windows Server 2022 + Visual Studio 2022 + CUDA 12.6.3 (Release)
- GitHub Check: 🪟 CUDA Build - Windows Server 2025 + Visual Studio 2026 + CUDA 13.2.0 (Release)
- GitHub Check: Codacy Static Code Analysis
🧰 Additional context used
📓 Path-based instructions (2)
Tests/PythonTests/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Tests/PythonTests/test_*.py: Add or preserve focused pytest coverage for every new or changed Python-visible API.
Name Python teststest_type.pyand run focused pytest files for Python API changes.
Files:
Tests/PythonTests/test_mpm_system_data.py
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Keep 2-D and 3-D behavior aligned; inspect sibling implementations, aliases, explicit instantiations, bindings, and tests before changing one dimensional side.
Keep the public C++ API and Python-visible behavior synchronized when changing public types, methods, defaults, enums, or solver behavior.
Fix shared behavior at the shared layer; search callers, overrides, bindings, tests, and dimensional specializations before editing, and avoid one-off caller guards when an invariant belongs in a common utility or base class.
Use existing patterns and dependencies before adding abstractions, libraries, templates, builders, numerical helpers, parallel loops, serialization code, or tests.
Run the smallest relevant validation: focused C++ tests for core behavior, both dimensions for shared templates, focused pytest for Python APIs, CUDA compilation/tests for CUDA changes, fresh configure/build for build changes, and round-trip tests for serialization.
Do not commit build output, test logs, caches, IDE state, generated build files, or unrelated formatting; keep commits focused and use the appropriate conventional prefix.
Before handoff, confirm the fix is at the shared behavior source, dimensional/Python/CUDA surfaces are synchronized when relevant, focused checks pass, touched C++/CUDA files are formatted, and skipped platform/GPU/performance validation is explicitly reported.
Files:
Tests/PythonTests/test_mpm_system_data.py
🔇 Additional comments (1)
Tests/PythonTests/test_mpm_system_data.py (1)
37-38: LGTM!
f9f441d to
aeb1b37
Compare
|
1 participant
This revision includes:
Summary by CodeRabbit
New Features
Bug Fixes
Tests