Add GSPH unit tests and CI physics validation tests #1499
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks @Guo-astro for opening this PR! You can do multiple things directly here: Once the workflow completes a message will appear displaying informations related to the run. Also the PR gets automatically reviewed by gemini, you can: |
Summary of ChangesHello @Guo-astro, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the testing infrastructure for the GSPH (Godunov Smoothed Particle Hydrodynamics) model. By adding detailed unit tests for fundamental numerical components and integrating physics-based validation tests into the CI pipeline, the changes aim to improve the robustness, reliability, and accuracy of GSPH simulations. This ensures that future modifications maintain the expected physical behavior and numerical stability. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive set of unit tests and CI physics validation tests for the GSPH implementation. The C++ unit tests for forces, integration, and Riemann solvers are very thorough and cover many edge cases. The Python-based Sod shock tube validation tests are also a great addition for ensuring physical correctness.
My feedback focuses on improving the maintainability and efficiency of the new test scripts. Specifically, I've suggested refactoring the duplicated Python test scripts into a single parameterized script, improving the efficiency of data handling in the error calculation, and cleaning up some minor issues like dead code and confusing logic in the C++ tests, with one C++ comment modified to reference a relevant rule on programmatic data construction in tests.
Overall, this is a high-quality contribution that significantly improves the test coverage and robustness of the GSPH code.
tdavidcl
reviewed
Dec 29, 2025
Member
|
we can let the CI run on this one to see the actual value that we should use but the ideal case would be to have here is a snippet from the sod tube in sph (as you can guess the tolerances are very brutal, it should break with the slightest unexpected change to the solver, which is what you want) test_pass = True
expect_rho = 0.00016154918188486815
expect_vx = 0.001162704743480841
expect_vy = 2.988130616021184e-05
expect_vz = 1.7413547093230376e-07
expect_P = 0.00012483646129766217
tol = 1e-11
def float_equal(val1, val2, prec):
return abs(val1 - val2) < prec
err_log = ""
if not float_equal(rho, expect_rho, tol * expect_rho):
err_log += "error on rho is outside of tolerances:\n"
err_log += f" expected error = {expect_rho} +- {tol*expect_rho}\n"
err_log += f" obtained error = {rho} (relative error = {(rho - expect_rho)/expect_rho})\n"
test_pass = False
if not float_equal(vx, expect_vx, tol * expect_vx):
err_log += "error on vx is outside of tolerances:\n"
err_log += f" expected error = {expect_vx} +- {tol*expect_vx}\n"
err_log += f" obtained error = {vx} (relative error = {(vx - expect_vx)/expect_vx})\n"
test_pass = False
if not float_equal(vy, expect_vy, tol * expect_vy):
err_log += "error on vy is outside of tolerances:\n"
err_log += f" expected error = {expect_vy} +- {tol*expect_vy}\n"
err_log += f" obtained error = {vy} (relative error = {(vy - expect_vy)/expect_vy})\n"
test_pass = False
if not float_equal(vz, expect_vz, tol * expect_vz):
err_log += "error on vz is outside of tolerances:\n"
err_log += f" expected error = {expect_vz} +- {tol*expect_vz}\n"
err_log += f" obtained error = {vz} (relative error = {(vz - expect_vz)/expect_vz})\n"
test_pass = False
if not float_equal(P, expect_P, tol * expect_P):
err_log += "error on P is outside of tolerances:\n"
err_log += f" expected error = {expect_P} +- {tol*expect_P}\n"
err_log += f" obtained error = {P} (relative error = {(P - expect_P)/expect_P})\n"
test_pass = False
if test_pass == False:
exit("Test did not pass L2 margins : \n" + err_log) |
Member
|
you have a fail in the unittests |
tdavidcl
requested changes
Dec 30, 2025
Member
tdavidcl
left a comment
tdavidcl
left a comment
There was a problem hiding this comment.
I'm fine with most of the PR, although i'm not a fan of introducing the Gaussian kernel as is. I think that it will just be confusing as by definition de gaussian kernel should ahve an infinite support. So i would be in favor of its removal for now in this PR until we find proper semantics for it (and it would be a latter PR in that case anyway).
Contributor
Author
|
Fair enough! |
Contributor
Author
Thank you for the feedback. I've removed the Gaussian kernel from this PR as requested. For this PR, I've switched the GSPH default to the M8 (quintic spline) kernel instead. |
Contributor
Author
|
|
Contributor
Author
|
|
tdavidcl
requested changes
Dec 31, 2025
Member
tdavidcl
left a comment
tdavidcl
left a comment
There was a problem hiding this comment.
probably final review before merge:
The change in the solvers are goods and most changes concerns exemple which blocks this PR for no reason. So it sounds like splitting all changes related to exemple (not the CI ones, they belongs in this one) is a good thing. Also there the blame is messed up due to intermediate commits. So here is what I would recommend:
- git reset --soft then force push the change to squash the history of this pr
- except from the change to ci tests remove all exemples change from this PR we can commit them in separate PR latter to not block this one.
Guo-astro
force-pushed
the
gsph-ci-tests
branch
from
2630743 to
00df468
Compare
Member
|
ok perfect I'm just waiting for the CI now |
tdavidcl
requested changes
Dec 31, 2025
Member
tdavidcl
left a comment
tdavidcl
left a comment
There was a problem hiding this comment.
to bypass the ci not providing pyvista (also we can fetch the data from the code without dumping)
- Add unit tests for GSPH modules: Riemann solvers, forces, integration - Add CI physics tests for Sod shock tube and extreme blast wave - Use ctx.collect_data() for direct memory access (no pyvista dependency) - Use strict tolerances (1e-8) for regression testing - Fix M4 kernel normalization constants - Remove non-existent MUSCL test from CI workflow
Guo-astro
force-pushed
the
gsph-ci-tests
branch
from
612af13 to
97a965e
Compare
tdavidcl
reviewed
Dec 31, 2025
tdavidcl
reviewed
Dec 31, 2025
tdavidcl
reviewed
Dec 31, 2025
Workflow reportworkflow report corresponding to commit eb9b17c Pre-commit check reportPre-commit check: ✅ Test pipeline can run. Clang-tidy diff reportDoxygen diff with
|
tdavidcl
approved these changes
Jan 1, 2026
src/shammodels/sph/src/modules/IterateSmoothingLengthDensity.cpp
Outdated
Show resolved
Hide resolved
DavidFang03
pushed a commit
to DavidFang03/Shamrock
that referenced
this pull request
Jan 6, 2026
## Summary - Add BDD-style unit tests for GSPH components: - GSPHRiemannTests: HLLC and iterative Riemann solvers - GSPHForceTests: Force calculations and energy conservation - GSPHIntegrationTests: Time integration schemes - Add CI physics validation tests for GSPH: - sod_tube_gsph.py: Sod shock tube with piecewise constant reconstruction - blast_wave_gsph.py: Extreme blast wave test (Inutsuka 2002) - Both compare against analytical solution with L2 error thresholds - Update CI workflow to run GSPH tests with 1 and 2 MPI ranks - Use M8 (quintic spline) kernel as default for GSPH ## Test plan - [x] Unit tests pass locally - [x] CI physics tests pass with expected L2 errors - [ ] Tests run correctly with both 1 and 2 MPI ranks --------- Co-authored-by: David--Cléris Timothée <timothee.davidcleris@proton.me>
Guo-astro
added a commit
to Guo-astro/shamrock
that referenced
this pull request
Jan 9, 2026
## Summary - Add BDD-style unit tests for GSPH components: - GSPHRiemannTests: HLLC and iterative Riemann solvers - GSPHForceTests: Force calculations and energy conservation - GSPHIntegrationTests: Time integration schemes - Add CI physics validation tests for GSPH: - sod_tube_gsph.py: Sod shock tube with piecewise constant reconstruction - blast_wave_gsph.py: Extreme blast wave test (Inutsuka 2002) - Both compare against analytical solution with L2 error thresholds - Update CI workflow to run GSPH tests with 1 and 2 MPI ranks - Use M8 (quintic spline) kernel as default for GSPH ## Test plan - [x] Unit tests pass locally - [x] CI physics tests pass with expected L2 errors - [ ] Tests run correctly with both 1 and 2 MPI ranks --------- Co-authored-by: David--Cléris Timothée <timothee.davidcleris@proton.me>
Guo-astro
added a commit
to Guo-astro/shamrock
that referenced
this pull request
Jan 9, 2026
## Summary - Add BDD-style unit tests for GSPH components: - GSPHRiemannTests: HLLC and iterative Riemann solvers - GSPHForceTests: Force calculations and energy conservation - GSPHIntegrationTests: Time integration schemes - Add CI physics validation tests for GSPH: - sod_tube_gsph.py: Sod shock tube with piecewise constant reconstruction - blast_wave_gsph.py: Extreme blast wave test (Inutsuka 2002) - Both compare against analytical solution with L2 error thresholds - Update CI workflow to run GSPH tests with 1 and 2 MPI ranks - Use M8 (quintic spline) kernel as default for GSPH ## Test plan - [x] Unit tests pass locally - [x] CI physics tests pass with expected L2 errors - [ ] Tests run correctly with both 1 and 2 MPI ranks --------- Co-authored-by: David--Cléris Timothée <timothee.davidcleris@proton.me>
Guo-astro
added a commit
to Guo-astro/shamrock
that referenced
this pull request
Jan 9, 2026
## Summary - Add BDD-style unit tests for GSPH components: - GSPHRiemannTests: HLLC and iterative Riemann solvers - GSPHForceTests: Force calculations and energy conservation - GSPHIntegrationTests: Time integration schemes - Add CI physics validation tests for GSPH: - sod_tube_gsph.py: Sod shock tube with piecewise constant reconstruction - blast_wave_gsph.py: Extreme blast wave test (Inutsuka 2002) - Both compare against analytical solution with L2 error thresholds - Update CI workflow to run GSPH tests with 1 and 2 MPI ranks - Use M8 (quintic spline) kernel as default for GSPH ## Test plan - [x] Unit tests pass locally - [x] CI physics tests pass with expected L2 errors - [ ] Tests run correctly with both 1 and 2 MPI ranks --------- Co-authored-by: David--Cléris Timothée <timothee.davidcleris@proton.me>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Add BDD-style unit tests for GSPH components:
Add CI physics validation tests for GSPH:
Update CI workflow to run GSPH tests with 1 and 2 MPI ranks
Use M8 (quintic spline) kernel as default for GSPH
Test plan