[GSPH] migrate merged_xyzh and merged_patchdata_ghost to solvergraph #1583
Conversation
Add MergedPatchDataEdge for merged PatchDataLayer storage management via solvergraph. Changes: - Add MergedPatchDataEdge.hpp for merged patchdata edge - Add solver_graph and merged patchdata edges to SolverStorage - Register edges in init_solver_graph() - Update all merged_xyzh usages to solvergraph pattern - Update all merged_patchdata_ghost usages to solvergraph pattern - Update UpdateDerivs.cpp to use solvergraph pattern Part of GSPH solvergraph migration (1 object per PR).
Summary of ChangesHello @Guo-astro, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the handling of merged particle data ( Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request successfully migrates merged_xyzh and merged_patchdata_ghost to the solvergraph pattern. This involves introducing a new MergedPatchDataEdge class to encapsulate the DistributedData<PatchDataLayer>, updating SolverStorage to use std::shared_ptr for these edges, and registering them with the SolverGraph. All usage sites have been correctly updated to reflect the new access patterns, and resource cleanup now properly utilizes the free_alloc() method of the new edge type. The changes are well-implemented and consistent with the stated goal of integrating these data structures into the SolverGraph framework. No issues of medium, high, or critical severity were found during the review.
|
Thanks @Guo-astro 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: |
Workflow reportworkflow report corresponding to commit d93cebd 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 8cab7f87..6c9c09f8 100644
--- a/src/shammodels/gsph/src/Solver.cpp
+++ b/src/shammodels/gsph/src/Solver.cpp
@@ -89,8 +89,7 @@ void shammodels::gsph::Solver<Tvec, Kern>::init_solver_graph() {
// Register merged patchdata edges for dependency tracking
storage.merged_xyzh = storage.solver_graph.register_edge(
- "merged_xyzh",
- solvergraph::MergedPatchDataEdge("merged_xyzh", "\\mathbf{xyzh}_{\\rm m}"));
+ "merged_xyzh", solvergraph::MergedPatchDataEdge("merged_xyzh", "\\mathbf{xyzh}_{\\rm m}"));
storage.merged_patchdata_ghost = storage.solver_graph.register_edge(
"merged_patchdata_ghost",
@@ -199,8 +198,8 @@ template<class Tvec, template<class> class Kern>
void shammodels::gsph::Solver<Tvec, Kern>::merge_position_ghost() {
StackEntry stack_loc{};
- shambase::get_check_ref(storage.merged_xyzh).data = (
- storage.ghost_handler.get().build_comm_merge_positions(storage.ghost_patch_cache.get()));
+ shambase::get_check_ref(storage.merged_xyzh).data
+ = (storage.ghost_handler.get().build_comm_merge_positions(storage.ghost_patch_cache.get()));
// Get field indices from xyzh_ghost_layout
const u32 ixyz_ghost
@@ -210,32 +209,38 @@ void shammodels::gsph::Solver<Tvec, Kern>::merge_position_ghost() {
// Set element counts
shambase::get_check_ref(storage.part_counts).indexes
- = shambase::get_check_ref(storage.merged_xyzh).get_data().template map<u32>(
- [&](u64 id, shamrock::patch::PatchDataLayer &mpdat) {
- return scheduler().patch_data.get_pdat(id).get_obj_cnt();
- });
+ = shambase::get_check_ref(storage.merged_xyzh)
+ .get_data()
+ .template map<u32>([&](u64 id, shamrock::patch::PatchDataLayer &mpdat) {
+ return scheduler().patch_data.get_pdat(id).get_obj_cnt();
+ });
// Set element counts with ghost
shambase::get_check_ref(storage.part_counts_with_ghost).indexes
- = shambase::get_check_ref(storage.merged_xyzh).get_data().template map<u32>(
- [&](u64 id, shamrock::patch::PatchDataLayer &mpdat) {
- return mpdat.get_obj_cnt();
- });
+ = shambase::get_check_ref(storage.merged_xyzh)
+ .get_data()
+ .template map<u32>([&](u64 id, shamrock::patch::PatchDataLayer &mpdat) {
+ return mpdat.get_obj_cnt();
+ });
// Attach spans to block coords
shambase::get_check_ref(storage.positions_with_ghosts)
.set_refs(
- shambase::get_check_ref(storage.merged_xyzh).get_data().template map<std::reference_wrapper<PatchDataField<Tvec>>>(
- [&, ixyz_ghost](u64 id, shamrock::patch::PatchDataLayer &mpdat) {
- return std::ref(mpdat.get_field<Tvec>(ixyz_ghost));
- }));
+ shambase::get_check_ref(storage.merged_xyzh)
+ .get_data()
+ .template map<std::reference_wrapper<PatchDataField<Tvec>>>(
+ [&, ixyz_ghost](u64 id, shamrock::patch::PatchDataLayer &mpdat) {
+ return std::ref(mpdat.get_field<Tvec>(ixyz_ghost));
+ }));
shambase::get_check_ref(storage.hpart_with_ghosts)
.set_refs(
- shambase::get_check_ref(storage.merged_xyzh).get_data().template map<std::reference_wrapper<PatchDataField<Tscal>>>(
- [&, ihpart_ghost](u64 id, shamrock::patch::PatchDataLayer &mpdat) {
- return std::ref(mpdat.get_field<Tscal>(ihpart_ghost));
- }));
+ shambase::get_check_ref(storage.merged_xyzh)
+ .get_data()
+ .template map<std::reference_wrapper<PatchDataField<Tscal>>>(
+ [&, ihpart_ghost](u64 id, shamrock::patch::PatchDataLayer &mpdat) {
+ return std::ref(mpdat.get_field<Tscal>(ihpart_ghost));
+ }));
}
template<class Tvec, template<class> class Kern>
@@ -766,8 +771,8 @@ void shammodels::gsph::Solver<Tvec, Kern>::communicate_merge_ghosts_fields() {
});
// Merge local and ghost data
- shambase::get_check_ref(storage.merged_patchdata_ghost).data = (
- ghost_handle.template merge_native<PatchDataLayer, PatchDataLayer>(
+ shambase::get_check_ref(storage.merged_patchdata_ghost).data
+ = (ghost_handle.template merge_native<PatchDataLayer, PatchDataLayer>(
std::move(interf_pdat),
[&](const shamrock::patch::Patch p, shamrock::patch::PatchDataLayer &pdat) {
PatchDataLayer pdat_new(ghost_layout_ptr);
@@ -1138,73 +1143,77 @@ void shammodels::gsph::Solver<Tvec, Kern>::compute_eos_fields() {
soundspeed_field.ensure_sizes(counts_with_ghosts);
// Iterate over merged_patchdata_ghost (includes local + ghost particles)
- shambase::get_check_ref(storage.merged_patchdata_ghost).get_data().for_each([&](u64 id, PatchDataLayer &mpdat) {
- u32 total_elements
- = shambase::get_check_ref(storage.part_counts_with_ghost).indexes.get(id);
- if (total_elements == 0)
- return;
-
- // Use SPH-summation density from communicated ghost data
- sham::DeviceBuffer<Tscal> &buf_density = mpdat.get_field_buf_ref<Tscal>(idensity_interf);
- auto &pressure_buf = pressure_field.get_field(id).get_buf();
- auto &soundspeed_buf = soundspeed_field.get_field(id).get_buf();
+ shambase::get_check_ref(storage.merged_patchdata_ghost)
+ .get_data()
+ .for_each([&](u64 id, PatchDataLayer &mpdat) {
+ u32 total_elements
+ = shambase::get_check_ref(storage.part_counts_with_ghost).indexes.get(id);
+ if (total_elements == 0)
+ return;
- sham::DeviceQueue &q = dev_sched->get_queue();
- sham::EventList depends_list;
+ // Use SPH-summation density from communicated ghost data
+ sham::DeviceBuffer<Tscal> &buf_density
+ = mpdat.get_field_buf_ref<Tscal>(idensity_interf);
+ auto &pressure_buf = pressure_field.get_field(id).get_buf();
+ auto &soundspeed_buf = soundspeed_field.get_field(id).get_buf();
- auto density = buf_density.get_read_access(depends_list);
- auto pressure = pressure_buf.get_write_access(depends_list);
- auto soundspeed = soundspeed_buf.get_write_access(depends_list);
+ sham::DeviceQueue &q = dev_sched->get_queue();
+ sham::EventList depends_list;
- const Tscal *uint_ptr = nullptr;
- if (has_uint) {
- uint_ptr = mpdat.get_field_buf_ref<Tscal>(iuint_interf).get_read_access(depends_list);
- }
+ auto density = buf_density.get_read_access(depends_list);
+ auto pressure = pressure_buf.get_write_access(depends_list);
+ auto soundspeed = soundspeed_buf.get_write_access(depends_list);
- auto e = q.submit(depends_list, [&](sycl::handler &cgh) {
- shambase::parallel_for(cgh, total_elements, "compute_eos_gsph", [=](u64 gid) {
- u32 i = (u32) gid;
+ const Tscal *uint_ptr = nullptr;
+ if (has_uint) {
+ uint_ptr
+ = mpdat.get_field_buf_ref<Tscal>(iuint_interf).get_read_access(depends_list);
+ }
- // Use SPH-summation density (from compute_omega, communicated to ghosts)
- Tscal rho = density[i];
- rho = sycl::max(rho, Tscal(1e-30));
-
- if (has_uint && uint_ptr != nullptr) {
- // Adiabatic EOS (reference: g_pre_interaction.cpp line 107)
- // P = (\gamma - 1) * \rho * u
- Tscal u = uint_ptr[i];
- u = sycl::max(u, Tscal(1e-30));
- Tscal P = (gamma - Tscal(1.0)) * rho * u;
-
- // Sound speed from internal energy (reference: solver.cpp line 2661)
- // c = sqrt(\gamma * (\gamma - 1) * u)
- Tscal cs = sycl::sqrt(gamma * (gamma - Tscal(1.0)) * u);
-
- // Clamp to reasonable values
- P = sycl::clamp(P, Tscal(1e-30), Tscal(1e30));
- cs = sycl::clamp(cs, Tscal(1e-10), Tscal(1e10));
-
- pressure[i] = P;
- soundspeed[i] = cs;
- } else {
- // Isothermal case
- Tscal cs = Tscal(1.0);
- Tscal P = cs * cs * rho;
-
- pressure[i] = P;
- soundspeed[i] = cs;
- }
+ auto e = q.submit(depends_list, [&](sycl::handler &cgh) {
+ shambase::parallel_for(cgh, total_elements, "compute_eos_gsph", [=](u64 gid) {
+ u32 i = (u32) gid;
+
+ // Use SPH-summation density (from compute_omega, communicated to ghosts)
+ Tscal rho = density[i];
+ rho = sycl::max(rho, Tscal(1e-30));
+
+ if (has_uint && uint_ptr != nullptr) {
+ // Adiabatic EOS (reference: g_pre_interaction.cpp line 107)
+ // P = (\gamma - 1) * \rho * u
+ Tscal u = uint_ptr[i];
+ u = sycl::max(u, Tscal(1e-30));
+ Tscal P = (gamma - Tscal(1.0)) * rho * u;
+
+ // Sound speed from internal energy (reference: solver.cpp line 2661)
+ // c = sqrt(\gamma * (\gamma - 1) * u)
+ Tscal cs = sycl::sqrt(gamma * (gamma - Tscal(1.0)) * u);
+
+ // Clamp to reasonable values
+ P = sycl::clamp(P, Tscal(1e-30), Tscal(1e30));
+ cs = sycl::clamp(cs, Tscal(1e-10), Tscal(1e10));
+
+ pressure[i] = P;
+ soundspeed[i] = cs;
+ } else {
+ // Isothermal case
+ Tscal cs = Tscal(1.0);
+ Tscal P = cs * cs * rho;
+
+ pressure[i] = P;
+ soundspeed[i] = cs;
+ }
+ });
});
- });
- // Complete all buffer event states
- buf_density.complete_event_state(e);
- if (has_uint) {
- mpdat.get_field_buf_ref<Tscal>(iuint_interf).complete_event_state(e);
- }
- pressure_buf.complete_event_state(e);
- soundspeed_buf.complete_event_state(e);
- });
+ // Complete all buffer event states
+ buf_density.complete_event_state(e);
+ if (has_uint) {
+ mpdat.get_field_buf_ref<Tscal>(iuint_interf).complete_event_state(e);
+ }
+ pressure_buf.complete_event_state(e);
+ soundspeed_buf.complete_event_state(e);
+ });
}
template<class Tvec, template<class> class Kern>
diff --git a/src/shammodels/gsph/src/modules/UpdateDerivs.cpp b/src/shammodels/gsph/src/modules/UpdateDerivs.cpp
index 217e11e2..c1efbb2b 100644
--- a/src/shammodels/gsph/src/modules/UpdateDerivs.cpp
+++ b/src/shammodels/gsph/src/modules/UpdateDerivs.cpp
@@ -84,9 +84,10 @@ void shammodels::gsph::modules::UpdateDerivs<Tvec, SPHKernel>::update_derivs_ite
= has_uint ? ghost_layout.get_field_idx<Tscal>(gsph::names::newtonian::uint) : 0;
// Get merged data and caches from storage
- auto &merged_xyzh = shambase::get_check_ref(storage.merged_xyzh).get_data();
- shamrock::solvergraph::Field<Tscal> &omega_field = shambase::get_check_ref(storage.omega);
- shambase::DistributedData<PatchDataLayer> &mpdats = shambase::get_check_ref(storage.merged_patchdata_ghost).get_data();
+ auto &merged_xyzh = shambase::get_check_ref(storage.merged_xyzh).get_data();
+ shamrock::solvergraph::Field<Tscal> &omega_field = shambase::get_check_ref(storage.omega);
+ shambase::DistributedData<PatchDataLayer> &mpdats
+ = shambase::get_check_ref(storage.merged_patchdata_ghost).get_data();
// Get pressure and soundspeed from storage (includes ghosts)
shamrock::solvergraph::Field<Tscal> &pressure_field = shambase::get_check_ref(storage.pressure);
@@ -300,9 +301,10 @@ void shammodels::gsph::modules::UpdateDerivs<Tvec, SPHKernel>::update_derivs_hll
u32 iuint_interf
= has_uint ? ghost_layout.get_field_idx<Tscal>(gsph::names::newtonian::uint) : 0;
- auto &merged_xyzh = shambase::get_check_ref(storage.merged_xyzh).get_data();
- shamrock::solvergraph::Field<Tscal> &omega_field = shambase::get_check_ref(storage.omega);
- shambase::DistributedData<PatchDataLayer> &mpdats = shambase::get_check_ref(storage.merged_patchdata_ghost).get_data();
+ auto &merged_xyzh = shambase::get_check_ref(storage.merged_xyzh).get_data();
+ shamrock::solvergraph::Field<Tscal> &omega_field = shambase::get_check_ref(storage.omega);
+ shambase::DistributedData<PatchDataLayer> &mpdats
+ = shambase::get_check_ref(storage.merged_patchdata_ghost).get_data();
// Get pressure and soundspeed from storage (includes ghosts)
shamrock::solvergraph::Field<Tscal> &pressure_field = shambase::get_check_ref(storage.pressure);
|
2 participants
Summary
Add
MergedPatchDataEdgefor merged PatchDataLayer storage management via solvergraph.Part of GSPH solvergraph migration (1 object per PR per maintainer request).
Changes:
MergedPatchDataEdge.hppfor merged patchdata edgesolver_graphand merged patchdata edges toSolverStorageinit_solver_graph()merged_xyzhusages to solvergraph patternmerged_patchdata_ghostusages to solvergraph patternUpdateDerivs.cppto use solvergraph patternTest plan