[GSPH] Disc physics in GSPH - #1957
Draft
y-lapeyre wants to merge 10 commits into
Draft
Conversation
|
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:
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 |
Contributor
|
Thanks @y-lapeyre 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: 29
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shammodels/gsph/src/Solver.cpp (1)
1859-1864: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRefresh
axyz_extafter applying periodic boundaries.
ext_forces.compute_ext_forces_indep_v()writes sink–gas accelerations from positions beforeapply_position_boundary(). For periodic boundaries,prediictorthen drifts to post-wrap positions whileaxyz_extstill uses pre-wrap positions. Move the external-force computation afterapply_position_boundary(t_current + dt)soupdate_derivs()adds the external acceleration to the same wrapped positions used by the hydro update.🤖 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/gsph/src/Solver.cpp` around lines 1859 - 1864, Move ext_forces.compute_ext_forces_indep_v() in the solver step to after apply_position_boundary(t_current + dt), keeping gen_serial_patch_tree() before boundary application. Ensure update_derivs() receives external accelerations computed from the wrapped positions.
🤖 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/common/include/shammodels/common/setup/GeneratorMCDisc.hpp`:
- Around line 24-93: Fix the GeneratorMCDisc template contract consistently:
remove or otherwise reconcile the unused TSolverConfig and TSetupNodeBase
parameters so the class declaration, Config usage, nested DiscIterator
definition, and explicit instantiations all use the same two-parameter form.
Update related constructor/member references and use lower_case names for
particle mass/count variables, including n_part_p and node-count state, avoiding
uppercase names such as Npart unless required by an existing API.
In `@src/shammodels/common/src/modules/setup/GeneratorMCDisc.cpp`:
- Around line 24-181: Align GeneratorMCDisc::DiscIterator::next,
GeneratorMCDisc::is_done, and GeneratorMCDisc::next_n with the corrected
GeneratorMCDisc declaration: fix the template-list closing syntax, include all
declared template parameters, and use the class’s actual header namespace.
Update the explicit instantiations to use the same complete template arguments
and namespace, matching the internally consistent gsph implementation.
In `@src/shammodels/gsph/include/shammodels/gsph/Model.hpp`:
- Around line 141-150: The add_sink method should use named field initialization
for sph::SinkParticle, explicitly assigning pos, velocity, sph_acceleration,
ext_acceleration, mass, angular_momentum, and accretion_radius instead of
positional initialization. Also change the shamlog_debug_ln tag from "SPH" to
"GSPH".
In `@src/shammodels/gsph/include/shammodels/gsph/modules/GSPHSetup.hpp`:
- Around line 63-84: Resolve the undefined public factory methods declared in
GSPHSetup: make_generator_from_context, make_combiner_add,
make_modifier_warp_disc, make_modifier_custom_warp, make_modifier_add_offset,
make_modifier_filter, and make_modifier_split_part. Implement each method in
GSPHSetup.cpp if they are part of the supported API; otherwise remove their
declarations from GSPHSetup.hpp, keeping only factories with available
definitions.
In
`@src/shammodels/gsph/include/shammodels/gsph/modules/setup/GeneratorMCDisc.hpp`:
- Around line 12-16: Update the Doxygen header in GeneratorMCDisc.hpp to include
an `@author` tag alongside the existing `@file` and `@brief` tags. Rename the Npart
variable to a lower_case name and update every reference consistently.
In `@src/shammodels/gsph/include/shammodels/gsph/Solver.hpp`:
- Around line 219-224: Update the compute_sink_cfl() documentation so its
`@return` description refers to the minimum CFL timestep across sink pairs, and
explicitly states that it returns infinity when fewer than two sinks exist.
In `@src/shammodels/gsph/include/shammodels/gsph/SolverConfig.hpp`:
- Around line 228-237: Update the documentation above
set_eos_locally_isothermalFA2014 to describe its single h_over_r parameter,
remove the stale cs0, q, and r0 entries, and correct the “fromFarris 2014” typo
to “from Farris 2014.”
- Line 76: Document the eta_sink member in SolverConfig alongside cfl_cour and
cfl_force, using a ///< description that identifies it as the safety factor for
the sink–sink potential timestep used by Solver::compute_sink_cfl().
In `@src/shammodels/gsph/src/modules/ExternalForces.cpp`:
- Around line 371-404: Update the accretion flag-and-compaction logic in the
surrounding external-forces method to use sham::DeviceBuffer and
shamalgs::stream_compact with the device scheduler, matching
SinkParticlesUpdate::accrete_particles. Remove the legacy sycl::buffer and
shamalgs::numeric::stream_compact usage while preserving the existing
keep/accrete flags and resulting compacted index lists.
- Around line 248-330: Remove the dead graph-construction block in
add_ext_forces(), including is_G_needed/is_c_needed, the unused constant-node
scheduling, the set_field_xyz/set_field_vxyz/set_field_axyz/set_field_sizes
nodes, and both empty EF_PointMass branches. Preserve the existing
external-force accumulation logic and unsupported-force handling; do not retain
nodes whose edges are never consumed.
- Around line 400-438: In the accretion block after stream_compact, guard the
optional from id_list_keep using its count and pass an empty ID list to
pdat.keep_ids when the count is zero. Resolve the unused acc_mass and acc_pxyz
in the same block by either applying both to the point-mass state or removing
their reduction and documenting that the central potential is static; if
retained, allreduce the accreted mass and momentum across ranks consistently
with SinkParticlesUpdate::accrete_particles.
- Around line 34-40: Move the to_shared helper out of the shambase namespace and
into this translation unit’s anonymous namespace, preserving its forwarding
behavior. Update the call sites around the existing shambase::to_shared usages
to call to_shared unqualified, without adding the helper to the third-party
namespace.
In `@src/shammodels/gsph/src/modules/GeneratorMCDisc.cpp`:
- Around line 117-131: Update need_cs in the GeneratorMCDisc sound-speed
generation block to use solver_config.is_eos_locally_isothermal() instead of the
hardcoded false value. Preserve the existing cs_profile validation and
population of vec_cs so locally isothermal configurations generate soundspeeds
and still raise when cs_profile is missing.
In `@src/shammodels/gsph/src/modules/GSPHSetup.cpp`:
- Around line 126-175: Remove the unused SetupLog struct and golden_number
constant from GSPHSetup.cpp, since the setup-log feature is not wired in. If
retaining SetupLog instead, add its direct includes and move step_counter
incrementing to each logging operation so dump_state gates on the actual
logged-state count and records distinct step values.
- Around line 68-110: Update GSPHSetup::apply_setup to honor part_reordering
after the patch-data insertion loop by invoking the existing particle-reordering
facility, using the included ParticleReordering.hpp symbols; if reordering
cannot be implemented here, emit the established warning instead. Also remove
the unused injected_parts accumulation or expose it through the intended API.
- Around line 112-123: Update the logging calls in the GSPH setup flow,
including the occurrence near the final balancing steps and the matching
occurrence near line 147, to use the `"GSPH setup"` tag instead of `"SPH setup"`
while preserving the existing messages and logging behavior.
In `@src/shammodels/gsph/src/modules/SinkParticlesUpdate.cpp`:
- Around line 300-301: Update the sink–gas force kernel around the force
calculation in SinkParticlesUpdate.cpp to apply the existing epsilon_grav
softening, matching the treatment used by compute_ext_forces(). Ensure the
softened denominator remains finite when d is zero, including when sink_racc is
zero, before contributing to axyz_ext or the sink acceleration reduction.
- Around line 402-410: Make the gravitational softening length configurable by
adding a SolverConfig field alongside eta_sink, replacing the hardcoded
epsilon_grav and epsilon_grav_sink values with that configuration value in their
respective calculation paths. Preserve the existing unit handling and confirm or
implement the intended softening formulation consistently, using the configured
length rather than adding a fixed constant to rij_scal cubed.
- Around line 53-63: Remove the unused outer u32 sink_id declaration so the loop
variable in the sink-particle iteration is not shadowed. In the logging code
using the "sph::Sink" tag, update the tag to the appropriate GSPH identifier.
- Around line 370-384: Validate that gathered_result_acc_sinks contains exactly
world_size() * sink_parts.size() entries and that every rank contributed sink
accelerations in the same order before indexing it in the sink_parts loop. If
the size or ordering cannot be established, aggregate results by a stable sink
identifier instead of positional offsets, and avoid out-of-bounds access.
In `@src/shammodels/gsph/src/pyGSPHModel.cpp`:
- Around line 181-197: Add docstrings to both set_eos_locally_isothermalFA2014
and set_eos_locally_isothermalFA2014_extended matching the existing EOS binding
style, describing each equation and parameter. For the extended binding,
document cs0, q, r0, and that n_sinks must equal the number of sinks configured
through add_sink.
- Around line 282-326: Enable sound-speed handling consistently in the GSPH
generator by replacing the hardcoded need_cs value in the build_cs_lambda flow
with the solver configuration’s is_eos_locally_isothermal() query, and make the
same change in GeneratorMCDisc.cpp so its soundspeed write is enabled for
locally isothermal EOS configurations. Preserve the existing validation for
mutually exclusive cs_field and cs_profile inputs.
- Around line 271-279: Update the returned lambda in the azimuthal velocity
construction to detect a zero cylindrical radius before normalizing etheta.
Return a zero vector when r is zero; otherwise preserve the existing etheta
normalization and vth_r(r) * etheta calculation.
- Around line 627-648: Update the add_sink binding to expose named arguments for
mass, pos, velocity, and accretion_radius, using py::kw_only() consistently with
the other bindings in this file. Add a concise docstring describing the method
and its parameters, while preserving the existing T::add_sink behavior.
- Around line 393-442: Align the sixth parameter of the apply_setup binding with
its corresponding py::arg name, using the same msg_size_limit or
rank_comm_size_limit identifier in both the lambda and warning. Also handle
use_new_setup and speculative_balancing consistently with the other unsupported
options: either emit old-setup ignored warnings for them, correcting the
use_new_setup default contradiction, or remove these arguments until new setup
support exists.
- Around line 707-708: Update the `get_setup` binding on `T` to include
`py::keep_alive<0, 1>()`, ensuring the owning model remains alive while the
returned `GSPHSetup` object is referenced from Python.
In `@src/shammodels/gsph/src/Solver.cpp`:
- Around line 1852-1859: Resolve the duplicate sink external-force computation
between sink_update.predictor_step and the explicit
sink_update.compute_ext_forces call. Trace the ext_acceleration input consumed
by the corrector near line 1916, determine whether it requires the pre- or
post-drift sink position state, then retain only the computation producing that
state by removing the other call; keep predictor_step and do_predictor_leapfrog
behavior otherwise unchanged.
- Around line 1680-1724: Wire compute_sink_cfl() into evolve_once() alongside
compute_dt_cfl() so the selected timestep honors both constraints. In
compute_sink_cfl(), return an unconstrained result when storage.sinks is empty,
use epsilon_grav_sink when calculating sink separation to avoid coincident-sink
singularities, and remove or apply the unused C_cour configuration consistently
with the intended CFL formula. Preserve +inf as the result for zero or one sink,
and ensure evolve_once() handles that value while taking the minimum timestep.
In `@src/shammodels/gsph/src/SolverConfig.cpp`:
- Line 38: Define an axyz_ext constant in the names::newtonian set with a brief
descriptive comment, then replace the raw "axyz_ext" literals in
SolverConfig::set_layout, ExternalForces.cpp, and SinkParticlesUpdate.cpp with
that constant at all four locations.
---
Outside diff comments:
In `@src/shammodels/gsph/src/Solver.cpp`:
- Around line 1859-1864: Move ext_forces.compute_ext_forces_indep_v() in the
solver step to after apply_position_boundary(t_current + dt), keeping
gen_serial_patch_tree() before boundary application. Ensure update_derivs()
receives external accelerations computed from the wrapped positions.
🪄 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: 7a57fd21-77ef-4499-96a1-a59d9d911512
📒 Files selected for processing (21)
src/shammodels/common/include/shammodels/common/setup/GeneratorMCDisc.hppsrc/shammodels/common/src/modules/setup/GeneratorMCDisc.cppsrc/shammodels/gsph/CMakeLists.txtsrc/shammodels/gsph/include/shammodels/gsph/Model.hppsrc/shammodels/gsph/include/shammodels/gsph/Solver.hppsrc/shammodels/gsph/include/shammodels/gsph/SolverConfig.hppsrc/shammodels/gsph/include/shammodels/gsph/modules/ComputeLoadBalanceValue.hppsrc/shammodels/gsph/include/shammodels/gsph/modules/ExternalForces.hppsrc/shammodels/gsph/include/shammodels/gsph/modules/GSPHSetup.hppsrc/shammodels/gsph/include/shammodels/gsph/modules/SinkParticlesUpdate.hppsrc/shammodels/gsph/include/shammodels/gsph/modules/SolverStorage.hppsrc/shammodels/gsph/include/shammodels/gsph/modules/setup/GeneratorMCDisc.hppsrc/shammodels/gsph/include/shammodels/gsph/modules/setup/IGSPHSetupNode.hppsrc/shammodels/gsph/src/Solver.cppsrc/shammodels/gsph/src/SolverConfig.cppsrc/shammodels/gsph/src/modules/ComputeLoadBalanceValue.cppsrc/shammodels/gsph/src/modules/ExternalForces.cppsrc/shammodels/gsph/src/modules/GSPHSetup.cppsrc/shammodels/gsph/src/modules/GeneratorMCDisc.cppsrc/shammodels/gsph/src/modules/SinkParticlesUpdate.cppsrc/shammodels/gsph/src/pyGSPHModel.cpp
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
Workflow reportworkflow report corresponding to commit c44b51e Light CI is enabled (the default for pull requests). This will only run the basic tests and not the full tests. Pre-commit check reportSome failures were detected in base source checks checks. Suggested changesDetailed changes :diff --git a/src/shammodels/gsph/src/Solver.cpp b/src/shammodels/gsph/src/Solver.cpp
index dee7fdbe..25fc6b0d 100644
--- a/src/shammodels/gsph/src/Solver.cpp
+++ b/src/shammodels/gsph/src/Solver.cpp
@@ -39,8 +39,8 @@
#include "shammodels/gsph/Solver.hpp"
#include "shammodels/gsph/SolverConfig.hpp"
#include "shammodels/gsph/config/FieldNames.hpp"
-#include "shammodels/gsph/modules/ExternalForces.hpp"
#include "shammodels/gsph/modules/ComputeLoadBalanceValue.hpp"
+#include "shammodels/gsph/modules/ExternalForces.hpp"
#include "shammodels/gsph/modules/GSPHUtilities.hpp"
#include "shammodels/gsph/modules/SinkParticlesUpdate.hpp"
#include "shammodels/gsph/modules/UpdateDerivs.hpp"
|
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
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.
This PR introduces the following:
It has been tested on:
There is significant overlap/duplication with the SPH solver. When these parts of the code get solver graphed, we will be able to use common modules for both solvers.