minimize save/restore allocations #909
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on enhancing 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.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (9)
src/core/def.hpp (3)
Line range hint
28-36: Consider adding compile-time checks and documentationThe
isUsablefunction is well-implemented using modern C++ features. However, consider the following improvements:
Add a compile-time check to ensure non-pointer types have an
isUsable()method. This can prevent cryptic compile errors if a type withoutisUsable()is passed.Add documentation explaining the function's purpose, usage, and requirements for the argument types.
Here's a suggested implementation with compile-time checks and documentation:
/** * @brief Checks if all provided arguments are usable. * * For pointer types, checks if the pointer is not null. * For non-pointer types, calls the isUsable() method. * * @tparam Args Variadic template parameter pack * @param args Arguments to check * @return true if all arguments are usable, false otherwise */ NO_DISCARD bool isUsable(auto const&... args) { auto check = [](auto const& arg) { if constexpr (std::is_pointer_v<std::decay_t<decltype(arg)>>) return arg != nullptr; else { static_assert(requires { arg.isUsable(); }, "Non-pointer types must have an isUsable() method"); return arg.isUsable(); } }; return (check(args) && ...); }This implementation adds a
static_assertto ensure non-pointer types have anisUsable()method, and includes documentation for better understanding and maintainability.
Line range hint
39-47: ImproveisSettableimplementation and consider refactoringThe
isSettablefunction is similar toisUsable, but with some key differences. Consider the following improvements:
- Add a compile-time check for the
isSettable()method on non-pointer types.- Add documentation explaining the function's purpose, usage, and the difference in pointer behavior compared to
isUsable.- Consider refactoring to reduce code duplication between
isUsableandisSettable.Here's a suggested implementation with improvements:
/** * @brief Checks if all provided arguments are settable. * * For pointer types, checks if the pointer is null. * For non-pointer types, calls the isSettable() method. * * @note This function considers null pointers as settable, * which is the opposite of isUsable(). * * @tparam Args Variadic template parameter pack * @param args Arguments to check * @return true if all arguments are settable, false otherwise */ NO_DISCARD bool isSettable(auto const&... args) { auto check = [](auto const& arg) { if constexpr (std::is_pointer_v<std::decay_t<decltype(arg)>>) return arg == nullptr; else { static_assert(requires { arg.isSettable(); }, "Non-pointer types must have an isSettable() method"); return arg.isSettable(); } }; return (check(args) && ...); }To reduce code duplication, consider creating a helper template function:
template<bool (CheckFunc)(auto const&)> NO_DISCARD bool checkAll(auto const&... args) { return (CheckFunc(args) && ...); } NO_DISCARD bool isUsable(auto const&... args) { return checkAll<[](auto const& arg) { if constexpr (std::is_pointer_v<std::decay_t<decltype(arg)>>) return arg != nullptr; else { static_assert(requires { arg.isUsable(); }, "Non-pointer types must have an isUsable() method"); return arg.isUsable(); } }>(args...); } NO_DISCARD bool isSettable(auto const&... args) { return checkAll<[](auto const& arg) { if constexpr (std::is_pointer_v<std::decay_t<decltype(arg)>>) return arg == nullptr; else { static_assert(requires { arg.isSettable(); }, "Non-pointer types must have an isSettable() method"); return arg.isSettable(); } }>(args...); }This refactoring reduces code duplication while maintaining the specific logic for each function.
Line range hint
1-48: Approve overall structure with minor suggestionThe overall structure of the file is well-organized, with macro definitions at the top and function definitions within the
PHARE::corenamespace. The use of macros for conditional compilation and string manipulation is appropriate for a C++ project.For consistency, consider adding a blank line before the closing brace of the namespace (line 47). This improves readability by clearly separating the namespace content from its closing brace.
src/core/data/particles/particle_array.hpp (4)
44-44: LGTM with a minor suggestion.The updated constructor with default parameters enhances flexibility in object creation. However, consider adding a comment explaining the implications of using default values, especially for the
boxparameter, to prevent unintended usage.
76-77: LGTM with a documentation suggestion.The new
data()functions provide useful direct access to the underlying particle data. However, to ensure proper usage:
- Add documentation comments explaining the purpose and any potential risks of using these functions.
- Consider marking these functions with
[[nodiscard]]instead ofNO_DISCARDfor better standard compliance.Example:
[[nodiscard]] auto data() const { return particles_.data(); } [[nodiscard]] auto data() { return particles_.data(); }
234-234: LGTM with documentation and encapsulation considerations.The
map()function provides access to the internalcellMap_, which can be useful for certain operations. However:
- Add a documentation comment explaining the purpose and usage of this function.
- Consider if exposing the internal
cellMap_directly is necessary, or if specific operations on the map could be provided as member functions instead, to maintain better encapsulation.Example documentation:
/** * @brief Get a const reference to the internal cell map. * @return Const reference to the CellMap_t object. * @note Use with caution as it exposes internal implementation details. */ auto& map() const { return cellMap_; }
247-254: LGTM with suggestions for error checking and documentation.The new
replace_fromoverload provides an efficient way to replace particle data from external sources. However, consider the following improvements:
- Add input validation to ensure
sizeis not zero andparticlesis not null.- Consider adding a debug assertion to check if the provided
mapsize matches thesizeparameter.- Add a documentation comment explaining the purpose, parameters, and any assumptions about the input data.
Example improvements:
/** * @brief Replace the particle array contents with provided data. * @param particles Pointer to an array of Particle_t objects. * @param size Number of particles in the array. * @param map CellMap_t object corresponding to the particle data. * @return Reference to this ParticleArray object. * @throw std::invalid_argument if particles is null or size is 0. */ auto& replace_from(Particle_t const* particles, std::size_t size, CellMap_t const& map) { if (particles == nullptr || size == 0) throw std::invalid_argument("Invalid input: null pointer or zero size"); assert(size == map.size() && "Size mismatch between particle data and cell map"); this->resize(size); this->cellMap_ = map; this->box_ = map.box(); std::copy(particles, particles + size, particles_.data()); return *this; }src/core/vector.hpp (2)
47-47: Use a more descriptive variable name for_cThe variable
_cserves as a counter for tracking consecutive low-usage instances but is not descriptively named.Line 47:
std::uint16_t _c = 0;Consider renaming
_cto something more descriptive, such aslowUsageCounterorconsecutiveLowUsageCount, to enhance code readability and maintainability.
15-34: Add comments to clarify resizing logic for better maintainabilityThe logic within the
getfunction is complex and may not be immediately clear to other developers.Lines 15-34:
template<bool copy_old = copy_old_> auto& get(std::size_t const& s) { // Resizing logic... }Adding comments that explain:
- The purpose of
_c,percentile,period, andrealloc_to.- How the resizing mechanism works.
- The conditions under which the vector is resized.
This will greatly aid in understanding and maintaining the code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- src/amr/solvers/solver_ppc.hpp (6 hunks)
- src/core/data/particles/particle_array.hpp (4 hunks)
- src/core/def.hpp (1 hunks)
- src/core/utilities/cellmap.hpp (1 hunks)
- src/core/vector.hpp (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/amr/solvers/solver_ppc.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_array.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/def.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/utilities/cellmap.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/vector.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (9)
src/core/data/particles/particle_array.hpp (1)
Line range hint
1-424: Overall assessment: Changes align with PR objectives and improve class functionality.The modifications to the
ParticleArrayclass enhance its flexibility and efficiency, which aligns well with the PR objective of minimizing save/restore allocations. The new constructor signature, data access methods, andreplace_fromoverload provide more efficient ways to create, access, and manipulate particle data.Key improvements:
- More flexible object creation with default constructor parameters.
- Direct access to underlying data for performance-critical operations.
- Efficient replacement of particle data from external sources.
These changes should contribute to the reported performance improvements in the harris algorithm execution.
To further improve the code:
- Add comprehensive documentation for new and modified functions.
- Consider additional error checking and input validation where appropriate.
- Evaluate any potential impacts on encapsulation and consider if any additional abstraction layers are needed.
src/amr/solvers/solver_ppc.hpp (8)
8-8: Include necessary header forcore::MinimizingVectorIncluding
"core/vector.hpp"is appropriate ascore::MinimizingVectoris utilized in theSaveStatestruct.
22-22: Includecellmap.hppforcore::CellMapusageThe inclusion of
"core/utilities/cellmap.hpp"is necessary for thecore::CellMaptype used within theSaveStatestruct.
28-28: Include<tuple>for usage ofstd::forward_as_tupleAdding
#include <tuple>ensures thatstd::forward_as_tupleused inSaveStateis properly declared.
135-135: Improved const-correctness insaveState_methodChanging the parameters of
saveState_toconst&enhances safety by preventing unintended modifications tolevelandviews.
153-177: Introduction ofSaveStatestruct enhances state managementThe new
SaveStatestruct provides a structured and efficient approach to managing particle states, which should reduce memory allocations and improve performance during save and restore operations.
179-180: Extend lifespan of saved states appropriatelyDefining
domainStateandpatchGhostStateat the class scope ensures that the saved states persist as needed between method calls.
227-265: Efficient state saving insaveState_methodThe refactored
saveState_method efficiently captures the state of particles by using theSaveStatestruct and minimizes memory allocations, aligning with the PR objectives to enhance performance.
275-290: Accurate restoration of state inrestoreState_methodThe
restoreState_method correctly restores the saved particle states fromSaveState, ensuring system integrity and consistency after operations.
| @@ -31,7 +31,7 @@ class CellMap | |||
|
|
|||
|
|
|||
| public: | |||
| CellMap(Box<cell_index_t, dim> box) | |||
| CellMap(Box<cell_index_t, dim> box = {}) | |||
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Potential issues with empty Box usage in CellMap
The recent change allows creating a CellMap instance without providing a Box, which can result in an empty Box. However, several parts of the codebase use box_ without verifying if it is empty:
loc[i] -= box_.lower[i];(line 214)cellIndexes_{box.shape().toArray<std::uint32_t>()}(line 36)
Only one existing check for an empty Box is found at line 228. This inconsiderate handling may lead to undefined behavior or runtime errors when Box is empty.
Recommendations:
- Add Null Checks: Ensure all methods accessing
box_verify that it is not empty before usage. - Handle Initialization Safely: Modify the initialization of
cellIndexes_to account for an emptyBox. - Comprehensive Testing: Implement tests to cover scenarios where
CellMapis initialized with an emptyBox.
🔗 Analysis chain
Consider the implications of allowing default construction with an empty Box
The addition of a default argument {} for the box parameter allows creating a CellMap instance without explicitly providing a Box. While this can simplify usage in some cases, it may lead to unintended behavior if the code assumes a non-empty Box.
Consider the following:
- Ensure that all methods using
box_(e.g.,addToCell,size) can handle an emptyBoxcorrectly. - Review the initialization of
cellIndexes_usingbox.shape()to ensure it behaves correctly with an emptyBox. - Add documentation to clarify the behavior and potential limitations of using an empty
Box. - Consider adding an
is_valid()or similar method to check if theCellMaphas been properly initialized with a non-emptyBox.
To verify the impact of this change, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential issues with empty Box usage
# Test: Search for methods that use box_ member
echo "Methods potentially affected by empty Box:"
rg --type cpp -n 'box_\.' src/core/utilities/cellmap.hpp
# Test: Check cellIndexes_ initialization
echo "\ncellIndexes_ initialization:"
rg --type cpp -n 'cellIndexes_.*box' src/core/utilities/cellmap.hpp
# Test: Search for existing null checks or empty box handling
echo "\nExisting empty box handling:"
rg --type cpp -n 'box_\.isEmpty\(\)|box_\.empty\(\)' src/core/utilities/cellmap.hpp
Length of output: 679
| std::copy(dInParts.data(), dInParts.data() + dSizes[arr_idx], dParts.data() + doff); | ||
| std::copy(pInParts.data(), pInParts.data() + pSizes[arr_idx], pParts.data() + poff); |
There was a problem hiding this comment.
Ensure safe copying of particle data with capacity checks
When using std::copy to transfer particle data, it's important to ensure that the destination buffers have sufficient capacity to prevent buffer overflows. Consider adding assertions to verify the capacities before copying.
Apply this diff to add capacity checks:
+ assert((doff + dSizes[arr_idx]) <= dParts.capacity());
+ assert((poff + pSizes[arr_idx]) <= pParts.capacity());
std::copy(dInParts.data(), dInParts.data() + dSizes[arr_idx], dParts.data() + doff);
std::copy(pInParts.data(), pInParts.data() + pSizes[arr_idx], pParts.data() + poff);📝 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::copy(dInParts.data(), dInParts.data() + dSizes[arr_idx], dParts.data() + doff); | |
| std::copy(pInParts.data(), pInParts.data() + pSizes[arr_idx], pParts.data() + poff); | |
| assert((doff + dSizes[arr_idx]) <= dParts.capacity()); | |
| assert((poff + pSizes[arr_idx]) <= pParts.capacity()); | |
| std::copy(dInParts.data(), dInParts.data() + dSizes[arr_idx], dParts.data() + doff); | |
| std::copy(pInParts.data(), pInParts.data() + pSizes[arr_idx], pParts.data() + poff); |
src/amr/solvers/solver_ppc.hpp
Outdated
| auto operator()() { return std::forward_as_tuple(particles(), sizes(), maps()); } | ||
| auto operator()(std::size_t nparts, std::size_t narrays) | ||
| { | ||
| if (nparts < particles.capacity()) | ||
| return std::forward_as_tuple(particles(nparts), sizes(narrays), maps(narrays)); | ||
| narrays += narrays * .01; | ||
| nparts += nparts * .001; | ||
| return std::forward_as_tuple(particles(nparts), sizes(narrays), maps(narrays)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider increasing capacity growth factors for better performance
In SaveState::operator(), when nparts is not less than particles.capacity(), the increments of nparts += nparts * .001 and narrays += narrays * .01 may not sufficiently increase capacity to prevent frequent reallocations. Consider using larger growth factors, such as doubling the sizes, to optimize performance with large datasets.
Apply this diff to adjust the capacity growth factors:
- narrays += narrays * .01;
- nparts += nparts * .001;
+ narrays = narrays * 2; // Double narrays
+ nparts = nparts * 2; // Double nparts📝 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 operator()() { return std::forward_as_tuple(particles(), sizes(), maps()); } | |
| auto operator()(std::size_t nparts, std::size_t narrays) | |
| { | |
| if (nparts < particles.capacity()) | |
| return std::forward_as_tuple(particles(nparts), sizes(narrays), maps(narrays)); | |
| narrays += narrays * .01; | |
| nparts += nparts * .001; | |
| return std::forward_as_tuple(particles(nparts), sizes(narrays), maps(narrays)); | |
| } | |
| auto operator()() { return std::forward_as_tuple(particles(), sizes(), maps()); } | |
| auto operator()(std::size_t nparts, std::size_t narrays) | |
| { | |
| if (nparts < particles.capacity()) | |
| return std::forward_as_tuple(particles(nparts), sizes(narrays), maps(narrays)); | |
| narrays = narrays * 2; // Double narrays | |
| nparts = nparts * 2; // Double nparts | |
| return std::forward_as_tuple(particles(nparts), sizes(narrays), maps(narrays)); | |
| } |
There was a problem hiding this comment.
imagine, nparts is very large, then consider what you're suggested.
There was a problem hiding this comment.
@PhilipDeegan, that's a valid point. Doubling nparts when it's very large could lead to excessive memory usage or undesired behavior. Thank you for pointing this out.
(*´-`)
✏️ Learnings added
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#909
File: src/amr/solvers/solver_ppc.hpp:164-172
Timestamp: 2024-10-17T12:43:49.581Z
Learning: When `nparts` is very large, doubling the capacity in `SaveState::operator()` may cause excessive memory usage or overflows. It's important to consider appropriate growth factors to prevent large memory allocations.
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.
| template<bool copy_old = copy_old_> | ||
| auto& get(std::size_t const& s) | ||
| { | ||
| if (s < v.capacity() * percentile) |
There was a problem hiding this comment.
Potential precision loss due to implicit conversion in comparison
In line 18:
if (s < v.capacity() * percentile)The expression v.capacity() * percentile results in a double, while s is a std::size_t (an unsigned integer). Comparing an integer to a floating-point number can lead to unexpected behavior due to implicit conversion and precision loss.
Consider explicitly casting the result to std::size_t to ensure correct comparison:
if (s < static_cast<std::size_t>(v.capacity() * percentile))This ensures that both sides of the comparison are of the same type, avoiding potential precision issues.
|
|
||
| if (_c == period) | ||
| { | ||
| std::vector<T> r(v.capacity() * realloc_to); |
There was a problem hiding this comment.
Implicit conversion from double to size_t may cause truncation
In line 25:
std::vector<T> r(v.capacity() * realloc_to);The expression v.capacity() * realloc_to produces a double, but the std::vector constructor expects a size_t for its size parameter. Implicitly converting a double to a size_t can truncate the value, possibly leading to smaller allocations than intended.
Apply this fix to ensure the size is correctly calculated:
- std::vector<T> r(v.capacity() * realloc_to);
+ std::vector<T> r(static_cast<std::size_t>(v.capacity() * realloc_to));Alternatively, you can use rounding functions if partial capacities should influence the size:
std::vector<T> r(static_cast<std::size_t>(std::ceil(v.capacity() * realloc_to)));| auto& operator()() { return v; } | ||
| auto& operator()() const { return v; } | ||
| auto& operator()(std::size_t const& s) { return get(s); } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Overloading operator() may reduce code readability
Overloading the function call operator to access or modify the internal vector can be confusing, as it's unconventional for object manipulation.
Lines 38-40:
auto& operator()() { return v; }
auto& operator()() const { return v; }
auto& operator()(std::size_t const& s) { return get(s); }Consider using explicit member function names for clarity:
- Replace
operator()()withdata()to access the vector. - Replace
operator()(std::size_t const& s)withresize(s)or retainget(s).
Refactored example:
auto& data() { return v; }
auto& data() const { return v; }
auto& resize(std::size_t const& s) { return get(s); }This makes the interface more intuitive and improves code expressiveness.
PhilipDeegan
force-pushed
the
save_vec
branch
from
e576417
to
3c384a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
src/core/data/particles/particle_array.hpp (4)
Line range hint
44-49: LGTM: Constructor update improves flexibility.The updated constructor with default parameters allows for more flexible instantiation of
ParticleArray. This change simplifies usage in some scenarios while maintaining the necessary checks for valid initialization.Consider adding a comment explaining the significance of the
box_size assertion to improve code readability and maintainability.
76-78: LGTM: New data() functions enhance flexibility.The addition of
data()member functions, both const and non-const, provides direct access to the underlying particle data. This enhances the flexibility of theParticleArrayclass. The use of theNO_DISCARDattribute is a good practice to prevent accidental value discard.Consider adding brief documentation comments for these functions to clarify their purpose and any potential risks associated with direct data access.
247-254: LGTM: New replace_from overload enhances functionality.The new
replace_fromoverload provides an efficient way to replace the entire particle array from raw data. It correctly resizes the array, updates the cell map, and copies the particles usingstd::copy.Consider adding bounds checking or assertions to ensure that the provided
sizematches the size of themap. This would help prevent potential buffer overruns or inconsistencies between the particle data and the cell map.
Line range hint
1-424: Overall: Solid improvements to ParticleArray class.The changes to the
ParticleArrayclass enhance its flexibility and functionality without introducing major architectural changes or performance issues. The new constructor signature,data()functions,map()function, andreplace_fromoverload are well-integrated and consistent with the existing code style.These modifications provide more options for initializing and manipulating particle data, which should improve the usability of the class in various scenarios. The changes maintain good const correctness and don't appear to introduce any thread safety issues.
As the
ParticleArrayclass continues to evolve, consider documenting the thread safety guarantees (or lack thereof) for the class. This will be valuable for users of the class, especially in parallel computing contexts often found in particle simulations.src/amr/solvers/solver_ppc.hpp (3)
153-175:SaveStatestruct looks well-designed, consider adding documentation.The
SaveStatestruct effectively encapsulates particle state data usingcore::MinimizingVectorfor optimized memory usage. Theoperator()overloads provide flexible access to the stored data.Consider adding brief documentation comments to explain the purpose of the struct and its members, especially the significance of the
copy_oldflag.
225-262: ImprovedsaveState_method, consider adding capacity checks.The refactored
saveState_method effectively uses the newSaveStatestruct and improves const-correctness. The use ofstd::copyfor transferring particle data is efficient.However, consider adding explicit capacity checks before copying data to prevent potential buffer overflows:
+ assert(doff + dSizes[arr_idx] <= dParts.capacity()); + assert(poff + pSizes[arr_idx] <= pParts.capacity()); std::copy(dInParts.data(), dInParts.data() + dSizes[arr_idx], dParts.data() + doff); std::copy(pInParts.data(), pInParts.data() + pSizes[arr_idx], pParts.data() + poff);
Line range hint
153-287: Effective refactoring for optimized particle data management.The introduction of the
SaveStatestruct and the refactoring ofsaveState_andrestoreState_methods significantly improve the particle data management in theSolverPPCclass. These changes likely contribute to better memory efficiency and performance, aligning well with the PR objective of minimizing save/restore allocations.The refactoring maintains the overall functionality of the class while introducing a more structured approach to handling particle data. This should lead to improved maintainability and potentially better performance in particle-heavy simulations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- src/amr/solvers/solver_ppc.hpp (6 hunks)
- src/core/data/particles/particle_array.hpp (4 hunks)
- src/core/def.hpp (1 hunks)
- src/core/utilities/cellmap.hpp (1 hunks)
- src/core/vector.hpp (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/def.hpp
- src/core/utilities/cellmap.hpp
- src/core/vector.hpp
🧰 Additional context used
📓 Path-based instructions (2)
src/amr/solvers/solver_ppc.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_array.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (3)
src/core/data/particles/particle_array.hpp (1)
234-234: LGTM: New map() function improves accessibility.The addition of the
map()member function enhances the accessibility of the cell mapping functionality. It provides const access to the internalcellMap_member, which is consistent with good encapsulation practices.src/amr/solvers/solver_ppc.hpp (2)
8-8: New includes look good.The addition of
core/vector.hpp,core/utilities/cellmap.hpp, and<tuple>headers suggests the introduction of new data structures and utilities, which aligns with the changes in the code. These inclusions are appropriate for the refactoring that has been done.Also applies to: 22-22, 28-28
270-287:restoreState_method looks good.The refactored
restoreState_method effectively uses theSaveStatestruct to restore particle data. The use of thereplace_frommethod for both domain and patch ghost particles is appropriate and likely optimized for this use case. The method correctly handles the restoration of particle data for multiple populations.
|
experimental |
master 800**2 harris for 10 timesteps
perf stat mpirun -n 8 python3 -Ou harris.pyvs this PR
Summary by CodeRabbit
New Features
SaveStatestruct for improved state management of particles.ParticleArrayconstructor for easier instantiation.isUsableandisSettableadded for enhanced usability checks.CellMapconstructor.MinimizingVectorstruct for efficient dynamic vector management.Bug Fixes