[GSPH] Add GSPH solver and VTK I/O #1453
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
3 tasks
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 expands the simulation capabilities by integrating a new Godunov Smoothed Particle Hydrodynamics (GSPH) solver. This solver offers a more advanced approach to fluid dynamics by employing Riemann solvers for inter-particle forces and includes robust features for time integration, boundary handling, and adaptive particle properties. Additionally, it introduces a VTK output module, enhancing the ability to visualize and analyze simulation results. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 PR introduces a significant new feature: the GSPH solver. The implementation is comprehensive, covering time integration, boundary conditions, neighbor search, and I/O. The code is well-structured into modules for configuration, storage, and computation, which is great for maintainability. The use of modern C++ features like std::variant for configuration is commendable.
My review focuses on a few key areas for improvement:
- Code Duplication: There are duplicated implementations of the Riemann solvers, which should be consolidated.
- Hardcoded Values: Some parts of the code have hardcoded values (like dimensionality or magic numbers) that should be generalized or made into named constants.
- Efficiency: Some operations involving particle data manipulation could be optimized.
- Clarity: Some parts of the implementation, particularly in the time integration and density calculation, could be clarified to improve readability and maintainability.
Overall, this is a solid contribution. Addressing these points will make the new solver more robust and easier to maintain.
Guo-astro
force-pushed
the
gsph-solver-io
branch
from
9e6f945 to
ba55c33
Compare
Guo-astro
force-pushed
the
gsph-solver-io
branch
2 times, most recently
from
eeb24cb to
ff836c8
Compare
Contributor
Author
|
Rebased onto main. Previous Gemini comments are outdated - they referred to files from PRs #1450, #1451, #1452 which are now merged into main. This PR now only contains:
The rebase also fixed:
|
Guo-astro
force-pushed
the
gsph-solver-io
branch
2 times, most recently
from
3506a11 to
b2268d4
Compare
tdavidcl
requested changes
Dec 27, 2025
Member
tdavidcl
left a comment
tdavidcl
left a comment
There was a problem hiding this comment.
Hi,
I'm fine with most of this PR, there is only a few changes that i would like to address before merging:
- In the exemples folder most things are scattered accross multiple files. But since there no pure python package for shamrock i prefer having scripts that are standalone for the time being (for example using the sedov.py directly in the script running the sedov test), also then the
__init__.pycan be removed. - The sedov.py script is nice, we already have
src/shamphys/include/shamphys/SedovTaylor.hppthat does this but where the time dependance is missing (I'm only using a reference dataset). Could you implement the logic of sedov.py in it ? - Since the use of VTK require moving it to shammodels/common this would be nice as a separate PR.
Contributor
Author
|
Changes made:
|
Member
Oh sorry maybe I wasn't clear. I meant that the PR is fine but that i would like to merge the VTK change first. You can keep this one as is but make a sub-pr for the vtk that i would merge first. |
2 tasks
Contributor
Author
tdavidcl
added a commit
that referenced
this pull request
Dec 27, 2025
This enables other models (e.g., GSPH) to reuse the same VTK output code. ## Context This is a prerequisite PR for #1453 (GSPH solver). The maintainer requested that the VTK refactoring be merged as a separate PR first. --------- Co-authored-by: David--Cléris Timothée <timothee.davidcleris@proton.me>
Member
|
i merged #1490 |
Contributor
Author
Adds main solver class and VTK output for GSPH: Solver.hpp/cpp: - Main GSPH solver orchestrating the simulation - Predictor-corrector time integration (leapfrog) - Ghost particle exchange via SPH's BasicSPHGhostHandler - Neighbor search using compressed BVH trees - Timestep calculation based on CFL and signal velocity modules/io/VTKDump.hpp/cpp: - VTK Legacy format output for visualization - Outputs positions, velocity, density, pressure, internal energy - Compatible with ParaView SPH code reuse (no duplication): - IterateSmoothingLengthDensity: uses SPH's module for h-iteration - LoopSmoothingLengthIter: uses SPH's outer loop - Ghost handling: BasicSPHGhostHandler for all BC types - Forces: sph_pressure_symetric() with Riemann solver pressure - Density utilities: rho_h(), h_rho(), newtown_iterate_new_h() - Safe division: inv_sat_zero() for numerical stability Performance improvement: - Density/omega computed ONCE after h converges (not on every iteration) - Previous approach recomputed density/omega 10+ times wastefully Fixes compatibility with main: - Use get_eos_gamma() instead of gamma member variable - Stub checkpoint methods (GSPHCheckpoint to be added separately)
- Add Model.hpp/cpp with high-level GSPH interface - Add pyGSPHModel.cpp with pybind11 bindings for Python - Use shared ShamrockDump mechanism for checkpoints (same as SPH) - Remove duplicate checkpoint methods from Solver (reuse SPH pattern) - Add example scripts for Sod shock tube and Sedov blast - Add analytical Riemann solver utilities for validation
Python scripts: - Delete duplicate run_gsph_sod_tube.py (same as gsph_sod_shock_tube.py) - Update animate_sod_tube.py to use shamrock.phys.SodTube instead of custom riemann.py - Replace riemann.py with sedov.py (keep only SedovAnalytical, use shamphys.SodTube for Sod) C++ VTK dump utilities: - Create shared VTKDumpUtils.hpp in shammodels/common/io/ - Refactor both SPH and GSPH VTKDump.cpp to use shared utilities - Eliminates 5 duplicated helper functions (start_dump, vtk_dump_add_patch_id, etc.) Net reduction: ~528 lines of duplicate code removed
- Move animation scripts from gsph/scripts/ to common/visualization/
- Make scripts generic for both SPH and GSPH solvers
- Fix broken import in gen_gsph_gif.py (now animate_sod_vtk.py)
- Use shamrock.phys.SodTube instead of deleted riemann.py
New structure:
common/visualization/
animate_sod_vtk.py - VTK-based Sod tube animation
animate_sod_csv.py - CSV-based Sod tube animation
animate_sedov_csv.py - CSV-based Sedov blast animation
All scripts now accept --solver SPH|GSPH argument for generic use.
Renamed functions to use intent-based naming: - set_value_in_a_box -> set_field_in_box - set_value_in_sphere -> set_field_in_sphere - set_field_value_lambda -> apply_field_from_position Added comprehensive docstrings with usage examples. Updated Python bindings and example script.
- Remove __init__.py files from exemples/common/ (standalone scripts preferred) - Embed SedovAnalytical class directly in animate_sedov_csv.py - Delete exemples/common/analytical/ directory with sedov.py module - Revert SPH VTKDump.cpp to original implementation (move refactoring to separate PR) - Remove VTKDumpUtils.hpp shared utility header (move refactoring to separate PR) - Rewrite GSPH VTKDump.cpp with standalone helper functions in anonymous namespace
Add SedovTaylorAnalytical class with: - Configurable parameters: gamma, E_blast, rho_0, ndim - shock_radius(t): compute shock radius at time t - shock_velocity(t): compute shock velocity at time t - post_shock_density(): compute post-shock density - get_value(r, t): compute density, velocity, pressure at (r, t) - solution_at_time(): compute full radial profile at time t Uses tabulated Sedov constants (xi_0) for common gamma/dimension combinations and self-similar profile approximations. References: - Sedov (1959) "Similarity and Dimensional Methods in Mechanics" - Taylor (1950) "The Formation of a Blast Wave"
Guo-astro
force-pushed
the
gsph-solver-io
branch
from
134517c to
02c2389
Compare
- Fix missing include: shamalgs/memory/memory.hpp -> shamalgs/memory.hpp - Add xyzh_ghost_layout to SolverStorage for BasicSPHGhostHandler - Update GhostHandle constructor calls with 4th argument - Rename set_reconstruct_first_order -> set_reconstruct_piecewise_constant - Update Python bindings to match SolverConfig API
Guo-astro
force-pushed
the
gsph-solver-io
branch
from
7dc2edf to
5047d55
Compare
Contributor
Author
|
Fixed the check error |
Workflow reportworkflow report corresponding to commit 4c0116d Pre-commit check reportPre-commit check: ✅ Test pipeline can run. Clang-tidy diff reportDoxygen diff with
|
tdavidcl
approved these changes
Dec 28, 2025
Member
tdavidcl
left a comment
tdavidcl
left a comment
There was a problem hiding this comment.
LGTM
Also the exemples folder will be removed in the future in favor of a symlink to doc/sphinx/examples, so it would be nice if the one you are adding ends up there in the future. It will allow their systematic testing and they will appear in the doc https://shamrock-code.github.io/Shamrock/sphinx/_as_gen/index.html
Guo-astro
added a commit
to Guo-astro/shamrock
that referenced
this pull request
Dec 29, 2025
Add pyGSPHModel.cpp to CMakeLists.txt Sources list. The file was added in PR Shamrock-code#1453 but was missing from the build configuration, causing shamrock.get_Model_GSPH() to not be available at runtime.
DavidFang03
pushed a commit
to DavidFang03/Shamrock
that referenced
this pull request
Jan 6, 2026
This enables other models (e.g., GSPH) to reuse the same VTK output code. ## Context This is a prerequisite PR for Shamrock-code#1453 (GSPH solver). The maintainer requested that the VTK refactoring be merged as a separate PR first. --------- Co-authored-by: David--Cléris Timothée <timothee.davidcleris@proton.me>
DavidFang03
pushed a commit
to DavidFang03/Shamrock
that referenced
this pull request
Jan 6, 2026
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
This PR adds the main solver class and VTK output for GSPH.
Depends on: #1452 (Physics modules)
Files added
Solver.hpp/cpp:
modules/io/VTKDump.hpp/cpp:
Key features
CMakeLists.txt updates
Test plan