[SPH] prepare migration of EOS to solvergraph - #1938
Merged
Conversation
Use merged_xyzh for position-dependent EOS and allocate uint_g as scalar. Co-authored-by: Cursor <cursoragent@cursor.com>
Remove unused RhoGetters, debug logging, and leftover locals/includes. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThe PR adds a runnable Sod shock-tube SPH example with plotting, checkpointing, and GIF generation. It also refactors SPH EOS evaluation from patch-wise getters to solvergraph spans and distributed kernels across multiple EOS variants. ChangesSPH EOS and Sod shock-tube flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SodSimulation
participant ComputeEos
participant DistributedKernel
participant OutputFields
participant SodTube
SodSimulation->>ComputeEos: evaluate EOS during timestepping
ComputeEos->>DistributedKernel: dispatch solvergraph spans
DistributedKernel->>OutputFields: write pressure and soundspeed
SodSimulation->>SodTube: sample analytic Sod solution
SodSimulation->>OutputFields: save numerical and analytic plot frames
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
Thanks @tdavidcl 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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@examples/sph/run_sod.py`:
- Line 110: Replace the periodic boundary configuration in the run_sod setup by
using boundary conditions appropriate for a single-interface Sod shock tube,
matching the non-periodic behavior expected by SodTube and avoiding a seam
between the left and right states.
- Around line 159-166: Guard the GIF generation and display block containing
show_image_sequence, PillowWriter, ani.save, and plt.show with the existing
shamrock.sys.world_rank() == 0 condition, so only rank zero creates and displays
the animation while preserving the current behavior within that rank.
In `@src/shammodels/sph/src/modules/ComputeEos.cpp`:
- Around line 401-423: Update the FA2014 `eos_internal` lambda to handle an
empty sink list before calculating the potential: when `scount == 0`, apply the
same fallback behavior as `SolverEOS_LocallyIsothermalFA2014Extended` and return
without producing zero pressure or soundspeed. Preserve the existing potential
and EOS calculations for non-empty sink lists.
🪄 Autofix (Beta)
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 83d17c98-977c-4074-aaab-4e57fa4ca062
📒 Files selected for processing (4)
examples/sph/run_sod.pysrc/shammodels/sph/include/shammodels/sph/modules/ComputeEos.hppsrc/shammodels/sph/src/modules/ComputeEos.cppsrc/shamphys/src/SodTube.cpp
| cfg.set_artif_viscosity_VaryingCD10( | ||
| alpha_min=0.0, alpha_max=1, sigma_decay=0.1, alpha_u=1, beta_AV=2 | ||
| ) | ||
| cfg.set_boundary_periodic() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not use periodic boundaries for this single-interface Sod comparison.
The periodic seam places the high-pressure left state next to the low-pressure right state, creating a second Riemann problem absent from SodTube. Its waves will invalidate the analytic comparison. Configure boundaries consistent with the single shock-tube solution instead.
🤖 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 `@examples/sph/run_sod.py` at line 110, Replace the periodic boundary
configuration in the run_sod setup by using boundary conditions appropriate for
a single-interface Sod shock tube, matching the non-periodic behavior expected
by SodTube and avoiding a seam between the left and right states.
Comment on lines
+159
to
+166
| glob_str = f"{dump_folder}sod_*.png" | ||
| ani = show_image_sequence(glob_str) | ||
|
|
||
| writer = PillowWriter(fps=15, metadata=dict(artist="Me"), bitrate=1800) | ||
| ani.save("_to_trash/sod.gif", writer=writer) | ||
|
|
||
| if shamrock.sys.world_rank() == 0: | ||
| plt.show() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Restrict GIF generation to rank zero.
Every rank reaches this block and writes _to_trash/sod.gif; concurrent writers can corrupt the artifact or fail. Wrap lines 159-166 in the existing world_rank() == 0 condition.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 162-162: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
🤖 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 `@examples/sph/run_sod.py` around lines 159 - 166, Guard the GIF generation and
display block containing show_image_sequence, PillowWriter, ani.save, and
plt.show with the existing shamrock.sys.world_rank() == 0 condition, so only
rank zero creates and displays the animation while preserving the current
behavior within that rank.
Comment on lines
+401
to
+423
| auto eos_internal = [](Tvec R, | ||
| Tscal rho_a, | ||
| u32 scount, | ||
| auto spos, | ||
| auto smass, | ||
| Tscal G, | ||
| Tscal h_over_r, | ||
| Tscal &pressure, | ||
| Tscal &soundspeed) { | ||
| Tscal mpotential = 0; | ||
| for (u32 i = 0; i < scount; i++) { | ||
| Tvec s_r = spos[i] - R; | ||
| Tscal s_m = smass[i]; | ||
| Tscal s_r_abs = sycl::length(s_r); | ||
| mpotential += G * s_m / s_r_abs; | ||
| } | ||
|
|
||
| P[item] = P_a; | ||
| cs[item] = cs_out; | ||
| }); | ||
| Tscal cs_out = h_over_r * sycl::sqrt(mpotential); | ||
| Tscal P_a = EOS::pressure_from_cs(cs_out * cs_out, rho_a); | ||
|
|
||
| pressure = P_a; | ||
| soundspeed = cs_out; | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
FA2014 branch lacks the zero-sink guard present in FA2014Extended.
Unlike SolverEOS_LocallyIsothermalFA2014Extended (Lines 531-534), this branch has no check for an empty sink list. With sink_cnt == 0 the potential loop leaves mpotential = 0, yielding cs_out = 0 and pressure = 0, which is unphysical and can trigger division-by-zero in downstream soundspeed-dependent computations (e.g. timestep control). Mirror the Extended branch's guard here.
🤖 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 `@src/shammodels/sph/src/modules/ComputeEos.cpp` around lines 401 - 423, Update
the FA2014 `eos_internal` lambda to handle an empty sink list before calculating
the potential: when `scount == 0`, apply the same fallback behavior as
`SolverEOS_LocallyIsothermalFA2014Extended` and return without producing zero
pressure or soundspeed. Preserve the existing potential and EOS calculations for
non-empty sink lists.
Contributor
Workflow reportworkflow report corresponding to commit 7df87d7 Pre-commit check reportPre-commit check: ✅ Test pipeline can run. Clang-tidy diff reportDoxygen diff with
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This was referenced Jul 21, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
No description provided.