some refactoring of messenger stuff and cleaning here and there #757
Conversation
WalkthroughThe changes primarily focus on refining the codebase for better clarity, consistency, and performance. Key modifications include the introduction of new classes ( Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 19
Files selected for processing (15)
- src/amr/CMakeLists.txt (1 hunks)
- src/amr/messenger_registration.hpp (2 hunks)
- src/amr/messengers/communicator.hpp (4 hunks)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (30 hunks)
- src/amr/messengers/hybrid_messenger_info.hpp (2 hunks)
- src/amr/messengers/refiner.hpp (1 hunks)
- src/amr/messengers/refiner_pool.hpp (1 hunks)
- src/amr/messengers/synchronizer.hpp (1 hunks)
- src/amr/messengers/synchronizer_pool.hpp (3 hunks)
- src/amr/physical_models/hybrid_model.hpp (2 hunks)
- src/amr/solvers/solver_ppc.hpp (1 hunks)
- src/amr/wrappers/hierarchy.hpp (1 hunks)
- src/core/data/vecfield/vecfield.hpp (2 hunks)
- src/phare_amr.hpp (3 hunks)
- src/simulator/phare_types.hpp (1 hunks)
Files skipped from review due to trivial changes (5)
- src/amr/CMakeLists.txt
- src/amr/messenger_registration.hpp
- src/amr/messengers/communicator.hpp
- src/amr/messengers/hybrid_messenger_info.hpp
- src/simulator/phare_types.hpp
Additional comments (Suppressed): 23
src/amr/physical_models/hybrid_model.hpp (2)
14-15: The inclusion of
vecfield.hppis new in this hunk. Ensure that this new dependency does not introduce circular dependencies or other issues.140-168: The
fillMessengerInfofunction has been updated. TheVecFieldDescriptortype has been replaced withVecFieldNamesfor several member variables ofhybridInfo. This change should be verified across the codebase to ensure that it doesn't break any existing functionality. Also, ensure that thedynamic_castat line 143 is safe and thatinfocan always be cast toHybridMessengerInfo. If not, this could lead to astd::bad_castexception.src/phare_amr.hpp (3)
1-3: No changes were made in this hunk. The include guard is correctly defined.
26-33: Two new headers have been included:
"amr/data/particles/refine/splitter.hpp"and"amr/data/particles/refine/particles_data_split.hpp". Ensure that these files exist and are in the correct location.42-55: The type alias
Splitterhas been renamed toSplitter_t. TheRefinementParamstype alias has been updated to useSplitter_tinstead ofSplitter. Ensure that all references toSplitterin the codebase have been updated toSplitter_t.src/amr/wrappers/hierarchy.hpp (1)
- 23-25: The
logger.hppheader file has been removed and replaced withmeta_utilities.hpp. Ensure that logging functionality is not required in this file, or if it is, that it's being handled appropriately by the new header.src/amr/solvers/solver_ppc.hpp (1)
- 161-171: The
fillMessengerInfofunction has been updated to use the newVecFieldNamesstruct instead of the oldVecFieldDescriptor. Ensure that this change is compatible with the rest of the codebase and that all instances ofVecFieldDescriptorhave been replaced withVecFieldNames. Also, the variableEpredis no longer used in this function. If it's not used elsewhere, consider removing it to avoid unused variable warnings.- auto const& Epred = electromagPred_.E;src/amr/messengers/synchronizer_pool.hpp (3)
42-47: The
registerLevelfunction has been simplified. Ensure that the removal of thelevelNumberandcoarseLevelvariables and the loop oversynchronizer.algosdoes not affect the functionality of the code. If these variables and the loop are necessary for the correct operation of the code, they should be reintroduced.52-57: The
syncfunction has been simplified. Ensure that the removal of the check for emptysynchronizer.algosand the loop oversynchronizer.algosdoes not affect the functionality of the code. If these checks and the loop are necessary for the correct operation of the code, they should be reintroduced.59-62: The constructor for
SynchronizerPoolhas been added. This is a good practice as it allows for better resource management and initialization of member variables.src/amr/messengers/refiner_pool.hpp (1)
- 148-149: The
RefinerPoolclass stores astd::mapof refiners and a shared pointer to aResourcesManager. If theResourcesManageris shared among multipleRefinerPoolinstances, changes to theResourcesManagerin oneRefinerPoolwill affect all otherRefinerPoolinstances that share the sameResourcesManager. Ensure that this is the intended behavior.Overall, the new code is well-structured and follows good practices such as using smart pointers and namespaces. The use of templates and type traits allows for flexibility and type safety. The code is also well-documented with clear comments explaining the purpose and usage of each function.
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (12)
11-29: The new hunk has added more include statements for different modules. Ensure that these modules are necessary for the current file and that they are available in the project. Also, check if these new modules introduce any circular dependencies.
36-42: The new hunk has added an include statement for the string library. Ensure that this library is necessary for the current file and that it is available in the project.
52-68: The new hunk has added a namespace qualifier
PHARE::core::toHybridQuantity::Scalarin thePropertystruct andFieldUserconstructor. This change is acceptable if theHybridQuantityclass is defined in thePHARE::corenamespace. Ensure that all references toHybridQuantity::Scalarin the codebase have been updated to include the namespace qualifier.94-110: The new hunk has added several type aliases and template definitions related to field refinement and coarsening operations. Ensure that these new types and templates are necessary and correctly defined. Also, check if these changes affect any existing code that uses the old types.
116-152: The new hunk has added several member initializations in the constructor. Ensure that these initializations are necessary and correctly implemented. Also, check if these changes affect any existing code that uses the old constructor.
166-176: The new hunk has added a method
allocatethat allocates resources forJold_,NiOldUser_, andViOld_. Ensure that these resources are necessary and correctly allocated. Also, check if these changes affect any existing code that uses the oldallocatemethod.179-190: The new hunk has added a method
registerQuantitiesthat takes two unique pointers toIMessengerInfoobjects. Ensure that these pointers are correctly handled in the method. Also, check if these changes affect any existing code that uses the oldregisterQuantitiesmethod.200-238: The new hunk has added a method
registerLevelthat registers a level with several refiners and synchronizers. Ensure that these registrations are necessary and correctly implemented. Also, check if these changes affect any existing code that uses the oldregisterLevelmethod.285-312: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [255-342]
The new hunk has added a method
initLevelthat initializes a level with several refiners and fills several refiners with data. Ensure that these initializations and fill operations are necessary and correctly implemented. Also, check if these changes affect any existing code that uses the oldinitLevelmethod.
357-362: The new hunk has added a method
fillElectricGhoststhat fills electric ghosts with data from several refiners. Ensure that these fill operations are necessary and correctly implemented. Also, check if these changes affect any existing code that uses the oldfillElectricGhostsmethod.367-372: The new hunk has added a method
fillCurrentGhoststhat fills current ghosts with data from several refiners. Ensure that these fill operations are necessary and correctly implemented. Also, check if these changes affect any existing code that uses the oldfillCurrentGhostsmethod.392-398: The new hunk has added a method
fillPatchGhostParticlesthat fills patch ghost particles with data from a refiner. Ensure that this fill operation is necessary and correctly implemented. Also, check if these changes affect any existing code that uses the oldfillPatchGhostParticlesmethod.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Files selected for processing (4)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (30 hunks)
- src/amr/messengers/synchronizer.hpp (1 hunks)
- src/amr/messengers/synchronizer_pool.hpp (3 hunks)
- src/core/data/vecfield/vecfield.hpp (2 hunks)
Additional comments (Suppressed): 22
src/core/data/vecfield/vecfield.hpp (1)
- 27-34: The copy constructor and copy assignment operator for
VecFieldhave been correctly deleted to prevent shallow copying of the object. The move constructor and move assignment operator have been defaulted, which is appropriate for this class as it does not manage any resources that would require a custom move operation. This change improves the safety and correctness of the code.src/amr/messengers/synchronizer.hpp (5)
10-11: The
Communicatorclass is being inherited privately. This is a good practice when you want to use the base class's functionality but don't want to expose its interface to the users of the derived class. However, it's important to ensure that the base class doesn't have any virtual functions that need to be overridden, as they won't be accessible in the derived class.17-31: The constructor
Synchronizer(core::VecFieldNames const& descriptor, std::shared_ptr<ResourcesManager> const& rm, std::shared_ptr<SAMRAI::hier::CoarsenOperator> coarsenOp)is using a lambda functionregisterCoarsento register coarsen operations for x, y, and z components of the vector field. This is a good use of lambda functions to avoid code duplication.34-42: The constructor
Synchronizer(std::string const& name, std::shared_ptr<ResourcesManager> const& rm, std::shared_ptr<SAMRAI::hier::CoarsenOperator> coarsenOp)is registering a coarsen operation for a single field. It's important to ensure that the field name provided exists in theResourcesManager, as thegetIDfunction might return a null value if the field doesn't exist.46-54: The
registerLevelfunction is creating a schedule for each algorithm and adding it to theCommunicatorbase class. It's important to ensure that the level number provided is greater than 0, as the function is trying to access the level number - 1 in the hierarchy. If the level number is 0, this will result in an error.57-64: The
syncfunction is coarsening data for each algorithm. It's important to ensure that the level number provided exists in theCommunicatorbase class, as thefindSchedulefunction might return a null value if the level doesn't exist.src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (12)
11-29: The new hunk includes additional header files that were not present in the old hunk. Ensure that these new includes are necessary and do not introduce circular dependencies or other issues.
11-29: > Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [52-42]
The new hunk includes the
<string>header file which was not present in the old hunk. Ensure that this new include is necessary and does not introduce circular dependencies or other issues.
54-66: The new hunk has added the
PHARE::core::namespace to theHybridQuantity::Scalartype. Ensure that this change is consistent with the rest of the codebase and does not introduce any issues.94-110: The new hunk introduces several new type aliases and templates that were not present in the old hunk. Ensure that these new types are necessary and correctly implemented.
116-152: The new hunk introduces several new member initializations in the constructor that were not present in the old hunk. Ensure that these new initializations are necessary and correctly implemented.
168-176: The new hunk introduces a call to
resourcesManager_->allocate(ViOld_, patch, allocateTime);that was not present in the old hunk. Ensure that this new call is necessary and correctly implemented.181-190: No significant changes detected in the new hunk compared to the old hunk.
200-238: The new hunk introduces several new calls to
registerLevelon various refiners that were not present in the old hunk. Ensure that these new calls are necessary and correctly implemented.285-312: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [255-342]
The new hunk introduces several new calls to
fillandregisterLevelon various refiners that were not present in the old hunk. Ensure that these new calls are necessary and correctly implemented.
357-364: The new hunk removes a debug print statement that was present in the old hunk. Ensure that this change does not affect the debugging process.
367-374: No significant changes detected in the new hunk compared to the old hunk.
392-398: No significant changes detected in the new hunk compared to the old hunk.
src/amr/messengers/synchronizer_pool.hpp (4)
42-47: The
registerLevelmethod has been simplified. It no longer retrieves the coarse level from the hierarchy and does not iterate over the algorithms to add them to the synchronizer. Ensure that this change does not affect the functionality of the code.52-57: The
syncmethod has been simplified. It no longer checks if any algorithms are configured and does not call thecoarsenData()function on the corresponding schedule for the given level number. Ensure that this change does not affect the functionality of the code.59-62: The constructor now takes a
std::shared_ptr<ResourcesManager>and initializes therm_member variable. This is a good practice as it ensures that theResourcesManageris initialized before theSynchronizerPoolis used.64-66: The
synchronizers_map now storesSynchronizer<ResourcesManager>objects instead ofCommunicator<Synchronizer, dimension>objects. Ensure that this change does not affect the functionality of the code.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- tests/simulator/test_advance.py (1 hunks)
Additional comments (Suppressed): 1
tests/simulator/test_advance.py (1)
- 294-298: The absolute tolerance for comparing two arrays has been increased from
4e-15to5e-15. Ensure that this change does not affect the precision of the tests and that it is still within acceptable limits for the application.- np.testing.assert_allclose(slice1, slice2, atol=4e-15, rtol=0) + np.testing.assert_allclose(slice1, slice2, atol=5e-15, rtol=0)
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- tests/simulator/test_advance.py (2 hunks)
Additional comments (Suppressed): 2
tests/simulator/test_advance.py (2)
290-298: The absolute tolerance for the
np.testing.assert_allclosefunction has been increased from4e-15to5e-15. This change seems to be based on empirical observations and is likely intended to account for rounding errors in the calculations. However, it's important to ensure that this change doesn't mask any potential issues in the underlying calculations or algorithms. If the discrepancy is consistently larger than expected, it might be worth investigating the cause.534-544: The absolute tolerance for the
np.testing.assert_allclosefunction has been increased from1e-15to2e-15. As with the previous comment, this change seems to be based on empirical observations and is likely intended to account for rounding errors in the calculations. However, it's important to ensure that this change doesn't mask any potential issues in the underlying calculations or algorithms. If the discrepancy is consistently larger than expected, it might be worth investigating the cause.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 9
Files selected for processing (16)
- src/amr/CMakeLists.txt (1 hunks)
- src/amr/messenger_registration.hpp (2 hunks)
- src/amr/messengers/communicator.hpp (4 hunks)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (30 hunks)
- src/amr/messengers/hybrid_messenger_info.hpp (2 hunks)
- src/amr/messengers/refiner.hpp (1 hunks)
- src/amr/messengers/refiner_pool.hpp (1 hunks)
- src/amr/messengers/synchronizer.hpp (1 hunks)
- src/amr/messengers/synchronizer_pool.hpp (3 hunks)
- src/amr/physical_models/hybrid_model.hpp (2 hunks)
- src/amr/solvers/solver_ppc.hpp (1 hunks)
- src/amr/wrappers/hierarchy.hpp (1 hunks)
- src/core/data/vecfield/vecfield.hpp (2 hunks)
- src/phare_amr.hpp (3 hunks)
- src/simulator/phare_types.hpp (1 hunks)
- tests/simulator/test_advance.py (2 hunks)
Files skipped from review due to trivial changes (3)
- src/amr/CMakeLists.txt
- src/amr/messenger_registration.hpp
- src/amr/messengers/communicator.hpp
Additional comments (Suppressed): 41
src/simulator/phare_types.hpp (1)
- 37-37: The type alias for
Splitterhas been changed fromSplittertoSplitter_t. Ensure that this change is reflected throughout the codebase and does not break any existing functionality.- using Splitter = typename amr_types::Splitter; + using Splitter = typename amr_types::Splitter_t;src/amr/physical_models/hybrid_model.hpp (2)
14-14: The inclusion of
vecfield.hppis new in this hunk. Ensure that this new dependency does not introduce circular dependencies or other issues.140-168: The
fillMessengerInfofunction has been updated to usecore::VecFieldNamesinstead ofamr::VecFieldDescriptor. This change should be verified across the codebase to ensure that all instances ofamr::VecFieldDescriptorhave been replaced withcore::VecFieldNamesand that this change does not break any functionality.src/amr/wrappers/hierarchy.hpp (1)
- 23-25: The inclusion of
core/utilities/meta/meta_utilities.hppis new. Ensure that this new dependency does not introduce any unwanted side effects or conflicts with existing code.src/amr/messengers/hybrid_messenger_info.hpp (2)
38-38: The
VecFieldDescriptorclass has been replaced withVecFieldNamesfrom thecorenamespace. Ensure that this change is compatible with the rest of the codebase and that all instances ofVecFieldDescriptorhave been updated accordingly.42-68: The
VecFieldDescriptorobjects have been replaced withVecFieldNamesobjects. This change simplifies the code and improves readability. However, ensure that theVecFieldNamesstruct provides all the necessary functionality that was previously provided by theVecFieldDescriptorclass.src/phare_amr.hpp (3)
1-3: No changes have been made in this hunk. The include guard is intact.
26-33: Two new header files have been included:
splitter.hppandparticles_data_split.hpp. Ensure that these files exist and are located in the correct directories. Also, verify that the functions or classes defined in these files are used in the current file.42-55: The type alias
Splitterhas been renamed toSplitter_t. Ensure that all references toSplitterin the codebase have been updated toSplitter_tto avoid any compilation errors.tests/simulator/test_advance.py (2)
290-298: The absolute tolerance for the
np.testing.assert_allclosefunction has been increased from4e-15to5e-15. This change might be due to the observed maximum absolute value being4.21e-15, as mentioned in the comment. Ensure that this change does not affect the precision of the tests and that it is still within acceptable limits.534-544: The absolute tolerance for the
np.testing.assert_allclosefunction has been increased from1e-15to2e-15. This change might be due to the test failing for the clang compiler with MPI at1.07e-15, as mentioned in the comment. Ensure that this change does not affect the precision of the tests and that it is still within acceptable limits.src/core/data/vecfield/vecfield.hpp (1)
- 27-34: The copy constructor and copy assignment operator have been correctly deleted to prevent shallow copying of the
VecFieldobject. The move constructor and move assignment operator are defaulted, which is appropriate for this class as it does not manage any resources that would require a custom move operation. This change improves the safety and correctness of the class.src/amr/messengers/synchronizer.hpp (2)
- 1-68: The
Synchronizerclass is well designed and follows the SOLID principles. It encapsulates the synchronization logic and provides a clear interface for registering and synchronizing levels. The use of lambda functions for registering coarsen operations is a good practice as it reduces code duplication. However, there is a potential issue with error handling in thesyncmethod.The
syncmethod throws astd::runtime_errorif no algorithms are configured. This is a good practice as it prevents the method from silently failing. However, it would be better to check this condition in the constructor or in theregisterLevelmethod. This way, the error can be caught earlier, and thesyncmethod can be made noexcept. This would improve the robustness of the class and make it easier to use.Here is a possible change:
* 34-42: + Synchronizer(std::string const& name, std::shared_ptr<ResourcesManager> const& rm, + std::shared_ptr<SAMRAI::hier::CoarsenOperator> coarsenOp) + { + auto id = rm->getID(name); + if (!id) + throw std::runtime_error("Invalid resource manager ID"); + this->add_algorithm()->registerCoarsen(*id, *id, coarsenOp); + } </blockquote></details> <details><summary>src/amr/messengers/synchronizer_pool.hpp (5)</summary><blockquote> * 16-37: The `add` function has been refactored to directly instantiate a `Synchronizer` object instead of calling `makeSynchronizer`. This change simplifies the code and removes the need for the `makeSynchronizer` function. However, it's important to ensure that the `Synchronizer` constructor is compatible with the arguments passed in the `add` function. Also, the `ResourcesManager` is now a template parameter for the `SynchronizerPool` class instead of the `add` function, which simplifies the function signature but requires the `ResourcesManager` to be the same for all synchronizers in the pool. Please verify that this change is intended and does not affect the functionality. * 42-47: The `registerLevel` function has been simplified. The new version directly calls the `registerLevel` function on each `Synchronizer` in the pool. This change removes the need to manually add algorithms and their schedules to the synchronizer, which simplifies the code and reduces the risk of errors. However, it's important to ensure that the `registerLevel` function of the `Synchronizer` class correctly handles the registration of the level. * 52-57: The `sync` function has been simplified. The new version directly calls the `sync` function on each `Synchronizer` in the pool. This change removes the need to manually coarsen data for each algorithm in the synchronizer, which simplifies the code and reduces the risk of errors. However, it's important to ensure that the `sync` function of the `Synchronizer` class correctly handles the synchronization of the data. * 59-62: The `SynchronizerPool` constructor now takes a `ResourcesManager` as an argument and stores it in a member variable. This change allows the `ResourcesManager` to be shared among all synchronizers in the pool, which simplifies the function signatures of the `add` functions. However, it's important to ensure that the `ResourcesManager` is thread-safe if it's shared among multiple synchronizers. * 64-67: The `synchronizers_` map now stores `Synchronizer` objects instead of `Communicator` objects. This change simplifies the code and removes the need for the `Communicator` class. However, it's important to ensure that the `Synchronizer` class provides all the necessary functionality that was previously provided by the `Communicator` class. </blockquote></details> <details><summary>src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (12)</summary><blockquote> * 14-15: The new hunk introduces two new includes for `field_refine_operator.hpp` and `field_coarsen_operator.hpp`. Ensure that these new dependencies are necessary and that they do not introduce circular dependencies or other issues. * 39-39: The new hunk introduces an include for `string`. Ensure that this new dependency is necessary and that it does not introduce circular dependencies or other issues. * 54-66: The new hunk introduces a namespace `PHARE::core::HybridQuantity::Scalar` for the `qty` variable and the `quantity` member of the `FieldUser` class. Ensure that this change is reflected throughout the codebase and that it does not introduce any issues. * 97-107: The new hunk introduces several new type aliases and templates. Ensure that these new types are necessary and that they do not introduce any issues. * 119-141: The new hunk introduces several new member variables for the class. Ensure that these new members are necessary and that they do not introduce any issues. * 174-176: The new hunk introduces calls to `resourcesManager_->allocate` for `Jold_`, `NiOldUser_`, and `ViOld_`. Ensure that these new calls are necessary and that they do not introduce any issues. * 189-190: The new hunk introduces a change in the function signature for `registerQuantities`. Ensure that this change is reflected throughout the codebase and that it does not introduce any issues. * 211-237: The new hunk introduces several new calls to `registerLevel` for various refiners. Ensure that these new calls are necessary and that they do not introduce any issues. * 323-324: The new hunk introduces calls to `fill` for `magneticInitRefiners_` and `electricInitRefiners_`, and comments out calls to `fill` for `magGhostsRefiners_` and `elecGhostsRefiners_`. Ensure that these changes are necessary and that they do not introduce any issues. * 360-361: The new hunk introduces calls to `fill` for `elecSharedNodesRefiners_` and `elecGhostsRefiners_`. Ensure that these new calls are necessary and that they do not introduce any issues. * 370-371: The new hunk introduces calls to `fill` for `currentSharedNodesRefiners_` and `currentGhostsRefiners_`. Ensure that these new calls are necessary and that they do not introduce any issues. * 395-395: The new hunk introduces a call to `fill` for `patchGhostPartRefiners_`. Ensure that this new call is necessary and that it does not introduce any issues. </blockquote></details> <details><summary>src/amr/messengers/refiner_pool.hpp (9)</summary><blockquote> * 23-24: The `RefinerPool` class has been updated to use a template parameter for the `ResourcesManager` and `RefinerType`. This change allows for more flexibility in the types of resources that can be managed by the `RefinerPool`. However, it's important to ensure that the `ResourcesManager` type used in practice provides the necessary functionality expected by the `RefinerPool` class. * 33-35: The `addStaticRefiners` function has been updated to take a `std::shared_ptr<RefineOperator>` and a `std::vector<std::string>` for keys. This change allows for more flexibility in the types of refine operators that can be used and the keys that can be associated with them. However, it's important to ensure that the `RefineOperator` type used in practice provides the necessary functionality expected by the `addStaticRefiners` function. * 62-66: The `addTimeRefiners` function has been updated to take a `std::shared_ptr<RefineOperator>` and a `std::shared_ptr<SAMRAI::hier::TimeInterpolateOperator>`. This change allows for more flexibility in the types of refine and time interpolate operators that can be used. However, it's important to ensure that the `RefineOperator` and `SAMRAI::hier::TimeInterpolateOperator` types used in practice provide the necessary functionality expected by the `addTimeRefiners` function. * 95-97: The `registerLevel` function has been simplified to loop over the `refiners_` map and call the `registerLevel` function on each refiner. This change simplifies the code and makes it easier to understand. However, it's important to ensure that the `registerLevel` function of each refiner provides the necessary functionality expected by the `registerLevel` function of the `RefinerPool` class. * 107-113: The `fill` function has been simplified to loop over the `refiners_` map and call the `fill` function on each refiner. This change simplifies the code and makes it easier to understand. However, it's important to ensure that the `fill` function of each refiner provides the necessary functionality expected by the `fill` function of the `RefinerPool` class. * 117-123: The `fill` function for `VecFieldT` has been updated to throw a `std::runtime_error` if no refiner is found for the given `VecFieldT`. This change improves error handling by providing a clear error message when a refiner is not found. However, it's important to ensure that this change does not introduce any unexpected behavior in the code. * 127-137: The `regrid` function has been simplified to loop over the `refiners_` map and call the `regrid` function on each refiner. This change simplifies the code and makes it easier to understand. However, it's important to ensure that the `regrid` function of each refiner provides the necessary functionality expected by the `regrid` function of the `RefinerPool` class. * 237-240: The `addStaticRefiner`, `addTimeRefiner` functions have been updated to throw a `std::runtime_error` if a refiner with the given key is already registered. This change improves error handling by providing a clear error message when a refiner is already registered. However, it's important to ensure that this change does not introduce any unexpected behavior in the code. * 212-216: The `addStaticRefiners`, `addStaticRefiner`, and `addTimeRefiners` functions have been updated to use the `addStaticRefiner` and `addTimeRefiner` functions to add refiners. This change simplifies the code and makes it easier to understand. However, it's important to ensure that the `addStaticRefiner` and `addTimeRefiner` functions provide the necessary functionality expected by the `addStaticRefiners` and `addTimeRefiners` functions. </blockquote></details> <details><summary>src/amr/solvers/solver_ppc.hpp (1)</summary><blockquote> * 161-171: The `dynamic_cast` on line 164 could potentially throw a `std::bad_cast` exception if the `info` object is not of type `amr::HybridMessengerInfo`. It would be safer to use `dynamic_cast` in a try-catch block to handle this exception gracefully. Also, ensure that all calls to this function throughout the codebase have been updated to match the new signature and the replacement of `VecFieldDescriptor` with `core::VecFieldNames`. ```diff - auto& hybridInfo = dynamic_cast<amr::HybridMessengerInfo&>(*info); + amr::HybridMessengerInfo* hybridInfoPtr = dynamic_cast<amr::HybridMessengerInfo*>(info.get()); + if (!hybridInfoPtr) { + // Handle the error appropriately, e.g., throw an exception or return an error code + throw std::runtime_error("Invalid IMessengerInfo type"); + } + auto& hybridInfo = *hybridInfoPtr;
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- tests/simulator/test_advance.py (2 hunks)
Additional comments (Suppressed): 2
tests/simulator/test_advance.py (2)
293-298: The absolute tolerance for the
np.testing.assert_allclosefunction has been increased from4e-15to5.5e-15. This change is justified by the comment that explains the empirical max absolute observed is5.2e-15. However, it's important to ensure that this change doesn't mask any potential issues in the code that could lead to larger discrepancies in the future.539-544: The absolute tolerance for the
np.testing.assert_allclosefunction has been increased from1e-15to2e-15. The comment explains that this change was made because a test failed at1.07e-15when using the Clang compiler with MPI. While this change seems reasonable, it's important to ensure that this doesn't mask any potential issues in the code that could lead to larger discrepancies in the future.
raising threshold for test_field_coarsening_via_subcycles to 2e-15 raising threshold advance test field overlap equal to 5.5e-15 after failure for mpi/clang build
There was a problem hiding this comment.
Review Status
Actionable comments generated: 11
Files selected for processing (16)
- src/amr/CMakeLists.txt (1 hunks)
- src/amr/messenger_registration.hpp (2 hunks)
- src/amr/messengers/communicator.hpp (4 hunks)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (30 hunks)
- src/amr/messengers/hybrid_messenger_info.hpp (2 hunks)
- src/amr/messengers/refiner.hpp (1 hunks)
- src/amr/messengers/refiner_pool.hpp (1 hunks)
- src/amr/messengers/synchronizer.hpp (1 hunks)
- src/amr/messengers/synchronizer_pool.hpp (3 hunks)
- src/amr/physical_models/hybrid_model.hpp (2 hunks)
- src/amr/solvers/solver_ppc.hpp (1 hunks)
- src/amr/wrappers/hierarchy.hpp (1 hunks)
- src/core/data/vecfield/vecfield.hpp (2 hunks)
- src/phare_amr.hpp (3 hunks)
- src/simulator/phare_types.hpp (1 hunks)
- tests/simulator/test_advance.py (2 hunks)
Files skipped from review due to trivial changes (3)
- src/amr/CMakeLists.txt
- src/amr/messenger_registration.hpp
- src/amr/wrappers/hierarchy.hpp
Additional comments (Suppressed): 52
src/simulator/phare_types.hpp (1)
- 37-37: The type alias
Splitterhas been renamed toSplitter_t. Ensure that all references to this type alias in the codebase have been updated to reflect this change.- using Splitter = typename amr_types::Splitter; + using Splitter = typename amr_types::Splitter_t;src/core/data/vecfield/vecfield.hpp (2)
27-37: The copy constructor and copy assignment operator have been explicitly deleted, which is a good practice for classes that manage resources to avoid shallow copying. The move constructor and move assignment operator are defaulted, which is also good for performance reasons. However, the copy constructor declaration on line 31 has been corrected from the previous version. Ensure that this change does not affect any existing code that might have been using the incorrect copy constructor.
238-256: The
VecFieldNamesstruct has been introduced to hold the names of the vector field and its components. This is a good practice for code organization and readability. The constructor takes aVecFieldobject and initializes the names based on theVecField's name and component names. This is a good use of encapsulation and makes the code more maintainable.src/amr/physical_models/hybrid_model.hpp (3)
14-14: The inclusion of
vecfield.hppis new in this hunk. Ensure that this new dependency does not introduce circular dependencies or other issues.145-156: The
VecFieldNamesstruct is being used instead ofVecFieldDescriptor. Ensure that this change is reflected in all parts of the codebase that interact with these fields.160-167: The
transform_lambda function is used to populate various fields inhybridInfowith the names of the ions. This is a good use ofstd::transformand lambda functions to avoid code duplication.src/amr/messengers/hybrid_messenger_info.hpp (3)
38-38: The
VecFieldDescriptorstruct has been replaced withVecFieldNamesfrom thecorenamespace. Ensure that this change does not affect the functionality of theHybridMessengerInfoclass and that all instances ofVecFieldDescriptorhave been updated toVecFieldNamesthroughout the codebase.41-68: The member variables of the
HybridMessengerInfoclass have been updated. TheVecFieldDescriptorobjects have been replaced withVecFieldNamesand thePopulationDescriptorobjects have been replaced withstd::string. Ensure that these changes are reflected in all parts of the code that use these member variables.47-47: The
modelIonDensitymember variable has been changed from aFieldDescriptorto astd::string. Ensure that this change does not affect the functionality of theHybridMessengerInfoclass and that all instances ofmodelIonDensityhave been updated throughout the codebase.src/phare_amr.hpp (4)
1-3: No changes have been made in this hunk. The include guard is correctly defined.
26-33: Two new header files have been included:
splitter.hppandparticles_data_split.hpp. Ensure that these files exist and are located in the correct directories. Also, verify that the functionality they provide is required in this file.45-47: The type alias
Splitterhas been renamed toSplitter_t. Ensure that all references toSplitterin the codebase have been updated toSplitter_tto prevent any compilation errors.51-52: The
RefinementParamstype alias now usesSplitter_tinstead ofSplitter. This change is consistent with the renaming ofSplittertoSplitter_t.src/amr/messengers/synchronizer.hpp (6)
1-4: The inclusion of the
communicator.hppandvecfield.hppheaders indicates that theSynchronizerclass is dependent on theCommunicatorandVecFieldclasses. Ensure that these dependencies are well managed and that any changes in these classes do not adversely affect theSynchronizerclass.10-11: The
Synchronizerclass is inheriting fromCommunicatorprivately. This means that public and protected members ofCommunicatorbecome private inSynchronizer, and are not accessible to classes that inherit fromSynchronizer. If this is the intended behavior, then it's fine. Otherwise, consider changing the inheritance to public or protected.17-31: The constructor
Synchronizer(core::VecFieldNames const& descriptor, std::shared_ptr<ResourcesManager> const& rm, std::shared_ptr<SAMRAI::hier::CoarsenOperator> coarsenOp)is using a lambda functionregisterCoarsento register coarsen operations for the x, y, and z components of a vector field. This is a good use of lambda functions to avoid code duplication.34-42: The constructor
Synchronizer(std::string const& name, std::shared_ptr<ResourcesManager> const& rm, std::shared_ptr<SAMRAI::hier::CoarsenOperator> coarsenOp)is registering a coarsen operation for a single field. This is a good use of constructor overloading to provide flexibility in the use of theSynchronizerclass.46-54: The
registerLevelmethod is creating a schedule for each algorithm inthis->algosand adding it to theCommunicatorbase class. This is a good use of theCommunicatorclass's functionality.57-64: The
syncmethod is callingcoarsenDataon the schedule associated with each algorithm inthis->algos. It throws astd::runtime_errorifthis->algosis empty. This is a good use of error handling to prevent runtime errors.src/amr/solvers/solver_ppc.hpp (1)
- 161-171: The function signature has been changed and now takes a
std::unique_ptr<amr::IMessengerInfo> const&instead of astd::unique_ptr<amr::IMessengerInfo>&. This change makes sense as the function does not modify theinfoobject and should therefore take a const reference. However, please verify that all calls to this function throughout the codebase have been updated to match the new signature.tests/simulator/test_advance.py (2)
290-298: The absolute tolerance for the
np.testing.assert_allclosefunction has been increased from4e-15to5.5e-15. This change is based on empirical observations and is intended to account for rounding errors that may occur during the filling of ghosts with schedules involving linear/spatial interpolations. Ensure that this change does not affect the accuracy of the tests and that it is still within acceptable limits.534-546: The absolute tolerance for the
np.testing.assert_allclosefunction has been increased from1e-15to2e-15. This change is based on a failure observed in a clang build for an MPI test. Ensure that this change does not affect the accuracy of the tests and that it is still within acceptable limits.src/amr/messengers/communicator.hpp (6)
14-17: The new hunk has removed several includes that were present in the old hunk. Please verify that these includes are not needed anymore or have been moved to other files. If they are still needed, this could lead to compilation errors or undefined behavior.
24-34: The
RefinerandSynchronizerstructs have been renamed toRefinerTypesandSynchronizerTypesrespectively. Ensure that all references to these structs in the codebase have been updated to reflect this change.38-39: The
Communicatorclass template now takes an additional template parameterdimension. Ensure that all instantiations of this class have been updated to include this parameter.47-48: The
is_refinerandis_synchronizerstatic constexpr variables have been moved inside theCommunicatorclass. Ensure that all references to these variables have been updated to reflect this change.65-73: The
add_algorithmfunction has been updated to useif constexprfor compile-time condition checking. This is a good use of modern C++ features to improve code readability and performance.105:
There's a typo in the comment "oevrload with rvalue schedule". It should be "overload with rvalue schedule".- // oevrload with rvalue schedule + // overload with rvalue schedule
- 116-118: The new hunk has removed several function definitions that were present in the old hunk. Please verify that these functions are not needed anymore or have been moved to other files. If they are still needed, this could lead to compilation errors or undefined behavior.
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (13)
11-29: The new hunk has added more header files to be included. Ensure that these new includes are necessary and do not introduce circular dependencies or conflicts.
36-42: The new hunk has added the inclusion of the
<string>library. Ensure that this new include is necessary and does not introduce conflicts.52-68: The new hunk has added the
PHARE::core::namespace to theHybridQuantity::Scalartype. This change should not affect the functionality of the code, but it does make the type more explicit. Ensure that this change is consistent with the rest of the codebase.94-110: The new hunk has added several type definitions and template type aliases. Ensure that these new types are necessary and do not introduce conflicts or unnecessary complexity.
116-156: The new hunk has added several member initializations in the constructor. Ensure that these new initializations are necessary and do not introduce memory leaks or other resource management issues.
166-176: The new hunk has added calls to
resourcesManager_->allocate()forJold_,NiOldUser_, andViOld_. Ensure that these new calls are necessary and do not introduce memory leaks or other resource management issues.189-190: The new hunk has added the
[[maybe_unused]]attribute to thefromCoarserInfoparameter in theregisterQuantities()function. This change indicates thatfromCoarserInfomay not be used in the function, which can suppress compiler warnings. Ensure that this change is accurate and does not hide any potential issues.200-238: The new hunk has added calls to
registerLevel()for several member objects. Ensure that these new calls are necessary and do not introduce any issues with level registration.255-264: The new hunk has added calls to
fill()formagneticInitRefiners_,electricInitRefiners_, anddomainParticlesRefiners_, and a call topatchGhostPartRefiners_.fill(). Ensure that these new calls are necessary and do not introduce any issues with data filling.285-312: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [270-342]
The new hunk has added calls to
fill()for several member objects. Ensure that these new calls are necessary and do not introduce any issues with data filling.
357-362: The new hunk has added calls to
fill()forelecSharedNodesRefiners_andelecGhostsRefiners_in thefillElectricGhosts()function. Ensure that these new calls are necessary and do not introduce any issues with data filling.367-372: The new hunk has added calls to
fill()forcurrentSharedNodesRefiners_andcurrentGhostsRefiners_in thefillCurrentGhosts()function. Ensure that these new calls are necessary and do not introduce any issues with data filling.392-398: The new hunk has added a call to
fill()forpatchGhostPartRefiners_in thefillPatchGhostParticles()function. Ensure that this new call is necessary and does not introduce any issues with data filling.src/amr/messengers/synchronizer_pool.hpp (5)
16-37: The
addfunction has been overloaded to accept either aDescriptoror acore::VecFieldNamesas the first argument. This is a good practice as it provides flexibility in the types of descriptors that can be added to theSynchronizerPool. However, ensure that all calls to this function throughout the codebase have been updated to match the new signatures.42-47: The
registerLevelfunction has been simplified. It no longer retrieves the level number or the coarse level from the hierarchy, and it no longer iterates over the algorithms of each synchronizer. Instead, it simply calls theregisterLevelfunction on each synchronizer. This change simplifies the code and reduces complexity. However, ensure that theregisterLevelfunction of theSynchronizerclass handles the logic that was previously in this function.52-57: The
syncfunction has been simplified. It no longer checks if the algorithms are configured or calls thecoarsenDatafunction on the schedule. Instead, it simply calls thesyncfunction on each synchronizer. This change simplifies the code and reduces complexity. However, ensure that thesyncfunction of theSynchronizerclass handles the logic that was previously in this function.59-62: The constructor of the
SynchronizerPoolclass now takes astd::shared_ptr<ResourcesManager>as an argument and initializes therm_member variable with it. This is a good practice as it allows theResourcesManagerto be shared among different instances of theSynchronizerPoolclass. However, ensure that all instances of theSynchronizerPoolclass are created with aResourcesManager.64-67: The
synchronizers_member variable now stores instances of theSynchronizerclass instead of theCommunicatorclass. This change is consistent with the introduction of theSynchronizerclass and the simplification of theCommunicatorclass. However, ensure that theSynchronizerclass provides the same functionality as theCommunicatorclass.src/amr/messengers/refiner_pool.hpp (6)
119-123: The function
fillthrows a runtime error when there is no refiner for the givenvec.name(). This is a good practice as it prevents further execution with invalid data. However, it would be beneficial to also log this error for easier debugging.159-164: The function
addStaticRefinersuses an assert to ensure that the sizes ofdestinationsandsourcesmatch. This is a good practice as it prevents further execution with invalid data. However, it would be beneficial to also log this error for easier debugging.237-240: The
addStaticRefinerandaddTimeRefinerfunctions throw a runtime error when a refiner with the given key is already registered. This is a good practice as it prevents further execution with invalid data. However, it would be beneficial to also log this error for easier debugging.240-241: The
addStaticRefinerandaddTimeRefinerfunctions use structured bindings (auto const [it, success]) to handle the result ofstd::map::insert. This is a good practice as it makes the code more readable and easier to understand.127-137: The
fillandregridfunctions use range-based for loops to iterate over therefiners_map. This is a good practice as it makes the code more readable and easier to understand.140-143: The constructor for
RefinerPooltakes ashared_ptrto aResourcesManagerand stores it in a member variable. This is a good practice as it ensures that theResourcesManagerwill not be destroyed while theRefinerPoolis still using it.
…EHUB#757) * some refactoring of messenger stuff and cleaning here and there * raising threshold for advance field test to 5e-15 raising threshold for test_field_coarsening_via_subcycles to 2e-15 raising threshold advance test field overlap equal to 5.5e-15 after failure for mpi/clang build
…EHUB#757) * some refactoring of messenger stuff and cleaning here and there * raising threshold for advance field test to 5e-15 raising threshold for test_field_coarsening_via_subcycles to 2e-15 raising threshold advance test field overlap equal to 5.5e-15 after failure for mpi/clang build
Summary by CodeRabbit
RefinerandSynchronizerclasses to improve data communication and interpolation between different levels of a patch hierarchy, enhancing the software's performance and reliability.Communicatorclass, improving code readability and maintainability.HybridModelandSolverPPCclasses to usecore::VecFieldNamestype, improving code consistency.refiner.hppandsynchronizer.hppfiles.These changes aim to enhance the software's performance, reliability, and maintainability, providing a more robust and user-friendly experience.