Skip to content
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.

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

minimize maxwellian vector capacity somewhat #898

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 6 additions & 4 deletions src/amr/data/field/refine/electric_field_refiner.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@

#include "core/def/phare_mpi.hpp"

#include <SAMRAI/hier/Box.h>

#include "amr/amr_constants.hpp"
#include "amr/resources_manager/amr_utils.hpp"

#include "core/utilities/constants.hpp"
#include "core/data/grid/gridlayoutdefs.hpp"
#include "core/utilities/point/point.hpp"

#include <SAMRAI/hier/Box.h>

#include <cstddef>

namespace PHARE::amr
Expand Down Expand Up @@ -43,8 +45,8 @@ class ElectricFieldRefiner
{
TBOX_ASSERT(coarseField.physicalQuantity() == fineField.physicalQuantity());

auto const locFineIdx = AMRToLocal(fineIndex, fineBox_);
auto constexpr refinementRatio = 2;
auto const locFineIdx = AMRToLocal(fineIndex, fineBox_);

auto coarseIdx{fineIndex};
for (auto& idx : coarseIdx)
idx = idx / refinementRatio;
Expand Down
73 changes: 45 additions & 28 deletions src/amr/data/particles/refine/particles_data_split.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,30 +2,30 @@
#define PHARE_PARTICLES_DATA_SPLIT_HPP


#include "core/def.hpp"
#include "phare_core.hpp"
#include "core/def/phare_mpi.hpp"
#include "core/utilities/constants.hpp"

#include "core/def.hpp"
#include "amr/data/particles/particles_data.hpp"
#include "amr/resources_manager/amr_utils.hpp"
#include "split.hpp"
#include "core/utilities/constants.hpp"
#include "phare_core.hpp"
#include "amr/amr_constants.hpp"
#include "amr/utilities/box/amr_box.hpp"
#include "amr/resources_manager/amr_utils.hpp"
#include "amr/data/particles/particles_data.hpp"

#include <SAMRAI/geom/CartesianPatchGeometry.h>
#include <SAMRAI/hier/Box.h>
#include <SAMRAI/hier/RefineOperator.h>
#include <SAMRAI/pdat/CellOverlap.h>

#include <functional>


namespace PHARE
{
namespace amr
{
enum class ParticlesDataSplitType {
coarseBoundary,
enum class ParticlesDataSplitType : std::uint8_t {
coarseBoundary = 0,
interior,
coarseBoundaryOld,
coarseBoundaryNew
Expand Down Expand Up @@ -53,6 +53,12 @@ namespace amr
template<typename ParticleArray, ParticlesDataSplitType splitType, typename Splitter>
class ParticlesRefineOperator : public SAMRAI::hier::RefineOperator
{
using Particle_t = typename ParticleArray::value_type;
static constexpr bool putParticlesInCoarseBoundary
= splitType == ParticlesDataSplitType::coarseBoundary
|| splitType == ParticlesDataSplitType::coarseBoundaryOld
|| splitType == ParticlesDataSplitType::coarseBoundaryNew;

public:
static constexpr auto dim = Splitter::dimension;
static constexpr auto interpOrder = Splitter::interp_order;
Expand Down Expand Up @@ -99,7 +105,7 @@ namespace amr
= std::dynamic_pointer_cast<ParticlesData<ParticleArray>>(
source.getPatchData(sourceComponent));

// Finnaly we need the cartesion geometry of both patch.
// Finaly we need the cartesion geometry of both patch.
auto patchGeomDestination
= std::dynamic_pointer_cast<SAMRAI::geom::CartesianPatchGeometry>(
destination.getPatchGeometry());
Expand Down Expand Up @@ -133,9 +139,15 @@ namespace amr
}
}

template<typename Fn>
static void _reserve(ParticleArray& particles, Fn&& counter)
{ // only reserve for regular refinement of domain to start with
if constexpr (!putParticlesInCoarseBoundary)
particles.reserve(counter() * nbRefinedPart);
}

/** @brief given two ParticlesData (destination and source),
* an overlap , a ratio and the geometry of both patches, perform the
* an overlap, a ratio and the geometry of both patches, perform the
* splitting of coarse particles onto the destination patch
*/
void refine_(ParticlesData<ParticleArray>& destParticlesData,
Expand Down Expand Up @@ -166,43 +178,48 @@ namespace amr
// same for destinationGhostBox and destinationDomainBox the later will allow to get an
// index relative to the interior

std::array const particlesArrays{&srcInteriorParticles, &srcGhostParticles};

Comment on lines +181 to +182
Copy link

@coderabbitai coderabbitai bot Oct 14, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Specify template arguments for std::array declaration

The declaration of particlesArrays is missing the required template arguments for std::array. Please specify the element type and the size to ensure proper compilation.

Apply this diff to correct the declaration:

- std::array const particlesArrays{&srcInteriorParticles, &srcGhostParticles};
+ std::array<ParticleArray const*, 2> const particlesArrays{&srcInteriorParticles, &srcGhostParticles};
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
std::array const particlesArrays{&srcInteriorParticles, &srcGhostParticles};
std::array<ParticleArray const*, 2> const particlesArrays{&srcInteriorParticles, &srcGhostParticles};

Copy link
Member Author

Choose a reason for hiding this comment

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

it compiles without the templates, what makes you think it should have the templates?

Copy link

Choose a reason for hiding this comment

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

@PhilipDeegan, you're right—since the code compiles and class template argument deduction is available from C++17 onwards, your code is valid as is. I apologize for the confusion.

(_/)
(._.)


✏️ Learnings added
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#898
File: src/amr/data/particles/refine/particles_data_split.hpp:181-182
Timestamp: 2024-10-16T09:54:01.233Z
Learning: The codebase uses C++17 features such as class template argument deduction, allowing template arguments to be omitted in declarations like `std::array`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

auto const count_expected = [&]() {
std::size_t incoming_estimate = 0;
for (auto const& destinationBox : destBoxes)
{
auto const coarseSplitBox
= coarsen(phare_box_from<dim>(getSplitBox(destinationBox)));
for (auto const& sourceParticlesArray : particlesArrays)
incoming_estimate += sourceParticlesArray->nbr_particles_in(coarseSplitBox);
}
return incoming_estimate;
};

_reserve(destDomainParticles, count_expected);

Splitter split;

// The PatchLevelFillPattern had compute boxes that correspond to the expected filling.
// In case of a coarseBoundary it will most likely give multiple boxes
// in case of interior, this will be just one box usually
for (auto const& destinationBox : destBoxes)
{
std::array particlesArrays{&srcInteriorParticles, &srcGhostParticles};
auto splitBox = getSplitBox(destinationBox);
auto const splitBox = getSplitBox(destinationBox);

auto isInDest = [&destinationBox](auto const& particle) //
auto const isInDest = [&destinationBox](auto const& particle) //
{ return isInBox(destinationBox, particle); };


for (auto const& sourceParticlesArray : particlesArrays)
{
for (auto const& particle : *sourceParticlesArray)
{
std::array<typename ParticleArray::value_type, nbRefinedPart>
refinedParticles;
auto particleRefinedPos = toFineGrid<interpOrder>(particle);
auto const particleRefinedPos = toFineGrid<interpOrder>(particle);

if (isInBox(splitBox, particleRefinedPos))
{
std::array<Particle_t, nbRefinedPart> refinedParticles;
split(particleRefinedPos, refinedParticles);


// we need to know in which of interior or levelGhostParticlesXXXX
// arrays we must put particles

bool constexpr putParticlesInCoarseBoundary
= splitType == ParticlesDataSplitType::coarseBoundary
|| splitType == ParticlesDataSplitType::coarseBoundaryOld
|| splitType == ParticlesDataSplitType::coarseBoundaryNew;



if constexpr (putParticlesInCoarseBoundary)
{
if constexpr (splitType == ParticlesDataSplitType::coarseBoundary)
Expand Down Expand Up @@ -243,9 +260,9 @@ namespace amr
std::back_inserter(destDomainParticles), isInDest);
}
} // end is candidate for split
} // end loop on particles
} // end loop on source particle arrays
} // loop on destination box
} // end loop on particles
} // end loop on source particle arrays
} // loop on destination box
}


Expand Down
22 changes: 19 additions & 3 deletions src/amr/utilities/box/amr_box.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@
#define PHARE_AMR_UTILITIES_BOX_BOX_HPP


#include "core/def.hpp"
#include "core/def/phare_mpi.hpp"
#include "core/utilities/box/box.hpp"

#include "amr/amr_constants.hpp"

#include "SAMRAI/hier/Box.h"
#include "core/utilities/box/box.hpp"
#include "core/def.hpp"


namespace PHARE::amr
{
Expand Down Expand Up @@ -85,6 +85,22 @@ struct Box : public PHARE::core::Box<Type, dim>
}
};

template<std::size_t dim>
auto refine(core::Box<int, dim> box)
{
for (std::uint8_t di = 0; di < dim; ++di)
box.lower[di] *= refinementRatio, box.upper[di] *= refinementRatio;
return box;
}
template<std::size_t dim>
auto coarsen(core::Box<int, dim> box)
{
for (std::uint8_t di = 0; di < dim; ++di)
box.lower[di] /= refinementRatio, box.upper[di] /= refinementRatio;

return box;
}

} // namespace PHARE::amr

#endif
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ class MaxwellianParticleInitializer : public ParticleInitializer<ParticleArray,
std::uint32_t const& nbrParticlesPerCell, std::optional<std::size_t> seed = {},
Basis const basis = Basis::Cartesian,
std::array<InputFunction, 3> const& magneticField = {nullptr, nullptr, nullptr},
double densityCutOff = 1e-5)
double const densityCutOff = 1e-5, double const over_alloc_factor = .1)
: density_{density}
, bulkVelocity_{bulkVelocity}
, thermalVelocity_{thermalVelocity}
Expand All @@ -54,6 +54,7 @@ class MaxwellianParticleInitializer : public ParticleInitializer<ParticleArray,
, nbrParticlePerCell_{nbrParticlesPerCell}
, basis_{basis}
, rngSeed_{seed}
, over_alloc_factor_{over_alloc_factor}
{
}

Expand Down Expand Up @@ -92,6 +93,7 @@ class MaxwellianParticleInitializer : public ParticleInitializer<ParticleArray,
std::uint32_t nbrParticlePerCell_;
Basis basis_;
std::optional<std::size_t> rngSeed_;
double over_alloc_factor_ = .1;
};


Expand Down Expand Up @@ -178,6 +180,17 @@ void MaxwellianParticleInitializer<ParticleArray, GridLayout>::loadParticles(
auto randGen = getRNG(rngSeed_);
ParticleDeltaDistribution<double> deltaDistrib;

auto const expected_size = std::accumulate(n, n + ndCellIndices.size(), std::size_t{0},
[&](auto const& sum, auto const& density_value) {
return (density_value > densityCutOff_)
? sum + nbrParticlePerCell_
: sum;
});
Comment on lines +183 to +188
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Pass 'sum' by value in the lambda for 'std::accumulate'

In the lambda function used with std::accumulate, the sum parameter should be passed by value instead of by const reference. Passing sum by value is more appropriate since it's an integer type and avoids unnecessary indirection. This aligns with standard practice and can lead to clearer code.

Apply this diff to adjust the lambda parameter:

-auto const expected_size = std::accumulate(n, n + ndCellIndices.size(), std::size_t{0},
-                                           [&](auto const& sum, auto const& density_value) {
+auto const expected_size = std::accumulate(n, n + ndCellIndices.size(), std::size_t{0},
+                                           [&](auto sum, auto const& density_value) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
auto const expected_size = std::accumulate(n, n + ndCellIndices.size(), std::size_t{0},
[&](auto const& sum, auto const& density_value) {
return (density_value > densityCutOff_)
? sum + nbrParticlePerCell_
: sum;
});
auto const expected_size = std::accumulate(n, n + ndCellIndices.size(), std::size_t{0},
[&](auto sum, auto const& density_value) {
return (density_value > densityCutOff_)
? sum + nbrParticlePerCell_
: sum;
});


auto const incoming_estimate
= (layout.AMRBox().surface_cell_count() * (nbrParticlePerCell_ * over_alloc_factor_));
particles.reserve(expected_size + incoming_estimate);

for (std::size_t flatCellIdx = 0; flatCellIdx < ndCellIndices.size(); ++flatCellIdx)
{
if (n[flatCellIdx] < densityCutOff_)
Expand Down
18 changes: 9 additions & 9 deletions src/core/data/particles/particle.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ NO_DISCARD auto cellAsPoint(Particle const& particle)
template<size_t dim>
struct Particle
{
std::array<std::uint8_t, 3> constexpr static sizes = {52, 64, 76};
static_assert(dim > 0 and dim < 4, "Only dimensions 1,2,3 are supported.");
static const size_t dimension = dim;

Expand All @@ -54,12 +55,11 @@ struct Particle

Particle() = default;

double weight = 0;
double charge = 0;

std::array<int, dim> iCell = ConstArray<int, dim>();
std::array<double, dim> delta = ConstArray<double, dim>();
std::array<double, 3> v = ConstArray<double, 3>();
double weight = 0; // 8
double charge = 0; // 8
std::array<int, dim> iCell = ConstArray<int, dim>(); // 4 * dim
std::array<double, dim> delta = ConstArray<double, dim>(); // 8 * dim
std::array<double, 3> v = ConstArray<double, 3>(); // 8 * 3
Comment on lines +58 to +62
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider a more maintainable approach for documenting member sizes.

While the added size information is useful, it might become outdated if types change. Consider using sizeof() in static assertions or constexpr variables to automatically calculate and verify sizes. This approach would be more maintainable and less prone to manual errors.

Example implementation:

static_assert(sizeof(weight) == 8, "Unexpected size for weight");
static_assert(sizeof(charge) == 8, "Unexpected size for charge");
static_assert(sizeof(iCell) == 4 * dim, "Unexpected size for iCell");
static_assert(sizeof(delta) == 8 * dim, "Unexpected size for delta");
static_assert(sizeof(v) == 8 * 3, "Unexpected size for v");

Alternatively, you could use Doxygen-style comments for better documentation:

/// @brief Particle weight
/// @details Size: 8 bytes
double weight = 0;


NO_DISCARD bool operator==(Particle<dim> const& that) const
{
Expand Down Expand Up @@ -121,9 +121,9 @@ inline constexpr auto is_phare_particle_type

template<std::size_t dim, template<std::size_t> typename ParticleA,
template<std::size_t> typename ParticleB>
NO_DISCARD typename std::enable_if_t<
is_phare_particle_type<dim, ParticleA<dim>> and is_phare_particle_type<dim, ParticleB<dim>>,
bool>
NO_DISCARD typename std::enable_if_t<is_phare_particle_type<dim, ParticleA<dim>>
and is_phare_particle_type<dim, ParticleB<dim>>,
bool>
operator==(ParticleA<dim> const& particleA, ParticleB<dim> const& particleB)
{
return particleA.weight == particleB.weight and //
Expand Down
11 changes: 9 additions & 2 deletions src/core/utilities/box/box.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ template<typename Type, std::size_t dim>
struct Box
{
static const size_t dimension = dim;

using value_type = Type;

Point<Type, dim> lower;
Point<Type, dim> upper;
Expand Down Expand Up @@ -130,9 +130,16 @@ struct Box
return iterator{this, {upper[0] + 1, upper[1] + 1, upper[2] + 1}};
}
}
using value_type = Type;


NO_DISCARD auto surface_cell_count() const
{
// assumes box never smaller than 3 in any direction
auto const shape_ = shape();
auto const nested = shape_ - 2;
nicolasaunai marked this conversation as resolved.
Show resolved Hide resolved
return core::product(shape_) - core::product(nested);
}

Comment on lines +135 to +142
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Add runtime validation for box size assumption.

The review comment is valid. The codebase shows no existing validation for the minimum box size requirement, and tests reveal that boxes smaller than 3 units are possible. The suggested runtime check is necessary to prevent undefined behavior in surface_cell_count().

🔗 Analysis chain

Add runtime validation for box size assumption.

The function assumes the box is never smaller than 3 in any direction, but this assumption is not enforced. Consider adding a runtime check.

 NO_DISCARD auto surface_cell_count() const
 {
     // assumes box never smaller than 3 in any direction
+    for (std::size_t i = 0; i < dim; ++i) {
+        if (shape()[i] < 3) {
+            throw std::invalid_argument("Box must be at least 3 units in each dimension");
+        }
+    }
     auto const shape_ = shape();
     auto const nested = shape_ - 2;
     return core::product(shape_) - core::product(nested);
 }
🏁 Scripts executed

The following scripts were executed for the analysis:

Error: No shell scripts found


Script:

#!/bin/bash
# Find box-related files
fd box

# Look for box class definition and size-related code
ast-grep --pattern 'class box'

# Search for any existing size validations in box-related files
rg "size|shape|dimension" -A 3 -B 3 src/core/utilities/box/

Length of output: 8065


Script:

#!/bin/bash
# Look for test cases involving box size or surface cells
rg -A 5 -B 5 "surface_cell_count|Box.*[<{].*[>}]" tests/core/utilities/box/test_box.cpp

# Check if there are any small box tests
rg "Box.*[<{].*[>}].*\{.*[123].*\}" tests/core/utilities/box/test_box.cpp

Length of output: 4878

NO_DISCARD constexpr static std::size_t nbrRemainBoxes()
{
if constexpr (dim == 1)
Expand Down
4 changes: 1 addition & 3 deletions tests/amr/data/particles/refine/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,4 @@ target_link_libraries(${PROJECT_NAME} PRIVATE
${GTEST_LIBS})


add_phare_test(${PROJECT_NAME} ${CMAKE_CURRENT_BINARY_DIR})


add_no_mpi_phare_test(${PROJECT_NAME} ${CMAKE_CURRENT_BINARY_DIR})
Loading
Loading