Skip to content

[SPH] move sink-sink CFL into ComputeCFLSinkSink solvergraph node - #2443

Merged
mergify[bot] merged 2 commits into
Shamrock-code:mainfrom
tdavidcl:sph-sink-sink-cfl-solvergraph-node
Sep 24, 2026
Merged

mergify[bot] merged 2 commits into
Shamrock-code:mainfrom
tdavidcl:sph-sink-sink-cfl-solvergraph-node

Conversation

@tdavidcl

Copy link
Copy Markdown
Member

No description provided.

Replace the inline host double loop computing the sink-sink CFL in
evolve_once() with a new ComputeCFLSinkSink<Tvec> node, modelled on
ComputeCFLDust1Fluid. The node reads G, C_force, eta_phi and the
synchronized sink_pos/sink_mass/sink_acc_ext edges and writes the rank
CFL into an IDataEdge<Tscal>. The !pos.empty() guard and the cfl_detail
entry stay in the solver, so the logged CFL table is unchanged.

Assisted-by: Claude Code
Write the node's LaTeX description from its edge tex symbols, following
the replace_all pattern used by the other SPH nodes. Also add the
missing `template` keyword on the synchronized_data get_edge_ptr calls,
which are dependent names inside the Solver template.

Assisted-by: Claude Code
@github-actions

Copy link
Copy Markdown
Contributor

Thanks @tdavidcl for opening this PR!

You can do multiple things directly here:
1 - Comment pre-commit.ci run to run pre-commit checks.
2 - Comment pre-commit.ci autofix to apply fixes.
3 - Add label autofix.ci to fix authorship & pre-commit for every commit made.
4 - Add label full-ci to run the full test suite (default is light CI; full CI also runs on Mergify merge-queue branches).
5 - Add label profile-build to run the compile-time build profile job even in light CI.
6 - Add label trigger-ci to create an empty commit to trigger the CI.

Once the workflow completes a message will appear displaying informations related to the run.

Also the PR gets automatically reviewed by gemini, you can:
1 - Comment /gemini review to trigger a review
2 - Comment /gemini summary for a summary
3 - Tag it using @gemini-code-assist either in the PR or in review comments on files

@coderabbitai

coderabbitai Bot commented Sep 23, 2026

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 7982b9f7-a966-4b8f-8e4d-68fae73eb98d

📥 Commits

Reviewing files that changed from the base of the PR and between d8c8585 and 0271b8f.

📒 Files selected for processing (2)
  • src/shammodels/sph/include/shammodels/sph/modules/ComputeCFLSinkSink.hpp
  • src/shammodels/sph/src/Solver.cpp

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The sink-sink CFL calculation now runs in a solver-graph node. evolve_once wires the node to parameter and sink data edges, evaluates it, and records its result in cfl_detail.

Changes

Sink-sink CFL calculation

Layer / File(s) Summary
Implement sink-sink CFL calculation
src/shammodels/sph/include/shammodels/sph/modules/ComputeCFLSinkSink.hpp
The new node computes the minimum pairwise time step for sinks with nonzero external acceleration and writes the result to cfl_dt. It provides a node label and LaTeX formula.
Wire node into evolution
src/shammodels/sph/src/Solver.cpp
evolve_once replaces the inline calculation with the node, wires its input edges, and records the output as sink_sink in cfl_detail.

Estimated code review effort: 2 (Simple) | ~15 minutes

Suggested reviewers: y-lapeyre

Merge Risk: ⚪ Minimal · up to 0271b

The extracted CFL calculation retains the existing timestep aggregation; no identified issue prevents merging after normal checks.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive No pull request description was provided, so the description does not explain the changeset. Add a description that explains the new ComputeCFLSinkSink node and the replacement of the inline sink-sink CFL calculation in Solver.cpp.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: moving sink-sink CFL computation into the ComputeCFLSinkSink solvergraph node.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@shamrock-code-admin

Copy link
Copy Markdown
Collaborator

@Mergifyio queue

@mergify

mergify Bot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

Merge Queue Status

This pull request spent 5 hours 50 minutes 49 seconds in the queue, including 3 hours 25 minutes 53 seconds running CI.

Required conditions to merge
  • check-success = all

@mergify mergify Bot added the queued label Sep 23, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Workflow report

workflow report corresponding to commit 0271b8f
Commiter email is timothee.davidcleris@proton.me

Light CI is enabled (the default for pull requests). This will only run the basic tests and not the full tests.
Full CI runs if the full-ci label is set, or automatically on Mergify merge-queue branches (mergify/merge-queue/*).
The merge gate job "on PR / all" is skipped in this case. Queue entry uses "on PR / all_light"; full CI runs in the merge queue.

Pre-commit check report

Pre-commit check: ✅

trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check for merge conflicts................................................Passed
check that executables have shebangs.....................................Passed
check that scripts with shebangs are executable..........................Passed
check for added large files..............................................Passed
check for case conflicts.................................................Passed
check for broken symlinks................................................Passed
check yaml...............................................................Passed
detect private key.......................................................Passed
No-tabs checker..........................................................Passed
Tabs remover.............................................................Passed
cmake-format.............................................................Passed
Validate GitHub Workflows................................................Passed
clang-format.............................................................Passed
ruff check...............................................................Passed
ruff format..............................................................Passed
Check doxygen headers....................................................Passed
Check license headers....................................................Passed
Check #pragma once.......................................................Passed
Check SYCL #include......................................................Passed
No ssh in git submodules remote..........................................Passed
No UTF-8 in files (except for authors)...................................Passed

Test pipeline can run.

Clang-tidy diff report


28298 warnings generated.
Suppressed 28299 warnings (28282 in non-user code, 16 due to line filter, 1 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

Doxygen diff with main

Removed warnings : 16
New warnings : 17
Warnings count : 8186 → 8187 (0.0%)

Detailed changes :
+ src/shammodels/sph/include/shammodels/sph/modules/ComputeCFLSinkSink.hpp:29: warning: Member NODE_EDGES(X_RO, X_RW) (macro definition) of file ComputeCFLSinkSink.hpp is not documented.
- src/shammodels/sph/src/Solver.cpp:1296: warning: Compound shammodels::sph::Debug_ph_dump is not documented.
+ src/shammodels/sph/src/Solver.cpp:1297: warning: Compound shammodels::sph::Debug_ph_dump is not documented.
- src/shammodels/sph/src/Solver.cpp:1297: warning: Member Tscal (typedef) of struct shammodels::sph::Debug_ph_dump is not documented.
+ src/shammodels/sph/src/Solver.cpp:1298: warning: Member Tscal (typedef) of struct shammodels::sph::Debug_ph_dump is not documented.
- src/shammodels/sph/src/Solver.cpp:1299: warning: Member nobj (variable) of struct shammodels::sph::Debug_ph_dump is not documented.
- src/shammodels/sph/src/Solver.cpp:1300: warning: Member gpart_mass (variable) of struct shammodels::sph::Debug_ph_dump is not documented.
+ src/shammodels/sph/src/Solver.cpp:1300: warning: Member nobj (variable) of struct shammodels::sph::Debug_ph_dump is not documented.
+ src/shammodels/sph/src/Solver.cpp:1301: warning: Member gpart_mass (variable) of struct shammodels::sph::Debug_ph_dump is not documented.
- src/shammodels/sph/src/Solver.cpp:1302: warning: Member buf_xyz (variable) of struct shammodels::sph::Debug_ph_dump is not documented.
- src/shammodels/sph/src/Solver.cpp:1303: warning: Member buf_hpart (variable) of struct shammodels::sph::Debug_ph_dump is not documented.
+ src/shammodels/sph/src/Solver.cpp:1303: warning: Member buf_xyz (variable) of struct shammodels::sph::Debug_ph_dump is not documented.
+ src/shammodels/sph/src/Solver.cpp:1304: warning: Member buf_hpart (variable) of struct shammodels::sph::Debug_ph_dump is not documented.
- src/shammodels/sph/src/Solver.cpp:1304: warning: Member buf_vxyz (variable) of struct shammodels::sph::Debug_ph_dump is not documented.
+ src/shammodels/sph/src/Solver.cpp:1305: warning: Member buf_vxyz (variable) of struct shammodels::sph::Debug_ph_dump is not documented.
- src/shammodels/sph/src/Solver.cpp:1308: warning: Member fill_blocks(PhantomDumpBlock &block, Debug_ph_dump< Tvec > &info) (function) of namespace shammodels::sph is not documented.
+ src/shammodels/sph/src/Solver.cpp:1309: warning: Member fill_blocks(PhantomDumpBlock &block, Debug_ph_dump< Tvec > &info) (function) of namespace shammodels::sph is not documented.
- src/shammodels/sph/src/Solver.cpp:1345: warning: Member make_interface_debug_phantom_dump(Debug_ph_dump< Tvec > info) (function) of namespace shammodels::sph is not documented.
+ src/shammodels/sph/src/Solver.cpp:1346: warning: Member make_interface_debug_phantom_dump(Debug_ph_dump< Tvec > info) (function) of namespace shammodels::sph is not documented.
- src/shammodels/sph/src/Solver.cpp:2236: warning: Member map_field_refs(PatchScheduler &sched, u32 field_idx, shamrock::solvergraph::FieldRefs< T > &refs) (function) of file Solver.cpp is not documented.
+ src/shammodels/sph/src/Solver.cpp:2237: warning: Member map_field_refs(PatchScheduler &sched, u32 field_idx, shamrock::solvergraph::FieldRefs< T > &refs) (function) of file Solver.cpp is not documented.
- src/shammodels/sph/src/Solver.cpp:2251: warning: Member map_field_refs_ext(PatchScheduler &sched, shambase::DistributedData< shamrock::patch::PatchDataLayer > &mpdats, u32 field_idx, shamrock::solvergraph::FieldRefs< T > &refs) (function) of file Solver.cpp is not documented.
+ src/shammodels/sph/src/Solver.cpp:2252: warning: Member map_field_refs_ext(PatchScheduler &sched, shambase::DistributedData< shamrock::patch::PatchDataLayer > &mpdats, u32 field_idx, shamrock::solvergraph::FieldRefs< T > &refs) (function) of file Solver.cpp is not documented.
- src/shammodels/sph/src/Solver.cpp:2270: warning: Member map_field_refs_ext(PatchScheduler &sched, shamrock::ComputeField< T > &field_data, shamrock::solvergraph::FieldRefs< T > &refs) (function) of file Solver.cpp is not documented.
+ src/shammodels/sph/src/Solver.cpp:2271: warning: Member map_field_refs_ext(PatchScheduler &sched, shamrock::ComputeField< T > &field_data, shamrock::solvergraph::FieldRefs< T > &refs) (function) of file Solver.cpp is not documented.

@mergify
mergify Bot merged commit 22831ec into Shamrock-code:main Sep 24, 2026
39 checks passed
@mergify mergify Bot removed the queued label Sep 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants