Implement the elastoplastic snow constitutive model - #171
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:
📝 WalkthroughWalkthroughChangesThe pull request adds a 2-D and 3-D elastoplastic snow constitutive model. It tracks elastic and plastic deformation, projects singular values, computes hardened Kirchhoff stress, validates inputs, and adds unit tests. Snow constitutive model
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SnowConstitutiveModel
participant SVDUtilities
participant SnowDeformationState
SnowConstitutiveModel->>SnowDeformationState: Read elastic and plastic deformation
SnowConstitutiveModel->>SVDUtilities: Decompose trial elastic deformation
SVDUtilities-->>SnowConstitutiveModel: Return singular values and rotation
SnowConstitutiveModel->>SnowDeformationState: Store projected elastic and plastic deformation
SnowConstitutiveModel->>SVDUtilities: Decompose elastic deformation for stress
SVDUtilities-->>SnowConstitutiveModel: Return elastic rotation
SnowConstitutiveModel-->>SnowDeformationState: Return Kirchhoff stress
Possibly related issues
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 | 43 |
| Duplication | 0 |
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.
🧹 Nitpick comments (5)
Includes/Core/Particle/MPM/SnowConstitutiveModel-Impl.hpp (2)
35-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant finiteness term.
Line 39 already rejects a non-finite
criticalStretch. Therefore1.0 + criticalStretchis always finite when line 40 runs. The term never changes the result.♻️ Proposed cleanup
!std::isfinite(criticalStretch) || criticalStretch < 0.0 || - !std::isfinite(1.0 + criticalStretch) || !std::isfinite(hardeningCoefficient) || hardeningCoefficient < 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/Particle/MPM/SnowConstitutiveModel-Impl.hpp` around lines 35 - 45, Remove the redundant std::isfinite(1.0 + criticalStretch) condition from the parameter validation in the Snow constitutive model, keeping the existing std::isfinite(criticalStretch) check and all other validation rules unchanged.
28-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNote the ordering of the Lamé computation and the parameter check.
The member initializer list divides by
(1.0 - 2.0 * poissonRatio)before the body validatespoissonRatio. ForpoissonRatio == 0.5the division produces infinity, and the body then throws because!std::isfinite(m_lambda0). The behavior is correct under IEEE-754. The division still raisesFE_DIVBYZEROand depends on non-finite arithmetic being preserved, so it breaks under-ffast-math.If you prefer an explicit order, validate the raw parameters in a static helper and call it from the initializer list before the Lamé terms.
🤖 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-Impl.hpp` around lines 28 - 33, Validate the raw poissonRatio and other constructor parameters before computing Lamé terms by introducing a static validation helper and invoking it from the initializer list before m_mu0 and m_lambda0. Ensure invalid values such as poissonRatio == 0.5 are rejected without performing the division, while preserving the existing validation behavior for valid inputs.Tests/UnitTests/SnowConstitutiveModelTests.cpp (2)
280-290: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd the 3-D counterpart for parameter validation.
Every other test in this file runs for both dimensions. This test exercises
SnowConstitutiveModel2only. The constructor validation lives in the class template, so a 3-D instantiation is a separate specialization that no test currently covers. Add the matchingSnowConstitutiveModel3assertions.💚 Proposed addition
EXPECT_THROW((SnowConstitutiveModel2{ 1.0, 0.2, 0.1, 0.1, -1.0 }), std::invalid_argument); + + EXPECT_THROW(SnowConstitutiveModel3{ 0.0 }, std::invalid_argument); + EXPECT_THROW((SnowConstitutiveModel3{ 1.0, 0.5 }), std::invalid_argument); + EXPECT_THROW((SnowConstitutiveModel3{ 1.0, 0.2, 1.0 }), + std::invalid_argument); + EXPECT_THROW((SnowConstitutiveModel3{ 1.0, 0.2, 0.1, -0.1 }), + std::invalid_argument); + EXPECT_THROW((SnowConstitutiveModel3{ 1.0, 0.2, 0.1, 0.1, -1.0 }), + std::invalid_argument); }As per coding guidelines: "Keep supported 2-D and 3-D behavior aligned; update dimensional counterparts, aliases, explicit instantiations, and tests when behavior applies to both dimensions".
🤖 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/SnowConstitutiveModelTests.cpp` around lines 280 - 290, Extend the InvalidParameters test to cover SnowConstitutiveModel3 with the same invalid constructor argument cases currently asserted for SnowConstitutiveModel2. Keep the existing 2-D assertions unchanged and ensure both template specializations validate identical parameter constraints.Source: Coding guidelines
205-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the macro body and identify the failing dimension.
Two issues exist in this macro.
First, the body is two statements without a
do { } while (0)wrapper. At the current call sites the macro is a standalone statement, so it compiles. If a later caller writesif (cond) EXPECT_FOR_2D_AND_3D(f);, only the 2-D call stays inside the branch.Second, each scenario helper runs for both dimensions inside one
TEST. When anEXPECT_*fails, the output does not state whether<2>or<3>failed. AddSCOPED_TRACEto report the dimension.The name also starts with
EXPECT_but the macro asserts nothing by itself. ConsiderRUN_FOR_2D_AND_3D.♻️ Proposed refactor
-#define EXPECT_FOR_2D_AND_3D(function) \ - function<2>(); \ - function<3>() +#define RUN_FOR_2D_AND_3D(function) \ + do \ + { \ + { \ + SCOPED_TRACE("N = 2"); \ + function<2>(); \ + } \ + { \ + SCOPED_TRACE("N = 3"); \ + function<3>(); \ + } \ + } while (0)Update each call site to use
RUN_FOR_2D_AND_3D.As per path instructions: "Use GoogleTest/GMock macros such as
TEST,EXPECT_*, andASSERT_*, and prefer one focused regression scenario over broad fixtures".🤖 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/SnowConstitutiveModelTests.cpp` around lines 205 - 207, Rename EXPECT_FOR_2D_AND_3D to RUN_FOR_2D_AND_3D and update every call site; wrap its two invocations in do-while(0) so it behaves as one statement, and add dimension-specific SCOPED_TRACE messages before running the 2-D and 3-D helpers.Source: Path instructions
Includes/Core/Particle/MPM/SnowConstitutiveModel.hpp (1)
82-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClose the Doxygen block.
Every other documentation block in this file ends with a
//!line. This block does not. Add the closing line for consistency.📝 Proposed fix
//! \return Fixed-corotated Kirchhoff stress. + //! [[nodiscard]] MatrixType ComputeKirchhoffStress(const State& state) const;As per coding guidelines: "Keep public C++ declarations, Doxygen comments, and public contracts 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 82 - 91, Complete the Doxygen block immediately preceding ComputeKirchhoffStress by adding the standard closing //! line, matching the documentation style used throughout the file.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.
Nitpick comments:
In `@Includes/Core/Particle/MPM/SnowConstitutiveModel-Impl.hpp`:
- Around line 35-45: Remove the redundant std::isfinite(1.0 + criticalStretch)
condition from the parameter validation in the Snow constitutive model, keeping
the existing std::isfinite(criticalStretch) check and all other validation rules
unchanged.
- Around line 28-33: Validate the raw poissonRatio and other constructor
parameters before computing Lamé terms by introducing a static validation helper
and invoking it from the initializer list before m_mu0 and m_lambda0. Ensure
invalid values such as poissonRatio == 0.5 are rejected without performing the
division, while preserving the existing validation behavior for valid inputs.
In `@Includes/Core/Particle/MPM/SnowConstitutiveModel.hpp`:
- Around line 82-91: Complete the Doxygen block immediately preceding
ComputeKirchhoffStress by adding the standard closing //! line, matching the
documentation style used throughout the file.
In `@Tests/UnitTests/SnowConstitutiveModelTests.cpp`:
- Around line 280-290: Extend the InvalidParameters test to cover
SnowConstitutiveModel3 with the same invalid constructor argument cases
currently asserted for SnowConstitutiveModel2. Keep the existing 2-D assertions
unchanged and ensure both template specializations validate identical parameter
constraints.
- Around line 205-207: Rename EXPECT_FOR_2D_AND_3D to RUN_FOR_2D_AND_3D and
update every call site; wrap its two invocations in do-while(0) so it behaves as
one statement, and add dimension-specific SCOPED_TRACE messages before running
the 2-D and 3-D helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06542a49-3f64-4355-935c-7d5cbd43e10e
📒 Files selected for processing (3)
Includes/Core/Particle/MPM/SnowConstitutiveModel-Impl.hppIncludes/Core/Particle/MPM/SnowConstitutiveModel.hppTests/UnitTests/SnowConstitutiveModelTests.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (16)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: 🪟 Build - Windows Server 2025 + Visual Studio 2026
- GitHub Check: 🪟 Build - Windows Server 2022 + Visual Studio 2022
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-17
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-18
- GitHub Check: 🐧 Build - Ubuntu 24.04 + gcc-13
- 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: 🧪 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: 🌞 Static Analysis - SonarCloud (Ubuntu 24.04 + gcc-14, ubuntu-24.04, gcc, 14)
- GitHub Check: 🐧 CUDA Build - Ubuntu 24.04 + gcc-12 + CUDA 12.6.3
- GitHub Check: 🪟 CUDA Build - Windows Server 2022 + Visual Studio 2022 + CUDA 12.6.3 (Release)
- GitHub Check: 🍎 Build - macOS 15.7.4 + Xcode 16.4
- GitHub Check: 🍎 Build - macOS 26.3 + Xcode 26.3
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cpp,cu,hpp,h}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{cpp,cu,hpp,h}: Keep supported 2-D and 3-D behavior aligned; update dimensional counterparts, aliases, explicit instantiations, and tests when behavior applies to both dimensions.
Put dimension-independent logic in shared templates and preserveFoo2/Foo3and pointer aliases exposed by public types.
Add explicit template instantiations for both supported dimensions when extending a dimensional template.
Preserve C++17 portability across GCC, Clang, and MSVC on Linux, macOS, and Windows; avoid compiler extensions unless isolated behind existing CMake checks.
Treat warnings as errors and fix project warnings instead of globally suppressing them.
Use project includes, keep code in theCubbyFlownamespace, follow existing-Impl.hppconventions for visible template definitions, and reuse nearby ownership aliases and builder APIs.
Use existing repository patterns and abstractions before adding new code, abstractions, or dependencies.
Use the existing parallel helpers and keep behavior correct across TBB, OpenMP, HPX, CPP11Thread, and Serial backends.
Format touched C++ and CUDA files with.clang-format; follow four-space indentation, an 80-column limit, sorted includes, and project brace style.
Files:
Tests/UnitTests/SnowConstitutiveModelTests.cppIncludes/Core/Particle/MPM/SnowConstitutiveModel.hppIncludes/Core/Particle/MPM/SnowConstitutiveModel-Impl.hpp
Tests/UnitTests/**/*.{cpp,hpp,h}
📄 CodeRabbit inference engine (AGENTS.md)
Use GoogleTest/GMock macros such as
TEST,EXPECT_*, andASSERT_*, and prefer one focused regression scenario over broad fixtures or new test frameworks.
Files:
Tests/UnitTests/SnowConstitutiveModelTests.cpp
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Do not commit build output, test logs, caches, IDE state, or unrelated local changes.
Keep each commit focused on one logical change and use the conventional prefixesfeat:,fix:,refactor:,test:,docs:,build:,ci:, orchore:.
Files:
Tests/UnitTests/SnowConstitutiveModelTests.cppIncludes/Core/Particle/MPM/SnowConstitutiveModel.hppIncludes/Core/Particle/MPM/SnowConstitutiveModel-Impl.hpp
Includes/Core/**/*.{hpp,h}
📄 CodeRabbit inference engine (AGENTS.md)
Keep public C++ declarations, Doxygen comments, and public contracts under
Includes/Core/; place non-inline implementations in the matchingSources/Core/domain.
Files:
Includes/Core/Particle/MPM/SnowConstitutiveModel.hppIncludes/Core/Particle/MPM/SnowConstitutiveModel-Impl.hpp
🔇 Additional comments (10)
Includes/Core/Particle/MPM/SnowConstitutiveModel.hpp (2)
1-90: LGTM!
92-113: LGTM!Includes/Core/Particle/MPM/SnowConstitutiveModel-Impl.hpp (4)
46-62: LGTM!
86-123: LGTM!
124-144: LGTM!
63-85: 🗄️ Data Integrity & IntegrationNo API issue found. The fixed-size SVD overload exists and normalizes negative singular values.
MakeScaleMatrix,Transposed(), andInverse()support the fixed-size matrix types.> Likely an incorrect or invalid review comment.Tests/UnitTests/SnowConstitutiveModelTests.cpp (4)
1-105: LGTM!
106-203: LGTM!
209-278: LGTM!
1-8: 📐 Maintainability & Code QualityThe
Tests/UnitTests/CMakeLists.txtglob includes all.cppfiles inTests/UnitTests, includingSnowConstitutiveModelTests.cpp. No build-manifest change is required.> Likely an incorrect or invalid review comment.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #171 +/- ##
==========================================
+ Coverage 82.39% 82.46% +0.06%
==========================================
Files 413 414 +1
Lines 23488 23570 +82
==========================================
+ Hits 19354 19436 +82
Misses 4134 4134 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Tests/UnitTests/SnowConstitutiveModelTests.cpp (1)
209-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd non-finite material-parameter cases.
This helper only tests finite boundary failures. It does not execute the constructor
std::isfinitechecks for any material parameter. AddNaNand infinity inputs so both dimensions verify the finite-parameter contract.🤖 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/SnowConstitutiveModelTests.cpp` around lines 209 - 216, Add NaN and positive/negative infinity material-parameter cases to the existing SnowConstitutiveModel<N> invalid-argument test helper, covering each constructor parameter dimension and ensuring the constructor’s std::isfinite validation is exercised while preserving the current finite boundary-failure cases.
🤖 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.
Nitpick comments:
In `@Tests/UnitTests/SnowConstitutiveModelTests.cpp`:
- Around line 209-216: Add NaN and positive/negative infinity material-parameter
cases to the existing SnowConstitutiveModel<N> invalid-argument test helper,
covering each constructor parameter dimension and ensuring the constructor’s
std::isfinite validation is exercised while preserving the current finite
boundary-failure cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7216775-2563-4b5e-b864-f486e4bf4e4b
📒 Files selected for processing (2)
Includes/Core/Particle/MPM/SnowConstitutiveModel-Impl.hppTests/UnitTests/SnowConstitutiveModelTests.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (16)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: 🌞 Static Analysis - SonarCloud (Ubuntu 24.04 + gcc-14, ubuntu-24.04, gcc, 14)
- GitHub Check: 🪟 Build - Windows Server 2025 + Visual Studio 2026
- GitHub Check: 🪟 Build - Windows Server 2022 + Visual Studio 2022
- 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: 🧪 Code Coverage - Codecov (Ubuntu 24.04 + gcc-14, ubuntu-24.04, gcc, 14)
- GitHub Check: 🐧 Build - Ubuntu 24.04 + gcc-13
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-18
- 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-16
- GitHub Check: 🍎 Build - macOS 26.3 + Xcode 26.3
- GitHub Check: 🐧 Build - Ubuntu 24.04 + gcc-12
- GitHub Check: 🐧 Build - Ubuntu 24.04 + clang-17
- GitHub Check: 🍎 Build - macOS 15.7.4 + Xcode 16.4
🧰 Additional context used
📓 Path-based instructions (4)
Includes/Core/**/*.{hpp,h}
📄 CodeRabbit inference engine (AGENTS.md)
Keep public C++ declarations, Doxygen comments, and public contracts under
Includes/Core/; place non-inline implementations in the matchingSources/Core/domain.
Files:
Includes/Core/Particle/MPM/SnowConstitutiveModel-Impl.hpp
**/*.{cpp,cu,hpp,h}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{cpp,cu,hpp,h}: Keep supported 2-D and 3-D behavior aligned; update dimensional counterparts, aliases, explicit instantiations, and tests when behavior applies to both dimensions.
Put dimension-independent logic in shared templates and preserveFoo2/Foo3and pointer aliases exposed by public types.
Add explicit template instantiations for both supported dimensions when extending a dimensional template.
Preserve C++17 portability across GCC, Clang, and MSVC on Linux, macOS, and Windows; avoid compiler extensions unless isolated behind existing CMake checks.
Treat warnings as errors and fix project warnings instead of globally suppressing them.
Use project includes, keep code in theCubbyFlownamespace, follow existing-Impl.hppconventions for visible template definitions, and reuse nearby ownership aliases and builder APIs.
Use existing repository patterns and abstractions before adding new code, abstractions, or dependencies.
Use the existing parallel helpers and keep behavior correct across TBB, OpenMP, HPX, CPP11Thread, and Serial backends.
Format touched C++ and CUDA files with.clang-format; follow four-space indentation, an 80-column limit, sorted includes, and project brace style.
Files:
Includes/Core/Particle/MPM/SnowConstitutiveModel-Impl.hppTests/UnitTests/SnowConstitutiveModelTests.cpp
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Do not commit build output, test logs, caches, IDE state, or unrelated local changes.
Keep each commit focused on one logical change and use the conventional prefixesfeat:,fix:,refactor:,test:,docs:,build:,ci:, orchore:.
Files:
Includes/Core/Particle/MPM/SnowConstitutiveModel-Impl.hppTests/UnitTests/SnowConstitutiveModelTests.cpp
Tests/UnitTests/**/*.{cpp,hpp,h}
📄 CodeRabbit inference engine (AGENTS.md)
Use GoogleTest/GMock macros such as
TEST,EXPECT_*, andASSERT_*, and prefer one focused regression scenario over broad fixtures or new test frameworks.
Files:
Tests/UnitTests/SnowConstitutiveModelTests.cpp
🔇 Additional comments (3)
Includes/Core/Particle/MPM/SnowConstitutiveModel-Impl.hpp (2)
17-17: LGTM!Also applies to: 29-46
48-49: 🎯 Functional CorrectnessRetain the C++20 ranges algorithms.
The project configures
CXX_STANDARD 23, sostd::ranges::all_ofdoes not prevent the configured build from compiling.> Likely an incorrect or invalid review comment.Tests/UnitTests/SnowConstitutiveModelTests.cpp (1)
29-30: LGTM!Also applies to: 72-73, 219-245, 248-298, 319-339
f98991d to
459903e
Compare
|
1 participant
This revision includes:
Summary by CodeRabbit
New Features
Tests