Conversation
📝 Walkthrough## Walkthrough
This update removes all handling and support for "patchGhost" particles and related diagnostics across the codebase, both in C++ and Python. It refactors particle copy, packing, and communication logic to exclude patch ghost regions, modifies field and particle fill patterns, introduces border sum refiners for field data, and updates tests, diagnostics, and utilities accordingly. Several new features and generic interfaces for field operations and boxing are introduced.
## Changes
| Files/Groups | Change Summary |
|------------------------------------------------------------------------------|---------------|
| `src/amr/data/field/field_data.hpp`, `src/core/data/field/field_box.hpp`, `src/core/utilities/types.hpp` | Refactored field data operations to use generic, operator-based field copying and summation; added support for border summation and generic field box utilities. |
| `src/amr/data/field/field_variable_fill_pattern.hpp` | Simplified field fill pattern, removed conditional interior overwrite logic, and added a new ghost interpolation overlap fill pattern. |
| `src/amr/data/field/refine/field_refine_operator.hpp` | Removed `AFieldRefineOperator` base class; updated inheritance and constructor logic. |
| `src/amr/data/particles/particles_data.hpp`, `src/amr/data/particles/particles_variable_fill_pattern.hpp` | Reworked particle data copy/pack/unpack logic to exclude patch ghost particles; introduced new fill pattern for domain-from-ghost particle exchanges. |
| `src/amr/level_initializer/hybrid_level_initializer.hpp` | Refactored moment computation to separate domain and ghost region deposition, aligning with new ghost handling. |
| `src/amr/messengers/communicator.hpp` | Added explicit special member functions for lifecycle management. |
| `src/amr/messengers/field_sum_transaction.hpp` | Added new transaction and factory classes for border sum operations on field data. |
| `src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp`, `src/amr/messengers/hybrid_messenger.hpp`, `src/amr/messengers/hybrid_messenger_info.hpp`, `src/amr/messengers/hybrid_messenger_strategy.hpp`, `src/amr/messengers/mhd_hybrid_messenger_strategy.hpp` | Removed shared node and patch ghost refiners, introduced border sum refiners for flux/density, and updated messenger interfaces for border filling. |
| `src/amr/messengers/refiner.hpp`, `src/amr/messengers/refiner_pool.hpp` | Updated refiner types and registration logic to support new border sum and ghost particle refiners; added optional fill pattern support. |
| `src/amr/physical_models/hybrid_model.hpp` | Appended new ghost flux and border field names to messenger info for each ion population. |
| `src/amr/resources_manager/amr_utils.cpp`, `src/amr/resources_manager/amr_utils.hpp` | Changed AMR box utilities to return references; added ghost box construction excluding level ghost cells. |
| `src/amr/resources_manager/resources_manager.hpp` | Added method to retrieve lists of resource IDs for multiple keys. |
| `src/amr/solvers/solver_ppc.hpp` | Introduced boxing mechanism for ion updater; removed patch ghost particle state saving/restoring; updated ion update and border fill logic. |
| `src/amr/utilities/box/amr_box.hpp`, `src/core/utilities/box/box.hpp`, `src/core/utilities/point/point.hpp` | Added utilities for unsigned box conversion, particle-in-box checks, transformation-based box shifts, and enhanced point arithmetic. |
| `src/core/data/grid/grid.hpp`, `src/core/data/grid/gridlayout.hpp` | Added copy constructor to grid; added ghost box query methods to grid layout. |
| `src/core/data/ions/ion_population/ion_population.hpp`, `src/core/data/ions/ions.hpp` | Added bounds-checked and unchecked accessors for ion populations; added assertion for initializer info. |
| `src/core/data/particles/particle_array.hpp`, `src/core/utilities/cellmap.hpp` | Simplified erase and partition methods; removed replace_from. |
| `src/core/numerics/ion_updater/ion_updater.hpp` | Generalized updater methods to use boxing; removed patch ghost logic; introduced selection boxing struct. |
| `src/core/numerics/moments/moments.hpp` | Removed patch ghost deposit tag and logic; added error handling for unknown deposit tags. |
| `src/diagnostic/detail/h5writer.hpp` | Added timestamp accessor. |
| `src/diagnostic/diagnostic_model_view.hpp` | Added support for momentum tensor refinement and border sum schedules. |
| `src/diagnostic/detail/types/fluid.hpp`, `src/diagnostic/detail/types/particle.hpp` | Removed patch ghost handling in population interpolation and particle diagnostics. |
| `src/python3/patch_level.hpp` | Removed patch ghost particle data from Python patch level interface. |
| `tests/amr/data/field/refine/test_field_refine.cpp`, `tests/amr/messengers/test_messengers.cpp` | Reorganized includes; updated resource registration in tests. |
| `tests/amr/data/particles/copy/test_particledata_copyNd.cpp`, `tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp`, `tests/amr/data/particles/stream_pack/test_main.cpp` | Updated or removed tests to reflect removal of patch ghost particles; tests now expect particles in domain containers. |
| `tests/core/numerics/ion_updater/test_updater.cpp`, `tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp` | Updated tests and benchmarks to use boxing and new updater logic. |
| `tests/diagnostic/__init__.py`, `tests/diagnostic/test_diagnostics.hpp`, `tests/diagnostic/test_diagnostics.ipp`, `tests/functional/conservation/conserv.py`, `tests/simulator/test_advance.py`, `tests/simulator/test_initialization.py`, `tests/simulator/initialize/density_check.py`, `tests/simulator/initialize/test_particles_init_1d.py`, `tests/simulator/initialize/test_particles_init_2d.py`, `tests/simulator/advance/test_particles_advance_1d.py`, `tests/simulator/advance/test_particles_advance_2d.py` | Removed patch ghost diagnostics and related tests; updated expectations and test logic for new ghost handling. |
| `pyphare/pyphare/pharein/diagnostics.py`, `pyphare/pyphare/pharesee/hierarchy/fromh5.py`, `pyphare/pyphare/pharesee/hierarchy/fromsim.py`, `pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py`, `pyphare/pyphare/pharesee/run/run.py` | Removed patch ghost handling in diagnostics, file recognition, hierarchy construction, and run mass computation. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant PatchA as Patch (src)
participant PatchB as Patch (dst)
participant BorderSumRefiner as BorderSumRefiner
participant Messenger as Messenger
Note over PatchA,PatchB: Field Border Summation (new flow)
PatchA->>BorderSumRefiner: Prepare field data for border region
BorderSumRefiner->>Messenger: Pack field border data
Messenger->>PatchB: Send border data
PatchB->>BorderSumRefiner: Unpack and sum into border regionsequenceDiagram
participant PatchA as Patch (src)
participant PatchB as Patch (dst)
participant ParticleRefiner as ParticleDomainFromGhostRefiner
participant Messenger as Messenger
Note over PatchA,PatchB: Particle domain-from-ghost transfer (new logic)
PatchA->>ParticleRefiner: Select ghost layer particles
ParticleRefiner->>Messenger: Pack ghost particles with coordinate shift
Messenger->>PatchB: Send ghost particles
PatchB->>ParticleRefiner: Unpack and append to domain particles
Estimated code review effort4 (~90 minutes) Suggested labels
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (13)
src/core/data/ions/ions.hpp (1)
256-256: Consider adding bounds checking for safety.The indexing operator lacks bounds checking, which could lead to undefined behavior if an invalid index is passed. Consider adding a bounds check or documenting the precondition that the index must be valid.
- auto& operator[](std::size_t const i) const { return populations_[i]; } + auto& operator[](std::size_t const i) const { + assert(i < populations_.size()); + return populations_[i]; + }Alternatively, consider providing a bounds-checked
at()method alongside the unchecked operator.src/core/data/ions/ion_population/ion_population.hpp (1)
46-50: Good defensive programming practice with assertion.The runtime assertion ensures data integrity by verifying the presence of the required "density" key. This follows good defensive programming practices and will help catch configuration errors early.
Consider whether this should be a runtime exception instead of an assertion if the missing density key represents a recoverable error rather than a programming bug.
src/amr/messengers/mhd_hybrid_messenger_strategy.hpp (1)
112-121: Consider documenting the implementation strategy for border fill methods.The new
fillFluxBordersandfillDensityBordersmethods are properly declared as overrides but have empty implementations with commented-out calls.Consider adding documentation explaining:
- Why these methods are currently no-ops
- When the commented implementation might be activated
- The relationship to the underlying strategy object
+ // TODO: Implement flux border filling when MHD-Hybrid coupling is active void fillFluxBorders(IonsT& /*ions*/, SAMRAI::hier::PatchLevel& /*level*/, double const /*fillTime*/) override { // strat_->fillFluxBorders(ions, level, fillTime); } + // TODO: Implement density border filling when MHD-Hybrid coupling is active void fillDensityBorders(IonsT& /*ions*/, SAMRAI::hier::PatchLevel& /*level*/, double const /*fillTime*/) override { // strat_->fillDensityBorders(ions, level, fillTime); }src/core/utilities/types.hpp (1)
502-514: LGTM: Clean operator abstractions.The
EqualsandPlusEqualstemplate structs provide clean, reusable operator abstractions. The design using reference members and genericautoparameters is appropriate for the intended use case in field data operations.Consider adding static_assert or concepts to constrain the template parameter
Dif specific requirements exist for the type.src/core/data/grid/gridlayout.hpp (1)
1233-1255: Consider adding bounds validation in _BoxFor.The
_BoxForhelper function doesn't validate that the computed box has valid bounds (i.e., lower <= upper). Invalid bounds could lead to subtle bugs.Add validation before returning the box:
template<typename Field, typename Fn> auto _BoxFor(Field const& field, Fn startToEnd) const { std::array<std::uint32_t, dimension> lower, upper; auto const [ix0, ix1] = startToEnd(field, Direction::X); lower[0] = ix0; upper[0] = ix1; if constexpr (dimension > 1) { auto const [iy0, iy1] = startToEnd(field, Direction::Y); lower[1] = iy0; upper[1] = iy1; } if constexpr (dimension == 3) { auto const [iz0, iz1] = startToEnd(field, Direction::Z); lower[2] = iz0; upper[2] = iz1; } + // Validate bounds + for (std::size_t i = 0; i < dimension; ++i) + { + if (lower[i] > upper[i]) + { + throw std::runtime_error("Invalid box bounds: lower > upper in dimension " + std::to_string(i)); + } + } return Box<std::uint32_t, dimension>{lower, upper}; }src/amr/data/particles/particles_variable_fill_pattern.hpp (1)
48-48: Consider addingvirtualkeyword to the destructor for consistency.While not strictly necessary when inheriting from a virtual base, it's good practice to explicitly mark virtual destructors.
- virtual ~ParticleDomainFromGhostFillPattern() {} + virtual ~ParticleDomainFromGhostFillPattern() override = default;src/amr/messengers/field_sum_transaction.hpp (1)
161-164: Consider implementing printClassData instead of throwing.The
printClassDatamethod throws an exception, which might cause issues if SAMRAI debugging or logging functionality expects this method to work properly.-void FieldBorderSumTransaction<FieldData_t>::printClassData(std::ostream& stream) const -{ - PHARE_LOG_SCOPE(2, "FieldBorderSumTransaction::printClassData"); - throw std::runtime_error("FieldBorderSumTransaction::printClassData!"); -} +void FieldBorderSumTransaction<FieldData_t>::printClassData(std::ostream& stream) const +{ + PHARE_LOG_SCOPE(2, "FieldBorderSumTransaction::printClassData"); + stream << "FieldBorderSumTransaction: " + << "dst_node=" << d_dst_node + << ", src_node=" << d_src_node + << ", item_id=" << d_item_id + << std::endl; +}src/core/data/field/field_box.hpp (1)
82-88: Consider adding automatic vector reservation in append_to.The comment suggests the caller should reserve the vector, but it would be safer to handle this internally.
template<typename Field_t> void FieldBox<Field_t>::append_to(std::vector<value_type>& vec) { - // reserve vec before use! + vec.reserve(vec.size() + lcl_box.size()); auto src_it = lcl_box.begin(); for (; src_it != lcl_box.end(); ++src_it) vec.push_back(field(*src_it)); }src/amr/solvers/solver_ppc.hpp (2)
153-168: Improve error message for debugging.The error message "boxing map insertion failure" is not descriptive enough for debugging.
- throw std::runtime_error("boxing map insertion failure"); + throw std::runtime_error("Failed to insert boxing for patch " + amr::to_string(patch->getGlobalId()) + " at level " + std::to_string(lvlNbr));
434-436: Consider adding error handling for missing boxing data.Using
at()will throw if the patch ID is not found in the map. Whilemake_boxesshould ensure all patches are present, defensive programming would be beneficial.- ionUpdater_.updatePopulations( - state.ions, state.electromagAvg, - levelBoxing.at(amr::to_string(state.patch->getGlobalId())), dt, mode); + auto patch_id = amr::to_string(state.patch->getGlobalId()); + auto it = levelBoxing.find(patch_id); + if (it == levelBoxing.end()) + throw std::runtime_error("Boxing data not found for patch " + patch_id); + ionUpdater_.updatePopulations( + state.ions, state.electromagAvg, it->second, dt, mode);src/amr/data/particles/particles_data.hpp (1)
760-772: Document the transformation asymmetry.The unpack method doesn't apply any transformation, unlike the pack method. This appears intentional (particles are pre-transformed during packing), but should be documented for clarity.
Add a comment explaining why no transformation is needed during unpacking:
void unpack_from_ghost(SAMRAI::tbox::MessageStream& stream, ParticlesDomainOverlap const& /*pOverlap*/) { PHARE_LOG_SCOPE(3, "ParticlesData::unpack_from_ghost"); + + // No transformation needed - particles were already transformed during packing std::size_t numberParticles = 0;src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
359-362: Consider using std::copy instead of memcpy for type safetyWhile
memcpyis efficient, usingstd::copywould provide better type safety and work correctly with non-trivial types if the field type changes in the future.- std::memcpy(sumVec[c].data(), pop.flux()[c].data(), - pop.flux()[c].size() * sizeof(value_type)); + std::copy(pop.flux()[c].data(), + pop.flux()[c].data() + pop.flux()[c].size(), + sumVec[c].data());Also applies to: 371-374
src/amr/messengers/refiner_pool.hpp (1)
211-212: Consider runtime validation instead of assertionsThe size consistency checks use
assertwhich is typically disabled in release builds. Consider either:
- Using runtime exceptions for size mismatches
- Using a single container of tuples/structs to ensure size consistency by design
- assert(destinations.size() == sources.size()); - assert(destinations.size() == keys.size()); + if (destinations.size() != sources.size() || destinations.size() != keys.size()) + throw std::invalid_argument("Size mismatch: destinations, sources, and keys must have the same size");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
src/amr/data/field/field_data.hpp(10 hunks)src/amr/data/field/field_variable_fill_pattern.hpp(5 hunks)src/amr/data/field/refine/field_refine_operator.hpp(2 hunks)src/amr/data/particles/particles_data.hpp(9 hunks)src/amr/data/particles/particles_variable_fill_pattern.hpp(1 hunks)src/amr/level_initializer/hybrid_level_initializer.hpp(2 hunks)src/amr/messengers/communicator.hpp(1 hunks)src/amr/messengers/field_sum_transaction.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(15 hunks)src/amr/messengers/hybrid_messenger.hpp(2 hunks)src/amr/messengers/hybrid_messenger_info.hpp(1 hunks)src/amr/messengers/hybrid_messenger_strategy.hpp(2 hunks)src/amr/messengers/mhd_hybrid_messenger_strategy.hpp(4 hunks)src/amr/messengers/refiner.hpp(8 hunks)src/amr/messengers/refiner_pool.hpp(5 hunks)src/amr/physical_models/hybrid_model.hpp(3 hunks)src/amr/resources_manager/amr_utils.cpp(1 hunks)src/amr/resources_manager/amr_utils.hpp(5 hunks)src/amr/resources_manager/resources_manager.hpp(2 hunks)src/amr/solvers/solver_ppc.hpp(8 hunks)src/amr/utilities/box/amr_box.hpp(2 hunks)src/core/data/field/field_box.hpp(1 hunks)src/core/data/grid/grid.hpp(2 hunks)src/core/data/grid/gridlayout.hpp(2 hunks)src/core/data/ions/ion_population/ion_population.hpp(1 hunks)src/core/data/ions/ions.hpp(2 hunks)src/core/data/particles/particle_array.hpp(2 hunks)src/core/numerics/ion_updater/ion_updater.hpp(7 hunks)src/core/numerics/pusher/pusher.hpp(1 hunks)src/core/utilities/box/box.hpp(5 hunks)src/core/utilities/cellmap.hpp(2 hunks)src/core/utilities/point/point.hpp(3 hunks)src/core/utilities/types.hpp(2 hunks)tests/amr/data/field/refine/test_field_refine.cpp(1 hunks)tests/amr/messengers/test_messengers.cpp(2 hunks)tests/core/numerics/ion_updater/test_updater.cpp(8 hunks)tests/simulator/advance/test_particles_advance_1d.py(0 hunks)tests/simulator/advance/test_particles_advance_2d.py(0 hunks)tests/simulator/initialize/test_particles_init_1d.py(0 hunks)tests/simulator/initialize/test_particles_init_2d.py(1 hunks)tests/simulator/test_diagnostic_timestamps.py(1 hunks)
💤 Files with no reviewable changes (3)
- tests/simulator/initialize/test_particles_init_1d.py
- tests/simulator/advance/test_particles_advance_1d.py
- tests/simulator/advance/test_particles_advance_2d.py
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/amr/messengers/communicator.hppsrc/amr/resources_manager/amr_utils.hppsrc/core/numerics/pusher/pusher.hppsrc/core/data/ions/ions.hppsrc/amr/data/field/refine/field_refine_operator.hppsrc/amr/resources_manager/resources_manager.hppsrc/core/data/ions/ion_population/ion_population.hppsrc/core/utilities/point/point.hppsrc/amr/messengers/hybrid_messenger.hppsrc/core/data/grid/grid.hppsrc/core/utilities/cellmap.hppsrc/amr/messengers/hybrid_messenger_info.hppsrc/amr/messengers/hybrid_messenger_strategy.hppsrc/amr/messengers/mhd_hybrid_messenger_strategy.hppsrc/amr/level_initializer/hybrid_level_initializer.hppsrc/core/utilities/types.hppsrc/core/data/particles/particle_array.hppsrc/core/data/grid/gridlayout.hppsrc/amr/physical_models/hybrid_model.hppsrc/amr/utilities/box/amr_box.hppsrc/amr/messengers/field_sum_transaction.hppsrc/amr/solvers/solver_ppc.hppsrc/core/utilities/box/box.hppsrc/core/data/field/field_box.hppsrc/amr/data/particles/particles_variable_fill_pattern.hppsrc/amr/messengers/hybrid_hybrid_messenger_strategy.hppsrc/core/numerics/ion_updater/ion_updater.hppsrc/amr/data/field/field_variable_fill_pattern.hppsrc/amr/messengers/refiner_pool.hppsrc/amr/messengers/refiner.hppsrc/amr/data/particles/particles_data.hppsrc/amr/data/field/field_data.hpp
🧬 Code Graph Analysis (7)
src/amr/messengers/hybrid_messenger.hpp (2)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (16)
ions(330-343)ions(330-331)ions(347-376)ions(347-348)ions(378-403)ions(378-379)ions(415-451)ions(415-416)ions(459-465)ions(459-460)level(477-502)level(477-480)level(607-619)level(607-607)level(754-769)level(754-754)src/amr/messengers/hybrid_messenger_strategy.hpp (6)
ions(78-79)ions(83-84)ions(88-89)ions(122-123)ions(125-126)level(116-116)
src/amr/messengers/hybrid_messenger_strategy.hpp (2)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (16)
ions(330-343)ions(330-331)ions(347-376)ions(347-348)ions(378-403)ions(378-379)ions(415-451)ions(415-416)ions(459-465)ions(459-460)level(477-502)level(477-480)level(607-619)level(607-607)level(754-769)level(754-754)src/amr/messengers/hybrid_messenger.hpp (7)
ions(299-303)ions(299-300)ions(314-318)ions(314-315)ions(321-325)ions(321-322)level(188-188)
src/amr/messengers/mhd_hybrid_messenger_strategy.hpp (1)
src/amr/messengers/mhd_messenger.hpp (1)
stratName(121-121)
src/amr/physical_models/hybrid_model.hpp (3)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (12)
level(477-502)level(477-480)level(607-619)level(607-607)level(754-769)level(754-754)info(659-685)info(659-659)info(690-732)info(690-690)info(736-749)info(736-736)src/amr/solvers/solver_ppc.hpp (10)
level(106-109)level(106-106)level(116-117)level(120-121)level(124-125)level(128-129)level(132-133)level(136-136)level(137-137)info(83-83)src/amr/solvers/solver.hpp (1)
info(81-81)
src/amr/utilities/box/amr_box.hpp (4)
src/core/data/particles/particle_array.hpp (15)
dim(21-65)dim(251-288)box(232-232)box(232-232)bool(78-81)bool(215-225)bool(322-325)i(75-75)i(75-75)i(76-76)i(76-76)i(305-305)i(305-305)i(306-306)i(306-306)src/core/utilities/box/box.hpp (11)
dim(99-115)dim(117-132)box(50-50)bool(50-53)bool(73-73)bool(243-257)lower(93-93)lower(98-98)upper(88-88)shift(282-288)shift(282-282)src/core/utilities/point/point.hpp (6)
bool(73-84)bool(86-86)i(68-68)i(68-68)i(69-69)i(69-69)src/core/utilities/types.hpp (4)
for_N(428-459)for_N(428-428)for_N(462-465)for_N(462-462)
src/amr/messengers/field_sum_transaction.hpp (2)
src/amr/data/particles/particles_data.hpp (15)
overlap(274-293)overlap(274-274)overlap(618-646)overlap(618-618)overlap(650-688)overlap(650-652)stream(318-338)stream(318-319)stream(340-354)stream(340-341)stream(371-372)stream(374-426)stream(374-375)stream(428-442)stream(428-429)src/amr/data/field/field_data.hpp (11)
overlap(208-211)overlap(208-208)overlap(417-434)overlap(417-417)stream(219-250)stream(219-220)stream(257-261)stream(257-258)stream(324-325)unpackStream(264-292)unpackStream(264-265)
src/amr/data/field/field_variable_fill_pattern.hpp (4)
src/amr/data/particles/particles_variable_fill_pattern.hpp (4)
dst_geometry(50-78)dst_geometry(51-55)fill_boxes(100-109)fill_boxes(101-104)src/amr/utilities/box/amr_box.hpp (6)
phare_box_from(25-31)phare_box_from(25-25)shift(153-157)shift(153-154)samrai_box_from(16-22)samrai_box_from(16-16)src/core/utilities/box/box.hpp (5)
box(50-50)grow(274-279)grow(274-274)shift(282-288)shift(282-282)src/amr/data/field/field_geometry.hpp (4)
box(155-258)box(155-156)box(267-276)box(267-267)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: Analyze (python)
- GitHub Check: build (macos-latest)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
🔇 Additional comments (74)
src/core/numerics/pusher/pusher.hpp (1)
23-24: LGTM! Good API improvement.Making
ParticleSelectorpublic appropriately exposes this type alias for external use, which aligns with the broader particle data handling refactoring mentioned in the PR objectives.tests/simulator/test_diagnostic_timestamps.py (1)
120-120: LGTM! Appropriate fix for MPI testing issues.Changing from
h5_flush_neverto a fixed flush interval of10000is a reasonable solution to address the MPI parallelism issues observed att=46.42300. This ensures data consistency during testing without significantly impacting performance.tests/amr/messengers/test_messengers.cpp (2)
263-264: LGTM! Good refactoring of resource registration.Simplifying the resource registration by registering the entire
stateobjects instead of individual sub-components (electromag,ions,B,V) makes the code cleaner and more maintainable. This aligns well with the broader messenger refactoring.
307-307: LGTM! Proper solver resource registration.Adding the call to
registerResourceson the hybrid solver ensures complete test setup and is consistent with the resource management patterns being established.tests/amr/data/field/refine/test_field_refine.cpp (1)
4-5: LGTM! Better include organization.The reorganization logically groups related includes together - core grid layout headers first, then AMR-specific includes, followed by external library headers. The addition of
gridlayoutimplyee.hppsuggests preparation for testing updated grid layout functionality.Also applies to: 13-17
src/amr/messengers/hybrid_messenger_info.hpp (1)
69-71: LGTM! Well-structured additions for enhanced field management.The new member variables
ghostFluxandsumBorderFieldsfollow the established naming conventions and appropriately extend the class to support ghost flux field tracking and border field summation functionality.src/amr/resources_manager/amr_utils.cpp (1)
34-40: LGTM! Improved interface with fluent design pattern.The change from
voidreturn toSAMRAI::hier::Box&enables method chaining and follows the fluent interface pattern. This maintains existing functionality while providing additional flexibility for callers.src/amr/messengers/communicator.hpp (1)
55-59: LGTM! Excellent application of the Rule of Five.The explicit special member functions provide clear object lifecycle semantics:
- Virtual destructor ensures proper cleanup in inheritance hierarchies
- Deleted copy constructor prevents accidental copying of resource-managing objects
- Defaulted move constructor allows efficient resource transfer
This is best practice for template classes managing SAMRAI resources.
src/core/utilities/cellmap.hpp (2)
173-173: Simplified parameter passing - acceptable trade-off.The change from
Range&&toRangeremoves perfect forwarding in favor of pass-by-value. This simplifies the interface and is acceptable sinceRangeis likely a lightweight object containing iterators.
451-451:Details
✅ Verification successful
Verify the partition return value calculation.
The change from
range.ibegin() + pivotto justpivotin themakeRangecall alters the end iterator calculation. Ensure thatpivotis the correct absolute iterator position rather than an offset.
🏁 Script executed:
#!/bin/bash # Description: Verify makeRange function signature and usage to confirm correct arguments # Expected: makeRange should accept (array, begin_iter, end_iter) where end_iter is absolute position # Search for makeRange function definition ast-grep --pattern 'makeRange($_, $_, $_) { $$$ }' # Search for other makeRange usage patterns rg -A 2 -B 2 "makeRange"Length of output: 7607
🏁 Script executed:
#!/bin/bash # Locate and display the context where `pivot` is computed and used rg -n "pivot" src/core/utilities/cellmap.hpp pivot_line=$(rg -n "pivot" src/core/utilities/cellmap.hpp | head -n1 | cut -d: -f1) start=$((pivot_line - 10)); [ $start -lt 1 ] && start=1 end=$((pivot_line + 10)) sed -n "${start},${end}p" src/core/utilities/cellmap.hppLength of output: 1552
🏁 Script executed:
#!/bin/bash # Extract the full partition implementation to verify pivot calculation start_line=$(rg -n "template<typename Range, typename Predicate" -n src/core/utilities/cellmap.hpp | head -n1 | cut -d: -f1) end_line=$((start_line + 200)) sed -n "${start_line},${end_line}p" src/core/utilities/cellmap.hppLength of output: 5883
🏁 Script executed:
#!/bin/bash # Locate the definition of CellMap::partition rg -n "CellMap<.*>::partition" -n src/core/utilities/cellmap.hpp | head -n5 # If found, print the surrounding implementation def_line=$(rg -n "CellMap<.*>::partition" -n src/core/utilities/cellmap.hpp | head -n1 | cut -d: -f1) start=$((def_line - 10)); [ $start -lt 1 ] && start=1 end=$((def_line + 200)) sed -n "${start},${end}p" src/core/utilities/cellmap.hppLength of output: 3460
No change needed: Pivot is already an absolute end index
The overload makeRange(Array&, std::size_t, std::size_t) expects absolute begin/end indices, and here pivot is initialized to range.iend() (an absolute index) and decremented in absolute terms. ReturningmakeRange(range.array(), range.ibegin(), pivot);correctly yields the subrange [ibegin, pivot).
src/amr/resources_manager/resources_manager.hpp (1)
452-452: Minor formatting improvement noted.The formatting adjustment aligns the parameter list properly, improving code readability.
tests/simulator/initialize/test_particles_init_2d.py (1)
26-26: Correct class name fix.The class name change from
Initialization1DTesttoInitialization2DTestproperly reflects that this file contains 2D tests (as evidenced byndim = 2on line 16). This fixes a naming inconsistency.src/core/data/grid/grid.hpp (2)
64-72: Excellent refactoring for centralized initialization.The addition of the copy constructor and removal of field_ initialization from constructors is a well-designed improvement:
- RAII compliance: Field initialization is now centralized at the member declaration, ensuring consistent initialization regardless of constructor used.
- DRY principle: Eliminates code duplication across multiple constructors.
- Copy semantics: The explicit copy constructor properly handles copying the grid data and metadata.
This change aligns with modern C++ best practices for member initialization.
93-93: Centralized member initialization follows best practices.Moving the field_ initialization to the member declaration ensures consistent initialization and eliminates potential issues with constructor ordering.
src/amr/messengers/mhd_hybrid_messenger_strategy.hpp (3)
22-22: Consistent const placement style.The change from
static const std::stringtostatic std::string constfollows the consistent const placement style where const appears after the type it modifies.
69-69: Parameter const qualification improvement.Adding
constqualification to the parameter enhances const-correctness and clearly indicates the parameter won't be modified.
162-163: Static member definition consistency maintained.The static member definition correctly matches the updated declaration, maintaining consistency across the codebase.
src/amr/data/field/refine/field_refine_operator.hpp (3)
9-14: Clean include removal improves compilation efficiency.Removing unused includes reduces compilation dependencies and improves build times. The remaining includes appear sufficient for the class functionality.
27-27: Class hierarchy simplification reduces complexity.Removing the inheritance from
AFieldRefineOperatorsimplifies the class hierarchy and eliminates an unnecessary abstraction layer. Direct inheritance fromSAMRAI::hier::RefineOperatoris more straightforward and maintainable.
36-37: Constructor simplification follows hierarchy changes.The simplified constructor initialization list correctly reflects the removal of the intermediate base class, maintaining proper initialization while reducing complexity.
src/amr/messengers/hybrid_messenger_strategy.hpp (2)
57-57: LGTM: Minor formatting improvement.Removing trailing whitespace improves code cleanliness.
122-127: LGTM: Well-designed interface extension.The new pure virtual methods
fillFluxBordersandfillDensityBordersfollow the established pattern of existing fill methods and maintain consistent parameter naming and types. The interface expansion is coherent with the existing design.src/core/utilities/types.hpp (1)
229-229: LGTM: Proper const qualification.Adding
constto thechar*return fromstd::getenvfollows best practices for immutable data.src/amr/level_initializer/hybrid_level_initializer.hpp (2)
82-97: LGTM: Improved data exchange sequence.The refactored initialization sequence properly separates domain particle deposits from level ghost operations, allowing for explicit border filling between phases. This design enhances the control over data exchange patterns and aligns with the new border filling capabilities.
The comment at line 82 accurately describes the algorithmic change.
99-113: LGTM: Consistent level ghost handling.The second loop correctly handles level ghost deposits and moment computations after border filling is complete. The conditional level ghost deposit based on
!isRootLevel(levelNumber)maintains the existing logic correctly.src/amr/resources_manager/amr_utils.hpp (5)
7-20: LGTM: Improved header organization.Moving SAMRAI headers into a grouped block after project-specific includes improves code organization and readability.
47-48: LGTM: Consistent return type.Changing
AMRToLocalto return a reference to the modified box is consistent with the typical pattern of in-place modification functions and provides better usability.
139-139: LGTM: Improved type deduction.Using
auto constexprinstead ofint constexprfor dimension allows better type deduction while maintaining compile-time evaluation.
213-218: LGTM: Generalized utility function.Converting the
to_stringfunction to a generic template withautoparameter improves reusability while maintaining the same functionality for the original use case.
192-211:Details
❓ Verification inconclusive
Verify the complex box computation logic.
The
makeNonLevelGhostBoxForfunction implements sophisticated logic to compute patch ghost boxes excluding level ghost cells. The algorithm merges overlapping neighbor boxes to determine the final ghost region.
🏁 Script executed:
#!/bin/bash # Verify the usage and integration of makeNonLevelGhostBoxFor ast-grep --pattern 'makeNonLevelGhostBoxFor($$$)'Length of output: 51
Verify
makeNonLevelGhostBoxForLogic and UsagePlease manually confirm the following for the
makeNonLevelGhostBoxForfunction in
src/amr/resources_manager/amr_utils.hpp(lines 192–211):
- The merging of neighbor boxes correctly excludes level-ghost cells in all dimensions, including edge and corner cases at domain boundaries.
- There are no missing neighbor contributions or unintended overlaps when calling
hier_nbrs.getSameLevelNeighbors(…).- Template instantiations (
GridLayoutT) with differentdimensionandnbrParticleGhosts()values behave as expected.- Adequate test coverage exists (or should be added) to validate the computed ghost box for representative grid layouts.
- Ensure this utility is actually invoked where intended; if it’s not used yet, either integrate it or remove dead code.
src/core/data/particles/particle_array.hpp (2)
99-99: LGTM: Simplified method signature.Consolidating the two
eraseoverloads into a single method takingIndexRange_by value eliminates unnecessary forwarding complexity while maintaining the same functionality.
200-206: LGTM: Well-designed delegation with safety check.The templated
partitionmethod properly delegates tocellMap_and includes a defensive assertion to ensure the returned range size doesn't exceed the input range size. The template design allows for flexible range types while maintaining type safety.src/amr/physical_models/hybrid_model.hpp (3)
46-46: Good placement of static member declaration.Moving the static member declaration closer to its usage improves code readability.
53-53: Correct removal of redundant virtual keyword.The
overridespecifier already impliesvirtual, so removing the explicitvirtualkeyword follows C++ best practices and reduces redundancy.Also applies to: 73-73
178-182: Correctly implements population of new messenger info fields.The loop properly adds flux and density fields for each ion population to support the new border summation features.
src/amr/messengers/hybrid_messenger.hpp (2)
334-342: Well-implemented delegation methods for border filling.The new
fillFluxBordersandfillDensityBordersmethods properly follow the established delegation pattern to the strategy object.
354-354: Good use of const correctness for the strategy pointer.Making the unique_ptr itself const prevents accidental reassignment of the strategy after construction while still allowing modification of the strategy object itself.
src/core/utilities/point/point.hpp (2)
133-146: Well-implemented operator+= overloads.Both scalar and array-like overloads are correctly implemented with appropriate use of template template parameters for generic array support.
206-215:⚠️ Potential issueAdd handling for non-4-byte types in as_unsigned().
The method only handles 4-byte types and lacks an else clause, which could lead to undefined behavior for other type sizes.
Add proper handling for all type sizes:
auto as_unsigned() const { PHARE_DEBUG_DO({ for (auto iDim = 0u; iDim < dim; ++iDim) assert(r[iDim] >= 0); }) if constexpr (sizeof(Type) == 4) return Point<std::uint32_t, dim>{this->template toArray<std::uint32_t>()}; - // else no return cause not yet handled + else if constexpr (sizeof(Type) == 8) + return Point<std::uint64_t, dim>{this->template toArray<std::uint64_t>()}; + else + static_assert(sizeof(Type) == 4 || sizeof(Type) == 8, + "as_unsigned() only supports 4-byte and 8-byte types"); }⛔ Skipped due to learnings
Learnt from: PhilipDeegan PR: PHAREHUB/PHARE#998 File: src/core/utilities/point/point.hpp:195-210 Timestamp: 2025-04-17T14:41:53.643Z Learning: In `src/core/utilities/point/point.hpp`, the `as_signed()` and `as_unsigned()` methods are intentionally designed to cause compilation errors for unsupported types (only 4-byte types are supported). This forces developers to implement proper conversions themselves when extending the functionality to new types. The comment "else no return cause not yet handled" indicates this is a deliberate design choice.tests/core/numerics/ion_updater/test_updater.cpp (2)
358-367: Correct introduction of boxing abstraction.The
Boxing_ttype alias andboxingmember properly encapsulate the particle selection logic, with the AMR box correctly grown by the number of particle ghosts.
493-495: Test updates correctly reflect the new particle management approach.The tests have been properly updated to:
- Use the boxing abstraction in all
updatePopulationscalls- Expect zero patch ghost particles, reflecting the new particle management strategy
- Maintain appropriate test coverage for the refactored functionality
Also applies to: 733-733, 779-779, 804-804, 824-824, 842-842
src/amr/utilities/box/amr_box.hpp (3)
33-39: LGTM!The function correctly converts SAMRAI boxes to PHARE boxes with unsigned coordinates, properly reusing existing functionality.
137-149: LGTM!The
as_pointoverloads correctly convert SAMRAI types to PHARE points using appropriate compile-time utilities.
152-157: LGTM!The function correctly shifts boxes using the transformation offset, properly delegating to the core shift function.
src/amr/data/particles/particles_variable_fill_pattern.hpp (2)
19-35: LGTM!The
ParticlesDomainOverlapclass correctly serves as a marker type for particle domain overlaps.
51-78: LGTM!The
calculateOverlapmethod correctly computes overlap regions by growing destination boxes and intersecting with the patch box.src/amr/messengers/field_sum_transaction.hpp (2)
186-222: LGTM!The factory class correctly implements the transaction factory interface with appropriate allocate method and intentional no-op preprocessing.
146-156:Details
✅ Verification successful
Verify that
unpackStreamAndSummethod exists in FieldData_t.The code calls
unpackStreamAndSumon the field data object. Ensure this method is properly implemented in all FieldData types that will be used with this transaction.
🏁 Script executed:
#!/bin/bash # Description: Verify that unpackStreamAndSum method exists in FieldData classes # Search for unpackStreamAndSum method definition ast-grep --pattern 'unpackStreamAndSum($_) { $$$ }' # Also search with rg for any declarations or definitions rg -A 5 "unpackStreamAndSum"Length of output: 1417
Confirmed
unpackStreamAndSumExists in FieldData
- The method is declared and defined in
src/amr/data/field/field_data.hpp:
- Declaration:
void unpackStreamAndSum(SAMRAI::tbox::MessageStream& stream, SAMRAI::hier::BoxOverlap const& overlap);- Definition:
template<typename GridLayoutT, typename Grid_t, typename PhysicalQuantity> void FieldData<GridLayoutT, Grid_t, PhysicalQuantity>::unpackStreamAndSum( SAMRAI::tbox::MessageStream& stream, SAMRAI::hier::BoxOverlap const& overlap) { using PlusEqualOp = core::PlusEquals<value_type>; unpackStream<PlusEqualOp>(stream, overlap, field); }- The call in
src/amr/messengers/field_sum_transaction.hpp(line 155) is therefore valid.No further action required.
src/core/utilities/box/box.hpp (4)
28-28: LGTM!Adding the static
dimensionmember is a good practice for template metaprogramming consistency.
242-257: LGTM!The improved parameter names and template signature enhance code readability.
259-264: LGTM!The particle overload with SFINAE trailing return type is well-designed for type safety.
310-320: LGTM!The
mergemethod correctly computes the minimal bounding box containing both boxes.src/amr/data/field/field_variable_fill_pattern.hpp (4)
32-37: LGTM! Constructor simplification is appropriate.The defaulted parameter makes this the default constructor while maintaining backward compatibility.
61-67: Good use of static member for pattern name.Using a static member for the pattern name is cleaner and more efficient than returning a temporary string.
142-146: Clarify the purpose of adding 1 to the ghost box upper bound.The lambda
_primal_ghost_boxadds 1 to the upper bound after growing by ghost width. This seems arbitrary and could lead to off-by-one errors.Please document why this +1 is necessary or remove it if not needed. Is this related to primal/dual grid indexing?
54-58:Details
❓ Verification inconclusive
Verify the assertion logic for overwrite_interior.
The assertion expects
fn_overwrite_interior == true, but the class comment (lines 19-21) states thatoverwrite_interiordefaults to "false" to avoid overwriting shared border nodes. This appears contradictory.Please verify whether this assertion is correct or if it should be removed/inverted. Run the following script to check how this method is called:
🏁 Script executed:
#!/bin/bash # Search for calls to calculateOverlap to understand the expected fn_overwrite_interior values ast-grep --pattern 'calculateOverlap($$$)' rg -A 5 "calculateOverlap.*fn_overwrite_interior"Length of output: 96
🏁 Script executed:
#!/bin/bash # Inspect the class comment and default for overwrite_interior_ sed -n '1,60p' src/amr/data/field/field_variable_fill_pattern.hpp # Show the constructor(s) to see how overwrite_interior_ is initialized rg -n "FieldVariableFillPattern\s*::\s*FieldVariableFillPattern" -C 2 src/amr/data/field/field_variable_fill_pattern.hpp # Search for any definitions or overloads of calculateOverlap in the repo rg -n "calculateOverlap" -A 2Length of output: 11343
Verify assertion logic for
fn_overwrite_interiorThe class comment (lines 19–21) states that
overwrite_interior_defaults tofalseso shared border nodes aren’t overwritten, yet the override asserts that the SAMRAI-providedfn_overwrite_interiormust always betrue. These behaviors seem at odds—please confirm:
- Does SAMRAI always call this method with
fn_overwrite_interior == true?- If not, should the
assert(fn_overwrite_interior == true)be removed or inverted?Location:
- src/amr/data/field/field_variable_fill_pattern.hpp:54–58
src/amr/solvers/solver_ppc.hpp (2)
99-103: LGTM! Proper cleanup in onRegrid.Clearing the boxing map and resetting the ion updater ensures clean state after regridding.
442-443: Good placement of border filling calls.The flux and density border filling is correctly placed after ion population updates and before ghost particle filling.
src/core/numerics/ion_updater/ion_updater.hpp (3)
119-158: Well-designed boxing abstraction.The
UpdaterSelectionBoxingstruct nicely encapsulates particle selection logic with clear predicates for different particle regions. The lambda captures are properly done to detach from class references.
176-183: Clean refactoring of domain particle update.Good use of local copy to avoid aliasing issues. The boxing selectors make the particle filtering logic much clearer.
244-252: Efficient patch ghost particle extraction.Good approach to extract patch ghosts from particles leaving the domain. The
reserve()call helps avoid reallocations.src/amr/data/particles/particles_data.hpp (2)
231-258: Well-structured overlap type dispatch.The dynamic dispatch based on overlap type is cleanly implemented with appropriate use of dynamic_cast and early returns.
708-719: Correct transformation handling in ghost copy.The use of negative offset to transform from destination to source space is correct. The offseter lambda properly shifts particle coordinates.
src/amr/data/field/field_data.hpp (2)
340-357: Excellent refactoring with operator templates.The use of FieldBox abstraction and operator templates greatly improves code reusability. The same logic can now handle both assignment and accumulation operations.
449-471: Clean implementation of sum operations.The sum and unpackStreamAndSum methods elegantly reuse the existing copy logic with the PlusEquals operator.
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (3)
651-657: Good refactoring with the makeKeys helperThe
makeKeyslambda effectively reduces code duplication and improves readability when extracting vector field names.
718-732: Border sum refiner registration looks correctThe implementation properly creates separate refiners for each population's flux and density fields using the new
FieldGhostInterpOverlapFillPattern. The use ofemplace_backis efficient.
335-343:Details
❌ Incorrect review comment
Verify the architectural change from patch to domain ghost particles
The method now uses
domainGhostPartRefiners_and clearspatchGhostParticles(). This represents a significant architectural change in ghost particle handling.
🏁 Script executed:
#!/bin/bash # Description: Verify that patchGhostParticles are no longer used elsewhere after being cleared # Search for any remaining uses of patchGhostParticles rg -A 3 "patchGhostParticles\(\)" --type cpp # Check if there are any tests that might be affected by this change fd -e cpp -e py test | xargs rg "patchGhost|patch.*ghost" -lLength of output: 8333
Incorrect review concern: patchGhostParticles remains in active use
A global search showspatchGhostParticles()is still referenced throughout core code paths and tests (e.g. inion_updater.hpp,moments.hpp, solver PPC, diagnostics, and numerous unit tests). The local.clear()in the hybrid messenger strategy is simply housekeeping before using the newdomainGhostPartRefiners_, not a wholesale removal of patch‐level ghost handling. You can disregard the original suggestion to verify a global architectural shift away from patch ghosts.Likely an incorrect or invalid review comment.
src/amr/messengers/refiner.hpp (4)
16-24: RefinerType enum changes are properly handledThe removal of
InteriorGhostParticlesandSharedBorderand addition ofPatchFieldBorderSumandExteriorGhostParticlesare correctly handled in theregisterLevelmethod's switch statement.
274-310: Excellent refactoring with helper methodsThe new helper methods (
register_resource,register_time_interpolated_resource,register_vector_field,register_time_interpolated_vector_field) effectively modularize the registration logic and improve code maintainability. The use of structured bindings withgetIDsListis clean and modern.
199-200: Good API design with optional fill pattern parametersThe addition of optional
variableFillPatternparameters with nullptr defaults maintains backward compatibility while enabling new functionality. This is a clean way to extend the API.Also applies to: 217-217, 244-244, 254-254
75-83:Details
❓ Verification inconclusive
Verify the hardcoded overlap width parameter
The
createSchedulecall uses0as the second parameter (overlap width). Please verify this is intentional for border sum operations.
🏁 Script executed:
#!/bin/bash # Description: Check other uses of createSchedule to understand the overlap parameter # Search for other createSchedule calls with explicit overlap values ast-grep --pattern 'createSchedule($_, $NUM, $_)' # Check FieldBorderSumTransactionFactory usage patterns rg -A 5 "FieldBorderSumTransactionFactory"Length of output: 2071
Verify overlap width for PatchFieldBorderSum schedule
The new
createSchedule(level, 0, …)invocation hard-codes the overlap width to0. In other refinement schedules we typically pass the ghost-cell width or rely on the default overload. Please double-check that a zero overlap is intentional for the border-sum transaction (and, if so, consider documenting it or using a named constant for clarity).• File: src/amr/messengers/refiner.hpp
Lines 75–83:algo->createSchedule(level, 0, …)src/amr/messengers/refiner_pool.hpp (2)
31-39: Good practice with explicit special member functionsExplicitly defining the constructor, virtual destructor, deleted copy constructor, and defaulted move constructor follows the Rule of Five and prevents unintended object slicing or copying.
186-191: Excellent error handling for duplicate keysThe addition of runtime exceptions for duplicate key registration prevents silent failures and provides clear error messages. This is good defensive programming.
Also applies to: 239-241, 265-268
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
PhilipDeegan
force-pushed
the
patchghostless
branch
from
6f47b98 to
e08d0bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/core/data/field/field_box.hpp (1)
61-68:⚠️ Potential issueAdd size compatibility check in operate_on_fields.
The function assumes
dstandsrcboxes have the same size but doesn't verify this, which could lead to undefined behavior if they differ.template<typename Operator> void operate_on_fields(auto& dst, auto const& src) { + assert(dst.lcl_box.size() == src.lcl_box.size()); auto src_it = src.lcl_box.begin(); auto dst_it = dst.lcl_box.begin(); for (; dst_it != dst.lcl_box.end(); ++src_it, ++dst_it) Operator{dst.field(*dst_it)}(src.field(*src_it)); }src/amr/resources_manager/resources_manager.hpp (1)
323-331: Consider improving error handling and method naming.The new
getIDsListmethod has several areas for improvement:
Generic error message: The
throw std::runtime_error("bad key")provides minimal debugging information. Consider including the problematic key in the error message.Method naming:
getIDsListis redundant since it returns an array, not a list. Consider renaming togetIDsfor consistency with the existing API.Exception performance: Throwing exceptions in a tight loop could impact performance if multiple keys are invalid.
- auto getIDsList(auto&&... keys) const + template<typename... Keys> + auto getIDs(Keys&&... keys) const { auto const Fn = [&](auto& key) { if (auto const id = getID(key)) return *id; - throw std::runtime_error("bad key"); + throw std::runtime_error("Invalid resource key: " + std::string(key)); }; return std::array{Fn(keys)...}; }src/amr/data/field/field_variable_fill_pattern.hpp (1)
115-124: Add validation for the primal field assumption.The comment states "ASSUMED ALL PRIMAL!" but there's no enforcement of this assumption. Consider adding validation.
Add a static assertion or runtime check to ensure the grid layout is indeed all primal:
template<typename Gridlayout_t> // ASSUMED ALL PRIMAL! class FieldGhostInterpOverlapFillPattern : public SAMRAI::xfer::VariableFillPattern { std::size_t constexpr static dim = Gridlayout_t::dimension; using FieldGeometry_t = FieldGeometryBase<dim>; + + // Validate the primal assumption + static_assert(/* Add appropriate check for primal layout */); public:src/amr/data/field/field_data.hpp (2)
227-228: Improve rotation error handling.Instead of silently returning when rotation is detected, the code should throw an exception for consistency with other error cases.
- if (transformation.getRotation() != NO_ROTATE) - return; // throw, we don't do rotations in phare.... + if (transformation.getRotation() != NO_ROTATE) + throw std::runtime_error("Rotations are not supported in PHARE");
271-272: Fix inconsistent rotation handling.This rotation check also silently returns instead of throwing.
- if (fieldOverlap.getTransformation().getRotation() != NO_ROTATE) - return; + if (fieldOverlap.getTransformation().getRotation() != NO_ROTATE) + throw std::runtime_error("Rotations are not supported in PHARE");
🧹 Nitpick comments (1)
src/amr/messengers/refiner.hpp (1)
274-311: Consider improving template parameter clarity for better maintainability.While the helper functions correctly implement the registration logic and support method chaining, the extensive use of
autoparameters may impact code readability and debugging experience.Consider making template parameters more explicit for better error messages and code clarity:
- auto& register_resource(auto& rm, auto& dst, auto& src, auto& scratch, auto&&... args) + template<typename ResourceManager, typename DstType, typename SrcType, typename ScratchType, typename... Args> + auto& register_resource(ResourceManager& rm, DstType& dst, SrcType& src, ScratchType& scratch, Args&&... args)This would improve:
- Compiler error message clarity
- IDE code completion and navigation
- Code documentation and understanding
However, the current implementation is functionally correct and the auto usage follows modern C++ patterns for generic programming.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
src/amr/data/field/field_data.hpp(10 hunks)src/amr/data/field/field_variable_fill_pattern.hpp(5 hunks)src/amr/data/field/refine/field_refine_operator.hpp(2 hunks)src/amr/data/particles/particles_data.hpp(9 hunks)src/amr/data/particles/particles_variable_fill_pattern.hpp(1 hunks)src/amr/level_initializer/hybrid_level_initializer.hpp(2 hunks)src/amr/messengers/communicator.hpp(1 hunks)src/amr/messengers/field_sum_transaction.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(15 hunks)src/amr/messengers/hybrid_messenger.hpp(2 hunks)src/amr/messengers/hybrid_messenger_info.hpp(1 hunks)src/amr/messengers/hybrid_messenger_strategy.hpp(2 hunks)src/amr/messengers/mhd_hybrid_messenger_strategy.hpp(4 hunks)src/amr/messengers/refiner.hpp(8 hunks)src/amr/messengers/refiner_pool.hpp(5 hunks)src/amr/physical_models/hybrid_model.hpp(3 hunks)src/amr/resources_manager/amr_utils.cpp(1 hunks)src/amr/resources_manager/amr_utils.hpp(5 hunks)src/amr/resources_manager/resources_guards.hpp(2 hunks)src/amr/resources_manager/resources_manager.hpp(5 hunks)src/amr/resources_manager/resources_manager_utilities.hpp(0 hunks)src/amr/solvers/solver_ppc.hpp(8 hunks)src/amr/utilities/box/amr_box.hpp(2 hunks)src/core/data/field/field_box.hpp(1 hunks)src/core/data/grid/grid.hpp(2 hunks)src/core/data/grid/gridlayout.hpp(2 hunks)src/core/data/ions/ion_population/ion_population.hpp(1 hunks)src/core/data/ions/ions.hpp(2 hunks)src/core/data/particles/particle_array.hpp(2 hunks)src/core/numerics/ion_updater/ion_updater.hpp(7 hunks)src/core/numerics/pusher/pusher.hpp(1 hunks)src/core/utilities/box/box.hpp(5 hunks)src/core/utilities/cellmap.hpp(2 hunks)src/core/utilities/point/point.hpp(3 hunks)src/core/utilities/types.hpp(2 hunks)tests/amr/data/field/refine/test_field_refine.cpp(1 hunks)tests/amr/messengers/test_messengers.cpp(2 hunks)tests/amr/resources_manager/test_resources_manager.cpp(2 hunks)tests/core/numerics/ion_updater/test_updater.cpp(8 hunks)tests/simulator/advance/test_particles_advance_1d.py(0 hunks)tests/simulator/advance/test_particles_advance_2d.py(0 hunks)tests/simulator/initialize/test_particles_init_1d.py(0 hunks)tests/simulator/initialize/test_particles_init_2d.py(1 hunks)
💤 Files with no reviewable changes (4)
- src/amr/resources_manager/resources_manager_utilities.hpp
- tests/simulator/initialize/test_particles_init_1d.py
- tests/simulator/advance/test_particles_advance_2d.py
- tests/simulator/advance/test_particles_advance_1d.py
🚧 Files skipped from review as they are similar to previous changes (28)
- src/core/data/ions/ions.hpp
- src/core/data/ions/ion_population/ion_population.hpp
- tests/amr/messengers/test_messengers.cpp
- src/core/numerics/pusher/pusher.hpp
- src/amr/messengers/communicator.hpp
- tests/simulator/initialize/test_particles_init_2d.py
- src/amr/resources_manager/amr_utils.cpp
- tests/amr/data/field/refine/test_field_refine.cpp
- src/core/utilities/cellmap.hpp
- src/amr/data/field/refine/field_refine_operator.hpp
- src/amr/messengers/hybrid_messenger_info.hpp
- src/amr/messengers/mhd_hybrid_messenger_strategy.hpp
- src/amr/level_initializer/hybrid_level_initializer.hpp
- src/core/data/grid/grid.hpp
- src/core/utilities/types.hpp
- src/core/data/particles/particle_array.hpp
- src/amr/resources_manager/amr_utils.hpp
- src/amr/messengers/hybrid_messenger_strategy.hpp
- src/amr/physical_models/hybrid_model.hpp
- src/amr/messengers/hybrid_messenger.hpp
- src/amr/data/particles/particles_variable_fill_pattern.hpp
- src/core/data/grid/gridlayout.hpp
- src/core/utilities/point/point.hpp
- src/amr/utilities/box/amr_box.hpp
- src/amr/messengers/field_sum_transaction.hpp
- src/core/utilities/box/box.hpp
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
- src/amr/messengers/refiner_pool.hpp
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/amr/resources_manager/resources_guards.hppsrc/amr/resources_manager/resources_manager.hppsrc/core/data/field/field_box.hppsrc/amr/data/particles/particles_data.hppsrc/amr/data/field/field_variable_fill_pattern.hppsrc/amr/solvers/solver_ppc.hppsrc/core/numerics/ion_updater/ion_updater.hppsrc/amr/messengers/refiner.hppsrc/amr/data/field/field_data.hpp
🧬 Code Graph Analysis (2)
src/amr/solvers/solver_ppc.hpp (5)
src/amr/messengers/hybrid_messenger_strategy.hpp (4)
hierarchy(52-53)hierarchy(56-59)level(116-116)patch(33-33)src/amr/messengers/hybrid_messenger.hpp (8)
hierarchy(109-113)hierarchy(109-110)hierarchy(120-126)hierarchy(120-123)level(188-188)level(188-188)patch(81-84)patch(81-81)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (14)
hierarchy(151-188)hierarchy(151-152)hierarchy(196-249)hierarchy(196-199)hierarchy(810-993)hierarchy(810-811)level(470-495)level(470-473)level(600-612)level(600-600)level(747-762)level(747-747)patch(115-122)patch(115-115)src/amr/multiphysics_integrator.hpp (6)
hierarchy(301-376)hierarchy(301-306)hierarchy(380-409)hierarchy(381-382)level(500-551)level(500-503)src/amr/resources_manager/amr_utils.hpp (6)
to_string(213-218)to_string(213-213)layoutFromPatch(137-189)layoutFromPatch(137-137)makeNonLevelGhostBoxFor(194-211)makeNonLevelGhostBoxFor(194-195)
src/amr/data/field/field_data.hpp (4)
src/amr/data/field/field_geometry.hpp (3)
box(155-258)box(155-156)box(267-276)src/amr/messengers/field_sum_transaction.hpp (5)
stream(60-60)stream(62-62)stream(66-66)unpackStream(146-156)unpackStream(146-146)src/amr/resources_manager/amr_utils.hpp (5)
AMRToLocal(47-48)AMRToLocal(55-56)AMRToLocal(64-64)AMRToLocal(79-92)AMRToLocal(79-80)src/core/data/field/field_box.hpp (2)
operate_on_fields(62-68)operate_on_fields(62-62)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-latest)
🔇 Additional comments (37)
src/amr/resources_manager/resources_guards.hpp (2)
45-48: LGTM! Clean simplification using fold expressions.The constructor now uses a concise fold expression to set resources directly, eliminating the need for tag structs and improving readability.
56-58: LGTM! Simplified destructor implementation.The destructor correctly calls
unsetResources_on each user using a fold expression, which is cleaner than the previous tag-based approach.tests/amr/resources_manager/test_resources_manager.cpp (2)
213-235: LGTM! Well-structured test for enumerate functionality.The test properly validates the new
enumeratemethod by checking resource user states during and after enumeration. The lambda-based approach maintains consistency with existing test patterns.
270-271: LGTM! Proper test registration.The new test is correctly registered in the typed test suite alongside the existing test.
tests/core/numerics/ion_updater/test_updater.cpp (4)
358-367: LGTM! Clean boxing abstraction integration.The addition of
Boxing_ttype alias and theboxingmember properly encapsulates the particle selection logic, making the test more aligned with the new architecture.
493-498: LGTM! Updated assertions reflect new particle handling.The assertions correctly reflect the new approach where patch ghost particles are not manually created (size 0), while domain and level ghost particles are properly present.
733-734: LGTM! Consistent boxing usage in test calls.All
ionUpdater.updatePopulationscalls have been correctly updated to use theboxingparameter instead of the directlayout, maintaining consistency with the new API.Also applies to: 779-780, 804-805, 824-825, 842-843
663-663: LGTM! Updated expectation for patch ghost particles.The expectation correctly reflects that patch ghost particles are no longer manually created in the test fixture.
src/core/data/field/field_box.hpp (2)
16-58: LGTM! Well-designed field abstraction.The
FieldBoxtemplate class provides a clean, dimension-agnostic interface for field operations. The multiple constructor overloads handle different box types appropriately, and the template parameter design is flexible.
72-88: LGTM! Well-implemented template methods.The
set_fromandappend_tomethods provide clean interfaces for data transfer operations. The use of iterators and the template operator pattern is appropriate.src/core/numerics/ion_updater/ion_updater.hpp (5)
59-61: LGTM! Clean abstraction with boxing template parameter.The refactoring to template key methods on
Boxing_tprovides better separation of concerns and makes the updater more flexible while maintaining type safety.Also applies to: 75-76, 78-79, 90-93
119-158: LGTM! Well-designed particle selection abstraction.The
UpdaterSelectionBoxingstruct effectively encapsulates particle selection logic with appropriate lambda captures and clear predicates. The design separates particle partitioning concerns from the updater logic.
176-185: LGTM! Simplified domain particle handling.The updated logic uses local copies and the boxing selectors effectively. The approach of pushing particles and accumulating those in the non-level ghost box is clean and efficient.
241-252: LGTM! Improved particle array management.The logic correctly handles domain particle pushing, identifies particles that become ghosts, copies non-level ghosts to patch ghost arrays, and properly erases ghost particles from the domain array.
254-271: LGTM! Clean level ghost particle handling.The level ghost particle processing correctly handles pushing, exports particles entering the domain, and interpolates both domain and patch ghost particles appropriately.
src/amr/resources_manager/resources_manager.hpp (3)
336-387: Well-designed RAII iterator pattern for patch-level resource management.The
enumeratemethod andLevelLooperstruct implement a clean iterator pattern with automatic resource management. The design ensures resources are properly set when accessing patches and unset when the iterator is destroyed, preventing resource leaks.
465-478: Clean refactoring of sub-resource handling logic.The
handle_sub_resourcesstatic method effectively consolidates the duplicate logic for handling both runtime and compile-time sub-resource lists, improving code maintainability.
482-509: Effective use of the helper function to reduce code duplication.The refactored
setResources_andunsetResources_methods now cleanly delegate sub-resource handling to thehandle_sub_resourceshelper, improving code clarity and maintainability.src/amr/data/field/field_variable_fill_pattern.hpp (1)
33-68: Good improvements to the FieldFillPattern class.The changes improve the class by:
- Making the constructor more flexible with a defaulted parameter
- Adding an assertion to validate expected behavior
- Using a static string for the pattern name, which is more efficient
src/amr/data/particles/particles_data.hpp (5)
244-260: Dynamic dispatch pattern properly handles multiple overlap types.The copy method correctly implements dynamic dispatch based on overlap type, handling both
ParticlesDomainOverlapandCellOverlapcases with appropriate error handling for unknown types.
276-295: Correct stream size calculation for new overlap type.The method properly calculates the data stream size for
ParticlesDomainOverlapby counting patch ghost particles in the transformed overlap boxes.
342-356: Consistent implementation of dynamic dispatch in packStream.The method follows the same clean dispatch pattern as the copy method, maintaining consistency across the codebase.
700-721: Well-implemented particle transfer with coordinate transformation.The
copy_from_ghostmethod correctly exports particles from patch ghost to domain particles, applying the necessary coordinate transformations through the offseter function.
760-773: Simple and correct unpacking implementation.The
unpack_from_ghostmethod correctly unpacks particles directly into the domain array, as the particles are already in the destination coordinate space.src/amr/solvers/solver_ppc.hpp (4)
99-104: Proper cleanup of cached data on regrid.The
onRegridmethod correctly clears the boxing cache and resets the ion updater when the grid structure changes, ensuring stale data is not used.
153-168: Well-implemented boxing initialization with proper error handling.The
make_boxesmethod correctly initializes boxing data for all patches in a level, with proper error handling for map insertion failures. The structured binding properly uses the iterator variable, addressing the previous unused variable issue.
170-176: Clean helper method for level setup.The
setup_levelmethod provides a clean interface for ensuring boxing data is initialized before use, following good defensive programming practices.
429-443: Proper integration of boxing mechanism and border filling.The modified
moveIons_method correctly:
- Uses the cached boxing data for ion updates
- Adds necessary border filling operations for flux and density after ion movement
This aligns well with the new ghost particle handling approach.
src/amr/data/field/field_data.hpp (2)
230-250: Excellent refactoring to generic operator-based field operations.The refactoring to use
FieldBoxwith generic operators is a significant improvement:
- Eliminates dimension-specific code duplication
- Enables easy extension with new operators (e.g.,
SetEqualOp,PlusEqualOp)- Improves code clarity and maintainability
This is a well-designed abstraction that follows modern C++ best practices.
Also applies to: 340-357
323-325: Clean implementation of summation operations.The new
sumandunpackStreamAndSummethods elegantly reuse the existing generic infrastructure by simply changing the operator toPlusEqualOp. This demonstrates the flexibility and power of the operator-based design.Also applies to: 449-472
src/amr/messengers/refiner.hpp (7)
7-11: LGTM: Appropriate includes added for new functionality.The new includes support the enhanced functionality:
field_sum_transaction.hppfor border sum operations, and standard library headers for tuple destructuring and exception handling.
22-24: LGTM: Enum values updated to reflect new refiner types.The addition of
PatchFieldBorderSumandExteriorGhostParticlesaligns with the refactoring objectives and new transaction mechanisms.
75-83: LGTM: Consistent implementation pattern for new refiner type.The
PatchFieldBorderSumconditional block follows the established pattern and correctly uses theFieldBorderSumTransactionFactorywith the appropriate template parameter.
128-132: LGTM: Simple and consistent implementation for exterior ghost particles.The implementation follows the same pattern as
PatchGhostFieldand is appropriate for theExteriorGhostParticlesrefiner type.
199-200: LGTM: Optional fill pattern support added consistently.The addition of the optional
VariableFillPatternparameter withnullptrdefault maintains backward compatibility while enabling new functionality.Also applies to: 216-218
204-206: LGTM: Constructor refactored to use helper functions.The replacement of inline logic with dedicated helper functions improves code maintainability and reduces duplication.
Also applies to: 221-223
241-258: LGTM: Consistent parameter naming and implementation.The constructor updates maintain consistency with other overloads and correctly handle the optional fill pattern parameter.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/amr/data/field/field_data.hpp (2)
227-228: Good improvement in error handling.The rotation check now properly throws an exception instead of silently returning, addressing the previous review feedback.
271-272: Consistent error handling for rotations.This rotation check also properly throws an exception, maintaining consistency with the fix in the packStream method.
🧹 Nitpick comments (4)
src/core/numerics/ion_updater/ion_updater.hpp (4)
27-27: Remove unused type alias.The
Thistype alias is defined but never used in the class.- using This = IonUpdater<Ions, Electromag, GridLayout>;
188-199: Consider extracting lambda into a named function.The
pushAndAccumulateGhostslambda is complex and could benefit from being extracted into a separate method for better readability and testability.+ template<typename Boxing_t> + void pushAndAccumulateGhosts_(auto const& inputArray, auto& pop, + Electromag const& em, Boxing_t const& boxing) { + tmp_particles_ = inputArray; // work on local copy + auto outRange = makeIndexRange(tmp_particles_); + auto enteredInDomain = pusher_->move(outRange, outRange, em, pop.mass(), interpolator_, + boxing.layout, boxing.inGhostBox, boxing.inDomainBox); + interpolator_(enteredInDomain, pop.density(), pop.flux(), boxing.layout); + } - auto pushAndAccumulateGhosts = [&](auto const& inputArray) { - tmp_particles_ = inputArray; // work on local copy - auto outRange = makeIndexRange(tmp_particles_); - auto enteredInDomain = pusher_->move(outRange, outRange, em, pop.mass(), interpolator_, - layout, boxing.inGhostBox, boxing.inDomainBox); - interpolator_(enteredInDomain, pop.density(), pop.flux(), layout); - }; if (pop.levelGhostParticles().size()) - pushAndAccumulateGhosts(pop.levelGhostParticles()); + pushAndAccumulateGhosts_(pop.levelGhostParticles(), pop, em, boxing);
201-210: Address the TODO comment.The TODO comment indicates uncertainty about the implementation logic. This should be resolved before merging to ensure correctness.
The comment suggests there might be issues with how particle contributions are handled for different ghost layer types. Would you like me to help clarify the logic or create a tracking issue for this?
243-250: Potential performance issue with vector operations.The code performs multiple vector operations (partition, reserve, copy, erase) that could be optimized. Consider if these operations can be combined or if a more efficient data structure could be used.
Consider using a single pass algorithm or custom container operations to reduce the number of vector manipulations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/amr/data/field/field_data.hpp(10 hunks)src/amr/messengers/mhd_hybrid_messenger_strategy.hpp(4 hunks)src/amr/utilities/box/amr_box.hpp(2 hunks)src/core/data/field/field_box.hpp(1 hunks)src/core/data/grid/gridlayout.hpp(2 hunks)src/core/numerics/ion_updater/ion_updater.hpp(7 hunks)src/core/utilities/box/box.hpp(5 hunks)src/core/utilities/point/point.hpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/amr/messengers/mhd_hybrid_messenger_strategy.hpp
- src/amr/utilities/box/amr_box.hpp
- src/core/data/grid/gridlayout.hpp
- src/core/utilities/point/point.hpp
- src/core/data/field/field_box.hpp
- src/core/utilities/box/box.hpp
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/numerics/ion_updater/ion_updater.hppsrc/amr/data/field/field_data.hpp
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
🔇 Additional comments (4)
src/core/numerics/ion_updater/ion_updater.hpp (2)
119-157: Well-designed particle selection abstraction.The
UpdaterSelectionBoxingstruct effectively encapsulates particle selection logic and provides clear, reusable selectors. The use of lambda copy captures ensures proper detachment from class references during copy construction.
175-183:Details
❓ Verification inconclusive
Verify particle copy semantics and performance.
The domain particles are copied locally (
tmp_particles_ = pop.domainParticles()), which could be expensive for large particle arrays. Consider if this copy is necessary or if working with references/views would be more efficient.
🏁 Script executed:
#!/bin/bash # Description: Check if there are alternative approaches to particle copying in the codebase # Expected: Find patterns of particle array manipulation that might avoid copying rg -A 5 -B 5 "domainParticles\(\)" --type cppLength of output: 21496
Evaluate necessity of deep-copying domain particles for each update
The call
auto& domain = (tmp_particles_ = pop.domainParticles());per population causes a full copy of the ParticleArray on every invocation of the updater. This may be expensive for large domains.
• Confirm whether
pusher_->move(...)actually mutates its input container. If it does not, you can avoid the copy by working directly onpop.domainParticles()via a non-owning view (e.g.std::spanor your index‐range type).
• If mutation is required, consider reusing a pre-allocatedtmp_particles_buffer (reserve once outside the loop) to eliminate repeated allocations and copies.Location:
- src/core/numerics/ion_updater/ion_updater.hpp around line 175
src/amr/data/field/field_data.hpp (2)
263-292: Well-designed generic unpackStream method.The templated approach with operator support provides excellent flexibility for different operation types (assignment vs. summation). The design is clean and follows the single responsibility principle.
340-357: Excellent use of FieldBox abstraction.The refactored copy method using
core::FieldBoxprovides a much cleaner interface and better encapsulates the field operations. The generic operator support enhances reusability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pyphare/pyphare/pharesee/hierarchy/fromsim.py (2)
87-92: Update outdated comments referencing patchGhost.The comments still reference patchGhost processing logic, but the code below (line 93) no longer processes patchGhost particles. Consider updating these comments to reflect the current implementation.
- # ok now let's add the patchGhost if present - # note that patchGhost patches may not be the same list as the - # domain patches... since not all patches may not have patchGhost while they do have - # domain... while looping on the patchGhost items, we need to search in - # the already created patches which one to which add the patchGhost particles + # ok now let's add the levelGhost if present + # note that levelGhost patches may not be the same list as the + # domain patches... since not all patches may not have levelGhost while they do have + # domain... while looping on the levelGhost items, we need to search in + # the already created patches which one to which add the levelGhost particles
100-115: Update variable names for clarity.The variable
patchGhost_part(line 100) and related comments still reference patchGhost terminology, but this code now only processes levelGhost particles.- patchGhost_part = Particles( + levelGhost_part = Particles( icells=np.asarray(dwpatch.data.iCell), deltas=np.asarray(dwpatch.data.delta), v=v, weights=np.asarray(dwpatch.data.weight), charges=np.asarray(dwpatch.data.charge), ) box = Box(dwpatch.lower, dwpatch.upper) - # now search which of the already created patches has the same box - # once found we add the new particles to the ones already present + # search for the patch with matching box and add levelGhost particles patch = [p for p in patches[ilvl] if p.box == box][0] patch.patch_datas[pop + "_particles"].dataset.add( - patchGhost_part + levelGhost_part )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
pyphare/pyphare/pharein/diagnostics.py(1 hunks)pyphare/pyphare/pharesee/hierarchy/fromh5.py(1 hunks)pyphare/pyphare/pharesee/hierarchy/fromsim.py(1 hunks)pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py(0 hunks)pyphare/pyphare/pharesee/run/run.py(1 hunks)src/diagnostic/detail/types/fluid.hpp(2 hunks)src/diagnostic/detail/types/particle.hpp(1 hunks)src/python3/patch_level.hpp(0 hunks)tests/diagnostic/__init__.py(1 hunks)tests/diagnostic/test_diagnostics.hpp(1 hunks)tests/diagnostic/test_diagnostics.ipp(1 hunks)tests/functional/conservation/conserv.py(1 hunks)tests/simulator/initialize/test_particles_init_1d.py(0 hunks)tests/simulator/initialize/test_particles_init_2d.py(1 hunks)tests/simulator/test_advance.py(1 hunks)tests/simulator/test_initialization.py(1 hunks)
💤 Files with no reviewable changes (3)
- src/python3/patch_level.hpp
- tests/simulator/initialize/test_particles_init_1d.py
- pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py
✅ Files skipped from review due to trivial changes (3)
- tests/diagnostic/init.py
- pyphare/pyphare/pharesee/hierarchy/fromh5.py
- src/diagnostic/detail/types/particle.hpp
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/diagnostic/test_diagnostics.hppsrc/diagnostic/detail/types/fluid.hpp
🧬 Code Graph Analysis (1)
tests/simulator/initialize/test_particles_init_2d.py (2)
tests/simulator/initialize/test_fields_init_2d.py (1)
Initialization2DTest(22-43)tests/simulator/test_initialization.py (1)
InitializationTest(28-722)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (12)
tests/functional/conservation/conserv.py (1)
87-87: LGTM: Consistent with patchGhost removal.The removal of "patchGhost" from particle diagnostics aligns with the broader codebase refactoring to eliminate patch ghost particle handling.
tests/diagnostic/test_diagnostics.ipp (1)
68-68: LGTM: Clean removal of patchGhost diagnostics.The removal of patchGhost diagnostic dictionary additions is consistent with the broader refactoring. The code maintains clean structure and proper syntax.
pyphare/pyphare/pharesee/hierarchy/fromsim.py (1)
93-93: LGTM: Correct implementation of levelGhost-only processing.The change to only iterate over "levelGhost" is consistent with the broader patchGhost removal refactoring.
pyphare/pyphare/pharein/diagnostics.py (1)
306-306: LGTM: Correct removal of patchGhost from valid quantities.Removing "patchGhost" from the particle_quantities list ensures validation consistency with the broader patchGhost removal refactoring.
tests/diagnostic/test_diagnostics.hpp (2)
229-229: LGTM: Correct adjustment of expected file count.Reducing expectedPopAttrFiles from 5 to 4 correctly reflects the removal of patchGhost files from the population attribute files, maintaining test accuracy.
246-250:Details
✅ Verification successful
Verify the math: expectedPopAttrFiles calculation.
With patchGhost removed, each population now contributes 4 files: domain, levelGhost, density, and flux. For 2 populations, this gives 2 × 4 = 8 files. However, the expectation check at line 287 uses
expectedPopNbr * expectedPopAttrFileswhich would be 2 × 4 = 8. Please verify this matches the actual number of population-specific files being created.
🏁 Script executed:
#!/bin/bash # Description: Verify the number of h5FileTypes entries that contain "/ions/pop/" # Expected: Should match expectedPopNbr * expectedPopAttrFiles = 2 * 4 = 8 echo "Searching for population-specific h5FileTypes in the test code..." rg -A 10 -B 5 "h5FileTypes.*emplace_back.*ions/pop" tests/diagnostic/test_diagnostics.hppLength of output: 232
🏁 Script executed:
#!/bin/bash # List all h5FileTypes.emplace_back calls with their line numbers echo "=== All emplace_back calls in test_diagnostics.hpp ===" rg -n "h5FileTypes.emplace_back" tests/diagnostic/test_diagnostics.hpp # Count total emplace_back occurrences in this file echo echo "=== Total emplace_back count ===" rg -c "h5FileTypes.emplace_back" tests/diagnostic/test_diagnostics.hpp # Show context around ionsPopPath to isolate population loop echo echo "=== Context around ionsPopPath ===" rg -n -A 5 -B 2 "ionsPopPath" tests/diagnostic/test_diagnostics.hppLength of output: 2042
No action required: population-specific file count is correct
The constant
expectedPopAttrFilesis set to 4 (lines 229–231), matching the fourh5FileTypes.emplace_backcalls (domain, levelGhost, density, flux) per population (lines 246–249). At runtime this loop runsexpectedPopNbrtimes, so
expectedPopNbr × expectedPopAttrFiles = 2 × 4 = 8
correctly reflects the total population-specific files created.– No changes needed.
tests/simulator/test_advance.py (1)
170-173: LGTM: Consistent removal of patchGhost particle diagnostics.The removal of "patchGhost" from the particle diagnostics quantities is consistent with the broader refactor to eliminate patchGhost particle support throughout the codebase.
pyphare/pyphare/pharesee/run/run.py (1)
205-205: LGTM: Consistent removal of patchGhost from mass attribute lookup.The removal of "patchGhost" from the list of quantities in the GetMass method aligns with the systematic elimination of patchGhost particle support. The method will no longer attempt to read mass attributes from patchGhost particle files.
tests/simulator/test_initialization.py (1)
187-192: LGTM: Consistent removal of patchGhost particle diagnostics.The removal of "patchGhost" from the particle diagnostics quantities list is consistent with the codebase-wide refactor to eliminate patchGhost particle support.
src/diagnostic/detail/types/fluid.hpp (2)
99-104: LGTM: Removal of patchGhost particles from momentum tensor computation.The removal of
patchGhostPartsindex range from the momentum tensor interpolation is consistent with the broader elimination of patchGhost particle support. The computation now only uses domain and level ghost particles, which should provide sufficient coverage.
124-129: LGTM: Consistent removal in population-specific momentum tensor computation.The removal of
patchGhostPartsindex range in the population-specific momentum tensor computation path maintains consistency with the changes in the total momentum tensor computation above.tests/simulator/initialize/test_particles_init_2d.py (1)
26-26: LGTM: Bug fix - correcting class name to match test dimension.The class name change from
Initialization1DTesttoInitialization2DTestcorrectly reflects that this file tests 2D particle initialization (ndim = 2, usesBox2D). This fixes the naming inconsistency.
| std::shared_ptr<SAMRAI::hier::RefineOperator> const& refineOp, Key const& key, | ||
| std::shared_ptr<SAMRAI::xfer::VariableFillPattern> fillPattern) | ||
| { | ||
| auto const [it, success] |
Check notice
Code scanning / CodeQL
Unused local variable
| std::shared_ptr<SAMRAI::hier::TimeInterpolateOperator> const& timeOp, std::string const& key, | ||
| std::shared_ptr<SAMRAI::xfer::VariableFillPattern> fillPattern) | ||
| { | ||
| auto const [it, success] = refiners_.insert( |
Check notice
Code scanning / CodeQL
Unused local variable
| auto& levelBoxing = boxing[lvlNbr]; // creates if missing | ||
|
|
||
| for (auto const& patch : level) | ||
| if (auto [it, suc] = levelBoxing.try_emplace( |
Check notice
Code scanning / CodeQL
Unused local variable
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/amr/data/particles/particles_data.hpp (1)
250-260: Consider refactoring dynamic dispatch pattern for consistency.The dynamic dispatch logic works correctly but could benefit from a more systematic approach. As noted in previous comments, consider refactoring this pattern similar to the pack method for better maintainability and consistency across the codebase.
void copy(SAMRAI::hier::PatchData const& source, SAMRAI::hier::BoxOverlap const& overlap) override { auto& pSource = dynamic_cast<ParticlesData const&>(source); - - if (auto particleOverlap = dynamic_cast<ParticlesDomainOverlap const*>(&overlap)) - copy_from_ghost(pSource, *particleOverlap); - - else if (auto pOverlap = dynamic_cast<SAMRAI::pdat::CellOverlap const*>(&overlap)) - copy_from_cell_overlap(pSource, *pOverlap); - - else - throw std::runtime_error("Unknown overlap type"); + + std::visit([&](auto&& overlap_impl) { + using T = std::decay_t<decltype(overlap_impl)>; + if constexpr (std::is_same_v<T, ParticlesDomainOverlap>) + copy_from_ghost(pSource, overlap_impl); + else if constexpr (std::is_same_v<T, SAMRAI::pdat::CellOverlap>) + copy_from_cell_overlap(pSource, overlap_impl); + else + static_assert(false, "Unknown overlap type"); + }, overlap); }src/amr/solvers/solver_ppc.hpp (1)
153-168: Fix unused variable warning from static analysis.The static analysis correctly identifies that the iterator
itfromtry_emplaceis unused. Either use the iterator for additional operations or simplify the insertion check.- for (auto const& patch : level) - if (auto [it, suc] = levelBoxing.try_emplace( - amr::to_string(patch->getGlobalId()), - Boxing_t{amr::layoutFromPatch<GridLayout>(*patch), - amr::makeNonLevelGhostBoxFor<GridLayout>(*patch, hierarchy)}); - !suc) - throw std::runtime_error("boxing map insertion failure"); + for (auto const& patch : level) + { + auto [_, success] = levelBoxing.try_emplace( + amr::to_string(patch->getGlobalId()), + Boxing_t{amr::layoutFromPatch<GridLayout>(*patch), + amr::makeNonLevelGhostBoxFor<GridLayout>(*patch, hierarchy)}); + if (!success) + throw std::runtime_error("boxing map insertion failure"); + }
🧹 Nitpick comments (2)
src/core/numerics/moments/moments.hpp (1)
54-55: Good addition of explicit error handling.The
std::runtime_errorprovides clear feedback for unsupported deposit tags, which is better than undefined behavior. Consider usingstatic_assertif the deposit tag types can be validated at compile time for better performance.else - throw std::runtime_error("unknown deposit tag"); + static_assert(std::is_same_v<DepositTag, DomainDeposit> || + std::is_same_v<DepositTag, LevelGhostDeposit>, + "unknown deposit tag");src/amr/solvers/solver_ppc.hpp (1)
179-180: Consider using numeric patch IDs for better performance.The nested
unordered_mapstructure is well-designed for organizing boxing data by level and patch. However, using string patch IDs (amr::to_string(patch->getGlobalId())) as map keys might have performance implications. Consider using numeric patch IDs directly if this structure is accessed frequently during simulation loops.-std::unordered_map<int /*level*/, std::unordered_map<std::string /*patchid*/, Boxing_t>> boxing; +std::unordered_map<int /*level*/, std::unordered_map<SAMRAI::tbox::GlobalId /*patchid*/, Boxing_t>> boxing;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/amr/data/particles/particles_data.hpp(14 hunks)src/amr/data/particles/refine/particles_data_split.hpp(1 hunks)src/amr/solvers/solver_ppc.hpp(9 hunks)src/core/numerics/moments/moments.hpp(2 hunks)tests/amr/data/particles/copy/test_particledata_copyNd.cpp(4 hunks)tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp(2 hunks)tests/amr/data/particles/stream_pack/test_main.cpp(4 hunks)tests/core/numerics/ion_updater/test_updater.cpp(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/numerics/moments/moments.hppsrc/amr/data/particles/refine/particles_data_split.hppsrc/amr/data/particles/particles_data.hppsrc/amr/solvers/solver_ppc.hpp
🧬 Code Graph Analysis (1)
tests/amr/data/particles/copy/test_particledata_copyNd.cpp (1)
src/core/utilities/types.hpp (2)
ConstArray(162-168)ConstArray(162-162)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
🔇 Additional comments (17)
tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp (1)
114-142: LGTM: Consistent update to test expectations.The test assertions have been systematically updated to verify particles in
domainParticlesinstead ofpatchGhostParticles, which aligns with the broader refactoring to remove patch ghost particle handling. The test logic remains sound and all particle attribute checks are preserved.tests/amr/data/particles/stream_pack/test_main.cpp (1)
122-267: LGTM: Systematic test update maintains integrity.All test assertions have been consistently updated to verify
domainParticlesinstead ofpatchGhostParticles. The particle streaming and packing test logic remains intact, and all particle attribute validations (velocity, cell index, delta, weight, charge) are preserved across all test functions.src/core/numerics/moments/moments.hpp (1)
5-5: LGTM: Proper header inclusion for error handling.Adding
<stdexcept>header is correct for thestd::runtime_errorusage introduced in the function.src/amr/data/particles/refine/particles_data_split.hpp (1)
175-241: LGTM: Simplified and cleaner particle refinement logic.The removal of
srcGhostParticlesiteration simplifies the refinement process while preserving all the essential logic for particle splitting, position conversion to fine grid, and conditional copying based onsplitType. The code is now more focused and easier to understand.tests/core/numerics/ion_updater/test_updater.cpp (5)
358-358: Good use of type alias for readability.The
Boxing_ttype alias improves code readability and makes the template instantiation more manageable.
367-367: Appropriate boxing initialization.The boxing object is correctly initialized with the layout and a grown AMR box using the particle ghost count, which aligns with the new boxing-based particle selection approach.
493-498: Consistent test assertions with refactoring goals.The assertions now correctly expect domain and level ghost particles to be present while expecting patch ghost particles to be absent (size 0), which aligns with the systematic removal of patch ghost particle handling.
731-731: Correct parameter change to boxing approach.The function call correctly uses
this->boxinginstead ofthis->layout, which is consistent with the new boxing-based particle selection mechanism.
661-661: Updated test expectation reflects refactoring.The expectation that
patchGhostParticles()size equals 0 is correct given the systematic removal of patch ghost particle creation and management.tests/amr/data/particles/copy/test_particledata_copyNd.cpp (3)
128-135: Good test isolation practice.The explicit clearing of particle containers and verification of clean state between test runs is excellent practice. This ensures test independence and prevents state leakage between test cases.
206-207: Correctly updated expectations for architectural change.The test expectations have been properly updated to reflect that particles copied during overlap operations now reside in
domainParticlesrather thanpatchGhostParticles. This aligns with the architectural shift described in the PR.
269-271: Consistent expectation updates across test cases.These assertion updates are consistent with the broader architectural change and correctly verify the new behavior where transformed particles are stored in
domainParticles.src/amr/data/particles/particles_data.hpp (3)
695-770: Well-structured template implementations.The template method implementations demonstrate good C++ practices:
- Proper separation from class definition
- Modern C++ features (structured bindings, auto)
- Consistent error handling and logging
- Clear variable naming and const correctness
707-715: Correct coordinate transformation implementation.The coordinate offset handling is implemented correctly with proper forward and inverse transformations. The offseter lambda provides a clean encapsulation of the transformation logic that's reused consistently across different operations.
131-134:Details
❓ Verification inconclusive
Verify that excluding patchGhostParticles from restart doesn't break functionality.
The
patchGhostParticlesare no longer being serialized during restart. Ensure this is intentional and that these particles can be properly reconstructed from neighbor patches upon restart without losing critical simulation state.
🏁 Script executed:
#!/bin/bash # Check if patchGhostParticles are being reconstructed elsewhere after restart rg -A 5 -B 5 "patchGhostParticles.*push_back\|patchGhostParticles.*insert\|patchGhostParticles.*export_particles"Length of output: 115
🏁 Script executed:
#!/bin/bash # Identify the particles_data.hpp file file=$(fd particles_data.hpp -t f) echo "Using file: $file" # Display the serialization (putToRestart) and deserialization (getFromRestart) functions echo "----- putToRestart -----" sed -n '/putToRestart/,/}/p' "$file" echo "----- getFromRestart -----" sed -n '/getFromRestart/,/}/p' "$file" # Search for any references to patchGhostParticles throughout the repo echo "----- References to patchGhostParticles -----" rg -n "patchGhostParticles" -C3Length of output: 26634
🏁 Script executed:
#!/bin/bash file=src/amr/data/particles/particles_data.hpp echo "putParticles calls in putToRestart():" grep -n "putParticles" "$file" | sed -n '131,140p' echo echo "getParticles calls in getFromRestart():" grep -n "getParticles" "$file" | sed -n '140,200p'Length of output: 396
Confirm transient nature of patchGhostParticles on restart
- The calls to putParticles/getParticles now cover only domainParticles, levelGhostParticlesNew/Old, and omit patchGhostParticles.
- Ensure that patchGhostParticles are meant to be ephemeral and that the ghost‐exchange or fill‐pattern machinery will reliably repopulate them immediately after a restart, before any code uses them.
- If persistent state for patchGhostParticles is required across restarts, either reintroduce their serialization in putToRestart/getFromRestart or document and provide a reconstruction step in the restart workflow.
src/amr/solvers/solver_ppc.hpp (2)
435-436: Well-positioned border filling functionality.The new
fillFluxBordersandfillDensityBorderscalls are properly positioned before particle and moment ghost filling. This ordering ensures that border data is updated before dependent operations, which should improve boundary condition accuracy.
99-103: Essential cleanup for AMR regridding.The
onRegrid()method properly cleans up state that becomes invalid when the grid structure changes. Clearing the boxing data and resetting the ion updater ensures that cached grid-dependent information is rebuilt correctly after regridding operations.
| // TYPED_TEST(AParticlesDataND, copiesSourceDomainParticleIntoGhostForDomainSrcOverGhostDest) | ||
| // { | ||
| // static constexpr auto dim = TypeParam{}(); |
Check notice
Code scanning / CodeQL
Commented-out code
| // // and in first ghost of the destination patchdata | ||
|
|
||
| this->particle.iCell = ConstArray<int, dim>(6); | ||
| // this->particle.iCell = ConstArray<int, dim>(6); |
Check notice
Code scanning / CodeQL
Commented-out code
| // this->sourceData.domainParticles.push_back(this->particle); | ||
| // this->destData.copy(this->sourceData); |
Check notice
Code scanning / CodeQL
Commented-out code
| // ASSERT_THAT(this->destData.patchGhostParticles.size(), Eq(1)); | ||
| // ASSERT_THAT(this->destData.domainParticles.size(), Eq(0)); | ||
| // } |
Check notice
Code scanning / CodeQL
Commented-out code
PhilipDeegan
force-pushed
the
patchghostless
branch
from
95f5a2d to
3fb8a42
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
tests/amr/data/particles/copy/test_particledata_copyNd.cpp (1)
65-79: Remove commented test or add proper documentation.This entire test is commented out without explanation. Based on the past review comments and static analysis, this should either be removed entirely if obsolete, or properly documented if temporarily disabled.
-// TYPED_TEST(AParticlesDataND, copiesSourceDomainParticleIntoGhostForDomainSrcOverGhostDest) -// { -// static constexpr auto dim = TypeParam{}(); - -// // particle is in the domain of the source patchdata -// // and in first ghost of the destination patchdata - -// this->particle.iCell = ConstArray<int, dim>(6); - -// this->sourceData.domainParticles.push_back(this->particle); -// this->destData.copy(this->sourceData); - -// ASSERT_THAT(this->destData.patchGhostParticles.size(), Eq(1)); -// ASSERT_THAT(this->destData.domainParticles.size(), Eq(0)); -// } +// NOTE: Test removed due to architectural change - patchGhostParticles are no longer +// used for domain-to-ghost copying. Particles now go to domainParticles instead.src/amr/data/field/field_data.hpp (1)
457-469: 🛠️ Refactor suggestionConsider adding compatibility validation.
The
summethod should verify that source and destination have compatible quantities before performing summation.+template<typename GridLayoutT, typename Grid_t, typename PhysicalQuantity> +void FieldData<GridLayoutT, Grid_t, PhysicalQuantity>::sum(SAMRAI::hier::PatchData const& src, + SAMRAI::hier::BoxOverlap const& overlap) +{ + using PlusEqualOp = core::PlusEquals<value_type>; + + TBOX_ASSERT_OBJDIM_EQUALITY2(*this, src); + + auto& fieldSource = dynamic_cast<FieldData const&>(src); + auto& fieldOverlap = dynamic_cast<FieldOverlap const&>(overlap); + + TBOX_ASSERT(quantity_ == fieldSource.quantity_); + + copy_<PlusEqualOp>(fieldSource, fieldOverlap, field); +}src/amr/data/particles/particles_data.hpp (1)
250-260: Dynamic dispatch pattern successfully implemented.This addresses the past review comment requesting refactoring with the "if (cast) {} else if (cast) else throw" pattern. The implementation correctly uses
dynamic_castfor type checking and provides appropriate error handling.src/amr/messengers/refiner.hpp (1)
256-256: Address static analysis warnings for unused variables.Static analysis has flagged unused local variables in the structured binding declarations. These appear to be legitimate warnings that should be addressed.
The structured bindings like
auto&& [idDst, idSrc] = rm->getIDsList(dst, src);generate unused variable warnings. Consider using[[maybe_unused]]attribute or restructuring to avoid unused variables.Also applies to: 271-271, 276-276, 285-285
🧹 Nitpick comments (4)
src/amr/data/particles/particles_data.hpp (1)
692-770: Well-implemented template methods with proper coordinate handling.The template method implementations correctly handle coordinate transformations and particle export operations. The move outside the class definition improves readability.
One minor suggestion: In
unpack_from_ghost(line 757), thepOverlapparameter is marked as unused with/*pOverlap*/. Consider using[[maybe_unused]]attribute for better consistency with modern C++ practices.-void ParticlesData<ParticleArray_t>::unpack_from_ghost(SAMRAI::tbox::MessageStream& stream, - ParticlesDomainOverlap const& /*pOverlap*/) +void ParticlesData<ParticleArray_t>::unpack_from_ghost(SAMRAI::tbox::MessageStream& stream, + [[maybe_unused]] ParticlesDomainOverlap const& pOverlap)src/amr/solvers/solver_ppc.hpp (1)
153-181: Well-structured boxing mechanism with proper error handling.The boxing implementation provides clean separation of ion updater selections per patch and level. The error handling for map insertion failures is appropriate.
Consider the performance implications of the nested
unordered_mapstructure for large hierarchies, though this is likely acceptable for typical use cases.src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
350-370: Simplify loop iteration pattern.The current pattern using
ions.begin() + iwith manual indexing could be simplified and made more readable using range-based loops or direct iteration.- for (std::size_t i = 0; i < ions.size(); ++i) - { // ... - auto& pop = *(ions.begin() + i); + for (auto& pop : ions) + { // ... (adjust refiners access accordingly)Note: This would require adjusting the refiner access pattern to not rely on the index
i.Also applies to: 378-395
src/amr/messengers/refiner.hpp (1)
274-310: Consider explicit parameter types for better code clarity.While the use of
autoparameters provides template flexibility, it reduces code readability and may complicate debugging. Consider using explicit template parameters or concepts to improve clarity.- auto& register_resource(auto& rm, auto& dst, auto& src, auto& scratch, auto&&... args) + template<typename ResourceManager, typename DstType, typename SrcType, typename ScratchType, typename... Args> + auto& register_resource(ResourceManager& rm, DstType& dst, SrcType& src, ScratchType& scratch, Args&&... args)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (59)
pyphare/pyphare/pharein/diagnostics.py(1 hunks)pyphare/pyphare/pharesee/hierarchy/fromh5.py(1 hunks)pyphare/pyphare/pharesee/hierarchy/fromsim.py(1 hunks)pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py(0 hunks)pyphare/pyphare/pharesee/run/run.py(1 hunks)src/amr/data/field/field_data.hpp(10 hunks)src/amr/data/field/field_variable_fill_pattern.hpp(5 hunks)src/amr/data/field/refine/field_refine_operator.hpp(2 hunks)src/amr/data/particles/particles_data.hpp(14 hunks)src/amr/data/particles/particles_variable_fill_pattern.hpp(1 hunks)src/amr/data/particles/refine/particles_data_split.hpp(1 hunks)src/amr/level_initializer/hybrid_level_initializer.hpp(2 hunks)src/amr/messengers/communicator.hpp(1 hunks)src/amr/messengers/field_sum_transaction.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(15 hunks)src/amr/messengers/hybrid_messenger.hpp(2 hunks)src/amr/messengers/hybrid_messenger_info.hpp(1 hunks)src/amr/messengers/hybrid_messenger_strategy.hpp(2 hunks)src/amr/messengers/mhd_hybrid_messenger_strategy.hpp(4 hunks)src/amr/messengers/refiner.hpp(8 hunks)src/amr/messengers/refiner_pool.hpp(5 hunks)src/amr/physical_models/hybrid_model.hpp(3 hunks)src/amr/resources_manager/amr_utils.cpp(1 hunks)src/amr/resources_manager/amr_utils.hpp(5 hunks)src/amr/resources_manager/resources_manager.hpp(1 hunks)src/amr/solvers/solver_ppc.hpp(9 hunks)src/amr/utilities/box/amr_box.hpp(2 hunks)src/core/data/field/field_box.hpp(1 hunks)src/core/data/grid/grid.hpp(2 hunks)src/core/data/grid/gridlayout.hpp(2 hunks)src/core/data/ions/ion_population/ion_population.hpp(1 hunks)src/core/data/ions/ions.hpp(2 hunks)src/core/data/particles/particle_array.hpp(2 hunks)src/core/numerics/ion_updater/ion_updater.hpp(7 hunks)src/core/numerics/moments/moments.hpp(2 hunks)src/core/numerics/pusher/pusher.hpp(1 hunks)src/core/utilities/box/box.hpp(5 hunks)src/core/utilities/cellmap.hpp(2 hunks)src/core/utilities/point/point.hpp(3 hunks)src/core/utilities/types.hpp(2 hunks)src/diagnostic/detail/types/fluid.hpp(2 hunks)src/diagnostic/detail/types/particle.hpp(1 hunks)src/python3/patch_level.hpp(0 hunks)tests/amr/data/field/refine/test_field_refine.cpp(1 hunks)tests/amr/data/particles/copy/test_particledata_copyNd.cpp(4 hunks)tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp(2 hunks)tests/amr/data/particles/stream_pack/test_main.cpp(4 hunks)tests/amr/messengers/test_messengers.cpp(2 hunks)tests/core/numerics/ion_updater/test_updater.cpp(8 hunks)tests/diagnostic/__init__.py(1 hunks)tests/diagnostic/test_diagnostics.hpp(1 hunks)tests/diagnostic/test_diagnostics.ipp(1 hunks)tests/functional/conservation/conserv.py(1 hunks)tests/simulator/advance/test_particles_advance_1d.py(0 hunks)tests/simulator/advance/test_particles_advance_2d.py(0 hunks)tests/simulator/initialize/test_particles_init_1d.py(0 hunks)tests/simulator/initialize/test_particles_init_2d.py(1 hunks)tests/simulator/test_advance.py(1 hunks)tests/simulator/test_initialization.py(1 hunks)
💤 Files with no reviewable changes (5)
- pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py
- tests/simulator/advance/test_particles_advance_1d.py
- src/python3/patch_level.hpp
- tests/simulator/advance/test_particles_advance_2d.py
- tests/simulator/initialize/test_particles_init_1d.py
✅ Files skipped from review due to trivial changes (3)
- tests/diagnostic/init.py
- src/diagnostic/detail/types/particle.hpp
- tests/diagnostic/test_diagnostics.hpp
🚧 Files skipped from review as they are similar to previous changes (42)
- src/amr/resources_manager/resources_manager.hpp
- pyphare/pyphare/pharesee/hierarchy/fromh5.py
- tests/amr/data/particles/stream_pack/test_main.cpp
- src/core/numerics/pusher/pusher.hpp
- tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp
- pyphare/pyphare/pharesee/hierarchy/fromsim.py
- tests/functional/conservation/conserv.py
- src/core/data/ions/ions.hpp
- src/core/data/ions/ion_population/ion_population.hpp
- src/core/utilities/cellmap.hpp
- src/core/numerics/moments/moments.hpp
- pyphare/pyphare/pharein/diagnostics.py
- pyphare/pyphare/pharesee/run/run.py
- tests/amr/data/field/refine/test_field_refine.cpp
- src/diagnostic/detail/types/fluid.hpp
- tests/simulator/test_advance.py
- src/amr/messengers/communicator.hpp
- tests/diagnostic/test_diagnostics.ipp
- src/amr/data/particles/refine/particles_data_split.hpp
- src/amr/level_initializer/hybrid_level_initializer.hpp
- src/amr/resources_manager/amr_utils.cpp
- src/core/data/grid/grid.hpp
- src/amr/messengers/hybrid_messenger_info.hpp
- src/amr/data/field/refine/field_refine_operator.hpp
- tests/amr/messengers/test_messengers.cpp
- tests/simulator/test_initialization.py
- src/amr/messengers/mhd_hybrid_messenger_strategy.hpp
- src/amr/physical_models/hybrid_model.hpp
- src/core/utilities/types.hpp
- src/core/utilities/point/point.hpp
- tests/simulator/initialize/test_particles_init_2d.py
- src/core/data/grid/gridlayout.hpp
- src/amr/messengers/hybrid_messenger.hpp
- src/amr/resources_manager/amr_utils.hpp
- src/amr/data/particles/particles_variable_fill_pattern.hpp
- src/amr/utilities/box/amr_box.hpp
- src/amr/messengers/hybrid_messenger_strategy.hpp
- src/core/data/field/field_box.hpp
- src/core/utilities/box/box.hpp
- src/amr/messengers/field_sum_transaction.hpp
- src/core/data/particles/particle_array.hpp
- src/core/numerics/ion_updater/ion_updater.hpp
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/amr/data/field/field_variable_fill_pattern.hppsrc/amr/data/particles/particles_data.hppsrc/amr/solvers/solver_ppc.hppsrc/amr/data/field/field_data.hppsrc/amr/messengers/refiner.hppsrc/amr/messengers/refiner_pool.hppsrc/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
🧬 Code Graph Analysis (2)
tests/amr/data/particles/copy/test_particledata_copyNd.cpp (1)
src/core/utilities/types.hpp (2)
ConstArray(162-168)ConstArray(162-162)
src/amr/data/field/field_variable_fill_pattern.hpp (4)
src/amr/data/particles/particles_variable_fill_pattern.hpp (5)
dst_geometry(50-78)dst_geometry(51-55)s_name_id(80-80)fill_boxes(100-109)fill_boxes(101-104)src/amr/utilities/box/amr_box.hpp (6)
phare_box_from(25-31)phare_box_from(25-25)shift(127-131)shift(127-128)samrai_box_from(16-22)samrai_box_from(16-16)src/core/utilities/box/box.hpp (5)
box(50-50)grow(268-273)grow(268-268)shift(276-282)shift(276-276)src/amr/data/field/field_geometry.hpp (4)
box(159-202)box(159-160)box(211-220)box(211-211)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
🔇 Additional comments (43)
tests/core/numerics/ion_updater/test_updater.cpp (4)
358-358: Good type alias for improved readability.The
Boxing_ttype alias clearly documents the templated boxing approach and improves code readability.
493-495: Consistent with the patch ghost particle removal.The test expectations correctly reflect the architectural change where patch ghost particles are no longer used, verifying that they remain empty while domain and level ghost particles exist.
731-731: Consistent API usage with boxing pattern.All calls to
ionUpdater.updatePopulationscorrectly use theboxingobject instead of the rawlayout, maintaining consistency with the new templated boxing approach.Also applies to: 777-777, 802-802, 822-822, 840-840
366-367:Details
❓ Verification inconclusive
Verify the boxing initialization is correct.
The boxing object is initialized with
layoutand a grown AMR box usingnbrParticleGhosts(). Ensure this correctly represents the intended particle selection region for the new architecture.
🏁 Script executed:
#!/bin/bash # Verify the boxing initialization and usage pattern rg -A 3 -B 3 "UpdaterSelectionBoxing.*nbrParticleGhosts"Length of output: 58
🏁 Script executed:
#!/bin/bash # Show the alias and context for Boxing_t in the test file sed -n '1,200p' tests/core/numerics/ion_updater/test_updater.cpp # Inspect the code around lines 350–400 to see how boxing is used and defined sed -n '350,400p' tests/core/numerics/ion_updater/test_updater.cppLength of output: 7890
🏁 Script executed:
#!/bin/bash # Locate and inspect the definition of UpdaterSelectionBoxing updater_file=$(rg -l "UpdaterSelectionBoxing<" . | head -n 1) echo "=== Definition: $updater_file ===" sed -n '1,200p' "$updater_file" # Locate and inspect the declaration of nbrParticleGhosts ghosts_file=$(rg -l "nbrParticleGhosts" . | head -n 1) echo "=== nbrParticleGhosts in: $ghosts_file ===" sed -n '1,200p' "$ghosts_file"Length of output: 12654
🏁 Script executed:
#!/bin/bash # Inspect UpdaterSelectionBoxing in ion_updater.hpp rg -n "UpdaterSelectionBoxing" -C 10 core/numerics/ion_updater/ion_updater.hpp # Inspect nbrParticleGhosts definition across core rg -n "nbrParticleGhosts" -C 5 core/Length of output: 243
Verify UpdaterSelectionBoxing initialization correctness
Location:
• tests/core/numerics/ion_updater/test_updater.cpp (lines 366–367)The test constructs its selection region as:
Boxing_t const boxing{ layout, grow(layout.AMRBox(), GridLayout::nbrParticleGhosts()) };Please confirm that:
GridLayout::nbrParticleGhosts()still returns the correct number of particle ghost layers.- Growing the AMR box by that amount yields the intended particle-selection region under the new grid/ghost-cell conventions.
- If
nbrParticleGhosts()or AMRBox geometry changed upstream, update this call to match the expected domain.tests/amr/data/particles/copy/test_particledata_copyNd.cpp (2)
128-135: Good practice: explicit container state verification.The explicit clearing and verification of container states before reuse ensures test reliability and makes the expected state clear.
206-207: Correctly updated expectations for new architecture.The test expectations correctly reflect the architectural change where particles are now copied to
domainParticlesinstead ofpatchGhostParticles.Also applies to: 269-271
src/amr/data/field/field_variable_fill_pattern.hpp (5)
6-8: Good inclusion of required headers.The new includes are appropriately added for the functionality used in the implementation.
33-35: Simplified constructor with clear default.The constructor simplification with a default parameter makes the class easier to use while maintaining backward compatibility.
54-58: Improved assertion and simplified logic.The assertion checks the expected default value and the logic is simplified by using the member variable directly instead of the function parameter.
116-186: Well-designed template class for specialized fill pattern.The
FieldGhostInterpOverlapFillPatternclass is well-structured with:
- Clear template constraints (PRIMAL assumption documented)
- Proper inheritance from SAMRAI interface
- Appropriate error handling with runtime exceptions
- Good use of lambda functions for code organization
The implementation correctly skips self-overlaps and computes ghost box intersections.
172-175: Appropriate error handling for unsupported operations.The runtime exceptions for unsupported operations (
getStencilWidth()andcomputeFillBoxesOverlap()) are appropriate for this specialized fill pattern that doesn't support refinement.Also applies to: 183-185
src/amr/data/field/field_data.hpp (6)
5-18: Well-organized includes and dependencies.The includes are properly organized and the new dependencies (
field_box.hpp,amr_box.hpp) are appropriately added for the refactored functionality.
42-50: Good type aliases and constants for clarity.The type aliases (
Super,value_type,SetEqualOp) and theNO_ROTATEconstant improve code readability and maintainability.
225-226: Improved error handling for rotation.The rotation check now properly throws an exception instead of silently returning, providing better error handling consistency.
228-244: Efficient refactoring using FieldBox abstraction.The refactored
packStreammethod uses the newFieldBoxabstraction effectively:
- Pre-allocates buffer with appropriate size
- Uses
append_tomethod for efficient data extraction- Maintains the transformation logic correctly
This is a good example of using abstraction to simplify complex code.
261-290: Well-designed template method with operator pattern.The templated
unpackStreammethod demonstrates good generic programming:
- Template parameter with sensible default (
SetEqualOp)- Clear separation of concerns with
FieldBox- Proper buffer management and seeking logic
- Type-safe operations with the operator pattern
338-355: Clean refactoring using FieldBox operations.The refactored
copy_method is much cleaner:
- Uses
FieldBoxabstraction for both source and destination- Leverages
operate_on_fieldsfor type-safe operations- Removes complex manual indexing logic
- Template parameter allows different operations (copy, sum, etc.)
src/amr/data/particles/particles_data.hpp (8)
231-242: LGTM! Clean separation of concerns.The forward declaration and extraction of cell overlap logic into a separate method improves code organization and readability.
273-283: LGTM! Correct stream size calculation.The method properly accounts for coordinate transformations and includes both header size and particle data in the calculation.
292-301: Consistent dynamic dispatch pattern.Good consistency with the copy method's dispatch pattern. The delegation to specific methods maintains clean separation of concerns.
326-346: Well-structured refactoring.Extracting the cell overlap packing logic into a separate method improves maintainability while preserving the original functionality.
348-361: Consistent dispatch pattern maintained.The dynamic dispatch implementation follows the established pattern throughout the class, ensuring consistent behavior across all operations.
381-423: Important architectural change: particles now go to domainParticles.The change on line 418 to add particles to
domainParticlesinstead ofpatchGhostParticlesaligns with the PR's objective of removing patch ghost particle handling. This is a key part of the architectural refactoring.
425-439: Dynamic dispatch pattern completed.This final implementation maintains consistency across all streaming operations, completing the refactoring to use dynamic dispatch throughout the class.
477-597: Consistent application of new particle handling architecture.The changes throughout the
copy_method consistently direct particles todomainParticles, completing the removal of patch ghost particle handling in the copy operations.src/amr/solvers/solver_ppc.hpp (5)
37-61: LGTM! Improved type safety and readability.The introduction of type aliases, particularly
IonUpdater_t, improves code clarity and prepares for the boxing mechanism integration.
99-103: Essential cleanup for regridding operations.The
onRegridmethod properly clears the boxing state and resets the ion updater, ensuring clean state management during regridding operations.
239-255: Consistent integration of boxing setup.The integration of
setup_levelensures proper boxing initialization before operations that require it. Good consistency in using the level reference throughout.
422-447: Proper integration of boxing and new border filling.The boxing integration correctly replaces direct layout usage with the structured boxing approach. The new
fillFluxBordersandfillDensityBorderscalls align with the introduction of border sum refiners mentioned in the AI summary.
394-394: Minor cleanup aligned with architectural changes.The variable naming change removes reference to patch ghost particles, consistent with the overall refactoring.
src/amr/messengers/refiner_pool.hpp (6)
31-38: Excellent modern C++ practices.The explicit special member functions follow RAII principles correctly: virtual destructor for inheritance safety, deleted copy constructor to prevent accidental copying, and defaulted move constructor for efficiency.
42-56: Well-designed enhancement with backward compatibility.The addition of the optional
VariableFillPatternparameter maintains backward compatibility through defaultnullptrvalues while enabling new specialized fill pattern functionality.Also applies to: 194-201
59-73: Consistent batch operations with proper validation.The shared fill pattern approach for batch refiner additions is appropriate for maintaining consistency. The size assertions provide good safety validation.
Also applies to: 204-226
79-116: Consistent time refiner enhancement.The time refiner methods follow the same optional fill pattern design as the static refiners, maintaining API consistency. Error handling for duplicate keys is properly preserved.
Also applies to: 230-268
175-176: Improved template organization.Moving template method implementations to the
PHARE::amrnamespace section improves code organization and separates interface from implementation clearly.Also applies to: 271-271
166-167: Consistent member variable organization.The updated type alias and member variable declarations align with the improved template organization.
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (4)
7-12: LGTM! Includes are well-organized and relevant.The new includes support the functionality being added for point operations, vector fields, hybrid quantities, and interpolation. The organization follows a logical grouping pattern.
98-99: Consistent resource management for summation fields.The registration and allocation of
sumVecandsumFieldfollows the established pattern for other messenger resources. These temporary fields will be used for border summation operations.Also applies to: 120-121
644-650: Clean utility function for key extraction.The
makeKeyshelper function provides a clean abstraction for extracting vector field names. The use ofstd::transformwith a lambda is appropriate and readable.
1021-1030: Well-structured type aliases and member declarations.The new type aliases for refiner pools and member variable declarations follow consistent naming conventions and provide clear semantic meaning for the different types of refiners being used.
src/amr/messengers/refiner.hpp (3)
22-24: Enum update aligns with architectural changes.The removal of
InteriorGhostParticlesandSharedBorderand addition ofPatchFieldBorderSumandExteriorGhostParticlesreflects the shift from patch ghost to domain ghost particle handling described in the PR objectives.
75-83: Proper transaction factory integration for border summation.The new
PatchFieldBorderSumcase correctly creates schedules withFieldBorderSumTransactionFactory, enabling the summation behavior for overlapping patch border fields. The schedule creation pattern is consistent with other refiner types.
199-200: Optional fill pattern parameters enhance flexibility.The addition of optional
VariableFillPatternparameters to constructors provides the flexibility needed for specialized fill patterns while maintaining backward compatibility through default values.Also applies to: 216-217, 243-244, 254-254
| std::memcpy(sumVec[c].data(), pop.flux()[c].data(), | ||
| pop.flux()[c].size() * sizeof(value_type)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify type safety of memcpy operations.
The use of std::memcpy assumes that the field data types are trivially copyable and that source and destination have identical layouts and sizes. Consider whether type-safe alternatives like std::copy or assignment operators would be more appropriate.
- std::memcpy(sumVec[c].data(), pop.flux()[c].data(),
- pop.flux()[c].size() * sizeof(value_type));
+ std::copy(pop.flux()[c].begin(), pop.flux()[c].end(), sumVec[c].begin());Also applies to: 367-369, 383-384, 392-393
🤖 Prompt for AI Agents
In src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp around lines 356-358,
367-369, 383-384, and 392-393, the use of std::memcpy assumes trivially copyable
types and identical layouts, which may not be safe. Replace std::memcpy with
std::copy or use assignment operators to ensure type safety and proper copying
semantics between source and destination containers.
There was a problem hiding this comment.
@PhilipDeegan
Totally fair. In case it helps your decision:
• If FieldT::value_type is a plain arithmetic type (float, double, etc.) and both sumVec[c].data() and pop.flux()[c].data() point to contiguous storage of equal length, the current std::memcpy is fine and marginally faster.
• If you ever switch to a non-trivial value type (e.g., a small struct with a user-defined constructor / destructor), memcpy would be UB, whereas std::copy remains safe.
You can make the intent explicit and keep the performance by adding
static_assert(std::is_trivially_copyable_v<value_type>,
"memcpy assumes trivially copyable Field value_type");That way you get a compile-time guard without changing runtime behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-13)
- GitHub Check: Analyze (python)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
🔇 Additional comments (2)
tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp (2)
20-22: Double-checkIonUpdater/Boxing_ttemplate coupling
Boxing_tis defined asUpdaterSelectionBoxing<IonUpdater, GridLayout_t>, which hard-codes the veryIonUpdateralias declared one line above.
This is fine as long asIonUpdater’s own third template parameter is still the layout and not the boxing type itself (the API recently changed). IfIonUpdaterwas retemplated to depend on the boxing, this circular definition will silently compile but select the wrong overloads.Please verify that the current
IonUpdaterdeclaration is still:template<class Populations, class EM, class Layout> class IonUpdater;and not
template<class Populations, class EM, class Boxing> class IonUpdater;to avoid a mismatch.
If the new signature takes a boxing, swapGridLayout_tforBoxing_tin the alias.
26-27: Possible off-by-one when enlarging the boxing domain
grow(layout.AMRBox(), GridLayout_t::nbrParticleGhosts())blindly inflates the full simulation box; ifnbrParticleGhosts()already counts the exterior ghost layer inclusive, the grown box will be one cell too wide, making the benchmark move particles that would normally remain outside the updater’s remit.Quick sanity diff in case
nbrParticleGhosts()returns the number of layers instead of the radius:-Boxing_t const boxing{layout, grow(layout.AMRBox(), GridLayout_t::nbrParticleGhosts())}; +Boxing_t const boxing{layout, + grow(layout.AMRBox(), GridLayout_t::nbrParticleGhosts() - 1)};Please confirm the semantic of
nbrParticleGhosts()against the newGridLayouthelper.
A one-cell slip will not crash the benchmark but will bias the timing results.
PhilipDeegan
force-pushed
the
patchghostless
branch
from
dc51623 to
b518499
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tests/core/numerics/ion_updater/test_updater.cpp (1)
757-765: Incorrect reference to old & new alpha ghost arrays
alphaLevelGhostOldandalphaLevelGhostNeware copied in the fixture, but the assertions here compare againstalphaLevelGhost, making the check ineffective and silently masking regressions.- checkIsUnTouched(populations[1].levelGhostParticlesOld(), ionsBufferCpy.alphaLevelGhost); - checkIsUnTouched(populations[1].levelGhostParticlesNew(), ionsBufferCpy.alphaLevelGhost); + checkIsUnTouched(populations[1].levelGhostParticlesOld(), ionsBufferCpy.alphaLevelGhostOld); + checkIsUnTouched(populations[1].levelGhostParticlesNew(), ionsBufferCpy.alphaLevelGhostNew);
♻️ Duplicate comments (2)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (2)
398-425:std::memcpyvs type safety – see prior discussionThe new
fillFluxBordersstill usesstd::memcpyonFieldT::value_type.
As already noted in earlier reviews, this is only safe for trivially-copyable scalar types.
Either add a static-assert guard or switch tostd::copy.(Repeats prior feedback.)
427-449: Same memcpy issue for density borders
fillDensityBordersmirrors the previous method and carries the same risk – please apply the same remedy here.
🧹 Nitpick comments (4)
src/amr/data/field/field_variable_fill_pattern.hpp (1)
167-171: Pattern name collisionBoth
FieldFillPatternandFieldGhostInterpOverlapFillPatternreturn the same
getPatternName()("BOX_GEOMETRY_FILL_PATTERN").
Colliding identifiers can break SAMRAI’s internal pattern registry and make
debugging painful. Provide a distinct string for the new pattern.-static inline std::string const s_name_id = "BOX_GEOMETRY_FILL_PATTERN"; +static inline std::string const s_name_id = "BOX_GEOMETRY_GHOST_INTERP_FILL_PATTERN";src/amr/data/particles/particles_data.hpp (1)
733-748: Vector reallocation in tight loop
pack_from_ghostpushes intooutBufferwithout reserving, despite already
knowing the exact particle count ingetOutGoingDataStreamSize.
Add a pre-reserve to avoid repeated reallocations.- std::vector<Particle_t> outBuffer; + std::vector<Particle_t> outBuffer; + outBuffer.reserve(getOutGoingDataStreamSize(pOverlap) / sizeof(Particle_t));This is a micro-optimisation but pays off when exchanging millions of particles.
src/amr/solvers/solver_ppc.hpp (2)
155-168: Avoid repeatedstd::stringallocations for patch-ids
make_boxesconverts everypatch->getGlobalId()to a temporarystd::stringon each insertion.
The same conversion is repeated later inmoveIons_()vialevelBoxing.at(amr::to_string(...)).Unless the solver is known to be CPU-bound elsewhere, using the numeric ID directly (e.g.
std::uint64_t) would cut a few allocations and hash calculations per patch at every time-step.- std::unordered_map<int, std::unordered_map<std::string, Boxing_t>> boxing; + using patch_id_t = std::uint64_t; // or decltype(patch->getGlobalId()) + std::unordered_map<int, std::unordered_map<patch_id_t, Boxing_t>> boxing; ... - amr::to_string(patch->getGlobalId()), + patch->getGlobalId(),
421-430: Potential hot-path string construction
amr::to_string(state.patch->getGlobalId())is executed for every population of every patch at every ion update. If the previous suggestion is applied (store numeric keys), this extra conversion and hashing vanishes entirely.
Otherwise cache the key once per patch outside the inner loop.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (60)
pyphare/pyphare/pharein/diagnostics.py(1 hunks)pyphare/pyphare/pharesee/hierarchy/fromh5.py(1 hunks)pyphare/pyphare/pharesee/hierarchy/fromsim.py(1 hunks)pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py(0 hunks)pyphare/pyphare/pharesee/run/run.py(1 hunks)src/amr/data/field/field_data.hpp(10 hunks)src/amr/data/field/field_variable_fill_pattern.hpp(5 hunks)src/amr/data/field/refine/field_refine_operator.hpp(2 hunks)src/amr/data/particles/particles_data.hpp(14 hunks)src/amr/data/particles/particles_variable_fill_pattern.hpp(1 hunks)src/amr/data/particles/refine/particles_data_split.hpp(1 hunks)src/amr/level_initializer/hybrid_level_initializer.hpp(2 hunks)src/amr/messengers/communicator.hpp(1 hunks)src/amr/messengers/field_sum_transaction.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(17 hunks)src/amr/messengers/hybrid_messenger.hpp(2 hunks)src/amr/messengers/hybrid_messenger_info.hpp(1 hunks)src/amr/messengers/hybrid_messenger_strategy.hpp(2 hunks)src/amr/messengers/mhd_hybrid_messenger_strategy.hpp(4 hunks)src/amr/messengers/refiner.hpp(7 hunks)src/amr/messengers/refiner_pool.hpp(5 hunks)src/amr/physical_models/hybrid_model.hpp(3 hunks)src/amr/resources_manager/amr_utils.cpp(1 hunks)src/amr/resources_manager/amr_utils.hpp(5 hunks)src/amr/resources_manager/resources_manager.hpp(1 hunks)src/amr/solvers/solver_ppc.hpp(9 hunks)src/amr/utilities/box/amr_box.hpp(2 hunks)src/core/data/field/field_box.hpp(1 hunks)src/core/data/grid/grid.hpp(2 hunks)src/core/data/grid/gridlayout.hpp(2 hunks)src/core/data/ions/ion_population/ion_population.hpp(1 hunks)src/core/data/ions/ions.hpp(2 hunks)src/core/data/particles/particle_array.hpp(2 hunks)src/core/numerics/ion_updater/ion_updater.hpp(7 hunks)src/core/numerics/moments/moments.hpp(2 hunks)src/core/numerics/pusher/pusher.hpp(1 hunks)src/core/utilities/box/box.hpp(5 hunks)src/core/utilities/cellmap.hpp(2 hunks)src/core/utilities/point/point.hpp(3 hunks)src/core/utilities/types.hpp(2 hunks)src/diagnostic/detail/types/fluid.hpp(2 hunks)src/diagnostic/detail/types/particle.hpp(1 hunks)src/python3/patch_level.hpp(0 hunks)tests/amr/data/field/refine/test_field_refine.cpp(1 hunks)tests/amr/data/particles/copy/test_particledata_copyNd.cpp(3 hunks)tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp(2 hunks)tests/amr/data/particles/stream_pack/test_main.cpp(4 hunks)tests/amr/messengers/test_messengers.cpp(2 hunks)tests/core/numerics/ion_updater/test_updater.cpp(8 hunks)tests/diagnostic/__init__.py(1 hunks)tests/diagnostic/test_diagnostics.hpp(1 hunks)tests/diagnostic/test_diagnostics.ipp(1 hunks)tests/functional/conservation/conserv.py(1 hunks)tests/simulator/advance/test_particles_advance_1d.py(0 hunks)tests/simulator/advance/test_particles_advance_2d.py(0 hunks)tests/simulator/initialize/test_particles_init_1d.py(0 hunks)tests/simulator/initialize/test_particles_init_2d.py(1 hunks)tests/simulator/test_advance.py(1 hunks)tests/simulator/test_initialization.py(1 hunks)tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp(2 hunks)
💤 Files with no reviewable changes (5)
- tests/simulator/advance/test_particles_advance_1d.py
- tests/simulator/advance/test_particles_advance_2d.py
- pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py
- src/python3/patch_level.hpp
- tests/simulator/initialize/test_particles_init_1d.py
🚧 Files skipped from review as they are similar to previous changes (48)
- src/core/data/ions/ions.hpp
- src/core/data/grid/grid.hpp
- tests/functional/conservation/conserv.py
- tests/diagnostic/init.py
- tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp
- pyphare/pyphare/pharesee/run/run.py
- tests/amr/data/particles/stream_pack/test_main.cpp
- src/core/numerics/pusher/pusher.hpp
- tests/simulator/test_advance.py
- src/core/numerics/moments/moments.hpp
- tests/diagnostic/test_diagnostics.hpp
- src/core/data/ions/ion_population/ion_population.hpp
- src/amr/resources_manager/resources_manager.hpp
- src/amr/messengers/communicator.hpp
- pyphare/pyphare/pharein/diagnostics.py
- tests/diagnostic/test_diagnostics.ipp
- src/amr/messengers/hybrid_messenger_info.hpp
- tests/amr/data/field/refine/test_field_refine.cpp
- tests/amr/messengers/test_messengers.cpp
- src/core/utilities/cellmap.hpp
- src/diagnostic/detail/types/fluid.hpp
- src/amr/data/field/refine/field_refine_operator.hpp
- src/diagnostic/detail/types/particle.hpp
- src/amr/resources_manager/amr_utils.cpp
- src/amr/level_initializer/hybrid_level_initializer.hpp
- tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp
- src/amr/messengers/mhd_hybrid_messenger_strategy.hpp
- src/core/utilities/types.hpp
- src/amr/data/particles/refine/particles_data_split.hpp
- tests/simulator/test_initialization.py
- tests/simulator/initialize/test_particles_init_2d.py
- pyphare/pyphare/pharesee/hierarchy/fromsim.py
- src/core/data/particles/particle_array.hpp
- src/core/data/grid/gridlayout.hpp
- pyphare/pyphare/pharesee/hierarchy/fromh5.py
- src/amr/physical_models/hybrid_model.hpp
- src/amr/messengers/hybrid_messenger_strategy.hpp
- src/amr/utilities/box/amr_box.hpp
- src/amr/messengers/hybrid_messenger.hpp
- tests/amr/data/particles/copy/test_particledata_copyNd.cpp
- src/core/utilities/box/box.hpp
- src/amr/resources_manager/amr_utils.hpp
- src/core/utilities/point/point.hpp
- src/amr/data/particles/particles_variable_fill_pattern.hpp
- src/amr/messengers/field_sum_transaction.hpp
- src/core/numerics/ion_updater/ion_updater.hpp
- src/amr/messengers/refiner_pool.hpp
- src/amr/messengers/refiner.hpp
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/data/field/field_box.hppsrc/amr/data/field/field_variable_fill_pattern.hppsrc/amr/data/field/field_data.hppsrc/amr/messengers/hybrid_hybrid_messenger_strategy.hppsrc/amr/solvers/solver_ppc.hppsrc/amr/data/particles/particles_data.hpp
🧬 Code Graph Analysis (1)
src/core/data/field/field_box.hpp (2)
src/core/data/grid/grid.hpp (2)
field_(87-87)field_(88-88)src/amr/data/field/field_data.hpp (2)
field(296-296)src(323-323)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: Analyze (python)
- GitHub Check: build (macos-13)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (3)
src/core/data/field/field_box.hpp (1)
61-69: Unnecessary functor construction inside hot loop
operate_on_fieldsinstantiates a newOperatorobject on every element.
With large fields this adds measurable overhead. Prefer constructing the functor once and re-using it, or make it a static callable.- for (; dst_it != dst.lcl_box.end(); ++src_it, ++dst_it) - Operator{dst.field(*dst_it)}(src.field(*src_it)); + Operator op; + for (; dst_it != dst.lcl_box.end(); ++src_it, ++dst_it) + op(dst.field(*dst_it), src.field(*src_it));Adapting this would require changing call-sites but yields a clear win.
[ suggest_optional_refactor ]src/amr/solvers/solver_ppc.hpp (1)
170-176: Unchecked lookup may throw
levelBoxing = boxing[level.getLevelNumber()]is safe because the precedingsetup_levelguarantees the key exists, yetmoveIons_()later useslevelBoxing.at(amr::to_string(state.patch->getGlobalId()))If
boxingis ever cleared (e.g. afteronRegrid()) andsetup_levelis not called again beforemoveIons_, thisatwill throw.
Consider:auto boxIt = levelBoxing.find(patchId); PHARE_ASSERT(boxIt != levelBoxing.end());or simply
levelBoxing[patchId]if default-construction is cheap.src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
1054-1058: Shared scratch fields may become data race hotspots
sumVecandsumFieldare stored as members and reused each time the strategy processes a level. If two levels (or two MPI ranks calling the same object from different threads) invokefillFluxBorders/fillDensityBordersconcurrently, the shared buffer will lead to races.If the messenger is ever accessed from multiple threads, consider allocating a temporary field inside each method or protecting the member fields with a mutex.
PhilipDeegan
force-pushed
the
patchghostless
branch
from
46cf147 to
89ba51f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
src/amr/data/field/field_variable_fill_pattern.hpp (1)
54-57: Assertion still present despite past review feedback.The assertion
assert(fn_overwrite_interior == true)remains despite the past review comment indicating it may be too restrictive. While the implementation now correctly uses the storedoverwrite_interior_value (line 56), the assertion could still cause unexpected crashes if SAMRAI passesfalse.src/amr/data/field/field_data.hpp (2)
227-228: Improved error handling for rotations.The explicit exception throwing for unsupported rotations addresses previous review feedback and provides better error clarity than silent returns.
275-275: Potential type inconsistency in buffer declaration.The buffer is declared as
std::vector<double>but the size calculation usesvalue_type. This could cause issues ifvalue_typeis notdouble.- std::vector<double> buffer(getDataStreamSize(overlap) / sizeof(value_type), 0.); + std::vector<value_type> buffer(getDataStreamSize(overlap) / sizeof(value_type), 0.);src/amr/solvers/solver_ppc.hpp (2)
153-168: Address unused variables and improve error handling.The static analysis correctly identifies unused variables. The boxing insertion logic should be simplified and error handling improved.
- for (auto const& patch : level) - if (auto [it, suc] = levelBoxing.try_emplace( - amr::to_string(patch->getGlobalId()), - Boxing_t{amr::layoutFromPatch<GridLayout>(*patch), - amr::makeNonLevelGhostBoxFor<GridLayout>(*patch, hierarchy)}); - !suc) - throw std::runtime_error("boxing map insertion failure"); + for (auto const& patch : level) + { + auto patchId = amr::to_string(patch->getGlobalId()); + auto [_, success] = levelBoxing.try_emplace( + patchId, + Boxing_t{amr::layoutFromPatch<GridLayout>(*patch), + amr::makeNonLevelGhostBoxFor<GridLayout>(*patch, hierarchy)}); + if (!success) + throw std::runtime_error("Boxing map insertion failure for patch " + patchId); + }
427-429: Verify boxing lookup safety.The
levelBoxing.at()call could throw if the patch ID is not found. The setup should guarantee the boxing exists, but defensive programming would be safer.- ionUpdater_.updatePopulations( - state.ions, state.electromagAvg, - levelBoxing.at(amr::to_string(state.patch->getGlobalId())), dt, mode); + auto patchId = amr::to_string(state.patch->getGlobalId()); + auto it = levelBoxing.find(patchId); + if (it == levelBoxing.end()) + throw std::runtime_error("Boxing not found for patch " + patchId); + ionUpdater_.updatePopulations( + state.ions, state.electromagAvg, it->second, dt, mode);src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (2)
439-440: Address memcpy type safety concerns.The use of
std::memcpyassumes that the field data types are trivially copyable and that source and destination have identical layouts. This is the same concern raised in previous reviews.Consider adding a static assertion to ensure type safety:
+static_assert(std::is_trivially_copyable_v<value_type>, + "memcpy assumes trivially copyable Field value_type"); std::memcpy(sumVec_[c].data(), ions[i].flux()[c].data(), ions[i].flux()[c].size() * sizeof(value_type));Also applies to: 447-448
462-463: Address memcpy type safety concerns.Same memcpy type safety issue as in
fillFluxBorders. Consider usingstd::copyor adding static assertions for type safety.Also applies to: 470-471, 476-477, 483-484
🧹 Nitpick comments (5)
src/diagnostic/diagnostic_model_view.hpp (1)
62-81: Correct implementation of momentum tensor border summation.The
fillPopMomTensormethod properly implements the sum transaction pattern for momentum tensor data, using memcpy for efficient data transfer and SAMRAI refinement schedules for border summation.Consider adding bounds checking for the
popidxparameter to prevent out-of-range access:void fillPopMomTensor(auto& lvl, auto const time, auto const popidx) { + if (popidx >= model_.state.ions.size()) { + throw std::out_of_range("Population index out of range in fillPopMomTensor"); + }src/core/numerics/ion_updater/ion_updater.hpp (2)
27-27: Consider adding template parameter documentation.The
using Thisalias is introduced but never used in the code. This appears to be dead code that should be removed.- using This = IonUpdater<Ions, Electromag, GridLayout>;
176-185: Performance optimization opportunity in domain particle handling.The code makes a local copy of domain particles with
tmp_particles_ = pop.domainParticles()but then only processes a subset. Consider optimizing this to avoid unnecessary copying.- auto& domain = (tmp_particles_ = pop.domainParticles()); // make local copy + auto& domain = pop.domainParticles(); // work directly on original auto outRange = makeIndexRange(domain); auto allowed = outRange = pusher_->move(outRange, outRange, em, pop.mass(), interpolator_, layout, boxing.noop, boxing.inNonLevelGhostBox);src/amr/solvers/solver_ppc.hpp (1)
179-181: Consider boxing data structure performance implications.The nested
unordered_mapstructure may have performance implications for frequent lookups. Consider if a flatter structure or caching would be beneficial.Consider profiling the boxing lookup performance and potentially caching frequently accessed boxing objects:
// Alternative: Add caching mechanism mutable std::unordered_map<std::string, Boxing_t*> boxingCache;src/amr/messengers/refiner.hpp (1)
276-312: Address unused variable warnings from static analysis.The helper methods are well-designed and improve code organization. However, static analysis has flagged unused variables in the structured bindings. Consider using
[[maybe_unused]]attribute orstd::ignorefor unused tuple elements.For example, if only certain elements of the tuple are needed:
auto&& [idDst, idSrc, [[maybe_unused]] idScrtch] = rm->getIDsList(dst, src, scratch);Or use
std::ignorefor completely unused elements:auto&& [idDst, idSrc, _] = rm->getIDsList(dst, src, scratch); std::ignore = _;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (63)
pyphare/pyphare/pharein/diagnostics.py(1 hunks)pyphare/pyphare/pharesee/hierarchy/fromh5.py(1 hunks)pyphare/pyphare/pharesee/hierarchy/fromsim.py(1 hunks)pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py(0 hunks)pyphare/pyphare/pharesee/run/run.py(1 hunks)src/amr/data/field/field_data.hpp(10 hunks)src/amr/data/field/field_variable_fill_pattern.hpp(5 hunks)src/amr/data/field/refine/field_refine_operator.hpp(2 hunks)src/amr/data/particles/particles_data.hpp(13 hunks)src/amr/data/particles/particles_variable_fill_pattern.hpp(1 hunks)src/amr/data/particles/refine/particles_data_split.hpp(2 hunks)src/amr/level_initializer/hybrid_level_initializer.hpp(2 hunks)src/amr/messengers/communicator.hpp(1 hunks)src/amr/messengers/field_sum_transaction.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(19 hunks)src/amr/messengers/hybrid_messenger.hpp(2 hunks)src/amr/messengers/hybrid_messenger_info.hpp(1 hunks)src/amr/messengers/hybrid_messenger_strategy.hpp(2 hunks)src/amr/messengers/mhd_hybrid_messenger_strategy.hpp(4 hunks)src/amr/messengers/refiner.hpp(7 hunks)src/amr/messengers/refiner_pool.hpp(5 hunks)src/amr/physical_models/hybrid_model.hpp(3 hunks)src/amr/resources_manager/amr_utils.cpp(1 hunks)src/amr/resources_manager/amr_utils.hpp(5 hunks)src/amr/resources_manager/resources_manager.hpp(1 hunks)src/amr/solvers/solver_ppc.hpp(9 hunks)src/amr/utilities/box/amr_box.hpp(2 hunks)src/core/data/field/field_box.hpp(1 hunks)src/core/data/grid/grid.hpp(2 hunks)src/core/data/grid/gridlayout.hpp(2 hunks)src/core/data/ions/ion_population/ion_population.hpp(1 hunks)src/core/data/ions/ions.hpp(2 hunks)src/core/data/particles/particle_array.hpp(2 hunks)src/core/numerics/ion_updater/ion_updater.hpp(7 hunks)src/core/numerics/moments/moments.hpp(2 hunks)src/core/numerics/pusher/pusher.hpp(1 hunks)src/core/utilities/box/box.hpp(4 hunks)src/core/utilities/cellmap.hpp(2 hunks)src/core/utilities/point/point.hpp(3 hunks)src/core/utilities/types.hpp(1 hunks)src/diagnostic/detail/h5writer.hpp(1 hunks)src/diagnostic/detail/types/fluid.hpp(1 hunks)src/diagnostic/detail/types/particle.hpp(1 hunks)src/diagnostic/diagnostic_model_view.hpp(4 hunks)src/python3/patch_level.hpp(0 hunks)tests/amr/data/field/refine/test_field_refine.cpp(1 hunks)tests/amr/data/particles/copy/test_particledata_copyNd.cpp(3 hunks)tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp(2 hunks)tests/amr/data/particles/stream_pack/test_main.cpp(4 hunks)tests/amr/messengers/test_messengers.cpp(2 hunks)tests/core/numerics/ion_updater/test_updater.cpp(8 hunks)tests/diagnostic/__init__.py(1 hunks)tests/diagnostic/test_diagnostics.hpp(1 hunks)tests/diagnostic/test_diagnostics.ipp(1 hunks)tests/functional/conservation/conserv.py(1 hunks)tests/simulator/advance/test_particles_advance_1d.py(0 hunks)tests/simulator/advance/test_particles_advance_2d.py(0 hunks)tests/simulator/initialize/density_check.py(7 hunks)tests/simulator/initialize/test_particles_init_1d.py(0 hunks)tests/simulator/initialize/test_particles_init_2d.py(1 hunks)tests/simulator/test_advance.py(1 hunks)tests/simulator/test_initialization.py(1 hunks)tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp(2 hunks)
💤 Files with no reviewable changes (5)
- tests/simulator/initialize/test_particles_init_1d.py
- tests/simulator/advance/test_particles_advance_1d.py
- src/python3/patch_level.hpp
- tests/simulator/advance/test_particles_advance_2d.py
- pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py
✅ Files skipped from review due to trivial changes (1)
- pyphare/pyphare/pharein/diagnostics.py
🚧 Files skipped from review as they are similar to previous changes (47)
- src/core/numerics/pusher/pusher.hpp
- src/diagnostic/detail/h5writer.hpp
- tests/diagnostic/test_diagnostics.ipp
- pyphare/pyphare/pharesee/hierarchy/fromsim.py
- tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp
- src/core/numerics/moments/moments.hpp
- tests/amr/messengers/test_messengers.cpp
- src/amr/resources_manager/resources_manager.hpp
- pyphare/pyphare/pharesee/hierarchy/fromh5.py
- tests/functional/conservation/conserv.py
- tests/diagnostic/test_diagnostics.hpp
- tests/amr/data/particles/stream_pack/test_main.cpp
- src/core/utilities/cellmap.hpp
- tests/diagnostic/init.py
- src/amr/resources_manager/amr_utils.cpp
- src/core/utilities/types.hpp
- src/core/data/ions/ion_population/ion_population.hpp
- src/amr/level_initializer/hybrid_level_initializer.hpp
- tests/simulator/test_advance.py
- pyphare/pyphare/pharesee/run/run.py
- src/core/data/grid/grid.hpp
- src/diagnostic/detail/types/particle.hpp
- src/amr/data/field/refine/field_refine_operator.hpp
- src/amr/messengers/communicator.hpp
- src/core/data/ions/ions.hpp
- tests/amr/data/field/refine/test_field_refine.cpp
- src/amr/messengers/hybrid_messenger_strategy.hpp
- src/amr/physical_models/hybrid_model.hpp
- tests/simulator/test_initialization.py
- src/core/data/particles/particle_array.hpp
- src/amr/messengers/mhd_hybrid_messenger_strategy.hpp
- tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp
- src/diagnostic/detail/types/fluid.hpp
- src/core/data/grid/gridlayout.hpp
- tests/simulator/initialize/density_check.py
- src/amr/data/particles/particles_variable_fill_pattern.hpp
- src/amr/messengers/hybrid_messenger_info.hpp
- src/amr/utilities/box/amr_box.hpp
- tests/simulator/initialize/test_particles_init_2d.py
- src/core/utilities/box/box.hpp
- src/amr/resources_manager/amr_utils.hpp
- src/amr/data/particles/particles_data.hpp
- src/amr/data/particles/refine/particles_data_split.hpp
- src/core/utilities/point/point.hpp
- src/amr/messengers/hybrid_messenger.hpp
- src/amr/messengers/refiner_pool.hpp
- src/amr/messengers/field_sum_transaction.hpp
🧰 Additional context used
📓 Path-based instructions (1)
**/*.hpp
⚙️ CodeRabbit Configuration File
Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Files:
src/core/data/field/field_box.hppsrc/core/numerics/ion_updater/ion_updater.hppsrc/amr/solvers/solver_ppc.hppsrc/diagnostic/diagnostic_model_view.hppsrc/amr/data/field/field_variable_fill_pattern.hppsrc/amr/data/field/field_data.hppsrc/amr/messengers/refiner.hppsrc/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
🧬 Code Graph Analysis (3)
src/core/data/field/field_box.hpp (1)
src/core/data/grid/grid.hpp (2)
field_(86-86)field_(87-87)
tests/amr/data/particles/copy/test_particledata_copyNd.cpp (1)
src/core/utilities/types.hpp (2)
ConstArray(159-165)ConstArray(159-159)
src/diagnostic/diagnostic_model_view.hpp (6)
src/core/data/ions/ions.hpp (9)
vector(220-223)i(170-175)i(170-170)i(177-182)i(177-177)i(250-250)i(250-250)i(251-251)i(251-251)src/core/hybrid/hybrid_quantities.hpp (2)
auto(45-45)auto(46-46)src/amr/messengers/refiner.hpp (8)
rm(276-281)rm(276-276)rm(284-290)rm(284-285)rm(293-299)rm(293-293)rm(302-312)rm(302-303)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (12)
ions(410-420)ions(410-411)ions(424-450)ions(424-425)ions(452-486)ions(452-453)ions(498-537)ions(498-499)ions(545-551)ions(545-546)patch(137-145)patch(137-137)src/amr/data/field/field_data.hpp (4)
patch(299-308)patch(299-299)patch(311-320)patch(311-311)src/amr/physical_models/hybrid_model.hpp (4)
patch(59-62)patch(59-59)patch(75-78)patch(75-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
🔇 Additional comments (46)
tests/amr/data/particles/copy/test_particledata_copyNd.cpp (3)
112-138: Enhanced test verification aligns with architectural changes.The explicit clearing and verification of particle containers (lines 112-119) improves test reliability by ensuring clean state between test phases. The introduction of
newCellvariable (line 120) enhances readability. The updated assertions (lines 129-137) correctly reflect the architectural shift where particles now go todomainParticlesinstead ofpatchGhostParticles, consistent with the removal of patch ghost particle handling mentioned in the AI summary.
190-191: Correct expectation update for new particle classification.The swapped assertions properly reflect the architectural change where particles are now classified as domain particles instead of patch ghost particles during overlap operations.
253-255: Consistent particle classification update with transformation handling.The updated assertions correctly expect particles in
domainParticlesrather thanpatchGhostParticles, and the transformation verification (line 255) properly validates the transformed cell index.tests/core/numerics/ion_updater/test_updater.cpp (3)
507-509: Updated particle container expectations align with architectural changes.The assertions correctly reflect the new particle classification where patch ghost particles are no longer used (size 0), while domain and level ghost particles remain populated.
746-746: Consistent interface update to use boxing abstraction.All calls to
ionUpdater.updatePopulationshave been correctly updated to use the newboxingparameter instead of the directlayout, reflecting the improved modular design for particle selection logic.Also applies to: 817-817, 837-837, 855-855
372-381: Assumption about level ghost cells holdsI’ve confirmed that the boxing region is only grown by
GridLayout::nbrParticleGhosts()(i.e. the patch‐ghost layer) and does not include any level‐ghost cells. All level‐ghost particles in the test are explicitly added afterward, so the comment “assumes no level ghost cells” is accurate across all scenarios. No further changes required.src/core/data/field/field_box.hpp (5)
16-26: Clean template design with appropriate type deduction.The
FieldBoxtemplate provides a good abstraction for field operations within spatial regions. The use ofstd::decay_tforvalue_typeextraction is appropriate, and the staticdimensionconstant provides compile-time access to dimensionality.
28-51: Well-designed constructors provide flexible initialization options.The three constructors accommodate different use cases effectively:
- Default initialization using layout ghost boxes
- Explicit local coordinate selection
- AMR coordinate selection with automatic conversion
This design provides good flexibility for different spatial region specifications.
61-69: Generic field operation function with good safety checks.The
operate_on_fieldsfunction provides a clean generic interface for element-wise operations between field boxes. The size compatibility assertion (line 64) prevents undefined behavior, and the simultaneous iteration pattern is implemented correctly.
73-80: Correct implementation of vector-to-field operation.The
set_frommethod correctly implements the pattern for applying operators from vector data to field elements within the local box, with proper handling of the seek offset for vector positioning.
82-89: Simple and correct field-to-vector extraction.The
append_tomethod correctly extracts field values to a vector. The comment about reserving the vector (line 85) is helpful for performance optimization.src/diagnostic/diagnostic_model_view.hpp (4)
8-39: Appropriate includes and type aliases for new functionality.The new includes support the momentum tensor diagnostic capabilities, and the type aliases improve code clarity and maintainability. The static
dimensionconstant provides useful compile-time access to dimensionality.
52-53: Proper initialization and tensor buffer for momentum diagnostics.The constructor call to
declareMomentumTensorAlgos()ensures proper initialization, and thesumTensor_member provides the necessary buffer for momentum tensor sum operations.Also applies to: 190-190
148-169: Proper setup of momentum tensor refinement algorithms.The
declareMomentumTensorAlgosmethod correctly initializes the refinement algorithms for momentum tensor operations, using the newFieldGhostInterpOverlapFillPatternand properly registering refinement operations for each tensor component.
171-189: Well-designed nested struct for refinement algorithm management.The
MTAlgostruct provides clean encapsulation of refinement algorithms and schedules. The lazy creation pattern ingetOrCreateScheduleis efficient, and the use ofFieldBorderSumTransactionFactoryaligns with the new sum transaction infrastructure.src/amr/data/field/field_variable_fill_pattern.hpp (3)
115-148: Excellent documentation explaining the fill pattern purpose.The comprehensive documentation clearly explains why this specialized fill pattern is needed for incomplete ghost domain moment nodes, with helpful diagrams and detailed explanations of interpolation order effects. This greatly aids code understanding and maintenance.
159-198: Correct implementation of specialized ghost overlap calculation.The
calculateOverlapmethod properly handles the specialized case of ghost interpolation overlaps. The self-overlap skip condition (line 169-170) is appropriate, and the primal ghost box calculation with upper bound adjustment (line 177) correctly handles primal grid geometry.
205-218: Appropriate disabling of unsupported interface methods.The runtime errors for
getStencilWidthandcomputeFillBoxesOverlapcorrectly indicate that these interface methods are not applicable for this specialized fill pattern, providing clear feedback if they are inadvertently called.src/core/numerics/ion_updater/ion_updater.hpp (4)
118-158: Excellent encapsulation of particle selection logic.The
UpdaterSelectionBoxingstruct provides a clean abstraction that decouples particle selection from the updater implementation. The lambda-based selectors with appropriate capture semantics ensure proper behavior even with object copying.
211-213: Good architectural decision for level ghost particle handling.The conditional processing of level ghost particles only when non-empty is a good optimization that avoids unnecessary work.
246-250: Patch ghost particle handling is intentional and should remain
- Ripgrep shows
patchGhostParticlesis still widely used across the codebase (unit tests intest_particles_fixtures.hpp,test_updater.cpp, AMR copy tests, hybrid messenger,ParticlesData,ParticlePack, etc.).- The IonUpdater code relies on appending non-level ghosts into
pop.patchGhostParticles()before running the interpolator on patch ghosts.- Existing tests in
test_updater.cppexpectpatchGhostParticles()to stay empty when there are no patch-ghost particles, and to be populated correctly otherwise.- Removing this copy would break both the interpolation logic and the tests that verify patch-ghost support.
No change needed here.
135-138: Lambda capture semantics are already consistent
Verified that all selectors inion_updater.hppuse initializer captures ([domainBox = domainBox],[ghostBox = ghostBox],[nonLevelGhostBox = nonLevelGhostBox], and[ghostBox = ghostBox, domainBox = domainBox]) and that thenoopselector rightly omits captures. No changes required.src/amr/data/field/field_data.hpp (6)
42-44: Clean template alias definitions.The type aliases
Super,value_type, andSetEqualOpimprove code readability and maintainability.
230-246: Efficient generic packing implementation.The refactored
packStreammethod usingFieldBoxandappend_toprovides a clean, generic approach that eliminates dimension-specific code duplication.
286-292: Excellent generic unpacking implementation.The use of
FieldBoxwith templated operators provides a flexible and clean implementation that supports both copy and sum operations.
323-325: New summation methods enhance AMR messaging capabilities.The addition of
sumandunpackStreamAndSummethods supports the new border sum transaction infrastructure, which is a key architectural improvement.
340-357: Generic operator-based copying is well-designed.The templated
copy_method with operator parameter allows for both assignment and summation operations using the same code path, reducing duplication and improving maintainability.
449-455: Clean implementation of summation methods.The
unpackStreamAndSumandsummethods properly use thePlusEqualsoperator to implement summation behavior, integrating well with the generic template design.src/amr/solvers/solver_ppc.hpp (3)
99-103: Good resource cleanup in onRegrid.The
onRegrid()method properly clears the boxing data structure and resets the ion updater, ensuring clean state after grid changes.
170-176: Well-designed level setup with boxing management.The
setup_level()method provides a clean interface that handles boxing lifecycle management transparently.
435-437: New border filling methods enhance AMR capabilities.The addition of
fillFluxBordersandfillDensityBordersmethods integrates with the new border sum transaction infrastructure, providing essential functionality for the updated AMR messaging system.src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (11)
8-46: LGTM - Include organization and type definitions are well-structured.The new includes support the enhanced functionality (field operations, interpolation, SAMRAI integration), and the type alias definitions improve code readability and maintainability.
119-121: LGTM - New temporary sum fields properly registered.The addition of
sumVec_,sumField_, andsumTensor_resources supports the new border sum functionality described in the PR objectives.
142-144: LGTM - Allocation calls match resource registration.The allocation of sum fields is consistent with their registration in the constructor.
235-241: LGTM - New refiner registration follows established patterns.The registration of domain ghost particle refiners and border sum refiners for flux and density follows the established pattern and correctly handles multiple refiners with loops.
494-534: LGTM - Simplified moment ghost filling aligns with architecture changes.The removal of patch ghost particle deposition and focus on level ghost particles with time interpolation is consistent with the "patchghostless" objectives.
747-762: LGTM - Improved code organization with helper function and consistent fill patterns.The use of
makeKeys()helper anddefaultFieldFillPatternimproves consistency and readability compared to the previous node-only refiner approach.
788-806: LGTM - Well-structured registration of new refiners with appropriate fill patterns.The registration of domain ghost particle refiners and border sum refiners uses appropriate specialized fill patterns and follows established coding patterns for multiple refiner registration.
737-743: LGTM - Clean utility function improves code reuse.The
makeKeyshelper function provides a clean, functional approach to extracting vector names and reduces code duplication.
1058-1091: LGTM - Well-organized member variables with clear documentation.The new sum fields and border sum refiner pools are properly typed, documented, and follow established naming conventions.
1148-1149: LGTM - Appropriate use of shared_ptr for fill pattern.The shared_ptr usage for the stateless fill pattern is consistent with SAMRAI patterns and provides appropriate lifecycle management.
415-420: We need to gather the relevant code context arounddomainGhostPartRefiners_insrc/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(declarations and how it’s used elsewhere) to understand whatfill()does and whether clearingpatchGhostParticles()afterward is still necessary. Please run:#!/bin/bash # Show class member declarations and related methods grep -R "domainGhostPartRefiners_" -n src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp # Show the typedef or class of domainGhostPartRefiners_ grep -R "class .*Refiner" -n src/amr/messengers # Print surrounding code for fillIonGhostParticles sed -n '380,450p' src/amr/messengers/hybrid_hybrid_messenger_strategy.hppThese snippets will clarify the type and behavior of
domainGhostPartRefiners_and whether it populates or depends onpatchGhostParticles.src/amr/messengers/refiner.hpp (4)
7-24: LGTM - Enum updates align with architecture changes.The addition of
PatchFieldBorderSumandExteriorGhostParticlesto theRefinerTypeenum and the new includes support the shift away from patch ghost handling described in the PR objectives.
77-84: LGTM - Proper implementation of border sum schedule creation.The
PatchFieldBorderSumcase correctly usesFieldBorderSumTransactionFactoryto handle the specialized summation requirements for border field data.
130-133: LGTM - Simple schedule creation for exterior ghost particles.The
ExteriorGhostParticlescase appropriately uses the basic schedule creation for particle communication within the same level.
201-207: LGTM - Constructor refactoring improves maintainability.The delegation to helper methods like
register_time_interpolated_vector_fieldreduces code duplication and improves maintainability. The optionalVariableFillPatternparameter provides needed flexibility.
| template<typename Operator = SetEqualOp> | ||
| void unpackStream(SAMRAI::tbox::MessageStream& stream, | ||
| const SAMRAI::hier::BoxOverlap& overlap, Grid_t& dst_grid) | ||
| { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify template parameter consistency.
The templated unpackStream method should use consistent types throughout. The Operator template parameter defaults to SetEqualOp which is appropriate.
Check if the buffer type consistency issue mentioned in past reviews has been addressed:
🏁 Script executed:
#!/bin/bash
# Check for buffer type declarations in unpackStream methods
ast-grep --pattern $'std::vector<$_> buffer'Length of output: 45
🏁 Script executed:
#!/bin/bash
# Display the unpackStream method with context
rg -C5 "template<typename Operator = SetEqualOp>.*unpackStream" src/amr/data/field/field_data.hpp -n
# Search for any use of 'buffer' in this file to verify type consistency
rg "buffer" src/amr/data/field/field_data.hpp -nLength of output: 815
Unify buffer element types in unpackStream
The unpackStream implementation in src/amr/data/field/field_data.hpp currently declares two different buffer types—std::vector<value_type> for the pack phase and std::vector<double> for the unpack phase—which will break when value_type != double. Please choose a single element type (either double if the stream always serializes as double, or value_type if you want a pure binary copy) and adjust both buffers plus their size calculations to match.
• Lines 230–231: change
std::vector<value_type> buffer;
buffer.reserve(getDataStreamSize_(overlap) / sizeof(double));to use the chosen type and sizeof(...) accordingly.
• Lines 275–276: change
std::vector<double> buffer(getDataStreamSize(overlap) / sizeof(value_type), 0.);to the same element type and division by its sizeof.
Example diff if you decide to standardize on double:
--- a/src/amr/data/field/field_data.hpp
+++ b/src/amr/data/field/field_data.hpp
@@ -230,7 +230,7 @@
- std::vector<value_type> buffer;
- buffer.reserve(getDataStreamSize_(overlap) / sizeof(double));
+ std::vector<double> buffer;
+ buffer.reserve(getDataStreamSize_(overlap) / sizeof(double));
@@ -275,7 +275,7 @@
- std::vector<double> buffer(getDataStreamSize(overlap) / sizeof(value_type), 0.);
+ std::vector<double> buffer(getDataStreamSize(overlap) / sizeof(double), 0.);🤖 Prompt for AI Agents
In src/amr/data/field/field_data.hpp around lines 230-231 and 275-276, the
unpackStream function uses inconsistent buffer element types:
std::vector<value_type> in the pack phase and std::vector<double> in the unpack
phase, causing issues when value_type is not double. To fix this, choose a
single element type (either double or value_type) and update both buffer
declarations and their size calculations to use that type consistently,
adjusting the sizeof operator accordingly to ensure compatibility.
| template<typename Boxing_t> | ||
| void updatePopulations(Ions& ions, Electromag const& em, Boxing_t const& boxing, double dt, | ||
| UpdaterMode = UpdaterMode::all); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve template parameter constraints.
The templated updatePopulations method accepts a generic Boxing_t parameter but lacks constraints to ensure the boxing type has the required interface (layout, selector functions, etc.).
Consider adding a concept or static assertions to document and enforce the Boxing_t requirements:
+ template<typename Boxing_t>
+ concept BoxingType = requires(Boxing_t const& boxing) {
+ boxing.layout;
+ boxing.noop;
+ boxing.inDomainBox;
+ boxing.inGhostBox;
+ boxing.inNonLevelGhostBox;
+ boxing.inGhostLayer;
+ };
+
- template<typename Boxing_t>
+ template<BoxingType Boxing_t>
void updatePopulations(Ions& ions, Electromag const& em, Boxing_t const& boxing, double dt,📝 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.
| template<typename Boxing_t> | |
| void updatePopulations(Ions& ions, Electromag const& em, Boxing_t const& boxing, double dt, | |
| UpdaterMode = UpdaterMode::all); | |
| // Add a concept to document and enforce the required Boxing_t interface | |
| template<typename Boxing_t> | |
| concept BoxingType = requires(Boxing_t const& boxing) { | |
| boxing.layout; | |
| boxing.noop; | |
| boxing.inDomainBox; | |
| boxing.inGhostBox; | |
| boxing.inNonLevelGhostBox; | |
| boxing.inGhostLayer; | |
| }; | |
| // Constrain updatePopulations to only accept types satisfying BoxingType | |
| template<BoxingType Boxing_t> | |
| void updatePopulations(Ions& ions, Electromag const& em, Boxing_t const& boxing, double dt, | |
| UpdaterMode = UpdaterMode::all); |
🤖 Prompt for AI Agents
In src/core/numerics/ion_updater/ion_updater.hpp around lines 58 to 60, the
template parameter Boxing_t for the updatePopulations method lacks constraints
to ensure it meets the required interface. Add a concept or static_assert
statements to enforce that Boxing_t provides the necessary functions and types
such as layout and selector functions. This will document the requirements
clearly and cause compilation errors if Boxing_t does not conform, improving
code safety and clarity.
* PatchGhostLess * comments * Update particles_variable_fill_pattern.hpp * slightly raise empirical threshold for PGL * blackened \m/ * torm * point conventions --------- Co-authored-by: Nicolas Aunai <nicolas.aunai@lpp.polytechnique.fr>
* PatchGhostLess * comments * Update particles_variable_fill_pattern.hpp * slightly raise empirical threshold for PGL * blackened \m/ * torm * point conventions --------- Co-authored-by: Nicolas Aunai <nicolas.aunai@lpp.polytechnique.fr>
* PatchGhostLess * comments * Update particles_variable_fill_pattern.hpp * slightly raise empirical threshold for PGL * blackened \m/ * torm * point conventions --------- Co-authored-by: Nicolas Aunai <nicolas.aunai@lpp.polytechnique.fr>
* PatchGhostLess * comments * Update particles_variable_fill_pattern.hpp * slightly raise empirical threshold for PGL * blackened \m/ * torm * point conventions --------- Co-authored-by: Nicolas Aunai <nicolas.aunai@lpp.polytechnique.fr>
* PatchGhostLess * comments * Update particles_variable_fill_pattern.hpp * slightly raise empirical threshold for PGL * blackened \m/ * torm * point conventions --------- Co-authored-by: Nicolas Aunai <nicolas.aunai@lpp.polytechnique.fr>
2 participants
replaces #1000