[GSPH] Use pre-computed fields from storage in VTKDump#1536
Closed
Guo-astro wants to merge 8 commits intoShamrock-code:mainfrom
Closed
[GSPH] Use pre-computed fields from storage in VTKDump#1536Guo-astro wants to merge 8 commits intoShamrock-code:mainfrom
Guo-astro wants to merge 8 commits intoShamrock-code:mainfrom
Conversation
Add FieldNames.hpp with centralized field name constants for GSPH. Add GSPHGhostHandler and GSPHUtilities for ghost particle handling. Newtonian physics uses standard field names (xyz, vxyz, uint, etc.) for consistency with other SPH methods. SR will use frame-specific names (e.g., uint_rest, uint_lab) when implemented.
Add constants for pressure, soundspeed, and gradient fields (grad_density, grad_pressure, grad_vx, grad_vy, grad_vz) to FieldNames.hpp. Update Solver.cpp to use these constants instead of hardcoded strings.
- Use hardcoded strings for field names (matching SPH pattern) - Organize edge constants under edges::infra:: and edges::physics:: - Remove unused kernel_call.hpp include from VTKDump.cpp - Fix hardcoded field index in VTKDump start_dump
Essentially during most PRs we end up adding stuff removing them in review, which create a git blame that does not exist after the squash. Simple solution -> forget the author update during PR but keep in auto generated PRs
Co-authored-by: David--Cléris Timothée <timothee.davidcleris@proton.me>
Contributor
|
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: |
Contributor
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 significantly enhances the efficiency and data consistency of GSPH simulations by refactoring the VTKDump module to leverage pre-computed physical fields. By directly reading density, pressure, and soundspeed from storage, it avoids unnecessary recalculations, leading to cleaner code and potentially faster I/O operations. The introduction of GSPH-specific ghost handling and standardized field naming further improves the modularity and maintainability of the GSPH solver. Highlights
Ignored Files
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}
Contributor
There was a problem hiding this comment.
Code Review
This pull request is a great step forward in improving the VTKDump functionality. By removing the duplicate density and pressure calculations and instead reading these pre-computed fields from storage, the code is now more efficient and easier to maintain. The addition of the sound speed to the VTK output is also a valuable enhancement.
The introduction of FieldNames.hpp to replace magic strings with named constants is an excellent change that significantly improves code readability and safety across the GSPH module.
I've added a few comments with suggestions for improvement, including a potential division-by-zero bug, an inefficient vector operation, and opportunities for further refactoring to improve code reuse. Overall, this is a high-quality contribution.
|
|
||
| i32 d = dx + dy + dz; | ||
|
|
||
| i32 df = -int(d * shearinfo.shear_value / sz); |
Contributor
There was a problem hiding this comment.
There is a potential division-by-zero on this line if sz is zero. This can happen if bsize components are zero or if shearinfo.shear_dir is orthogonal to bsize. This would lead to a crash or undefined behavior. Please add a check to handle the case where sz is zero.
Suggested change
| i32 df = -int(d * shearinfo.shear_value / sz); | |
| i32 df = (std::abs(sz) > std::numeric_limits<T>::epsilon()) ? -int(d * shearinfo.shear_value / sz) : 0; |
Comment on lines
+140
to
+177
| void modify_interface_native( | ||
| shambase::DistributedDataShared<InterfaceIdTable> &builder, | ||
| shambase::DistributedDataShared<T> &mod, | ||
| std::function<void(u64, u64, InterfaceBuildInfos, sham::DeviceBuffer<u32> &, u32, T &)> | ||
| fct) { | ||
| StackEntry stack_loc{}; | ||
|
|
||
| struct Args { | ||
| u64 sender; | ||
| u64 receiver; | ||
| InterfaceIdTable &build_table; | ||
| }; | ||
|
|
||
| std::vector<Args> vecarg; | ||
|
|
||
| builder.for_each([&](u64 sender, u64 receiver, InterfaceIdTable &build_table) { | ||
| if (build_table.ids_interf.get_size() == 0) { | ||
| throw shambase::make_except_with_loc<std::runtime_error>( | ||
| "there is an empty id table in the interface, it should have been removed"); | ||
| } | ||
|
|
||
| vecarg.push_back({sender, receiver, build_table}); | ||
| }); | ||
|
|
||
| u32 i = 0; | ||
| mod.for_each([&](u64 sender, u64 receiver, T &ref) { | ||
| InterfaceIdTable &build_table = vecarg[i].build_table; | ||
|
|
||
| fct(sender, | ||
| receiver, | ||
| build_table.build_infos, | ||
| build_table.ids_interf, | ||
| build_table.ids_interf.get_size(), | ||
| ref); | ||
|
|
||
| i++; | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
The implementation of modify_interface_native relies on builder and mod having the same number of elements and a consistent iteration order. This is fragile because an inconsistency could lead to an out-of-bounds access on vecarg or silent mismatches if mod has more elements than builder.
To make this safer, consider adding a check to validate that the sizes are equal before iterating. If a size() method is not available on DistributedDataShared, you could count elements in mod first, or add a check inside the mod.for_each loop to prevent reading past the end of vecarg.
Comment on lines
+88
to
+90
| list_possible.resize(list_possible.size() + 1); | ||
| list_possible[list_possible.size() - 1] | ||
| = i32_3{xoff + off_d.x(), yoff + off_d.y(), zoff + off_d.z()}; |
Contributor
There was a problem hiding this comment.
Using resize(size() + 1) in a loop is generally inefficient as it can lead to multiple reallocations of the vector's underlying storage. A more idiomatic and efficient way to add an element to a vector is to use push_back(). This is clearer and has amortized constant time complexity.
Suggested change
| list_possible.resize(list_possible.size() + 1); | |
| list_possible[list_possible.size() - 1] | |
| = i32_3{xoff + off_d.x(), yoff + off_d.y(), zoff + off_d.z()}; | |
| list_possible.push_back(i32_3{xoff + off_d.x(), yoff + off_d.y(), zoff + off_d.z()}); |
Comment on lines
+128
to
+158
| void vtk_dump_add_solvergraph_field( | ||
| PatchScheduler &sched, | ||
| shamrock::LegacyVtkWritter &writter, | ||
| u32 field_idx, | ||
| shamrock::solvergraph::Field<T> &field, | ||
| std::string field_dump_name) { | ||
| StackEntry stack_loc{}; | ||
|
|
||
| using namespace shamrock::patch; | ||
| u64 num_obj = sched.get_rank_count(); | ||
|
|
||
| if (num_obj > 0) { | ||
| std::unique_ptr<sycl::buffer<T>> field_vals = sched.rankgather_field<T>(field_idx); | ||
| std::unique_ptr<sycl::buffer<T>> ret = std::make_unique<sycl::buffer<T>>(num_obj); | ||
|
|
||
| writter.write_field(field_dump_name, field_vals, num_obj); | ||
| u64 ptr = 0; | ||
| sched.for_each_patch_data([&](u64 id_patch, Patch cur_p, PatchDataLayer &pdat) { | ||
| using namespace shamalgs::memory; | ||
| using namespace shambase; | ||
|
|
||
| if (pdat.get_obj_cnt() > 0) { | ||
| write_with_offset_into( | ||
| shamsys::instance::get_compute_scheduler().get_queue(), | ||
| get_check_ref(ret), | ||
| field.get_buf(id_patch), | ||
| ptr, | ||
| pdat.get_obj_cnt()); | ||
|
|
||
| ptr += pdat.get_obj_cnt(); | ||
| } | ||
| }); | ||
|
|
||
| writter.write_field(field_dump_name, ret, num_obj); |
Contributor
There was a problem hiding this comment.
The function vtk_dump_add_solvergraph_field contains boilerplate logic to gather data from a shamrock::solvergraph::Field across patches. This logic is useful and could be needed elsewhere. To improve code reuse and maintainability, consider refactoring this gathering logic into a new method on the shamrock::solvergraph::Field class itself, for example, a rankgather() method. This would simplify vtk_dump_add_solvergraph_field and make the gathering logic available for other uses.
Guo-astro
force-pushed
the
gsph-vtkdump-use-storage
branch
3 times, most recently
from
cc3b684 to
48338de
Compare
Refactor VTKDump to use pre-computed fields (density, pressure, soundspeed) from SolverStorage instead of recalculating them. This ensures VTK output matches the simulation's computed values. Also uses centralized field name constants from FieldNames.hpp throughout the GSPH module, organized by physics mode for future SR support: - names::common::* for shared fields (xyz, hpart) - names::newtonian::* for Newtonian-specific fields (vxyz, axyz, uint, etc.)
Guo-astro
force-pushed
the
gsph-vtkdump-use-storage
branch
from
48338de to
57f1f2e
Compare
- Guard against division by zero in for_each_patch_shift when sz is negligible - Replace inefficient resize() + index assignment with push_back() - Add size validation in modify_interface_native to ensure builder and mod match
Guo-astro
force-pushed
the
gsph-vtkdump-use-storage
branch
from
a34c58d to
bd2ae11
Compare
Contributor
Workflow reportworkflow report corresponding to commit 8024b83 Pre-commit check reportPre-commit check: ✅ Test pipeline can run. Clang-tidy diff reportDoxygen diff with
|
Contributor
Author
|
Closing in favor of a new PR based on the updated main branch (after #1532 merge) |
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
3 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.
Summary
Changes
Test plan