[Solvergraph] Optional edges and use it in SPH - EOS - #1937
Conversation
|
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: |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughAdds nullable solver-graph edges, distributed span-based EOS evaluation, updates the standard Sod example for L2 reporting, and adds a dusty TVA Sod-tube validation simulation with analysis, checkpointing, visualization, and test-result output. ChangesSPH Sod simulations and EOS support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ComputeEos
participant SolverGraph
participant DistributedKernel
participant EOSModel
ComputeEos->>SolverGraph: provide hfactd, pmass, and optional spans
SolverGraph->>DistributedKernel: dispatch EOS computation
DistributedKernel->>EOSModel: calculate pressure and sound speed
EOSModel-->>ComputeEos: return EOS output spans
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/shammodels/sph/src/modules/ComputeEos.cpp`:
- Around line 71-78: Update the validation condition using has_rho and has_h to
reject configurations where both density sources are present or neither is
present, while allowing exactly one of spans_rho or spans_h. Preserve the
existing invalid_argument exception and message.
In `@src/shamrock/include/shamrock/solvergraph/INode.hpp`:
- Around line 109-117: Update INode::get_rw_edge_optional to return a mutable
reference by replacing the const reference wrapper construction with the
non-const form, so it matches std::optional<std::reference_wrapper<T>> and
allows get_edges() to instantiate successfully.
🪄 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: 34e760b8-764a-40d9-a7e3-960606298cd3
📒 Files selected for processing (3)
src/shammodels/sph/src/modules/ComputeEos.cppsrc/shamrock/include/shamrock/solvergraph/INode.hppsrc/shamrock/include/shamrock/solvergraph/INullOptEdge.hpp
| /// Get a read write edge and cast it to the type T, return an optional | ||
| template<class T> | ||
| inline std::optional<std::reference_wrapper<T>> get_rw_edge_optional(int slot) { | ||
| auto &edge = rw_edges.at(slot); | ||
|
|
||
| auto ptr = std::dynamic_pointer_cast<T>(edge); | ||
| if (ptr) { | ||
| return std::cref(*ptr); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -A14 -B2 'get_rw_edge_optional' \
src/shamrock/include/shamrock/solvergraph/INode.hpp
rg -n 'std::cref\(\*ptr\)|std::ref\(\*ptr\)' \
src/shamrock/include/shamrock/solvergraph/INode.hppRepository: Shamrock-code/Shamrock
Length of output: 2576
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '80,140p' src/shamrock/include/shamrock/solvergraph/INode.hpp
printf '\n---\n'
sed -n '280,320p' src/shamrock/include/shamrock/solvergraph/INode.hpp
printf '\n---\n'
python3 - <<'PY'
from textwrap import dedent
code = dedent(r'''
`#include` <functional>
`#include` <memory>
`#include` <optional>
template<class T>
std::optional<std::reference_wrapper<T>> f(std::shared_ptr<T> edge) {
auto ptr = std::dynamic_pointer_cast<T>(edge);
if (ptr) {
return std::cref(*ptr);
}
return std::nullopt;
}
''')
open('/tmp/check.cpp','w').write(code)
print(code)
PY
printf '\n---\n'
if command -v g++ >/dev/null 2>&1; then
g++ -std=c++20 -fsyntax-only /tmp/check.cpp 2>&1 | sed -n '1,120p'
elif command -v clang++ >/dev/null 2>&1; then
clang++ -std=c++20 -fsyntax-only /tmp/check.cpp 2>&1 | sed -n '1,120p'
else
echo "No compiler available"
fiRepository: Shamrock-code/Shamrock
Length of output: 5832
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '1,220p' src/shamrock/include/shamrock/solvergraph/INode.hpp | nl -ba | sed -n '1,220p'Repository: Shamrock-code/Shamrock
Length of output: 200
Return std::ref(*ptr) here. std::cref(*ptr) yields std::reference_wrapper<const T>, which does not match std::optional<std::reference_wrapper<T>> and breaks get_edges() when optional RW edges are instantiated.
🤖 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/shamrock/include/shamrock/solvergraph/INode.hpp` around lines 109 - 117,
Update INode::get_rw_edge_optional to return a mutable reference by replacing
the const reference wrapper construction with the non-const form, so it matches
std::optional<std::reference_wrapper<T>> and allows get_edges() to instantiate
successfully.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/shammodels/sph/src/modules/ComputeEos.cpp`:
- Around line 948-973: The dust EOS branches in the has_epsilon_field() and
has_s_j_field() paths are no-ops, leaving EOS fields stale. Either restore their
compute_eos_internal implementations using the current span-based interface, or
explicitly fail both branches with shambase::throw_unimplemented() so
unsupported dust configurations cannot proceed silently.
- Line 276: Remove the unconditional logger::raw_ln debug statement from
compute_eos_internal, leaving the surrounding EOS computation unchanged.
🪄 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: e8ec3599-9052-44a2-8665-d10a3466af0f
📒 Files selected for processing (4)
src/shammodels/sph/include/shammodels/sph/modules/ComputeEos.hppsrc/shammodels/sph/src/modules/ComputeEos.cppsrc/shamrock/include/shamrock/solvergraph/INode.hppsrc/shamrock/include/shamrock/solvergraph/INullOptEdge.hpp
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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 (4)
src/shammodels/sph/src/modules/ComputeEos.cpp (4)
176-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate isothermal EOS logic vs.
ComputeEOSIsothermal::internal_eos.This
eos_internallambda (lines 185-189) is a copy ofComputeEOSIsothermal::internal_eos(lines 55-61) defined earlier in this same file. Two independent copies of the same physics is exactly the kind of drift that let the validation bug diverge (one copy got fixed, the other didn't). Consider having this branch call the node's staticinternal_eosdirectly.🤖 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 176 - 217, Remove the local eos_internal lambda in the isothermal branch and reuse ComputeEOSIsothermal::internal_eos for both rho- and h-based kernel paths, preserving the existing argument flow and outputs.
218-274: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUnchecked
spans_uint.value()in the Adiabatic branch.
spans_uint.value().get().get_spans()is called unconditionally without checkinghas_value(). All current call sites incompute_eos()always populatespans_uint, so this isn't exercised today, but the public signature (perComputeEos.hpp) allowsspans_uintto bestd::nullopt, and calling this branch that way would throwstd::bad_optional_accessinstead of a clear domain error.🤖 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 218 - 274, The Adiabatic branch in compute_eos must validate spans_uint before dereferencing it. Add an explicit domain-error path when spans_uint is absent, before either distributed_data_kernel_call uses spans_uint.value(), while preserving the existing rho- and h-based computation when it is present.
863-921: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate rho_g/uint_g computation between epsilon and s_j dust branches.
This block (863-921) and the
has_s_j_field()block (922-981) are nearly identical, differing only in howepsilon_sumis computed (direct field vs.s_j^2/rho_a). Consider factoring the sharedrho_g/uint_gfield setup, ghost-field lookup, and kernel dispatch into a helper parameterized by anepsilon_sumcallback.🤖 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 863 - 921, Factor the duplicated rho_g/uint_g setup, ghost-field lookup, and distributed_data_kernel_call shared by the has_epsilon_field() and has_s_j_field() branches into a helper. Parameterize the helper with the branch-specific epsilon_sum calculation, while preserving each branch’s existing input fields and EOS computation through compute_eos_internal.
906-911: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUnguarded division/multiplication by
(1 - epsilon_sum)can silently produce NaN/Inf/negative gas density. Both dust-to-gas conversion kernels computeepsilon_sumfrom field data with no check that it stays below 1 before using(1 - epsilon_sum), so numerical drift or a bad dust configuration silently corruptsrho_g/uint_ginstead of failing loudly.
src/shammodels/sph/src/modules/ComputeEos.cpp#L906-L911: clamp or assertepsilon_sum < 1before computingrho_g_a/uint_g_ain the epsilon-field branch.src/shammodels/sph/src/modules/ComputeEos.cpp#L966-L971: apply the same clamp/assert in the s_j-field branch.🤖 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 906 - 911, Guard the dust-to-gas conversion against epsilon_sum reaching or exceeding 1 before computing rho_g_a and uint_g_a in both ComputeEos.cpp sites at lines 906-911 and 966-971; clamp or assert epsilon_sum < 1 in the epsilon-field and s_j-field branches so invalid field data cannot produce non-finite or negative gas values.
♻️ Duplicate comments (1)
src/shammodels/sph/src/modules/ComputeEos.cpp (1)
69-76: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winInverted validation regressed in the new
ComputeEOSIsothermalnode.This is the exact same inverted condition flagged and marked "addressed" in a prior review at this same line range: the check throws when exactly one of
rho/his given (the only valid case) and silently passes through when both or neither are given. Any normal single-source call to this node will now throw"Must have either rho or h", and if neither is set,spans_pressure/spans_soundspeedare left at whateverensure_sizesproduced (never written by lines 90-118).🐛 Proposed fix
- if ((has_rho && !has_h) || (!has_rho && has_h)) { + if (has_rho == has_h) { throw shambase::make_except_with_loc<std::invalid_argument>( - "Must have either rho or h"); + "Must have exactly one of rho or h"); }🤖 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 69 - 76, Correct the validation condition in ComputeEOSIsothermal so it throws when both spans_rho and spans_h are present or when neither is present, while allowing exactly one source. Ensure the invalid neither-input path does not proceed to leave spans_pressure or spans_soundspeed unwritten.
🤖 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 `@src/shammodels/sph/src/modules/ComputeEos.cpp`:
- Around line 176-217: Remove the local eos_internal lambda in the isothermal
branch and reuse ComputeEOSIsothermal::internal_eos for both rho- and h-based
kernel paths, preserving the existing argument flow and outputs.
- Around line 218-274: The Adiabatic branch in compute_eos must validate
spans_uint before dereferencing it. Add an explicit domain-error path when
spans_uint is absent, before either distributed_data_kernel_call uses
spans_uint.value(), while preserving the existing rho- and h-based computation
when it is present.
- Around line 863-921: Factor the duplicated rho_g/uint_g setup, ghost-field
lookup, and distributed_data_kernel_call shared by the has_epsilon_field() and
has_s_j_field() branches into a helper. Parameterize the helper with the
branch-specific epsilon_sum calculation, while preserving each branch’s existing
input fields and EOS computation through compute_eos_internal.
- Around line 906-911: Guard the dust-to-gas conversion against epsilon_sum
reaching or exceeding 1 before computing rho_g_a and uint_g_a in both
ComputeEos.cpp sites at lines 906-911 and 966-971; clamp or assert epsilon_sum <
1 in the epsilon-field and s_j-field branches so invalid field data cannot
produce non-finite or negative gas values.
---
Duplicate comments:
In `@src/shammodels/sph/src/modules/ComputeEos.cpp`:
- Around line 69-76: Correct the validation condition in ComputeEOSIsothermal so
it throws when both spans_rho and spans_h are present or when neither is
present, while allowing exactly one source. Ensure the invalid neither-input
path does not proceed to leave spans_pressure or spans_soundspeed unwritten.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f4fe94b6-1b3e-4bc2-ab2f-52776ca169e4
📒 Files selected for processing (1)
src/shammodels/sph/src/modules/ComputeEos.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 230: Update the result-writing call in examples/sph/run_sod.py at lines
230-230 to use a with open(..., encoding="utf-8") as output context manager and
pass output to json.dump. Apply the same context-managed write in
examples/sph/run_sod_dust_tva.py at lines 299-299, preserving the existing
test_result.json output and indentation.
🪄 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: c75a7521-b3a4-4f47-a990-131bc29afc30
📒 Files selected for processing (2)
examples/sph/run_sod.pyexamples/sph/run_sod_dust_tva.py
| } | ||
| print(result) | ||
|
|
||
| json.dump(result, open(f"{dump_folder}/test_result.json", "w"), indent=4) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Close the test-result files deterministically.
Use a context manager so the JSON is flushed and closed even when execution or garbage collection differs.
examples/sph/run_sod.py#L230-L230: wrap the result writer inwith open(..., encoding="utf-8") as output:.examples/sph/run_sod_dust_tva.py#L299-L299: apply the same context-managed write.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 230-230: Use a context manager for opening files
(SIM115)
📍 Affects 2 files
examples/sph/run_sod.py#L230-L230(this comment)examples/sph/run_sod_dust_tva.py#L299-L299
🤖 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 230, Update the result-writing call in
examples/sph/run_sod.py at lines 230-230 to use a with open(...,
encoding="utf-8") as output context manager and pass output to json.dump. Apply
the same context-managed write in examples/sph/run_sod_dust_tva.py at lines
299-299, preserving the existing test_result.json output and indentation.
Source: Linters/SAST tools
tdavidcl
changed the title
Workflow reportworkflow report corresponding to commit f728e17 Light CI is enabled (the default for pull requests). This will only run the basic tests and not the full tests. Pre-commit check reportPre-commit check: ✅ Test pipeline can run. Clang-tidy diff reportDoxygen diff with
|
1 participant
No description provided.