minimize maxwellian vector capacity somewhat #898
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request introduce modifications across several files, primarily focusing on the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
src/core/data/particles/particle.hpp (1)
42-43: Approve with suggestion: Consider using named constants for size valuesThe addition of the
sizesarray and the static assertion improves type safety and helps catch potential issues at compile-time. This is a good practice.However, to improve maintainability, consider replacing the magic numbers (52, 64, 76) with named constants. This would make the code more self-documenting and easier to update if the structure changes in the future.
Example improvement:
constexpr static std::uint8_t PARTICLE_SIZE_1D = 52; constexpr static std::uint8_t PARTICLE_SIZE_2D = 64; constexpr static std::uint8_t PARTICLE_SIZE_3D = 76; std::array<std::uint8_t, 3> constexpr static sizes = {PARTICLE_SIZE_1D, PARTICLE_SIZE_2D, PARTICLE_SIZE_3D};src/core/utilities/box/box.hpp (1)
135-141: Approve with suggestions:surface_cell_count()methodThe addition of the
surface_cell_count()method is a good feature. However, I have a few suggestions to improve it:
Add a runtime assertion to check the assumption that the box is never smaller than 3 in any direction. This will help catch potential misuse.
Consider adding a comment explaining the calculation for better maintainability.
Use
std::size_tas the explicit return type instead ofautofor clarity.Here's a suggested implementation incorporating these changes:
NO_DISCARD std::size_t surface_cell_count() const { // Calculate the number of cells on the surface of the box // by subtracting the inner volume from the total volume auto const shape_ = shape(); for (std::size_t i = 0; i < dim; ++i) { assert(shape_[i] >= 3 && "Box must be at least 3 cells wide in each dimension"); } auto const nested = shape_ - 2; return core::product(shape_) - core::product(nested); }These changes will improve the robustness and readability of the method.
src/amr/solvers/solver_ppc.hpp (2)
155-170: LGTM! Consider adding a comment explaining the overallocation strategy.The
add_tomethod effectively manages the capacity of ParticleArray objects in the map while preserving overallocation. This approach can help reduce memory reallocation overhead.Consider adding a brief comment explaining the rationale behind preserving overallocation, as it might not be immediately obvious to other developers why this approach was chosen over a simple assignment.
225-227: LGTM! Consider extracting the key generation to a separate function.The use of the new
add_tomethod simplifies the logic and ensures consistent handling of ParticleArray objects. This change improves code maintainability and consistency.To further improve readability, consider extracting the key generation logic into a separate function:
std::string generateKey(const std::string& patchId, const std::string& popName) { return patchId + "_" + popName; } // Usage in saveState_: std::string patchId = std::to_string(state.patch->getGlobalId()); for (auto& pop : state.ions) { std::string key = generateKey(patchId, pop.name()); add_to(tmpDomain, key, pop.domainParticles()); add_to(patchGhost, key, pop.patchGhostParticles()); }This refactoring would make the
saveState_method more readable and easier to maintain.tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cpp (1)
103-118: Consider adding a test to verify particles are loaded within the domainThe 2D initializer lacks a test to ensure that particles are loaded within the correct domain boundaries, similar to the
loadsParticlesInTheDomaintest in the 1D case. Adding such a test would enhance the reliability of your 2D particle initialization.src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp (1)
189-189: Consider makingover_alloc_factorconfigurableThe
over_alloc_factoris currently hardcoded to0.1and marked with a TODO comment. To improve flexibility and adhere to clean code principles, consider retrieving this value from a configuration or dictionary instead of hardcoding it.Would you like assistance in implementing this change?
🛑 Comments failed to post (1)
tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cpp (1)
112-112:
⚠️ Potential issueUse integer arithmetic for 'outer_cell_count' to avoid type conversion issues
Using
std::powwith integer arguments returns adouble, which can lead to unintended type conversions or precision loss. It's better to perform this calculation using integer arithmetic.Apply this diff to correct the calculation:
-auto outer_cell_count = std::pow(50, 2) - std::pow(48, 2); +auto outer_cell_count = (50 * 50) - (48 * 48);📝 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.auto outer_cell_count = (50 * 50) - (48 * 48);
PhilipDeegan
force-pushed
the
min_cap
branch
2 times, most recently
from
aa6ea49 to
03b725b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp (1 hunks)
- src/core/data/particles/particle.hpp (3 hunks)
- src/core/utilities/box/box.hpp (2 hunks)
- tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cpp (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp
- src/core/data/particles/particle.hpp
- src/core/utilities/box/box.hpp
🔇 Additional comments (3)
tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cpp (3)
15-20: LGTM: Improved include statements and namespace organizationThe addition of the grid layout test header and the reorganization of namespaces enhance code structure and readability. These changes align well with the introduction of the new 2D test class.
28-28: LGTM: Consistent GridLayoutT type aliasThe update to the GridLayoutT type alias in the 1D class maintains consistency with the newly introduced 2D class structure. This change improves code uniformity without altering functionality.
79-101: LGTM: Well-structured 2D Maxwellian particle initializer test classThe new AMaxwellianParticleInitializer2D class is well-structured and consistent with the existing 1D class. This addition enhances test coverage for 2D scenarios.
However, I noticed that the number of particles per cell (600) differs from the 1D case (10000). Could you provide a brief explanation for this difference? It might be helpful to add a comment in the code explaining the reasoning behind these values.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cpp (1)
79-101: LGTM: Well-structured 2D Maxwellian Particle Initializer test classThe new
AMaxwellianParticleInitializer2Dclass is well-designed and consistent with its 1D counterpart. It correctly uses 2D-specific types and initializations.One minor suggestion:
Consider adding a brief comment explaining why the
nbrParticlesPerCellis set to 600 for 2D, as opposed to 10000 for 1D. This would help clarify the reasoning behind the different particle counts.src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp (1)
181-188: Efficient particle count estimation, but consider refining lambda capture.The introduction of
expected_sizecalculation is an excellent optimization for memory allocation. It efficiently estimates the number of particles by counting cells above the density cutoff.Consider refining the lambda capture to be more specific:
std::size_t const expected_size = [&ndCellIndices, &densityCutOff_ = this->densityCutOff_, nbrParticlePerCell = this->nbrParticlePerCell_](auto const& _n) { std::size_t size = 0; for (std::size_t flatCellIdx = 0; flatCellIdx < ndCellIndices.size(); ++flatCellIdx) if (_n[flatCellIdx] >= densityCutOff_) ++size; return size * nbrParticlePerCell; }(n);This change explicitly captures the required variables, improving code clarity and potentially preventing unintended variable captures.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp (1 hunks)
- src/core/data/particles/particle.hpp (3 hunks)
- src/core/utilities/box/box.hpp (2 hunks)
- tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cpp (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/data/particles/particle.hpp
- src/core/utilities/box/box.hpp
🧰 Additional context used
📓 Path-based instructions (1)
src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (4)
tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cpp (3)
15-20: LGTM: Improved includes and namespace organizationThe addition of the
test_gridlayout.hppheader and the introduction of thePHARE::initializer::test_fn::func_1dnamespace enhance code organization and provide necessary dependencies for the GridLayout testing.
28-29: LGTM: Updated GridLayoutT type aliasThe update to use
GridLayoutImplYeefor theGridLayoutTtype alias ensures consistency with the 2D implementation and aligns with the current grid layout system.
73-77: LGTM: Introduced namespace for 2D test functionsThe introduction of the
PHARE::initializer::test_fn::func_2dnamespace effectively separates 2D test functions from 1D, enhancing code organization and maintainability.src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp (1)
195-195: Excellent use ofreservefor performance optimization.The addition of
particles.reserve(allocate_size)before the particle generation loop is a great optimization. This pre-allocation strategy has several benefits:
- It reduces the number of potential reallocations during particle insertion.
- It improves performance by minimizing memory operations.
- It works seamlessly with the previously calculated
allocate_size.This change, combined with the
expected_sizeandallocate_sizecalculations, should significantly improve the memory management and performance of the particle initialization process.
| TEST_F(AMaxwellianParticleInitializer2D, loadsTheCorrectNbrOfParticles) | ||
| { | ||
| // vector push back allocation observations | ||
| // 100 ppc = 262144 - 250000 = 12144 == 12144 * 64 / 1e6 == .7MB overallocated | ||
| // 600 ppc = 2097152 - 1500000 = 597152 * 64 / 1e6 == 38MB overallocated | ||
|
|
||
| auto const expectedNbrParticles = nbrParticlesPerCell * product(layout.AMRBox().shape()); | ||
| initializer->loadParticles(particles, layout); | ||
| EXPECT_EQ(expectedNbrParticles, particles.size()); | ||
| auto outer_cell_count = std::pow(50, 2) - std::pow(48, 2); | ||
| EXPECT_EQ(particles.capacity(), | ||
| particles.size() + (outer_cell_count * nbrParticlesPerCell * .1)); | ||
|
|
||
| // new method | ||
| // 100 ppc = (1511760 - 1500000) * 64 / 1e6 == 0.12544 overallocated | ||
| // 600 ppc = (1511760 - 1500000) * 64 / 1e6 == 0.75264 overallocated | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Good test case, but consider refining comments and calculations
The test case for 2D particle loading is thorough and checks important aspects like particle count and capacity. However, there are a few areas for improvement:
-
The comments about vector push back allocation (lines 105-107 and 117-118) seem to be observations or calculations. Consider moving these to separate documentation or adding them as more detailed code comments to explain their significance.
-
The capacity calculation on lines 113-114 includes an adjustment for outer cells. It might be helpful to add a brief comment explaining the rationale behind this adjustment.
-
The "new method" comment on line 116 seems incomplete or out of place. Consider removing it or expanding on its meaning if it's still relevant.
To improve clarity and maintainability, consider extracting the capacity calculation into a separate function with a descriptive name, e.g.:
auto calculateExpectedCapacity(const TestGridLayout<GridLayoutT>& layout, std::uint32_t nbrParticlesPerCell) {
auto outer_cell_count = std::pow(layout.nbrCells()[0], 2) - std::pow(layout.nbrCells()[0] - 2, 2);
return nbrParticlesPerCell * product(layout.AMRBox().shape()) +
(outer_cell_count * nbrParticlesPerCell * 0.1);
}Then use this function in the test:
EXPECT_EQ(particles.capacity(), calculateExpectedCapacity(layout, nbrParticlesPerCell));This would make the test more readable and the calculation more reusable.
src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp
Outdated
Show resolved
Hide resolved
|
quick comment : this overallocation touches the allocation made in the particle initializer. How about refined level particle arrays? They probably are allocation by pushback from splitting or something like that? |
src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp
Outdated
Show resolved
Hide resolved
src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (8)
.github/workflows/cmake_ubuntu.yml (1)
81-85: Consider impact on Maxwellian vector capacity optimizationWhile these build configuration changes don't directly address the PR's stated objective of minimizing Maxwellian vector capacity, they may indirectly affect the behavior and performance characteristics related to memory allocation and vector capacity:
- The Debug build type typically includes additional runtime checks and may alter memory layout.
- The -O3 optimization might lead to aggressive inlining and other optimizations that could mask or exacerbate memory usage patterns.
These factors could potentially interfere with accurately assessing the impact of any changes made to minimize vector capacity.
To ensure that the PR's main objective is met:
- Consider separating build configuration changes from functional changes to isolate their effects.
- Implement specific tests that measure the Maxwellian vector capacity before and after your changes, ensuring they run in both debug and release configurations.
- Add memory profiling to your test suite to quantify the impact of your changes on overall memory usage, particularly in the particle initializer mentioned in the PR comments.
This approach will help validate that the vector capacity optimization is effective across different build configurations and doesn't introduce unintended side effects.
src/core/data/particles/particle.hpp (2)
42-42: Consider adding a comment and using a larger integer type forsizes.The purpose of the
sizesarray is not immediately clear. Consider adding a comment to explain its usage. Additionally,std::uint8_tmight be limiting if the sizes grow larger than 255 bytes in the future. Consider usingstd::uint16_torstd::size_tfor better future-proofing.- std::array<std::uint8_t, 3> constexpr static sizes = {52, 64, 76}; + // Total size in bytes of the Particle struct for dimensions 1, 2, and 3 + std::array<std::size_t, 3> constexpr static sizes = {52, 64, 76};
Line range hint
1-153: Consider additional improvements for overall code quality.While the code is generally well-structured, consider the following improvements:
- Add more comprehensive documentation, especially for template parameters and member functions.
- Consider grouping related functionality (e.g., particle operations) into separate namespaces or classes for better organization.
- Use
[[nodiscard]]attribute instead ofNO_DISCARDmacro for better standard compliance.- Consider adding unit tests for the
Particlestruct and related functions if not already present.src/core/utilities/box/box.hpp (1)
135-141: Good implementation, but consider some improvements.The
surface_cell_count()function is a useful addition. However, there are a few points to consider:
The assumption that the box is never smaller than 3 in any direction is not enforced. Consider adding a runtime check to ensure this assumption holds.
The comment on line 139 could be more descriptive. Explain why you're subtracting 2 from each dimension (i.e., to exclude the inner volume).
If this function is performance-critical, you might want to consider caching the result or using a more direct calculation method.
Consider updating the implementation like this:
NO_DISCARD auto surface_cell_count() const { // Ensure box is never smaller than 3 in any direction for (size_t i = 0; i < dim; ++i) { assert(upper[i] - lower[i] + 1 >= 3 && "Box must be at least 3 cells wide in each dimension"); } auto const shape_ = shape(); // Subtract 2 from each dimension to get the inner volume auto const inner_shape = shape_ - 2; return core::product(shape_) - core::product(inner_shape); }This implementation adds a runtime check for the minimum box size and clarifies the purpose of subtracting 2 from each dimension.
tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cpp (2)
90-90: Suggestion: Replace Magic Number with Named ConstantUsing a named constant instead of the hardcoded value
50improves readability and ease of maintenance.Consider defining a constant:
constexpr std::size_t gridSize = 50;And then initialize
layoutwith:: layout{gridSize}
112-112: Suggestion: Avoid Hardcoded Values in Capacity CalculationReplacing hardcoded values with variables enhances flexibility and maintainability.
Use variables derived from
layout:auto gridSize = layout.nbrCells()[0]; auto outer_cell_count = std::pow(gridSize, 2) - std::pow(gridSize - 2, 2);src/amr/data/particles/refine/particles_data_split.hpp (2)
108-108: Fix typographical errors in commentThe comment has typographical errors: "Finaly" should be "Finally", and "cartesion" should be "Cartesian".
182-190: Confirm the use of scope forincoming_estimatecalculationThe code block from lines 182 to 190 introduces a new scope for calculating
incoming_estimate. Ensure that this additional scope is necessary and that variables defined within are appropriately managed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- .github/workflows/cmake_ubuntu.yml (1 hunks)
- src/amr/data/particles/refine/particles_data_split.hpp (6 hunks)
- src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp (4 hunks)
- src/core/data/particles/particle.hpp (3 hunks)
- src/core/utilities/box/box.hpp (2 hunks)
- tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cpp (2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/amr/data/particles/refine/particles_data_split.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/data/particles/particle.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/utilities/box/box.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (15)
.github/workflows/cmake_ubuntu.yml (1)
81-85: Verify impact on CI/CD pipeline performance and consistencyThe changes to the build configuration may have significant effects on the CI/CD pipeline:
- Build time might be affected due to the conflicting Debug build type and -O3 optimization.
- Test execution speed could be unpredictable with this configuration.
- Resource usage on CI runners might increase, potentially leading to longer job durations or failures.
- The unusual build configuration might make it difficult to reproduce CI issues in local development environments.
To assess the impact of these changes, please run the following checks:
Please review the results to ensure that the changes do not negatively impact the CI/CD pipeline performance or introduce new issues.
src/core/data/particles/particle.hpp (1)
124-126: Improved readability of equality operator template.The reformatting of the
std::enable_if_tcondition improves code readability without changing functionality. This is a good change.src/core/utilities/box/box.hpp (1)
29-29: LGTM: Good addition of type alias.The new
value_typealias improves code readability and follows C++ best practices. It provides a clear representation of the underlying type used in theBoxstruct.tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cpp (5)
15-15: Approved: Inclusion of TestGridLayout HeaderThe addition of
test_gridlayout.hppis appropriate for utilizingTestGridLayoutin the tests.
76-78: Approved: Introduction of 2D Test NamespaceThe new namespace
PHARE::initializer::test_fn::func_2dlogically organizes the 2D test functions and maintains consistency with the existing structure.
79-84: Approved: Definition of 2D Particle Initializer Test ClassThe
AMaxwellianParticleInitializer2Dclass is correctly defined for testing 2D particle initialization, mirroring the structure of the existing 1D test class.
97-97: Clarify Member Variable Initialization OrderEnsure that member variables are declared in the same order they are initialized to prevent any potential issues with initialization order, especially if dependencies exist between them.
Verify that the declaration order matches the initialization order:
Declaration order:
TestGridLayout<GridLayoutT> layout; ParticleArrayT particles{layout.AMRBox()};Initialization order in constructor:
: layout{gridSize} , initializer{...}Since
particlesdepends onlayout, ensure that this order is intentional and functions correctly.
105-119: Duplicate Comment: Refactor Capacity Calculation and Improve Comment ClarityThe previous review comments about refining the capacity calculation and enhancing comment clarity are still applicable.
src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp (2)
47-47: Constructor parameter 'over_alloc_factor' added for configurabilityThe addition of the
over_alloc_factorparameter to the constructor enhances the configurability of the class, allowing users to adjust the over-allocation factor as needed.
57-57: Member variable 'over_alloc_factor_' correctly initializedThe member variable
over_alloc_factor_is appropriately initialized in the constructor initializer list with the provided parameter.src/amr/data/particles/refine/particles_data_split.hpp (5)
5-14: Includes added are appropriate and necessaryThe additional include statements provide necessary headers for the new code additions and improve modularity.
56-61: Addition of type alias and constexpr variable enhances code clarityDefining
Particle_tandputParticlesInCoarseBoundaryimproves code readability and maintainability by simplifying type references and clarifying conditional logic.
142-146: Verify conditional logic in_reservefunctionThe
_reservefunction only callsparticles.reserve(size)whenputParticlesInCoarseBoundaryisfalse. Please confirm that this is the intended behavior, ensuring that reservation is skipped when particles are placed in the coarse boundary.
207-207: Particle position refinement is correctly implementedUsing
toFineGridto computeparticleRefinedPosensures particles are accurately mapped to the fine grid for refinement.
211-211: Initialization and splitting of refined particlesInitializing
refinedParticlesand applying thesplitfunction correctly divides particles for the refinement process.
| -DCMAKE_BUILD_TYPE=Debug -Dasan=OFF \ | ||
| -DCMAKE_C_COMPILER_LAUNCHER=ccache \ | ||
| -DCMAKE_CXX_COMPILER_LAUNCHER=ccache \ | ||
| -DlowResourceTests=ON -DdevMode=ON -Dbench=ON \ | ||
| -DCMAKE_CXX_FLAGS="-DPHARE_DIAG_DOUBLES=1 " -Dphare_configurator=ON | ||
| -DCMAKE_CXX_FLAGS="-O3 -DPHARE_DIAG_DOUBLES=1 " -Dphare_configurator=ON |
There was a problem hiding this comment.
Conflicting build configuration settings
The changes in the CMake configuration present a potential conflict:
- The build type has been changed from
RelWithDebInfotoDebug(line 81). - An aggressive optimization flag
-O3has been added to theCMAKE_CXX_FLAGS(line 85).
These settings are contradictory:
- Debug builds typically include minimal optimizations to facilitate debugging.
- The
-O3flag enables maximum optimizations, which can interfere with debugging by reordering or optimizing out code.
This combination may lead to unexpected behavior during debugging and potentially mask issues that would be apparent in a true debug build.
Consider one of the following approaches:
- If debugging is the priority, remove the
-O3flag and keep theDebugbuild type. - If performance is crucial even for debugging, consider reverting to
RelWithDebInfobuild type, which balances optimization and debugging capabilities.
Please clarify the intended goal of these changes to determine the most appropriate configuration.
There was a problem hiding this comment.
RelWithDebInfo includes -DNDEBUG which disables asserts, we want asserts with some optimizations. if we want to debug this build we will change it later
There was a problem hiding this comment.
@PhilipDeegan, understood. Keeping asserts enabled with some optimizations makes sense in your context. I've noted this for future reviews.
(_/)
(•ᴥ•)💡
✏️ Learnings added
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#898
File: .github/workflows/cmake_ubuntu.yml:81-85
Timestamp: 2024-10-15T16:27:04.394Z
Learning: In this project, the `RelWithDebInfo` build type includes `-DNDEBUG`, which disables asserts. To keep asserts enabled while having some optimizations, the `Debug` build type is used with the `-O3` optimization flag.
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.
| 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 |
There was a problem hiding this comment.
🛠️ 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;| 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; | ||
| }); |
There was a problem hiding this comment.
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.
| 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; | |
| }); |
src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp
Outdated
Show resolved
Hide resolved
| std::array const particlesArrays{&srcInteriorParticles, &srcGhostParticles}; | ||
|
|
There was a problem hiding this comment.
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.
| std::array const particlesArrays{&srcInteriorParticles, &srcGhostParticles}; | |
| std::array<ParticleArray const*, 2> const particlesArrays{&srcInteriorParticles, &srcGhostParticles}; |
There was a problem hiding this comment.
it compiles without the templates, what makes you think it should have the templates?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- .github/workflows/cmake_ubuntu.yml (1 hunks)
- src/amr/data/field/refine/electric_field_refiner.hpp (2 hunks)
- src/amr/data/particles/refine/particles_data_split.hpp (6 hunks)
- src/amr/utilities/box/amr_box.hpp (2 hunks)
- src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp (4 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/amr/data/field/refine/electric_field_refiner.hpp
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/cmake_ubuntu.yml
- src/amr/data/particles/refine/particles_data_split.hpp
- src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp
🧰 Additional context used
📓 Path-based instructions (1)
src/amr/utilities/box/amr_box.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (1)
src/amr/utilities/box/amr_box.hpp (1)
5-9: LGTM: Include directives are well-organized.The new include directives are appropriately placed and follow a logical order from core definitions to more specific AMR-related headers. This organization enhances code readability and maintainability.
|
Todo refinement vector capacity test similar to maxwellian init |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (9)
tests/amr/data/particles/refine/test_split.cpp (2)
41-44: Approve new type alias and struct, suggest documentation.The addition of
BasicHierarchy_tandHierarchy_tprovides a good structure for the new test cases. This setup allows for more specific testing of the hierarchy with the given configuration.Consider adding brief comments to explain the purpose of these new types and the chosen template parameters for
BasicHierarchy_t. This would enhance code readability and maintainability.
46-52: Approve new test case, suggest enhancements.The new test case
checkCapacityPostRefinementis a valuable addition to verify the behavior of domain particles after refinement.Consider the following enhancements:
- Replace
assertwithASSERT_*orEXPECT_*macros from Google Test for better error reporting.- Add a brief comment explaining the expected behavior and why these specific values (2800, 4032) are important.
- Consider parameterizing the test for different refinement levels or particle counts to increase test coverage.
Example:
TEST_F(Hierarchy_t, checkCapacityPostRefinement) { // Verify that after refinement, the domain particles have the expected size and capacity auto domainParticles = domainParticlesForLevel(1); ASSERT_EQ(domainParticles.size(), 1); EXPECT_EQ(domainParticles[0]->size(), 2800); EXPECT_EQ(domainParticles[0]->capacity(), 4032); }tests/amr/data/particles/refine/test_particle_data_refine_tag_strategy.hpp (1)
45-50: Improved efficiency and const-correctness. Consider further optimization.The changes to the
loadCellfunction are good improvements:
- Modifying the passed array in-place instead of returning a new one is more efficient.
- Using
constfor variables that don't change is a good practice.However, consider further optimization:
void loadCell(ParticleArray<dimension>& particles, std::array<int, 3> const& cellIndices) { float const middle = 0.5f; float const delta = 0.30f; // ... rest of the function }This change would allow the compiler to optimize the function call by passing the cell indices directly as an array, potentially avoiding some copies.
tests/amr/data/particles/refine/test_particledata_refine_basic_hierarchy.hpp (2)
81-81: LGTM: Improved constructor flexibility with default parameterThe addition of a default value for the
_ratioparameter enhances the flexibility of theBasicHierarchyclass instantiation. This change maintains backward compatibility while allowing for easier use in scenarios where the default ratio is acceptable.Consider adding a brief comment explaining the significance of the default ratio value (2) to improve code documentation.
171-175: LGTM: New method added for accessing domain particlesThe addition of the
domainParticlesForLevelmethod provides a way to access domain particle information for a specific level, which enhances the functionality of theBasicHierarchyclass.Consider adding documentation for this method, including:
- A brief description of its purpose
- The meaning of the
levelNumberparameter- The type and meaning of the return value
The method appears to be a simple pass-through to
tagStrategy_->domainParticlesForLevel. Consider if this design is optimal:
- Could this functionality be accessed directly through
tagStrategy_?- If this method is necessary, would it be better to return a reference or pointer to
tagStrategy_instead, allowing more flexible use of its methods?Please review the design choice and ensure it aligns with the overall architecture and usage patterns of the
BasicHierarchyclass.src/amr/data/particles/refine/particles_data_split.hpp (4)
27-31: LGTM: New enum class for ParticlesDataSplitTypeThe addition of the
ParticlesDataSplitTypeenum class is a good improvement. It provides clear categorization for different types of particle data splits and enhances type safety. The use ofstd::uint8_tas the underlying type is space-efficient.Consider adding a brief comment explaining the purpose of each enum value for better clarity.
142-147: LGTM: New _reserve method for conditional space reservationThe addition of the
_reservemethod is a good optimization. It conditionally reserves space in the particle array based on theputParticlesInCoarseBoundaryflag, which can improve performance by reducing reallocation. The use ofif constexprfor compile-time branching is appropriate.Consider adding a brief comment explaining the purpose of this method and why reservation is only done for regular refinement.
181-196: LGTM: Improved particle count estimation and space reservationThe addition of
particlesArraysand thecount_expectedlambda function are good improvements. They provide a more accurate estimation of required space, which can enhance performance by reducing reallocation. The use of_reservewith this estimated count is an effective optimization.Consider adding a brief comment explaining the purpose and logic of the
count_expectedlambda for better code documentation.
Line range hint
213-265: LGTM: Improved particle refinement logicThe updated particle refinement logic is well-structured and efficient. The use of
std::arrayfor refined particles can improve performance compared to dynamic allocation. The conditional compilation for different split types allows for efficient code generation tailored to each case.Consider adding brief comments explaining the purpose of each major section within this block of code to improve readability for future maintainers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- src/amr/data/particles/refine/particles_data_split.hpp (6 hunks)
- tests/amr/data/particles/refine/test_particle_data_refine_tag_strategy.hpp (3 hunks)
- tests/amr/data/particles/refine/test_particledata_refine_basic_hierarchy.hpp (4 hunks)
- tests/amr/data/particles/refine/test_split.cpp (3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/amr/data/particles/refine/particles_data_split.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.tests/amr/data/particles/refine/test_particle_data_refine_tag_strategy.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.tests/amr/data/particles/refine/test_particledata_refine_basic_hierarchy.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (12)
tests/amr/data/particles/refine/test_split.cpp (3)
8-8: LGTM: Necessary include for the new test fixture.The addition of this include statement is appropriate as it brings in the necessary definitions for the
BasicHierarchyused in the new test fixture.
27-27: LGTM: Consistent update of Splitters definition.The Splitters type definition has been correctly updated to use the new
Splitter_talias, maintaining consistency with the earlier change.
17-17: Approve type alias change and suggest consistency.The change from
SplittertoSplitter_timproves code clarity. It's a good practice to use the_tsuffix for type aliases.Please ensure all occurrences of
Splitterare updated toSplitter_tthroughout the file for consistency. You can use the following command to check:✅ Verification successful
Type alias change verified successfully.
All active occurrences of
Splitterhave been updated toSplitter_t. Only commented code containsSplitter, which is acceptable.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
rg --type cpp 'Splitter(?!_t)' tests/amr/data/particles/refine/test_split.cppLength of output: 305
Script:
#!/bin/bash rg --pcre2 --type cpp 'Splitter(?!_t)' tests/amr/data/particles/refine/test_split.cppLength of output: 487
tests/amr/data/particles/refine/test_particle_data_refine_tag_strategy.hpp (2)
158-158: LGTM: Consistent update with loadCell function changeThe call to
loadCellhas been correctly updated to match its new signature. This change maintains the existing logic while adapting to the more efficient implementation ofloadCell.
202-214: New method added. Consider performance and safety improvements.The new
domainParticlesForLevelmethod provides a useful way to access particle data across all patches in a level. However, there are a few points to consider:
Performance: The use of
dynamic_castin a loop might impact performance if this method is called frequently. Consider if a static_cast would be sufficient here.Memory safety: Returning raw pointers could lead to ownership and lifetime issues. Consider returning
std::vector<std::reference_wrapper<ParticleArray<dimension>>>instead.Const-correctness: If this method doesn't modify the particle arrays, consider making it const and returning const references.
Here's a suggested improvement:
std::vector<std::reference_wrapper<const ParticleArray<dimension>>> domainParticlesForLevel(std::shared_ptr<SAMRAI::hier::PatchHierarchy> const& hierarchy, int levelNumber) const { std::vector<std::reference_wrapper<const ParticleArray<dimension>>> particle_arrays; auto level = hierarchy->getPatchLevel(levelNumber); for (auto const& patch : *level) for (auto const& [name, dataId] : dataToAllocate_) if (auto particlesData = static_cast<ParticlesData<ParticleArray<dimension>>*>( patch->getPatchData(dataId))) particle_arrays.emplace_back(particlesData->domainParticles); return particle_arrays; }Could you clarify the intended usage of this method? This would help in determining if these suggestions are appropriate for your use case.
✅ Verification successful
Verification of
domainParticlesForLevelUsageThe
domainParticlesForLevelmethod is currently only utilized within test files. Given its limited scope in testing scenarios, the performance and memory safety concerns are minimal. Therefore, the suggested improvements may not be necessary for the current usage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for uses of domainParticlesForLevel to understand its context rg "domainParticlesForLevel" --type cppLength of output: 602
tests/amr/data/particles/refine/test_particledata_refine_basic_hierarchy.hpp (2)
51-54: LGTM: Improved type aliases enhance code readabilityThe addition of
Splitter_tandParticlesRefineOperator_ttype aliases is a good practice. These aliases encapsulate complex type definitions, making the code more readable and easier to maintain. The use of template parameters also provides flexibility for different use cases.
104-104: LGTM: Simplified refineOperator_ instantiationThe use of the
ParticlesRefineOperator_ttype alias in therefineOperator_instantiation improves code readability and consistency. This change simplifies the code without altering its functionality, contributing to better maintainability.src/amr/data/particles/refine/particles_data_split.hpp (5)
5-6: LGTM: New header inclusionsThe addition of new headers for core definitions and utilities is appropriate. This change likely supports new functionality or helps in reducing dependencies in other files.
Also applies to: 8-8
12-14: LGTM: AMR-related header inclusionsThe addition of AMR-related utility headers is appropriate for this file's purpose of handling particle data in an AMR context.
56-61: LGTM: Type alias and compile-time constant additionsThe new type alias
Particle_timproves code readability. The static constexpr booleanputParticlesInCoarseBoundaryis a good use of compile-time constants for determining particle placement based on the split type. These additions enhance code clarity and potentially improve performance by allowing compile-time optimizations.
150-152: LGTM: Improved method documentationThe updated comment for the
refine_method provides a clearer and more specific description of its purpose. This improvement in documentation enhances code readability and maintainability.
204-206: LGTM: Optimized splitBox calculation and improved readabilityMoving the
splitBoxcalculation outside the loop is a good optimization that avoids redundant calculations. The introduction of theisInDestlambda function improves code readability by encapsulating the condition for checking if a particle is in the destination box. These changes enhance both performance and code clarity.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
tests/amr/data/particles/refine/test_particledata_refine_basic_hierarchy.hpp (1)
171-175: Good addition of domainParticlesForLevel method, but needs documentationThe new
domainParticlesForLevelmethod is a valuable addition to theBasicHierarchyclass. It provides a way to access domain particle information for a specific level, which enhances the class's functionality. The method correctly delegates the work to thetagStrategy_object, maintaining good separation of concerns.However, the method lacks documentation. To improve code maintainability and make it easier for other developers to understand and use this method, please add a documentation comment.
Consider adding a documentation comment for the
domainParticlesForLevelmethod. For example:/** * @brief Retrieves domain particles for a specific level in the hierarchy. * @param levelNumber The level number for which to retrieve domain particles. * @return The domain particles for the specified level. */ auto domainParticlesForLevel(int levelNumber) const { return tagStrategy_->domainParticlesForLevel(hierarchy_, levelNumber); }src/amr/data/particles/refine/particles_data_split.hpp (5)
5-14: LGTM! Consider grouping related headers.The new header inclusions are appropriate for the functionality of this file. However, consider grouping related headers together for better organization. For example:
// Core headers #include "core/def.hpp" #include "phare_core.hpp" #include "core/def/phare_mpi.hpp" #include "core/utilities/constants.hpp" // AMR headers #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" // Local headers #include "split.hpp"This grouping can improve readability and make it easier to manage dependencies.
56-61: LGTM! Consider adding a brief comment forputParticlesInCoarseBoundary.The additions of
Particle_tandputParticlesInCoarseBoundaryare excellent improvements:
- The
Particle_talias enhances code readability and maintainability.- The
putParticlesInCoarseBoundarystatic constexpr boolean encapsulates a complex condition, making the code more self-explanatory.To further improve clarity, consider adding a brief comment explaining the purpose of
putParticlesInCoarseBoundary:// Determines if particles should be placed in the coarse boundary based on the split type static constexpr bool putParticlesInCoarseBoundary = ...This comment would provide context for future maintainers.
142-147: LGTM! Consider adding a brief comment explaining the purpose of_reserve.The new
_reservemethod is a great addition:
- It improves performance by reserving memory for particles when needed.
- The use of perfect forwarding (
Fn&&) allows for efficient passing of the counter function.- The conditional reservation prevents unnecessary allocations for coarse boundary particles.
To enhance clarity, consider adding a brief comment explaining the method's purpose:
/** * @brief Reserves memory for particles in the given array. * @param particles The particle array to reserve memory for. * @param counter A function that returns the number of particles to reserve. * @note Only reserves for regular refinement of domain particles. */ template<typename Fn> static void _reserve(ParticleArray& particles, Fn&& counter)This documentation would provide context for future maintainers and users of the class.
183-196: LGTM! Consider extractingcount_expectedto a named function for better readability.The addition of
count_expectedlambda and its use with_reserveis an excellent optimization:
- It efficiently estimates the number of incoming particles.
- Using this estimate with
_reservecan significantly improve performance by reducing reallocations.To enhance readability and potentially reuse this logic, consider extracting the lambda to a named function:
std::size_t estimateIncomingParticles(const std::array<const ParticleArray*, 2>& particlesArrays, const SAMRAI::pdat::CellOverlap::BoxContainer& destBoxes) const { 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; } // Usage _reserve(destDomainParticles, [&]() { return estimateIncomingParticles(particlesArrays, destBoxes); });This change would improve code organization and make the estimation logic more reusable and testable.
Line range hint
204-265: LGTM! Consider further refactoring for improved readability and maintainability.The updates to the particle refinement logic are excellent improvements:
- The use of
std::copy_ifenhances both performance and readability.- The
isInDestlambda effectively encapsulates the condition for particle inclusion.- The logic for different split types is now more clearly separated.
To further improve the code, consider the following suggestions:
- Extract the particle copying logic into a separate method to reduce the nesting level:
template<typename DestArray> void copyRefinedParticles(const std::array<Particle_t, nbRefinedPart>& refinedParticles, DestArray& destArray, const auto& isInDest) { std::copy_if(std::begin(refinedParticles), std::end(refinedParticles), std::back_inserter(destArray), isInDest); } // Usage if constexpr (putParticlesInCoarseBoundary) { if constexpr (splitType == ParticlesDataSplitType::coarseBoundary) copyRefinedParticles(refinedParticles, destCoarseBoundaryParticles, isInDest); else if constexpr (splitType == ParticlesDataSplitType::coarseBoundaryOld) copyRefinedParticles(refinedParticles, destCoarseBoundaryOldParticles, isInDest); else copyRefinedParticles(refinedParticles, destCoarseBoundaryNewParticles, isInDest); } else { copyRefinedParticles(refinedParticles, destDomainParticles, isInDest); }
Consider using a switch statement for the split type logic if there are more than these three cases.
Remove or replace the commented-out
std::coutstatements with proper logging if debugging information is needed.These changes would further enhance the code's readability and maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- src/amr/data/particles/refine/particles_data_split.hpp (6 hunks)
- tests/amr/data/particles/refine/test_particle_data_refine_tag_strategy.hpp (3 hunks)
- tests/amr/data/particles/refine/test_particledata_refine_basic_hierarchy.hpp (4 hunks)
- tests/amr/data/particles/refine/test_split.cpp (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/amr/data/particles/refine/test_particle_data_refine_tag_strategy.hpp
- tests/amr/data/particles/refine/test_split.cpp
🧰 Additional context used
📓 Path-based instructions (2)
src/amr/data/particles/refine/particles_data_split.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.tests/amr/data/particles/refine/test_particledata_refine_basic_hierarchy.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (4)
tests/amr/data/particles/refine/test_particledata_refine_basic_hierarchy.hpp (3)
51-54: Excellent addition of type aliases for improved readabilityThe introduction of
Splitter_tandParticlesRefineOperator_ttype aliases is a great improvement. These aliases encapsulate complex type information, making the code more readable and easier to maintain. The meaningful names clearly indicate their purpose, which will help developers understand the code more quickly.
104-104: Good use of the new type alias in refineOperator_ initializationThe modification to use
ParticlesRefineOperator_tin the initialization ofrefineOperator_is a positive change. It simplifies the code, improves readability, and maintains consistency with the newly introduced type aliases. This change enhances code maintainability without altering the underlying functionality.
81-81: Good addition of default ratio, but verify existing usageAdding a default value of 2 for the
_ratioparameter increases the flexibility of theBasicHierarchyconstructor. This change allows the constructor to be called without explicitly providing a ratio, which can be convenient in some scenarios.However, it's important to ensure that this change doesn't lead to unexpected behavior in existing code that may rely on explicitly setting the ratio.
Please verify that existing code using this constructor is not affected by this change. You may want to search for all instances where
BasicHierarchyis instantiated to ensure they behave as expected with this new default value.✅ Verification successful
Default
_ratioparameter addition verifiedThe addition of a default value of
2for the_ratioparameter in theBasicHierarchyconstructor has been reviewed. The only instantiation found is within a test file, and using the default value does not introduce issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for BasicHierarchy instantiations rg --type cpp "BasicHierarchy\s*<.*>\s*\([^)]*\)" -g '!test_particledata_refine_basic_hierarchy.hpp'Length of output: 254
src/amr/data/particles/refine/particles_data_split.hpp (1)
27-31: LGTM! Good improvements to the enum class.The changes to the
ParticlesDataSplitTypeenum class are beneficial:
- Specifying
std::uint8_tas the underlying type improves type safety and reduces memory usage.- Explicitly setting
coarseBoundary = 0ensures a consistent starting value and makes the enum's behavior more predictable.These modifications enhance the code's clarity and efficiency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
tests/amr/data/particles/refine/test_particle_data_refine_tag_strategy.hpp (2)
45-50: Improved function signature and const correctness.The changes to the
loadCellfunction are well-implemented:
- The new signature with
voidreturn type andParticleArray<dimension>& particlesparameter improves efficiency by modifying the array in-place.- The use of
constfor_3diCell,middle, anddeltaenhances code clarity and prevents accidental modifications.These changes align with good C++ practices and should improve performance slightly.
Consider using
constexprformiddleanddeltaif their values are known at compile-time:constexpr double middle = 0.5; constexpr double delta = 0.30;
202-214: New methoddomainParticlesForLeveladded.The new
domainParticlesForLevelmethod is a useful addition for accessing domain particles at a specific level. It correctly iterates through patches and collects the necessary particle arrays.Consider returning
std::vector<std::reference_wrapper<ParticleArray<dimension>>>instead of raw pointers to improve safety and prevent potential issues with dangling pointers:auto domainParticlesForLevel(std::shared_ptr<SAMRAI::hier::PatchHierarchy> const& hierarchy, int levelNumber) { std::vector<std::reference_wrapper<ParticleArray<dimension>>> particle_arrays; auto level = hierarchy->getPatchLevel(levelNumber); for (auto& patch : *level) for (auto const& [name, dataId] : dataToAllocate_) particle_arrays.emplace_back( std::dynamic_pointer_cast<ParticlesData<ParticleArray<dimension>>>( patch->getPatchData(dataId)) ->domainParticles); return particle_arrays; }This change ensures that the caller can't accidentally delete the particle arrays and provides a safer interface.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- tests/amr/data/particles/refine/test_particle_data_refine_tag_strategy.hpp (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tests/amr/data/particles/refine/test_particle_data_refine_tag_strategy.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
📓 Learnings (1)
📓 Common learnings
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.455Z Learning: The codebase uses C++17 features such as class template argument deduction, allowing template arguments to be omitted in declarations like `std::array`.
🔇 Additional comments (1)
tests/amr/data/particles/refine/test_particle_data_refine_tag_strategy.hpp (1)
158-158: SimplifiedloadCellcall ininitializeLevelData.The changes in the
initializeLevelDatamethod correctly implement the newloadCellfunction signature. The simplification of theloadCellcall improves code readability while maintaining the existing logic for iterating over cells.
| EXPECT_EQ(domainParticles[0]->size(), 2800); | ||
| EXPECT_EQ(domainParticles[0]->capacity(), 4032); |
There was a problem hiding this comment.
diff still unexplained.
to be considered also https://github.com/PhilipDeegan/PHARE/blob/mkn/src/core/vector.hpp#L337-L370
Summary by CodeRabbit
New Features
Improvements
Refactor