Skip to content

[SPH] migrate non inertial forces to sgraph#1306

Merged
tdavidcl merged 13 commits intoShamrock-code:mainfrom
tdavidcl:patch-2025-10-02-15-24
Oct 8, 2025
Merged

[SPH] migrate non inertial forces to sgraph#1306
tdavidcl merged 13 commits intoShamrock-code:mainfrom
tdavidcl:patch-2025-10-02-15-24

Conversation

@tdavidcl
Copy link
Member

@tdavidcl tdavidcl commented Oct 2, 2025

No description provided.

@tdavidcl tdavidcl added the draft label Oct 2, 2025
@tdavidcl tdavidcl marked this pull request as ready for review October 6, 2025 11:00
@tdavidcl
Copy link
Member Author

tdavidcl commented Oct 6, 2025

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the calculation of non-inertial external forces, such as Lense-Thirring and Shearing Box forces, to use the SolverGraph system. This is a good step towards a more modular and maintainable implementation.

My review has identified a critical issue with the Lense-Thirring force calculation which appears to be a pre-existing sign error. I've also pointed out several medium-severity issues, including misleading comments and variable names, some dead code, and an opportunity to simplify the logic for setting up the solver graph. Addressing these points will improve the correctness and clarity of the code.

tdavidcl and others added 3 commits October 6, 2025 23:52
…ceLenseThirring.hpp

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…ceShearingBoxNonInertial.hpp

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2025

Workflow report

workflow report corresponding to commit 6ac333c
Commiter email is timothee.davidcleris@proton.me
GitHub page artifact URL GitHub page artifact link (can expire)

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
Validate GitHub Workflows................................................Passed
clang-format.............................................................Passed
black....................................................................Passed
ruff check...............................................................Passed
Check doxygen headers....................................................Passed
Check license headers....................................................Passed
Check #pragma once.......................................................Passed
Check SYCL #include......................................................Passed
No ssh in git submodules remote..........................................Passed

Test pipeline can run.

Clang-tidy diff report

No relevant changes found.
Well done!

You should now go back to your normal life and enjoy a hopefully sunny day while waiting for the review.

Doxygen diff with main

Removed warnings : 2
New warnings : 27
Warnings count : 6982 → 7007 (0.4%)

Detailed changes :
+ src/shammodels/common/include/shammodels/common/modules/AddForceLenseThirring.hpp:31: warning: Compound shammodels::common::modules::AddForceLenseThirring is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceLenseThirring.hpp:38: warning: Compound shammodels::common::modules::AddForceLenseThirring::Edges is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceLenseThirring.hpp:39: warning: Member constant_G (variable) of struct shammodels::common::modules::AddForceLenseThirring::Edges is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceLenseThirring.hpp:40: warning: Member constant_c (variable) of struct shammodels::common::modules::AddForceLenseThirring::Edges is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceLenseThirring.hpp:41: warning: Member central_mass (variable) of struct shammodels::common::modules::AddForceLenseThirring::Edges is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceLenseThirring.hpp:42: warning: Member central_pos (variable) of struct shammodels::common::modules::AddForceLenseThirring::Edges is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceLenseThirring.hpp:43: warning: Member a_spin (variable) of struct shammodels::common::modules::AddForceLenseThirring::Edges is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceLenseThirring.hpp:44: warning: Member dir_spin (variable) of struct shammodels::common::modules::AddForceLenseThirring::Edges is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceLenseThirring.hpp:46: warning: Member spans_positions (variable) of struct shammodels::common::modules::AddForceLenseThirring::Edges is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceLenseThirring.hpp:47: warning: Member spans_velocities (variable) of struct shammodels::common::modules::AddForceLenseThirring::Edges is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceLenseThirring.hpp:48: warning: Member sizes (variable) of struct shammodels::common::modules::AddForceLenseThirring::Edges is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceLenseThirring.hpp:49: warning: Member spans_accel_ext (variable) of struct shammodels::common::modules::AddForceLenseThirring::Edges is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceLenseThirring.hpp:52: warning: Member set_edges(std::shared_ptr< shamrock::solvergraph::IDataEdge< Tscal > > constant_G, std::shared_ptr< shamrock::solvergraph::IDataEdge< Tscal > > constant_c, std::shared_ptr< shamrock::solvergraph::IDataEdge< Tscal > > central_mass, std::shared_ptr< shamrock::solvergraph::IDataEdge< Tvec > > central_pos, std::shared_ptr< shamrock::solvergraph::IDataEdge< Tscal > > a_spin, std::shared_ptr< shamrock::solvergraph::IDataEdge< Tvec > > dir_spin, std::shared_ptr< shamrock::solvergraph::IFieldSpan< Tvec > > spans_positions, std::shared_ptr< shamrock::solvergraph::IFieldSpan< Tvec > > spans_velocities, std::shared_ptr< shamrock::solvergraph::Indexes< u32 > > sizes, std::shared_ptr< shamrock::solvergraph::IFieldSpan< Tvec > > spans_accel_ext) (function) of class shammodels::common::modules::AddForceLenseThirring is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceLenseThirring.hpp:76: warning: Member get_edges() (function) of class shammodels::common::modules::AddForceLenseThirring is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceShearingBoxNonInertial.hpp:31: warning: Compound shammodels::common::modules::AddForceShearingBoxNonInertial is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceShearingBoxNonInertial.hpp:38: warning: Compound shammodels::common::modules::AddForceShearingBoxNonInertial::Edges is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceShearingBoxNonInertial.hpp:39: warning: Member omega_0 (variable) of struct shammodels::common::modules::AddForceShearingBoxNonInertial::Edges is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceShearingBoxNonInertial.hpp:40: warning: Member q (variable) of struct shammodels::common::modules::AddForceShearingBoxNonInertial::Edges is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceShearingBoxNonInertial.hpp:42: warning: Member spans_positions (variable) of struct shammodels::common::modules::AddForceShearingBoxNonInertial::Edges is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceShearingBoxNonInertial.hpp:43: warning: Member spans_velocities (variable) of struct shammodels::common::modules::AddForceShearingBoxNonInertial::Edges is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceShearingBoxNonInertial.hpp:44: warning: Member sizes (variable) of struct shammodels::common::modules::AddForceShearingBoxNonInertial::Edges is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceShearingBoxNonInertial.hpp:45: warning: Member spans_accel_ext (variable) of struct shammodels::common::modules::AddForceShearingBoxNonInertial::Edges is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceShearingBoxNonInertial.hpp:48: warning: Member set_edges(std::shared_ptr< shamrock::solvergraph::IDataEdge< Tscal > > omega_0, std::shared_ptr< shamrock::solvergraph::IDataEdge< Tscal > > q, std::shared_ptr< shamrock::solvergraph::IFieldSpan< Tvec > > spans_positions, std::shared_ptr< shamrock::solvergraph::IFieldSpan< Tvec > > spans_velocities, std::shared_ptr< shamrock::solvergraph::Indexes< u32 > > sizes, std::shared_ptr< shamrock::solvergraph::IFieldSpan< Tvec > > spans_accel_ext) (function) of class shammodels::common::modules::AddForceShearingBoxNonInertial is not documented.
+ src/shammodels/common/include/shammodels/common/modules/AddForceShearingBoxNonInertial.hpp:59: warning: Member get_edges() (function) of class shammodels::common::modules::AddForceShearingBoxNonInertial is not documented.
+ src/shammodels/sph/src/modules/ExternalForces.cpp:218: warning: Member register_constant_set(shamrock::solvergraph::SolverGraph &solver_graph, std::string name, std::function< T()> getter) (function) of file ExternalForces.cpp is not documented.
- src/shammodels/sph/src/modules/ExternalForces.cpp:35: warning: Member to_shared(T &&t) (function) of namespace shambase is not documented.
- src/shammodels/sph/src/modules/ExternalForces.cpp:35: warning: Member to_shared(T &&t) (function) of namespace shambase is not documented.
+ src/shammodels/sph/src/modules/ExternalForces.cpp:39: warning: Member to_shared(T &&t) (function) of namespace shambase is not documented.
+ src/shammodels/sph/src/modules/ExternalForces.cpp:39: warning: Member to_shared(T &&t) (function) of namespace shambase is not documented.

Copy link
Collaborator

@y-lapeyre y-lapeyre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@tdavidcl tdavidcl added in-review and removed draft labels Oct 8, 2025
@tdavidcl tdavidcl merged commit bc51815 into Shamrock-code:main Oct 8, 2025
114 of 145 checks passed
@tdavidcl tdavidcl deleted the patch-2025-10-02-15-24 branch October 8, 2025 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants