[Stagging][AMR] Try to unify grid related utilities in a helper class#1732
[Stagging][AMR] Try to unify grid related utilities in a helper class#1732tdavidcl wants to merge 1 commit intoShamrock-code:mainfrom
Conversation
|
Thanks @tdavidcl 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: |
![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
This pull request refactors the ComputeCellAABB module by introducing a BlockAMRCartesianGridUtils helper struct to centralize grid-to-float space conversions and cell size calculations. The review feedback identifies a potential precision loss in get_cell_size due to integer division, notes a redundant include for AMRBlock.hpp, and suggests moving the utility struct to a header file to facilitate code reuse across the project.
| namespace shammodels::basegodunov::modules { | ||
|
|
||
| template<class Tvec, class TgridVec> | ||
| struct BlockAMRCartesianGridUtils { | ||
|
|
||
| using Tscal = shambase::VecComponent<Tvec>; | ||
| using Tgridscal = shambase::VecComponent<TgridVec>; | ||
|
|
||
| static constexpr u32 dim = shambase::VectorProperties<TgridVec>::dimension; | ||
|
|
||
| //// grid space function | ||
|
|
||
| inline static Tgridscal get_cell_size(TgridVec block_min, TgridVec block_max, u32 Nside) { | ||
| return ((block_max - block_min)).x() / Nside; | ||
| } | ||
|
|
||
| //// real space functions | ||
|
|
||
| inline static Tvec to_float_space(TgridVec grid_pos, Tscal dxfact) { | ||
| return grid_pos.template convert<Tscal>() * dxfact; | ||
| } | ||
|
|
||
| inline static Tscal to_float_space(Tgridscal grid_pos, Tscal dxfact) { | ||
| return Tscal(grid_pos) * dxfact; | ||
| } | ||
| }; | ||
|
|
||
| } // namespace shammodels::basegodunov::modules |
There was a problem hiding this comment.
The current implementation of get_cell_size uses integer division if Tgridscal is an integer type (e.g., i64). This changes the behavior from the previous implementation, which used floating-point division, and can lead to precision loss if the block size in grid units is not a multiple of Nside. To maintain accuracy and support non-power-of-two aligned grids, the division should be performed in floating-point space. Additionally, to_float_space should be updated to handle the resulting floating-point grid size without truncating it back to an integer.
namespace shammodels::basegodunov::modules {
template<class Tvec, class TgridVec>
struct BlockAMRCartesianGridUtils {
using Tscal = shambase::VecComponent<Tvec>;
using Tgridscal = shambase::VecComponent<TgridVec>;
static constexpr u32 dim = shambase::VectorProperties<TgridVec>::dimension;
//// grid space function
inline static Tscal get_cell_size(TgridVec block_min, TgridVec block_max, u32 Nside) {
return static_cast<Tscal>((block_max - block_min).x()) / static_cast<Tscal>(Nside);
}
//// real space functions
inline static Tvec to_float_space(TgridVec grid_pos, Tscal dxfact) {
return grid_pos.template convert<Tscal>() * dxfact;
}
inline static Tscal to_float_space(Tscal grid_pos, Tscal dxfact) {
return grid_pos * dxfact;
}
};
} // namespace shammodels::basegodunov::modules|
|
||
| #include "shammodels/ramses/modules/ComputeCellAABB.hpp" | ||
| #include "shambackends/kernel_call_distrib.hpp" | ||
| #include "shammodels/common/amr/AMRBlock.hpp" |
There was a problem hiding this comment.
The include for AMRBlock.hpp appears to be redundant as it is not used in this file. Removing unused includes helps maintain code cleanliness and can improve compilation times.
| namespace shammodels::basegodunov::modules { | ||
|
|
||
| template<class Tvec, class TgridVec> | ||
| struct BlockAMRCartesianGridUtils { |
There was a problem hiding this comment.
The BlockAMRCartesianGridUtils struct is defined within a .cpp file, which restricts its use to this translation unit. To improve maintainability and follow the repository practice of refactoring shared logic into helper functions, this helper should be moved to a dedicated header file (e.g., include/shammodels/ramses/utils/GridUtils.hpp) so it can be shared across different modules.
References
- Refactor duplicated logic into a helper function or lambda to improve readability and maintainability.
Workflow reportworkflow report corresponding to commit 738ef2b Light CI is enabled. This will only run the basic tests and not the full tests. Pre-commit check reportPre-commit check: ✅ Test pipeline can run. Doxygen diff with
|
1 participant
No description provided.