Add semi-implicit grid integration for snow MPM - #178
Conversation
📝 WalkthroughWalkthroughAdds a public conjugate-residual solver and a snow first Piola stress differential. Extends ChangesSnow MPM semi-implicit integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SnowMPMSolver
participant ActiveNodes
participant ElasticHessian
participant CR
participant Grid
SnowMPMSolver->>ActiveNodes: build active-node and constraint arrays
SnowMPMSolver->>ElasticHessian: assemble grid operator
SnowMPMSolver->>CR: solve velocity correction
CR-->>SnowMPMSolver: return residual and iteration count
SnowMPMSolver->>Grid: commit successful velocities or restore prior state
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 | 123 |
| Duplication | 12 |
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: 1
🧹 Nitpick comments (5)
Includes/Core/Particle/MPM/SnowConstitutiveModel.hpp (1)
93-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete the Doxygen block for the new public method.
Update,ComputeKirchhoffStress, andComputeWaveSpeeddocument their parameters and return values. Add the same forComputeFirstPiolaStressDifferential, and state the throwing behavior, because the implementation rejects a non-finitedifferentialand a non-finite result withstd::invalid_argument.📝 Proposed documentation
//! //! \brief Computes the first Piola stress differential. //! //! Evaluates `(d^2 Psi / d F_E d F_E) : differential` while holding the //! plastic deformation fixed. //! + //! \param[in] state Current deformation state. + //! \param[in] differential Elastic deformation-gradient differential. + //! + //! \return First Piola stress differential. + //! [[nodiscard]] MatrixType ComputeFirstPiolaStressDifferential( const State& state, const MatrixType& differential) const;As per path instructions: "Keep public C++ declarations and Doxygen comments under
Includes/Core/".🤖 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/SnowConstitutiveModel.hpp` around lines 93 - 100, Complete the Doxygen block for the public ComputeFirstPiolaStressDifferential method by documenting both parameters, its returned stress differential, and that it throws std::invalid_argument when differential or the computed result is non-finite. Keep the declaration and documentation within the existing Includes/Core/ location.Source: Path instructions
Tests/UnitTests/CGTests.cpp (1)
97-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
CRbreakdown path.The three new tests cover convergence, zero iterations, and an already-converged input. They do not reach the new failure branches, which set
residualNormto infinity forq == 0,rho == 0, or a non-finite residual.SnowMPMSolver::SolveGridVelocitiestreats that infinite residual as the failure signal, so the contract deserves a direct test.💚 Proposed additional test
TEST(CR, SingularSystemReportsFailure) { using BLASType = BLAS<double, Vector2D, Matrix2x2D>; const Matrix2x2D matrix(1.0, 0.0, 0.0, 0.0); const Vector2D rhs(0.0, 1.0); Vector2D x, r, d, q, s; unsigned int iterations = 0; double residual = 0.0; CR<BLASType>(matrix, rhs, 10, 1e-14, &x, &r, &d, &q, &s, &iterations, &residual); EXPECT_FALSE(std::isfinite(residual)); }🤖 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/CGTests.cpp` around lines 97 - 151, Add a CR breakdown-path test near the existing CR tests, using a singular Matrix2x2D and incompatible rhs to trigger failure. Verify that CR reports failure by setting residual to a non-finite value, while preserving the existing convergence and early-exit coverage.Includes/Core/Solver/Particle/MPM/SnowMPMSolver-Impl.hpp (2)
56-93: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWrite the constrained rows of
outputexplicitly.
Multiplyassigns only unconstrained rows.output->Resize(input.GetRows(), 0.0)does not clear elements that already exist, so constrained rows keep whatever the caller's work vector held. The current call site works only becauseSolveGridVelocitiesallocates every CR vector with 0.0 and keeps the right-hand side zero on constrained rows. Make the operator self-contained so the invariant does not depend on the caller.🛡️ Proposed fix
output->Resize(input.GetRows(), 0.0); + output->Fill(0.0);🤖 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/Solver/Particle/MPM/SnowMPMSolver-Impl.hpp` around lines 56 - 93, Update SnowMPMSolver<N>::LinearSystem::Multiply to explicitly assign zero to every constrained output row, rather than relying on VectorND::Resize to clear existing storage. Preserve the current mass-plus-Hessian computation for unconstrained rows and ensure each constrained row is written deterministically on every call.
562-580: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftCache per-particle quantities and parallelize the Hessian application.
ApplyElasticHessianruns once per CR iteration. For each call and each particle it walks the stencil twice and callsComputeFirstPiolaStressDifferential, which recomputes the SVD, the inverse, the determinant, and the hardening factor. With the default limit of 100 iterations, the same SVD is computed 100 times per particle per step. The loop is also serial, whileConstrainParticlesToDomainin the same file usesParallelFor.Two improvements keep the operator identical:
- Precompute per-particle SVD, hardening, volume, and
F_E^Tonce perSolveGridVelocitiescall, and pass them to a differential overload.- Parallelize the particle loop with per-node accumulation, for example thread-local buffers reduced into
output, so the scatter stays deterministic.🤖 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/Solver/Particle/MPM/SnowMPMSolver-Impl.hpp` around lines 562 - 580, The Hessian application currently recomputes per-particle constitutive quantities serially on every iteration. Update SolveGridVelocities and ApplyElasticHessian to precompute and retain each particle’s SVD, hardening factor, volume, and F_E^T once per solve, then use a differential ComputeFirstPiolaStressDifferential overload that consumes those cached values; parallelize the particle loop using thread-local per-node accumulations and a deterministic reduction into output, preserving the existing operator result.Includes/Core/Math/CG.hpp (1)
51-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the tolerance convention and the failure signal for
CR.
CRcompares the residual L2 norm againsttolerance, whilePCGcompares the squared residual againstSquare(tolerance). A caller that reuses thePCGconvention will request a tolerance that is off by a square.CRalso reports failure by writing a non-finitelastResidualNormand leaves a partially updatedx. Both facts belong in the public header.📝 Proposed documentation
//! //! \brief Solves a symmetric linear system with conjugate residual. //! +//! \param[in] A Symmetric (possibly indefinite) system operator. +//! \param[in] b Right-hand side. +//! \param[in] maxNumberOfIterations Iteration limit; zero permits only an +//! already-converged input. +//! \param[in] tolerance Absolute residual L2-norm target. Unlike `PCG`, this +//! value is not squared internally. +//! \param[out] x Solution; also used as the initial guess. +//! \param[out] r, d, q, s Caller-provided work vectors. +//! \param[out] lastNumberOfIterations Performed iteration count. +//! \param[out] lastResidualNorm Final residual L2 norm. On breakdown or a +//! non-finite intermediate value, this is set to infinity and `x` holds a +//! partially updated result. +//! template <typename BLASType>🤖 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/Math/CG.hpp` around lines 51 - 61, Update the public documentation for CR to state that tolerance is compared directly with the residual L2 norm, unlike PCG’s squared-residual convention. Also document that failure is signaled by writing a non-finite lastResidualNorm and that x may be partially updated; keep the function signature and behavior unchanged.
🤖 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/Solver/Particle/MPM/SnowMPMSolver.hpp`:
- Around line 32-33: Update the semi-implicit integration documentation near
SolveGridVelocities to state that the path can throw both std::runtime_error and
std::invalid_argument, including failures propagated from
ComputeFirstPiolaStressDifferential. Preserve the existing exception behavior
rather than translating or altering exceptions.
---
Nitpick comments:
In `@Includes/Core/Math/CG.hpp`:
- Around line 51-61: Update the public documentation for CR to state that
tolerance is compared directly with the residual L2 norm, unlike PCG’s
squared-residual convention. Also document that failure is signaled by writing a
non-finite lastResidualNorm and that x may be partially updated; keep the
function signature and behavior unchanged.
In `@Includes/Core/Particle/MPM/SnowConstitutiveModel.hpp`:
- Around line 93-100: Complete the Doxygen block for the public
ComputeFirstPiolaStressDifferential method by documenting both parameters, its
returned stress differential, and that it throws std::invalid_argument when
differential or the computed result is non-finite. Keep the declaration and
documentation within the existing Includes/Core/ location.
In `@Includes/Core/Solver/Particle/MPM/SnowMPMSolver-Impl.hpp`:
- Around line 56-93: Update SnowMPMSolver<N>::LinearSystem::Multiply to
explicitly assign zero to every constrained output row, rather than relying on
VectorND::Resize to clear existing storage. Preserve the current
mass-plus-Hessian computation for unconstrained rows and ensure each constrained
row is written deterministically on every call.
- Around line 562-580: The Hessian application currently recomputes per-particle
constitutive quantities serially on every iteration. Update SolveGridVelocities
and ApplyElasticHessian to precompute and retain each particle’s SVD, hardening
factor, volume, and F_E^T once per solve, then use a differential
ComputeFirstPiolaStressDifferential overload that consumes those cached values;
parallelize the particle loop using thread-local per-node accumulations and a
deterministic reduction into output, preserving the existing operator result.
In `@Tests/UnitTests/CGTests.cpp`:
- Around line 97-151: Add a CR breakdown-path test near the existing CR tests,
using a singular Matrix2x2D and incompatible rhs to trigger failure. Verify that
CR reports failure by setting residual to a non-finite value, while preserving
the existing convergence and early-exit 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: dbf02c33-f759-4905-b550-fa34350bcd37
📒 Files selected for processing (11)
Includes/Core/Math/CG-Impl.hppIncludes/Core/Math/CG.hppIncludes/Core/Particle/MPM/SnowConstitutiveModel-Impl.hppIncludes/Core/Particle/MPM/SnowConstitutiveModel.hppIncludes/Core/Solver/Particle/MPM/SnowMPMSolver-Impl.hppIncludes/Core/Solver/Particle/MPM/SnowMPMSolver.hppSources/API/Python/Solver/Particle/MPM/SnowMPMSolver.cppTests/PythonTests/test_snow_mpm_solver.pyTests/UnitTests/CGTests.cppTests/UnitTests/SnowConstitutiveModelTests.cppTests/UnitTests/SnowMPMSolverTests.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (16)
- GitHub Check: 🌞 Static Analysis - SonarCloud (Ubuntu 24.04 + gcc-14, ubuntu-24.04, gcc, 14)
- GitHub Check: 🧪 Code Coverage - Codecov (Ubuntu 24.04 + gcc-14, ubuntu-24.04, gcc, 14)
- GitHub Check: 🍎 Build - macOS 15.7.4 + Xcode 16.4
- GitHub Check: 🍎 Build - macOS 26.3 + Xcode 26.3
- GitHub Check: 🪟 Build - Windows Server 2022 + Visual Studio 2022
- GitHub Check: 🐧 Build - Ubuntu 24.04 + gcc-13
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-18
- GitHub Check: 🐧 Build - Ubuntu 24.04 + gcc-12
- GitHub Check: 🪟 Build - Windows Server 2025 + Visual Studio 2026
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-17
- GitHub Check: 🐧 Build - Ubuntu 24.04 + gcc-14
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-16
- GitHub Check: 🪟 CUDA Build - Windows Server 2022 + Visual Studio 2022 + CUDA 12.6.3 (Release)
- GitHub Check: 🐧 CUDA Build - Ubuntu 24.04 + gcc-12 + CUDA 12.6.3
- 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 (7)
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_snow_mpm_solver.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_snow_mpm_solver.pyIncludes/Core/Particle/MPM/SnowConstitutiveModel.hppTests/UnitTests/SnowConstitutiveModelTests.cppSources/API/Python/Solver/Particle/MPM/SnowMPMSolver.cppIncludes/Core/Math/CG.hppIncludes/Core/Math/CG-Impl.hppIncludes/Core/Particle/MPM/SnowConstitutiveModel-Impl.hppTests/UnitTests/SnowMPMSolverTests.cppIncludes/Core/Solver/Particle/MPM/SnowMPMSolver.hppTests/UnitTests/CGTests.cppIncludes/Core/Solver/Particle/MPM/SnowMPMSolver-Impl.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/SnowConstitutiveModel.hppIncludes/Core/Math/CG.hppIncludes/Core/Math/CG-Impl.hppIncludes/Core/Particle/MPM/SnowConstitutiveModel-Impl.hppIncludes/Core/Solver/Particle/MPM/SnowMPMSolver.hppIncludes/Core/Solver/Particle/MPM/SnowMPMSolver-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/SnowConstitutiveModel.hppIncludes/Core/Math/CG.hppIncludes/Core/Math/CG-Impl.hppIncludes/Core/Particle/MPM/SnowConstitutiveModel-Impl.hppIncludes/Core/Solver/Particle/MPM/SnowMPMSolver.hppIncludes/Core/Solver/Particle/MPM/SnowMPMSolver-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/SnowConstitutiveModel.hppTests/UnitTests/SnowConstitutiveModelTests.cppSources/API/Python/Solver/Particle/MPM/SnowMPMSolver.cppIncludes/Core/Math/CG.hppIncludes/Core/Math/CG-Impl.hppIncludes/Core/Particle/MPM/SnowConstitutiveModel-Impl.hppTests/UnitTests/SnowMPMSolverTests.cppIncludes/Core/Solver/Particle/MPM/SnowMPMSolver.hppTests/UnitTests/CGTests.cppIncludes/Core/Solver/Particle/MPM/SnowMPMSolver-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/SnowConstitutiveModelTests.cppTests/UnitTests/SnowMPMSolverTests.cppTests/UnitTests/CGTests.cpp
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/Solver/Particle/MPM/SnowMPMSolver.cpp
🧠 Learnings (2)
📚 Learning: 2026-08-08T14:25:24.956Z
Learnt from: utilForever
Repo: utilForever/CubbyFlow PR: 175
File: Includes/Core/Solver/Particle/MPM/SnowMPMSolver-Impl.hpp:217-224
Timestamp: 2026-08-08T14:25:24.956Z
Learning: In CubbyFlow MPM implementations, use the shared MPMSytemData policy of clamping out-of-domain cubic B-spline stencil indices to boundary grid nodes for both P2G and G2P operations. This intentionally aggregates out-of-domain weights at boundaries while preserving total stencil weight. SnowMPMSolver and other MPM solvers must follow this policy; any change requires a coordinated MPM-layer update with matching 2-D and 3-D boundary tests.
Applied to files:
Includes/Core/Solver/Particle/MPM/SnowMPMSolver.hppIncludes/Core/Solver/Particle/MPM/SnowMPMSolver-Impl.hpp
📚 Learning: 2026-08-08T14:24:02.736Z
Learnt from: utilForever
Repo: utilForever/CubbyFlow PR: 175
File: Includes/Core/Solver/Particle/MPM/SnowMPMSolver-Impl.hpp:174-178
Timestamp: 2026-08-08T14:24:02.736Z
Learning: MPM solver implementations should rely on MPMSystemData<N>::ResizeGrid() and its shared ValidateGridParameters() validation rather than duplicating grid-parameter checks. This validation rejects zero or maximum-size resolution components, non-finite or non-positive grid spacing, and non-finite grid origin values.
Applied to files:
Includes/Core/Solver/Particle/MPM/SnowMPMSolver-Impl.hpp
🪛 Cppcheck (2.21.0)
Tests/UnitTests/SnowMPMSolverTests.cpp
[warning] 546-546: The address of variable 'x' might be accessed at non-zero index.
(objectIndex)
[warning] 555-555: The address of variable 'x' might be accessed at non-zero index.
(objectIndex)
[warning] 690-690: The address of variable 'x' might be accessed at non-zero index.
(objectIndex)
[warning] 827-827: The address of variable 'x' might be accessed at non-zero index.
(objectIndex)
[warning] 836-836: The address of variable 'x' might be accessed at non-zero index.
(objectIndex)
🔇 Additional comments (10)
Includes/Core/Math/CG-Impl.hpp (1)
16-17: LGTM!Also applies to: 37-104
Includes/Core/Particle/MPM/SnowConstitutiveModel-Impl.hpp (1)
63-63: LGTM!Also applies to: 102-102, 133-200
Tests/UnitTests/SnowConstitutiveModelTests.cpp (1)
37-82: LGTM!Also applies to: 306-310, 396-400
Includes/Core/Solver/Particle/MPM/SnowMPMSolver.hpp (1)
14-15: LGTM!Also applies to: 66-90, 118-142, 155-161
Includes/Core/Solver/Particle/MPM/SnowMPMSolver-Impl.hpp (2)
141-196: LGTM!Also applies to: 210-210, 271-271, 306-327, 340-340, 454-472, 474-559, 652-757, 854-885
608-622: 🩺 Stability & AvailabilityNo change needed:
Matrix::operator*=performs matrix multiplication.ElemIMulprovides the separate element-wise operation, so the Hessian action is correct.> Likely an incorrect or invalid review comment.Sources/API/Python/Solver/Particle/MPM/SnowMPMSolver.cpp (1)
38-47: LGTM!Tests/PythonTests/test_snow_mpm_solver.py (1)
43-60: LGTM!Tests/UnitTests/SnowMPMSolverTests.cpp (2)
9-9: LGTM!Also applies to: 94-101, 115-176, 200-343, 352-352, 361-361, 385-385, 406-418, 432-439, 450-463, 481-571, 602-602, 618-627, 636-636, 646-674, 686-703, 720-742, 768-774, 814-843
102-114: 🩺 Stability & AvailabilityNo compilation issue in
AddCompressedParticleLattice.ForEachIndexsupports begin/endVector<IndexType, N>bounds, includingVectorUZ<N>.> Likely an incorrect or invalid review comment.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #178 +/- ##
==========================================
+ Coverage 83.62% 83.78% +0.16%
==========================================
Files 418 418
Lines 23855 24152 +297
==========================================
+ Hits 19948 20236 +288
- Misses 3907 3916 +9 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Tests/UnitTests/CGTests.cpp (1)
97-121: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd 3-D counterparts for the new CR cases.
All four tests instantiate
BLAS<double, Vector2D, Matrix2x2D>. The added coverage does not exercise the 3-D instantiation. Add equivalent 3-D cases, or parameterize these tests, for the symmetric, zero-iteration, already-converged, and singular cases.As per coding guidelines,
Tests/UnitTests/**/*.{cpp,hpp}requires tests for both 2-D and 3-D counterparts when behavior applies to both.Also applies to: 123-142, 144-162, 164-183
🤖 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/CGTests.cpp` around lines 97 - 121, Add 3-D coverage matching each CR test case—symmetric, zero-iteration, already-converged, and singular—using the corresponding Vector3D and Matrix3x3D BLAS instantiation. Either duplicate the existing cases or parameterize them, while preserving their current assertions and expected behavior for both dimensions.Source: Coding guidelines
Tests/UnitTests/SnowMPMSolverTests.cpp (1)
318-336: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAssert grid-velocity rollback.
SolveGridVelocitiesrestoresGridVelocities()fromGridVelocitiesBeforeUpdate()when the solve throws. This test snapshots only particle state. The test can pass even if grid rollback is broken. Snapshotdata->GridVelocities()beforeUpdateand compare it afterEXPECT_THROW.🤖 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/SnowMPMSolverTests.cpp` around lines 318 - 336, Extend the rollback test around solver.Update in the existing SnowMPMSolver test by snapshotting data->GridVelocities() before the call and comparing every grid velocity after EXPECT_THROW. Preserve the existing particle-state assertions and verify the post-failure grid velocities match the snapshot.
🧹 Nitpick comments (1)
Tests/UnitTests/SnowMPMSolverTests.cpp (1)
251-260: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCompare the complete particle state in the small-step test.
The loop checks only
Velocities()andDeformationStates().elastic. A mismatch inPositions()orDeformationStates().plasticcan pass. Compare positions and both deformation components before accepting explicit/semi-implicit agreement.🤖 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/SnowMPMSolverTests.cpp` around lines 251 - 260, Extend the particle comparison loop in the small-step test to also compare each particle’s Positions() and DeformationStates().plastic, using the same similarity tolerance as the existing velocity and elastic-state checks. Keep the current NumberOfParticles(), velocity, and elastic comparisons unchanged.
🤖 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.
Outside diff comments:
In `@Tests/UnitTests/CGTests.cpp`:
- Around line 97-121: Add 3-D coverage matching each CR test case—symmetric,
zero-iteration, already-converged, and singular—using the corresponding Vector3D
and Matrix3x3D BLAS instantiation. Either duplicate the existing cases or
parameterize them, while preserving their current assertions and expected
behavior for both dimensions.
In `@Tests/UnitTests/SnowMPMSolverTests.cpp`:
- Around line 318-336: Extend the rollback test around solver.Update in the
existing SnowMPMSolver test by snapshotting data->GridVelocities() before the
call and comparing every grid velocity after EXPECT_THROW. Preserve the existing
particle-state assertions and verify the post-failure grid velocities match the
snapshot.
---
Nitpick comments:
In `@Tests/UnitTests/SnowMPMSolverTests.cpp`:
- Around line 251-260: Extend the particle comparison loop in the small-step
test to also compare each particle’s Positions() and
DeformationStates().plastic, using the same similarity tolerance as the existing
velocity and elastic-state checks. Keep the current NumberOfParticles(),
velocity, and elastic comparisons unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4f457dfc-cd40-49e4-8d3c-527bc79631ef
📒 Files selected for processing (6)
Includes/Core/Math/CG.hppIncludes/Core/Particle/MPM/SnowConstitutiveModel.hppIncludes/Core/Solver/Particle/MPM/SnowMPMSolver-Impl.hppIncludes/Core/Solver/Particle/MPM/SnowMPMSolver.hppTests/UnitTests/CGTests.cppTests/UnitTests/SnowMPMSolverTests.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- Includes/Core/Particle/MPM/SnowConstitutiveModel.hpp
- Includes/Core/Math/CG.hpp
- Includes/Core/Solver/Particle/MPM/SnowMPMSolver.hpp
- Includes/Core/Solver/Particle/MPM/SnowMPMSolver-Impl.hpp
📜 Review details
⏰ Context from checks skipped due to timeout. (16)
- GitHub Check: Codacy Static Code Analysis
- 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-14
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-18
- GitHub Check: 🌞 Static Analysis - SonarCloud (Ubuntu 24.04 + gcc-14, ubuntu-24.04, gcc, 14)
- GitHub Check: 🐧 Build - Ubuntu 24.04 + gcc-13
- GitHub Check: 🪟 Build - Windows Server 2022 + Visual Studio 2022
- GitHub Check: 🍎 Build - macOS 26.3 + Xcode 26.3
- GitHub Check: 🍎 Build - macOS 15.7.4 + Xcode 16.4
- GitHub Check: 🪟 Build - Windows Server 2025 + Visual Studio 2026
- GitHub Check: 🐧 CUDA Build - Ubuntu 24.04 + gcc-12 + CUDA 12.6.3
- GitHub Check: 🪟 CUDA Build - Windows Server 2025 + Visual Studio 2026 + CUDA 13.2.0 (Release)
- GitHub Check: 🧪 Code Coverage - Codecov (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)
🧰 Additional context used
📓 Path-based instructions (3)
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/CGTests.cppTests/UnitTests/SnowMPMSolverTests.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:
Tests/UnitTests/CGTests.cppTests/UnitTests/SnowMPMSolverTests.cpp
**/*
📄 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/UnitTests/CGTests.cppTests/UnitTests/SnowMPMSolverTests.cpp
🪛 Cppcheck (2.21.0)
Tests/UnitTests/SnowMPMSolverTests.cpp
[warning] 681-681: The address of variable 'x' might be accessed at non-zero index.
(objectIndex)
🔇 Additional comments (1)
Tests/UnitTests/SnowMPMSolverTests.cpp (1)
9-9: LGTM!Also applies to: 77-78, 95-177, 201-230, 264-301, 353-353, 362-362, 386-386, 407-419, 433-440, 451-464, 482-543, 549-572, 603-689, 701-718, 735-757, 783-789
|
1 participant
This revision includes:
Summary by CodeRabbit
New Features
Bug Fixes
Tests