Skip to content

[Stagging][AMR] Try to unify grid related utilities in a helper class #1732

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
tdavidcl wants to merge 1 commit into
base: main
Choose a base branch
from
+37 −12
Draft

[Stagging][AMR] Try to unify grid related utilities in a helper class #1732

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 37 additions & 12 deletions src/shammodels/ramses/src/modules/ComputeCellAABB.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,47 @@

#include "shammodels/ramses/modules/ComputeCellAABB.hpp"
#include "shambackends/kernel_call_distrib.hpp"
#include "shammodels/common/amr/AMRBlock.hpp"
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Mar 31, 2026

Choose a reason for hiding this comment

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

medium

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.

#include "shamrock/patch/PatchDataField.hpp"
#include "shamsys/NodeInstance.hpp"

namespace shammodels::basegodunov::modules {

template<class Tvec, class TgridVec>
struct BlockAMRCartesianGridUtils {
Comment on lines +23 to +26
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Mar 31, 2026

Choose a reason for hiding this comment

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

medium

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
  1. Refactor duplicated logic into a helper function or lambda to improve readability and maintainability.


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
Comment on lines +23 to +50
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Mar 31, 2026

Choose a reason for hiding this comment

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

high

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


namespace {

template<class Tvec, class TgridVec>
struct KernelComputeCellAABB {
using Tscal = shambase::VecComponent<Tvec>;

using Grid = shammodels::basegodunov::modules::BlockAMRCartesianGridUtils<Tvec, TgridVec>;

inline static void kernel(
const shambase::DistributedData<shamrock::PatchDataFieldSpanPointer<TgridVec>>
&spans_block_min,
Expand All @@ -40,33 +72,26 @@ namespace {

const shambase::DistributedData<u32> &block_counts = sizes;

Tscal one_over_Nside = 1. / block_nside;

Tscal dxfact = grid_coord_to_pos_fact;

sham::distributed_data_kernel_call(
shamsys::instance::get_compute_scheduler_ptr(),
sham::DDMultiRef{spans_block_min, spans_block_max},
sham::DDMultiRef{spans_block_cell_sizes, spans_cell0block_aabb_lower},
block_counts,
[one_over_Nside, dxfact](
[dxfact, block_nside](
u32 i,
const TgridVec *__restrict acc_block_min,
const TgridVec *__restrict acc_block_max,
Tscal *__restrict bsize,
Tscal *__restrict csize,
Tvec *__restrict aabb_lower) {
TgridVec lower = acc_block_min[i];
TgridVec upper = acc_block_max[i];

Tvec lower_flt = lower.template convert<Tscal>() * dxfact;
Tvec upper_flt = upper.template convert<Tscal>() * dxfact;

Tvec block_cell_size = (upper_flt - lower_flt) * one_over_Nside;

Tscal res = block_cell_size.x();
auto cell_size = Grid::get_cell_size(lower, upper, block_nside);

bsize[i] = res;
aabb_lower[i] = lower_flt;
csize[i] = Grid::to_float_space(cell_size, dxfact);
aabb_lower[i] = Grid::to_float_space(lower, dxfact);
});
}
};
Expand Down
Loading