eb-less particles - mesh to particle eb handled directory in boris #781

PhilipDeegan · 2023-11-18T19:05:53Z

mesh to particle for GPGPU will likely be per particle anyway

I would argue this is also better for cache, as there's only one particle loop now for interpoloate/accelerate

Summary by CodeRabbit

New Features
- Introduced a new search functionality with a dedicated search bar for improved user experience.
- Added a box() function to the ParticleArray class for enhanced data access.
Improvements
- Enhanced the Interpolator class to support single particle processing, providing more precise electromagnetic field interpolation.
- Refined the BorisPusher class with new pre-push and post-push step functions for better particle movement control.
- Added a dimension constant to the Box struct for clearer dimensionality representation.
- Implemented a default template argument in the Point class's toArray method for increased flexibility.
Bug Fixes
- Removed unnecessary particle attribute assertions from various test cases to align with updated particle data handling.
Refactor
- Streamlined the Particle struct by removing unused electromagnetic field variables and updating related functions.
- Simplified test cases for interpolators by using range-based loops for better readability and maintainability.
Documentation
- Updated test case descriptions to reflect the removal of specific particle attribute checks.
Chores
- Updated include directives from "benchmark/benchmark.hpp" to "benchmark/benchmark.h" across several files.
Style
- Applied consistent formatting changes to various test cases and classes for improved code clarity.

coderabbitai · 2023-11-18T19:06:01Z

Walkthrough

## Walkthrough
The changes across the codebase reflect a significant refactoring effort, primarily focusing on the `Particle` and `Interpolator` classes, with the removal of electromagnetic field components from particles and a shift towards handling single particles instead of ranges. The `BorisPusher` class has been updated to accommodate these changes. Tests have been adjusted to remove checks for removed attributes. Additionally, there's a general cleanup, introduction of new utility functions, and updates to benchmarking tools to align with the new code structure.

## Changes

| File Path | Change Summary |
|-----------|----------------|
| `src/core/data/particles/particle.hpp` | Removed electromagnetic field components from `Particle` struct; updated comparison and output operators. |
| `src/core/data/particles/particle_array.hpp` | Added `box()` member function. |
| `src/core/numerics/interpolator/interpolator.hpp` | Changed `operator()` to handle single particle; return type changed to `auto`. |
| `src/core/numerics/pusher/boris.hpp` | Added `prePushStep_` and `postPushStep_`; refactored `accelerate_` to handle single particle; removed `virtual` from `setMeshAndTimeStep`. |
| `src/core/utilities/box/box.hpp`<br>`src/core/utilities/point/point.hpp` | Added `dimension` constant to `Box`; added default template argument to `Point::toArray`. |
| `tests/.../copy/test_particledata_copyNd.cpp`<br>`tests/.../copy_overlap/test_particledata_copy_periodicNd.cpp`<br>`tests/.../stream_pack/test_main.cpp` | Removed assertions for electromagnetic field components in particle-related tests. |
| `tests/core/data/gridlayout/...` | Added `test_gridlayout.hpp` inclusion; removed `TestGridLayout` class template. |
| `tests/core/data/particles/test_main.cpp` | Removed test case for initializing particle fields to zero. |
| `tests/core/numerics/interpolator/test_main.cpp`<br>`tests/core/numerics/pusher/test_pusher.cpp` | Refactored test cases to use range-based loops; updated `Interpolator` class usage. |
| `tools/bench/core/bench.hpp`<br>`tools/bench/core/numerics/pusher/pusher.cpp`<br>`tools/bench/hi5/write_particles.cpp` | Updated include directives; introduced `LocalisedCellFlattener` and updated benchmarking code. |
| `res/cmake/def.cmake` | Added `-Wdouble-promotion` compiler flag. |
| `src/amr/data/particles/refine/splitter.hpp` | Introduced type deduction with `std::decay_t`. |

Tips

Chat with CodeRabbit Bot (`@coderabbitai`)

If you reply to a review comment from CodeRabbit, the bot will automatically respond.
To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
Note: Review comments are made on code diffs or files, not on the PR overview.
Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

coderabbitai

Review Status

Actionable comments generated: 10

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between d183651 and 4abed81.

Files selected for processing (10)

src/core/data/particles/particle.hpp (3 hunks)
src/core/numerics/interpolator/interpolator.hpp (5 hunks)
src/core/numerics/pusher/boris.hpp (4 hunks)
src/core/utilities/box/box.hpp (1 hunks)
tests/amr/data/particles/copy/test_particledata_copyNd.cpp (2 hunks)
tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp (1 hunks)
tests/amr/data/particles/stream_pack/test_main.cpp (2 hunks)
tests/core/data/particles/test_main.cpp (1 hunks)
tests/core/numerics/interpolator/test_main.cpp (3 hunks)
tests/core/numerics/pusher/test_pusher.cpp (5 hunks)

Additional comments: 16

src/core/data/particles/particle.hpp (3)

64-70: The equality operator has been correctly updated to reflect the removal of the electromagnetic field components from the Particle struct. This change is consistent with the summary provided.

93-98: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [77-98]

The output stream operator has been updated to remove the electromagnetic field components. This is in line with the changes to the Particle struct and ensures that the output will not attempt to print the removed fields.

121-129: The template equality operator has been updated to remove the comparison of the electromagnetic field components. This change is necessary to maintain the correctness of the equality check after the fields have been removed from the Particle struct.

tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp (1)

135-138: The removal of electromagnetic field components from the Particle struct is consistent with the pull request's goal of refactoring for GPGPU optimization. The test has been updated to no longer check these fields, which is correct given the context. However, ensure that the new approach to handling electromagnetic fields is covered by other tests, possibly in Interpolator or BorisPusher classes.

tests/amr/data/particles/copy/test_particledata_copyNd.cpp (2)

117-119: The removal of assertions for electromagnetic field attributes (Ex, Ey, Ez, Bx, By, Bz) is consistent with the pull request summary stating that these fields have been removed from the Particle struct. This change aligns with the new design where electromagnetic fields are not stored within the Particle struct but are interpolated individually for each particle. The remaining assertions check that other particle attributes are preserved after the copy operation, which is still relevant for ensuring data integrity.

132-134: Similar to the previous hunk, the removal of assertions for electromagnetic field attributes is consistent with the structural changes to the Particle struct. The test continues to verify the preservation of other particle attributes (weight, charge, iCell, delta, and v), which is important for ensuring the correctness of the copy operation in the absence of electromagnetic field components.

tests/amr/data/particles/stream_pack/test_main.cpp (2)

226-229: The removal of assertions related to electromagnetic field components is consistent with the changes described in the pull request summary. Since the Particle struct no longer contains these fields, the tests should not check for them.

263-266: Similar to the previous hunk, the removal of assertions for electromagnetic field components aligns with the structural changes to the Particle struct. The tests have been correctly updated to reflect these changes.

tests/core/data/particles/test_main.cpp (1)

38-43: The removal of the test case that checked the initialization of electromagnetic field components in the Particle struct is consistent with the changes described in the pull request summary. Since the Particle struct no longer contains these fields, the corresponding test case is obsolete and should be removed.

src/core/numerics/interpolator/interpolator.hpp (1)

484-519: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [469-513]

The Interpolator class has been refactored to interpolate electromagnetic fields for individual particles. This change aligns with the pull request's goal of optimizing for GPGPU by shifting to a per-particle computation model. The use of tuples to return multiple values is a good practice in this context, as it allows for returning multiple related values without the need for out-parameters or modifying the state of the object. However, ensure that the tuple's structure is well-documented and that any code receiving the tuple is prepared to handle it correctly.

tests/core/numerics/pusher/test_pusher.cpp (5)

20-26: The Trajectory struct is well-defined and includes vectors for position and velocity components. However, it's important to ensure that the size of these vectors is managed correctly throughout the code to prevent out-of-bounds access.

71-94: The Interpolator class mock is correctly returning a tuple of electric and magnetic field components. This change aligns with the pull request summary, which states that the Interpolator class has been modified to work with individual particles. The hard-coded values for the fields should be clearly documented as part of a testing strategy, as they are not representative of a production environment.

139-145: The initialization of particlesIn with a single Particle instance is correct. However, it's important to ensure that the Particle constructor and its parameters are correctly aligned with the new struct definition, especially since the electromagnetic field components have been removed from the Particle struct.

188-194: The pusher->move method is now being called with the updated arguments, which is consistent with the changes described in the pull request summary. The use of selector for both the inDomain and outDomain arguments suggests that the same selection criteria are applied to both input and output ranges, which should be verified to ensure it matches the intended logic.

212-218: Similar to the previous comment, the pusher->move method is called with the updated arguments. It's important to verify that the selector logic is correctly applied and that the Interpolator and BoundaryCondition instances are properly configured to work with the new per-particle computation model.

src/core/numerics/pusher/boris.hpp (1)

16-22: The class definition and template parameters are clear and well-structured.

tests/core/numerics/interpolator/test_main.cpp

coderabbitai

Review Status

Actionable comments generated: 8

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between d183651 and 577caf6.

Files selected for processing (10)

src/core/data/particles/particle.hpp (3 hunks)
src/core/numerics/interpolator/interpolator.hpp (5 hunks)
src/core/numerics/pusher/boris.hpp (4 hunks)
src/core/utilities/box/box.hpp (1 hunks)
tests/amr/data/particles/copy/test_particledata_copyNd.cpp (2 hunks)
tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp (1 hunks)
tests/amr/data/particles/stream_pack/test_main.cpp (2 hunks)
tests/core/data/particles/test_main.cpp (1 hunks)
tests/core/numerics/interpolator/test_main.cpp (3 hunks)
tests/core/numerics/pusher/test_pusher.cpp (5 hunks)

Files skipped from review due to trivial changes (3)

tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp
tests/core/data/particles/test_main.cpp
tests/core/numerics/interpolator/test_main.cpp

Additional comments: 14

tests/amr/data/particles/copy/test_particledata_copyNd.cpp (2)

117-119: The removal of assertions for electromagnetic field components (Ex, Ey, Ez, Bx, By, Bz) is consistent with the changes described in the pull request summary. Since these fields are no longer stored within each Particle struct, the test cases should not check for their equality post-copy operation. The remaining assertions check for the correctness of other particle attributes after the copy, which is appropriate.

132-134: Similar to the previous hunk, the removal of assertions for electromagnetic field components is consistent with the structural changes to the Particle struct. The test still verifies the correctness of the copy operation for other particle attributes, which is necessary to ensure the integrity of the copy functionality.

tests/amr/data/particles/stream_pack/test_main.cpp (2)

226-229: The assertions for delta, weight, and charge are preserved, which is consistent with the summary stating that only electromagnetic field components have been removed from the Particle struct. These properties are still relevant to the particle's state and should be tested for correctness after operations like packing and unpacking in the stream.

263-266: Similar to the previous hunk, the assertions for delta, weight, and charge are correctly retained for the same reasons. It's important to ensure that these particle attributes are preserved across the packing and unpacking processes.

tests/core/numerics/pusher/test_pusher.cpp (4)

71-94: The Interpolator class is a mock designed for testing purposes, which is fine in this context. However, ensure that the hardcoded values for the electromagnetic fields (pEx, pEy, pEz, pBx, pBy, pBz) are consistent with the expected test conditions and that any changes to these values are reflected in the test expectations.

139-145: The initialization of particlesIn with a single particle is clear. However, ensure that the hardcoded values for the particle's position and velocity are appropriate for the test cases. If these values are meant to be varied or come from an external source, consider parameterizing the test or reading from a configuration file.

188-194: The move method is called within a loop to simulate the particle trajectory. Ensure that the rangeIn and rangeOut are correctly managed between iterations, and that the pusher's state is consistent across calls. If the pusher relies on mutable state that could be affected by consecutive calls, this could be a source of error.

212-218: Similar to the previous comment, verify that the move method's behavior is correct and that the rangeIn and rangeOut are managed correctly. Additionally, ensure that the 2D test case is comprehensive and covers the necessary scenarios for the application.

src/core/data/particles/particle.hpp (4)

64-70: The operator== has been correctly updated to remove comparisons for the electromagnetic field components (Ex, Ey, Ez, Bx, By, Bz) since these are no longer part of the Particle struct. This change is consistent with the pull request's goal of refactoring the code to compute these fields on-the-fly.

73-74: The operator<< function declaration has been updated to use const& for the particle parameter, which is a good practice to avoid unnecessary copies when streaming the object to an output stream. This change is subtle but improves performance.

93-97: The operator<< function implementation has been updated to reflect the removal of the electromagnetic field components from the Particle struct. The function now only outputs the remaining relevant member variables. This change is consistent with the pull request's refactoring goals.

122-129: The templated operator== function has been updated to use typename and enable_if_t for type checking, ensuring that the comparison is only enabled for valid particle types. This is a good use of SFINAE (Substitution Failure Is Not An Error) to provide compile-time checks for the function's applicability.

src/core/numerics/interpolator/interpolator.hpp (1)

485-520: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [461-514]

The refactoring of the Interpolator class to operate on a single particle and return a tuple with the interpolated electromagnetic fields is a significant change. The code is well-documented, and the use of templates for different interpolation orders and dimensions is a good practice for code reusability and maintainability. The use of structured bindings (e.g., auto& [pE, pB] = particle_EB;) in C++17 makes the code more readable. However, ensure that all calls to this function are updated to handle the new return type (a tuple instead of modifying a particle's fields directly).

src/core/numerics/pusher/boris.hpp (1)

255-257: The dto2m variable is correctly declared in the private section of the class, which is good for encapsulation. However, it should be initialized in the setMeshAndTimeStep method or constructor to ensure it is always set before being used.

src/core/numerics/interpolator/interpolator.hpp

+        auto& [pEx, pEy, pEz] = pE;
+        auto& [pBx, pBy, pBz] = pB;
+
+        auto const& [Ex, Ey, Ez] = Em.E();


src/core/numerics/interpolator/interpolator.hpp

+        auto& [pBx, pBy, pBz] = pB;
+
+        auto const& [Ex, Ey, Ez] = Em.E();
+        auto const& [Bx, By, Bz] = Em.B();


src/core/numerics/pusher/boris.hpp

    {
-        double dto2m = 0.5 * dt_ / mass;
+        auto& [pE, pB]        = particleEB;
+        auto& [pEx, pEy, pEz] = pE;


src/core/numerics/pusher/boris.hpp

-        double dto2m = 0.5 * dt_ / mass;
+        auto& [pE, pB]        = particleEB;
+        auto& [pEx, pEy, pEz] = pE;
+        auto& [pBx, pBy, pBz] = pB;


coderabbitai

Review Status

Actionable comments generated: 9

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between d183651 and d1d7268.

Files selected for processing (18)

res/cmake/def.cmake (1 hunks)
src/amr/data/particles/refine/splitter.hpp (2 hunks)
src/core/data/particles/particle.hpp (3 hunks)
src/core/data/particles/particle_array.hpp (1 hunks)
src/core/numerics/interpolator/interpolator.hpp (5 hunks)
src/core/numerics/pusher/boris.hpp (4 hunks)
src/core/utilities/box/box.hpp (2 hunks)
src/core/utilities/point/point.hpp (1 hunks)
tests/amr/data/particles/copy/test_particledata_copyNd.cpp (2 hunks)
tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp (1 hunks)
tests/amr/data/particles/stream_pack/test_main.cpp (2 hunks)
tests/core/data/gridlayout/gridlayout_test.hpp (1 hunks)
tests/core/data/particles/test_main.cpp (2 hunks)
tests/core/numerics/interpolator/test_main.cpp (9 hunks)
tests/core/numerics/pusher/test_pusher.cpp (7 hunks)
tools/bench/core/bench.hpp (2 hunks)
tools/bench/core/numerics/pusher/pusher.cpp (1 hunks)
tools/bench/hi5/write_particles.cpp (1 hunks)

Files skipped from review due to trivial changes (7)

res/cmake/def.cmake
src/core/data/particles/particle_array.hpp
src/core/utilities/box/box.hpp
src/core/utilities/point/point.hpp
tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp
tests/core/data/gridlayout/gridlayout_test.hpp
tools/bench/hi5/write_particles.cpp

Additional comments: 33

tests/amr/data/particles/copy/test_particledata_copyNd.cpp (2)

117-120: The removal of assertions for electric and magnetic field components (Ex, Ey, Ez, Bx, By, Bz) is consistent with the summary stating that these fields are no longer stored in the Particle struct. This change should be verified to ensure that it aligns with the new design and that any necessary adjustments have been made elsewhere in the code to handle these fields appropriately.

132-135: Similar to the previous comment, the removal of assertions for electric and magnetic field components in the ghost particle checks is consistent with the structural changes described. Ensure that the new approach for handling these fields is correctly implemented and tested elsewhere.

src/core/data/particles/particle.hpp (3)

61-70: The removal of electric and magnetic field components from the Particle struct and the corresponding update to the operator== function is consistent with the summary provided. This change should indeed simplify the particle data structure and potentially improve cache efficiency, which is important for computational physics simulations that handle large numbers of particles. However, it's crucial to ensure that all parts of the codebase that relied on these fields have been updated accordingly.

93-97: The update to the operator<< function to remove the output of the removed electric and magnetic field components is correct. This change should reflect the new structure of the Particle struct. It's important to verify that the output format is still consistent with the expectations of any logging or debugging tools that use this function.

121-129: The update to the operator== function template to use std::enable_if_t for type checking is a modern C++ approach that ensures the operator is only instantiated for valid particle types. This is a good use of SFINAE (Substitution Failure Is Not An Error) to provide compile-time type checking. It's important to ensure that all particle comparisons in the codebase are still valid after this change.

tests/core/data/particles/test_main.cpp (2)

22-28: The change in the AParticle constructor reflects the removal of electric and magnetic field components from the Particle struct. This is consistent with the pull request summary stating that these fields are now computed per particle and no longer stored within each Particle instance. The use of integer values for the initializer list is appropriate if the fields being initialized are expected to be integers. However, if these fields are meant to be floating-point numbers (which seems likely given the context of a computational physics codebase), this might be an error. The reviewer should verify the intended data types for these fields.

38-39: The test ParticleChargeIsInitiliazedOK is correctly checking the initialization of the charge member of the Particle class. This test is important to ensure that the charge property of a particle is set correctly upon initialization. No issues here.

tests/amr/data/particles/stream_pack/test_main.cpp (2)

226-229: The removal of electric and magnetic field assertions is consistent with the summary stating that these fields have been removed from the Particle struct. This change should improve cache efficiency and is likely a preparation for GPGPU optimizations. The remaining assertions check that other particle attributes are preserved during the packing process, which is important for ensuring data integrity.

263-266: Similar to the previous hunk, the removal of electric and magnetic field assertions is in line with the refactoring described in the summary. The tests continue to ensure that the remaining particle attributes are correctly preserved, which is crucial for the correctness of the simulation after the refactoring.

src/core/numerics/interpolator/interpolator.hpp (2)

485-520: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [461-514]

The Interpolator class has been refactored to operate on single particles, which is a significant change from the previous range-based approach. This change is likely to improve performance, especially in preparation for GPGPU optimizations, as it simplifies the data access patterns and may reduce memory bandwidth requirements. The code is well-structured, and the use of templates for compile-time polymorphism is appropriate for the context of high-performance computing in computational physics. The logging statements (lines 476 and 512) are useful for debugging and performance analysis.

551-562: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [524-559]

The ParticleToMesh class still contains methods that operate on ranges of particles (lines 526-553 and 555-558). This seems inconsistent with the refactoring goal of moving to single-particle computations. It's possible that these methods are kept for backward compatibility or for different use cases where batch processing is still preferred. If these methods are no longer needed, they should be removed to avoid maintaining dead code. If they are kept, ensure that their presence is justified and documented.

tests/core/numerics/pusher/test_pusher.cpp (6)

71-94: The mock Interpolator class is well implemented for the purpose of testing. It returns hard-coded field values which are used to verify the correctness of the BorisPusher class without the need for a complete electromagnetic field setup.

139-145: The setup for the APusher test class is correct. It initializes the necessary objects and sets up the test environment. However, ensure that the emplace_back call correctly initializes the Particle object with the intended values, as this is critical for the test's validity.

184-194: The loop correctly updates the actual arrays with the new positions of the particles. The call to pusher->move is also correct, as it updates the particles' positions based on the interpolator and boundary conditions. The use of std::copy to update rangeIn with the new positions from rangeOut is also appropriate.

209-218: Similar to the previous comment, the loop in the APusher2D test case is correctly implemented and follows the same pattern as the APusher3D test case.

231-237: The loop in the APusher1D test case is correctly implemented. It updates the actual array with the new positions and calls pusher->move to move the particles. The test case is consistent with the other dimensional test cases.

268-274: The initialization of the random number generator and distributions is correct. The loop populates particlesIn with randomly positioned particles, which is suitable for the test case. Ensure that the random values used here are appropriate for the test's expectations and that the test is deterministic or handles randomness correctly.

tools/bench/core/numerics/pusher/pusher.cpp (3)

1-6: The reorganization of includes and removal of redundant ones is a good practice to avoid unnecessary compilation dependencies and improve compile times. Ensure that all removed includes are indeed not used in this file or any headers it includes.

9-58: The refactoring of the push function to use a test grid layout and the Electromag class from the core::bench namespace is a significant change. It appears to be aimed at improving the testability and performance of the code. The use of core::IndexRange<ParticleArray> for particle iteration instead of ParticleArray::iterator is a good change for readability and may also have performance benefits if IndexRange is optimized for the use case.

The use of core::bench::particle and core::bench::disperse for particle setup is a good abstraction, making the setup code more concise and potentially reusable. The removal of the std::sort(domainParticles) line (commented out) should be verified to ensure that the sorting is no longer needed or is being handled elsewhere.

The creation of rangeIn and rangeOut using PHARE::core::makeIndexRange is a clean way to handle ranges of particles. The BorisPusher_t setup and usage seem correct and are adapted to the new interface.

The use of a no-op lambda for particle selection (no_op) is an interesting choice and might be a placeholder for future functionality or a way to disable certain operations during benchmarking.

Overall, the changes seem to be well thought out and aimed at improving the structure and performance of the code. However, it is crucial to ensure that all affected parts of the codebase are updated to work with these changes and that the changes do not introduce any regressions.

60-60: The benchmarks are correctly parameterized for different dimensions and interpolation orders. This is a good practice for performance testing across a range of scenarios. Ensure that the new benchmark configurations are consistent with the intended test cases and that the results are being interpreted correctly.

tools/bench/core/bench.hpp (1)

2-5: The change from benchmark.hpp to benchmark.h suggests that the benchmark library's header file might have been renamed or restructured. Verify that this change is consistent with the version of the benchmark library being used in the project.

src/amr/data/particles/refine/splitter.hpp (2)

55-63: The use of std::decay_t for type deduction is a good practice as it removes any reference or cv-qualifiers from the type, which can prevent potential type-related issues when using the types in calculations or function templates. This change enhances the type safety and maintainability of the code.

70-87: The refactoring here is consistent with the changes in the type deduction. The use of static_cast<Weight_t> and static_cast<Delta_t> ensures that the types are correctly converted, which is important for maintaining precision and avoiding implicit type conversions that could lead to data loss or unexpected behavior. The calculation of power[dimension - 1] is a neat way to handle different dimensions without branching logic, assuming dimension is always within the correct range.

However, it's important to note that the power array assumes that refRatio is a compile-time constant. If refRatio can change at runtime, this approach would not be valid.

The loop that updates fineParticle.delta and fineParticle.iCell is clear and concise. The use of std::floor and the subsequent adjustment of delta and iCell is a common pattern in particle-in-cell codes to handle particle movement across cell boundaries. The static cast to int32_t is appropriate since cell indices are typically represented as integers.

One minor point to consider is the potential for idx to overflow if nbRefinedParts is very large. This is unlikely in practice but could be a concern in very large simulations or if nbRefinedParts is not properly bounded.

tests/core/numerics/interpolator/test_main.cpp (9)

274-279: The loop initializing particle positions is correct. However, it's important to ensure that the index 5 and the delta value 0.32 are valid within the context of the grid layout and the simulation domain. If these values are arbitrary, it might be beneficial to define them as constants or retrieve them from a configuration to avoid magic numbers.

298-317: The tests for electromagnetic field interpolation at particle positions are well-structured. The use of std::abs to compare floating-point numbers with a small epsilon (1e-8) is appropriate for checking the accuracy of the interpolation. However, it's important to ensure that the epsilon value is suitable for the expected precision of the fields in the simulation.

385-390: Similar to the previous comment on particle initialization, ensure that the hardcoded values are appropriate for the simulation context.

409-427: The test structure is consistent with the previous 1D test, which is good for maintainability. The same considerations about the epsilon value and the cleanup of buffer pointers apply here.

500-505: Again, verify that the hardcoded values for particle initialization are correct and consider defining them as constants if they are used in multiple places.

524-543: The 3D test follows the same pattern as the 1D and 2D tests, which is good. Ensure that the epsilon value for floating-point comparison is suitable for the simulation's precision requirements.

605-621: The initialization of particles for the 1D collection test is correct. However, the use of constexpr for Interpolator::interp_order is good practice as it ensures compile-time checks. The hardcoded values for iCell and delta should be verified for correctness within the simulation context.

632-661: The same considerations for hardcoded values and the use of constexpr apply here as in the previous comment.

667-703: The same considerations for hardcoded values and the use of constexpr apply here as in the previous comment.

src/core/numerics/pusher/boris.hpp (1)

255-257: The dto2m should be declared in the private section of the class to ensure encapsulation. This is already done, so no change is needed here. However, it's important to ensure that dto2m is initialized before it's used in any calculations to avoid using an uninitialized variable.

tools/bench/core/bench.hpp

coderabbitai

Review Status

Actionable comments generated: 17

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between d183651 and 31ce3ba.

Files selected for processing (19)

res/cmake/def.cmake (1 hunks)
src/amr/data/particles/refine/splitter.hpp (2 hunks)
src/core/data/particles/particle.hpp (3 hunks)
src/core/data/particles/particle_array.hpp (1 hunks)
src/core/numerics/interpolator/interpolator.hpp (5 hunks)
src/core/numerics/pusher/boris.hpp (4 hunks)
src/core/utilities/box/box.hpp (2 hunks)
src/core/utilities/point/point.hpp (1 hunks)
tests/amr/data/particles/copy/test_particledata_copyNd.cpp (2 hunks)
tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp (1 hunks)
tests/amr/data/particles/stream_pack/test_main.cpp (2 hunks)
tests/core/data/gridlayout/gridlayout_test.hpp (1 hunks)
tests/core/data/gridlayout/test_gridlayout.hpp (1 hunks)
tests/core/data/particles/test_main.cpp (2 hunks)
tests/core/numerics/interpolator/test_main.cpp (9 hunks)
tests/core/numerics/pusher/test_pusher.cpp (7 hunks)
tools/bench/core/bench.hpp (2 hunks)
tools/bench/core/numerics/pusher/pusher.cpp (1 hunks)
tools/bench/hi5/write_particles.cpp (1 hunks)

Files skipped from review due to trivial changes (6)

res/cmake/def.cmake
src/core/data/particles/particle_array.hpp
src/core/utilities/point/point.hpp
tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp
tests/core/data/gridlayout/gridlayout_test.hpp
tools/bench/hi5/write_particles.cpp

Additional comments: 25

tests/amr/data/particles/stream_pack/test_main.cpp (2)

226-229: The removal of assertions related to electromagnetic field components is consistent with the pull request's summary, which indicates that these fields are no longer stored within each particle. This change is likely due to the refactoring for better performance on GPGPU computations. The remaining assertions check for the preservation of other particle attributes, which is appropriate.

263-266: Similar to the previous hunk, the removal of assertions for electromagnetic field components is in line with the changes described in the pull request summary. The tests continue to assert the correctness of other particle attributes after the packing process, ensuring that the refactoring has not affected these properties.

src/core/data/particles/particle.hpp (3)

61-70: The operator== for Particle has been correctly updated to compare only the relevant members after the removal of the electromagnetic field components. This change is consistent with the pull request's goal of refactoring the particle handling.

93-97: The operator<< for Particle has been updated to remove the printing of the removed electromagnetic field components. This is a necessary change following the removal of these fields from the Particle struct.

121-129: The templated operator== function has been updated to compare only the relevant members of ParticleA and ParticleB. This change is necessary to maintain the correct behavior of equality checks after the electromagnetic field components have been removed from the Particle struct.

tests/core/data/particles/test_main.cpp (2)

23-27: The change in the AParticle constructor reflects the removal of electromagnetic field components from the Particle struct. This is consistent with the pull request summary stating that these components have been removed. The new initializer list values appear to be non-floating point for the second, fourth, and fifth elements, which is acceptable if the Particle struct's corresponding members are of integer or similar non-floating point types.

38-39: The test ParticleChargeIsInitiliazedOK has been updated to remove the newline between the test name and the opening brace. This is a minor style change and does not affect the functionality or correctness of the test.

tests/amr/data/particles/copy/test_particledata_copyNd.cpp (2)

117-119: The removal of assertions for electromagnetic field components (Ex, Ey, Ez, Bx, By, Bz) is consistent with the pull request summary stating that these fields have been removed from the Particle struct. This change in the test case reflects the updated design where electromagnetic fields are no longer stored within each particle. The remaining assertions check that other particle attributes are preserved after the copy operation, which is appropriate for the test's purpose.

132-134: Similar to the previous hunk, the removal of assertions for electromagnetic field components is consistent with the structural changes to the Particle struct. The test continues to validate the preservation of other particle attributes (weight, charge, iCell, delta, and v) after the copy operation, ensuring that the refactoring has not affected the correctness of the copy functionality.

tests/core/data/gridlayout/test_gridlayout.hpp (1)

1-27: The introduction of the TestGridLayout class is a good example of extending functionality for testing purposes. It inherits from GridLayout and provides a default constructor and a make function for easy instantiation with a given number of cells. The use of auto static constexpr for dim is a modern C++ feature that ensures the dimension is known at compile time, which is beneficial for performance.

However, there are a few points to consider:

Ensure that the GridLayout class is designed to be inherited from. If it was not designed with inheritance in mind, this could lead to issues such as slicing or improper use of member functions.

The constructor TestGridLayout(std::uint32_t cells) initializes GridLayout with arrays of 1.0 / cells and cells, which seems to be setting up a uniform grid layout. Confirm that this is the intended behavior for all tests that will use this class.

The make function is a static member that returns a TestGridLayout object. This is a factory method pattern, which is good for encapsulation and object creation but ensure that it is consistent with how objects are typically created in the codebase.

Overall, the changes seem to be well-aligned with the goal of refactoring for better GPU computation and testing. Just ensure that the inheritance and object creation patterns are consistent with the rest of the codebase.

src/amr/data/particles/refine/splitter.hpp (2)

55-63: The use of std::decay_t for type deduction is appropriate here as it ensures that the types Weight_t and Delta_t are decayed versions of the types of particle.weight and particle.delta[0], respectively. This is useful for removing any reference or cv-qualifiers that might be part of the type of particle.weight and particle.delta[0]. The use of constexpr for dimension, refRatio, and power is also correct, as these are compile-time constants.

70-87: The refactoring here correctly replaces direct type usage with the deduced types Weight_t and Delta_t. This ensures that the types are consistent with the types of particle.weight and particle.delta. The static cast to Weight_t in line 73 and to Delta_t in line 83 is necessary due to the change in the type of pattern.weight_ and pattern.deltas_. The calculation of integra and the update to fineParticle.delta and fineParticle.iCell in lines 84-86 are correctly using Delta_t, which is the deduced type for delta components. This should maintain the precision of the calculations and avoid any implicit type conversions that could lead to loss of precision or unexpected behavior.

However, it's important to ensure that the changes in the types do not introduce any performance regressions, especially since this code is intended for high-performance computing scenarios. The use of std::floor and the arithmetic operations may have different performance characteristics depending on the types of Delta_t.

src/core/numerics/pusher/boris.hpp (3)

93-103: The calculation of dto2m is now correctly moved out of the loop, which should improve performance by avoiding redundant calculations. However, it should be ensured that dto2m is initialized before move is called, possibly in setMeshAndTimeStep or the constructor, as previously suggested. This is to ensure that dto2m is always set to a valid value before being used.

186-250: The accelerate_ function has been updated to use the dto2m member variable, which is a good change for performance and maintainability. However, ensure that dto2m is properly initialized before this function is called. Also, the structured bindings used to unpack the tuple returned by the interpolator are a modern C++ feature that enhances readability and maintainability.

255-257: The dto2m variable is now declared in the private section of the class, which is good for encapsulation. Ensure that it is initialized before being used in any calculations to avoid undefined behavior.

src/core/utilities/box/box.hpp (1)

25-28: The addition of a static constant dimension to the Box struct is a good practice for providing easy access to the dimensionality of the box, which can be useful in various calculations and algorithms that operate on the box.

tools/bench/core/numerics/pusher/pusher.cpp (1)

1-60: - The code has been updated to use a new GridLayout_t and Electromag_t which are likely part of the refactoring to improve cache coherence and GPGPU computations. This change should be verified to ensure that it integrates well with the rest of the codebase and that any dependencies on the old layout and electromagnetic field representations have been updated accordingly.

The ParticleArray and Particle_t types are used to create a domain of particles and disperse them within a given range. It's important to ensure that the dispersion logic aligns with the new particle handling approach.

The BorisPusher_t is configured with a mesh size and time step, which are critical parameters for the simulation. These values should be carefully reviewed to ensure they are appropriate for the intended simulations.

The Interpolator class is used without any configuration, which implies that it should have sensible defaults or be stateless. This should be verified.

The no_op lambda function is used as a placeholder for particle selection. This indicates that the selection logic is either not needed for the benchmark or will be implemented later. It's important to ensure that this is intentional and documented.

The benchmarking code at the end of the file is well-structured, providing performance metrics for different dimensions and interpolation orders. It's crucial to verify that the benchmarking results are consistent with expectations after the refactoring.

Overall, the changes seem to be well-aligned with the goal of improving the codebase for GPU computations and cache coherence. However, it's essential to perform thorough testing and verification to ensure that the new approach works correctly and efficiently.

tools/bench/core/bench.hpp (1)

5-5: The change from benchmark.hpp to benchmark.h suggests that the benchmark library's header file might have been renamed or restructured. This is a common change when libraries evolve, but it's important to ensure that the new header file is available in the include path and that it provides all the necessary declarations that the codebase relies on.

tests/core/numerics/pusher/test_pusher.cpp (5)

20-26: The Trajectory struct has been updated to remove the electromagnetic field components. This change aligns with the overall refactoring strategy of the codebase to centralize the computation of electromagnetic fields and avoid storing them within each particle. This should indeed help with cache coherence and is a good move for GPGPU computations where memory bandwidth is a premium resource.

71-94: The Interpolator class has been refactored to return a tuple of arrays representing the electromagnetic fields. This change is consistent with the new approach of computing fields on-the-fly. The use of auto and structured bindings in C++17 makes the code more readable and less error-prone by avoiding manual indexing into arrays.

139-145: The removal of the extra comma after the nt member variable initialization is a minor syntax correction that prevents potential compiler warnings or errors. This is a good catch and improves the code quality.

231-237: The test cases have been updated to reflect the new approach of handling particles and fields. The loop structure and the use of the pusher->move function seem correct. However, ensure that the pusher->move function has been appropriately updated to work with the new Interpolator and that it no longer expects particles to carry their own electromagnetic field components.

268-274: The change from std::uniform_real_distribution<float> to std::uniform_real_distribution<double> is consistent with the addition of the -Wdouble-promotion flag in the build system, which helps to catch implicit type conversions from float to double. This change ensures that the type of the distribution matches the type of the variables being assigned, which is a good practice to prevent precision loss and potential bugs.

src/core/numerics/interpolator/interpolator.hpp (2)

485-520: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [461-514]

The refactoring of the Interpolator class to operate on single particles is consistent with the pull request's goal of improving performance for GPGPU computations. The use of tuples to return multiple values is a good practice for returning multiple related values from a function. However, ensure that the consumers of this function are updated to handle the tuple return type.

502-503: The previous comments from the security bot about unused local variables seem to have been addressed in this hunk. The variables Ex, Ey, Ez, Bx, By, and Bz are now being used to interpolate the electromagnetic fields onto the particle. This resolves the issue of unused local variables.

tools/bench/core/bench.hpp

tests/core/numerics/interpolator/test_main.cpp

coderabbitai

Review Status

Actionable comments generated: 17

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between d183651 and fe2cb2a.

Files selected for processing (19)

res/cmake/def.cmake (1 hunks)
src/amr/data/particles/refine/splitter.hpp (2 hunks)
src/core/data/particles/particle.hpp (3 hunks)
src/core/data/particles/particle_array.hpp (1 hunks)
src/core/numerics/interpolator/interpolator.hpp (5 hunks)
src/core/numerics/pusher/boris.hpp (4 hunks)
src/core/utilities/box/box.hpp (2 hunks)
src/core/utilities/point/point.hpp (1 hunks)
tests/amr/data/particles/copy/test_particledata_copyNd.cpp (2 hunks)
tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp (1 hunks)
tests/amr/data/particles/stream_pack/test_main.cpp (2 hunks)
tests/core/data/gridlayout/gridlayout_test.hpp (2 hunks)
tests/core/data/gridlayout/test_gridlayout.hpp (1 hunks)
tests/core/data/particles/test_main.cpp (2 hunks)
tests/core/numerics/interpolator/test_main.cpp (9 hunks)
tests/core/numerics/pusher/test_pusher.cpp (7 hunks)
tools/bench/core/bench.hpp (2 hunks)
tools/bench/core/numerics/pusher/pusher.cpp (1 hunks)
tools/bench/hi5/write_particles.cpp (1 hunks)

Files skipped from review due to trivial changes (5)

res/cmake/def.cmake
src/core/data/particles/particle_array.hpp
src/core/utilities/point/point.hpp
tests/amr/data/particles/stream_pack/test_main.cpp
tools/bench/hi5/write_particles.cpp

Additional comments: 28

tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp (1)

135-138: The removal of electromagnetic field components from the Particle struct is consistent with the pull request's summary. The test case has been updated to reflect this change by removing assertions related to electromagnetic field values. This change should be verified to ensure that it aligns with the intended design and that any necessary updates to the test cases have been made to maintain coverage.

tests/amr/data/particles/copy/test_particledata_copyNd.cpp (2)

117-119: The removal of assertions for electromagnetic field components (Ex, Ey, Ez, Bx, By, Bz) from the test case suggests that these attributes are no longer part of the Particle struct. This change should be verified to ensure that it aligns with the intended design and that any code relying on these attributes has been appropriately updated or refactored.

132-134: Similar to the previous comment, the removal of assertions for electromagnetic field components in the ghost particle checks should be verified for consistency with the new design. It's important to ensure that the test coverage is still adequate and that the removal of these checks does not lead to untested behavior.

tests/core/data/particles/test_main.cpp (2)

22-28: The removal of the initialization of electric and magnetic fields in the AParticle constructor is consistent with the summary stating that electromagnetic field components have been removed from the Particle struct. This change should be verified to ensure that all parts of the codebase that previously depended on these fields being initialized within the Particle struct are updated to work with the new design.

38-43: The test case ParticleChargeIsInitializedOK has been updated to remove the newline between the test name and the opening brace, which is a minor style change and does not affect the functionality of the test. This change is acceptable as long as it aligns with the project's coding style guidelines.

tests/core/data/gridlayout/gridlayout_test.hpp (2)

5-11: The inclusion of "test_gridlayout.hpp" suggests that there might be a refactoring to move some of the test logic into a separate header file. This is a common practice to improve modularity and reusability of test code. Ensure that the corresponding implementation file (test_gridlayout.cpp or similar) is updated accordingly to include the new header file and that all tests are still passing.

24-28: The removal of the TestGridLayout class and its associated methods indicates a significant refactor. Ensure that any tests that relied on the TestGridLayout class have been updated to work with the new structure. This might involve updating test cases to use the new test_gridlayout.hpp or other mechanisms provided in the refactor. Additionally, verify that the removal of this class does not leave any orphaned code segments that might still be referencing it.

tests/core/data/gridlayout/test_gridlayout.hpp (1)

1-27: The introduction of the TestGridLayout class is well-structured and follows C++ best practices. The use of auto static constexpr for dim is a modern C++ feature that ensures compile-time initialization of constants. The default constructor and the constructor taking a std::uint32_t argument are correctly defined. The make function is a good addition for creating instances in a clean and readable way.

However, there are a couple of points to consider:

The inclusion of gridlayoutimplyee.hpp is not demonstrated in the provided code. Ensure that it is required for the subsequent implementation not shown here.

The use of PHARE::core::ConstArray<double, dim>(1.0 / cells) assumes that division by zero is handled upstream. If cells can be zero, this should be checked to prevent runtime errors.

tools/bench/core/bench.hpp (2)

2-8: The change from benchmark/benchmark.hpp to benchmark/benchmark.h is likely due to an update in the benchmark library's header file naming convention. This should be verified to ensure compatibility with the version of the benchmark library being used in the project.

202-212: The sort function is specialized for PHARE::core::ParticleArray within the std namespace. Extending the std namespace is generally discouraged unless it's for a specialization of a template that is already in the std namespace for a user-defined type. If PHARE::core::ParticleArray is a user-defined type and not a type alias for a standard container, this is acceptable. However, if it's a type alias, then this should be moved to the PHARE::core namespace.

src/amr/data/particles/refine/splitter.hpp (2)

55-63: The use of std::decay_t is appropriate for type deduction to ensure that the types Weight_t and Delta_t are decayed types of the particle.weight and particle.delta[0] respectively. This is useful to remove any reference or cv-qualifiers that might be part of the type of particle.weight and particle.delta[0]. This change enhances the robustness of the code by ensuring that the types used in calculations are consistent with the types of the particle properties.

70-87: The loop correctly iterates over the split patterns and applies the weight and delta adjustments to the refined particles. The use of static_cast for pattern.weight_ and pattern.deltas_[rpIndex][iDim] ensures that the types are correctly converted to Weight_t and Delta_t, which is important for maintaining precision and avoiding implicit type conversions that could lead to data loss or unexpected behavior. The calculation of integra and the adjustment of fineParticle.delta and fineParticle.iCell are correctly implemented to handle the particle's position within the cell and the grid. The use of std::floor is appropriate for obtaining the integral part of the delta.

However, it's important to ensure that the particles vector has enough capacity to avoid reallocation during the loop, which could invalidate references or pointers to elements in the vector. This is especially important in a multithreaded context where such reallocations could lead to data races or undefined behavior. The assertion at the beginning of the dispatch function helps to ensure this, but it's also important to ensure that the particles vector is appropriately resized or reserved before calling this function.

src/core/numerics/pusher/boris.hpp (1)

255-257: The dto2m variable has been correctly moved to the private section of the class, which is good for encapsulation. This change ensures that dto2m is not accessible from outside the class, which can prevent unintended modifications from external code and maintain the integrity of the class's internal state.

tests/core/numerics/pusher/test_pusher.cpp (6)

71-94: The Interpolator class mock is well implemented for the test's purpose. It provides a fixed electromagnetic field which is necessary for a deterministic test outcome. However, ensure that the hardcoded values match the expected test conditions.

139-145: The constructor of APusher initializes several member variables and sets up the initial state for the tests. It's clear and concise. However, ensure that the hardcoded values for the Particle constructor match the expected initial conditions for the tests.

184-194: The loop correctly updates the actual arrays with the new positions of the particles. The call to pusher->move is also correct, but ensure that the move method is designed to handle the rangeIn and rangeOut correctly, especially since they are the same in this case.

209-218: Similar to the previous comment, the loop for the 2D pusher test is correct. Ensure that the move method and the Interpolator are correctly implemented to handle 2D cases.

231-237: The loop for the 1D pusher test is correct. As with the other dimensions, ensure that the move method and the Interpolator are correctly implemented to handle 1D cases.

268-274: The random initialization of particles is correct and uses a std::uniform_real_distribution<double> which matches the floating-point precision used in the rest of the test. This is important for maintaining consistency in the tests.

tools/bench/core/numerics/pusher/pusher.cpp (1)

1-60: The changes in this hunk reflect a significant overhaul of the particle pushing mechanism. The introduction of a new grid layout, particle initialization, and electromagnetic field setup are all aimed at improving the efficiency of the code, particularly for GPU architectures. The use of constexpr for cells and n_parts is good for performance as these values are known at compile time. The use of templates for dimension and interpolation order (dim and interp) is also a good practice for generic programming.

The Interpolator class is now being used to operate on single particles, which is a change from the previous design where it might have been used on collections of particles. This could potentially lead to more fine-grained control over the interpolation process but might also introduce overhead if not managed correctly, especially in a GPU context where parallelism is key.

The BorisPusher class has been modified to use interpolated fields for particle velocity updates, which is a significant change in the algorithm and should be carefully reviewed to ensure correctness.

The use of std::vector<Particle_t>(n_parts, core::bench::particle<dim>()) for initializing the domainParticles vector with n_parts particles is straightforward and ensures that all particles are initialized with the same state. However, this could be a potential performance bottleneck if core::bench::particle<dim>() is a complex operation. It might be beneficial to consider lazy initialization or a pool of pre-initialized particles if this becomes a performance issue.

The no_op lambda function is used as a placeholder for particle selection, which suggests that this functionality might be extended in the future. It's important to ensure that this does not become a forgotten piece of code and is either properly implemented or removed if not needed.

The while (state.KeepRunningBatch(1)) loop is typical in benchmarking code to run the test multiple times for accurate timing. However, it's important to ensure that the compiler does not optimize away the loop body. The use of benchmark::State suggests that the benchmarking framework is handling this, but it should be verified.

The BENCHMARK_TEMPLATE macros at the end of the file are used to instantiate benchmarks for different dimensions and interpolation orders. This is a good use of macros to avoid repetitive code.

Overall, the changes seem to be well thought out and aimed at improving the performance and maintainability of the code. However, given the complexity and the potential for subtle bugs in computational physics code, thorough testing and validation are crucial.

tests/core/numerics/interpolator/test_main.cpp (3)

274-279: The hardcoded values for iCell and delta should be verified to ensure they are within valid ranges for the simulation. If these values are specific to the test setup and not arbitrary, it would be beneficial to document the reason behind choosing these specific values. If they are arbitrary, consider defining them as constants with descriptive names to avoid magic numbers.

385-390: The same comment regarding hardcoded values for iCell and delta applies here as in hunk 0. Ensure these values are appropriate for the test cases and consider using named constants if they are arbitrary.

500-505: Again, verify that the hardcoded values for iCell and delta are within valid ranges for the simulation and consider using named constants if they are arbitrary.

src/core/utilities/box/box.hpp (2)

25-28: The addition of a static constant dimension to the Box struct is a good practice for providing easy access to the dimensionality of the box, which can be useful in various calculations and algorithms that operate on the box.

263-267: The grow function template has been correctly updated to take an additional template parameter OType for the size parameter. This change allows the function to work with different numeric types for the size parameter, enhancing type safety and flexibility. However, it's important to ensure that all calls to this function are updated to provide the correct type for the size parameter, and that the grow method inside the Box class is also updated to accept an OType parameter to maintain consistency.

src/core/numerics/interpolator/interpolator.hpp (1)

485-520: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [461-514]

The Interpolator class has been updated to handle individual particles, which is a significant change from the previous range-based approach. The code is well-structured, and the use of templates for different interpolation orders and dimensions is a good practice for code reuse and maintainability. The logging statements (lines 476 and 512) are useful for debugging and performance tracking. However, ensure that the logging framework used does not introduce significant overhead, especially in high-performance computing scenarios where this code is likely to be used.

src/core/data/particles/particle.hpp (2)

61-70: The removal of electromagnetic field components from the Particle struct is a significant change. Ensure that all parts of the codebase that previously relied on these fields being part of the Particle struct have been updated to work with the new design. This change could potentially improve performance by reducing the size of the Particle struct, which is beneficial especially when dealing with large numbers of particles in simulations.

121-129: The operator== function has been modified to use enable_if_t for type checking, which is a C++11 feature that allows for SFINAE (Substitution Failure Is Not An Error). This change ensures that the comparison operator is only enabled for valid Particle types. This is a good use of modern C++ features to ensure type safety and should work well to prevent incorrect usage at compile time.

src/core/numerics/pusher/boris.hpp

tests/core/numerics/interpolator/test_main.cpp

coderabbitai

Review Status

Actionable comments generated: 13

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between d183651 and 91d73a6.

Files selected for processing (17)

src/core/data/particles/particle.hpp (3 hunks)
src/core/data/particles/particle_array.hpp (1 hunks)
src/core/numerics/interpolator/interpolator.hpp (5 hunks)
src/core/numerics/pusher/boris.hpp (4 hunks)
src/core/utilities/box/box.hpp (2 hunks)
src/core/utilities/point/point.hpp (1 hunks)
tests/amr/data/particles/copy/test_particledata_copyNd.cpp (2 hunks)
tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp (1 hunks)
tests/amr/data/particles/stream_pack/test_main.cpp (2 hunks)
tests/core/data/gridlayout/gridlayout_test.hpp (2 hunks)
tests/core/data/gridlayout/test_gridlayout.hpp (1 hunks)
tests/core/data/particles/test_main.cpp (1 hunks)
tests/core/numerics/interpolator/test_main.cpp (3 hunks)
tests/core/numerics/pusher/test_pusher.cpp (5 hunks)
tools/bench/core/bench.hpp (2 hunks)
tools/bench/core/numerics/pusher/pusher.cpp (1 hunks)
tools/bench/hi5/write_particles.cpp (1 hunks)

Files skipped from review due to trivial changes (6)

src/core/data/particles/particle_array.hpp
src/core/utilities/point/point.hpp
tests/amr/data/particles/copy/test_particledata_copyNd.cpp
tests/core/data/particles/test_main.cpp
tools/bench/core/bench.hpp
tools/bench/hi5/write_particles.cpp

Additional comments: 15

tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp (1)

135-138: The removal of these assertions is consistent with the pull request's summary, which indicates that electromagnetic field components are no longer stored within each particle. This change should be verified to ensure that it aligns with the new design and that all necessary updates to the tests have been made.

tests/amr/data/particles/stream_pack/test_main.cpp (2)

226-229: The removal of the assertions for the electromagnetic field components (Ex, Ey, Ez, Bx, By, Bz) is consistent with the pull request summary, which states that these components have been removed from the Particle struct. The remaining assertions check for the preservation of other particle attributes (delta, weight, charge), which is appropriate given the refactoring context. Ensure that the removal of field components is intentional and that all related code has been updated accordingly.

263-266: Similar to the previous hunk, the assertions for the electromagnetic field components have been removed, and the test now checks for the preservation of other particle attributes after the packing process. This change aligns with the refactoring goals described in the pull request summary. As before, verify that the removal of field components is intentional and that all related code has been updated accordingly.

tests/core/data/gridlayout/gridlayout_test.hpp (1)

5-11: The inclusion of "test_gridlayout.hpp" suggests that there might be new test cases or utilities that are now part of the testing suite. Ensure that this new file is correctly placed in the project's directory structure and that it contains relevant content for the grid layout tests.

tests/core/numerics/pusher/test_pusher.cpp (5)

20-26: The Trajectory struct is simple and serves its purpose. The existing comment about potential future enhancements is still valid. If additional functionality becomes necessary, methods should be added to this struct.

71-94: The Interpolator class has been refactored to return a tuple of electric and magnetic field arrays. This change aligns with the pull request's goal of centralizing field computations. The hard-coded values within the operator() function are presumably for testing purposes and should reflect the expected field values for the test scenario.

139-145: The APusher test setup initializes particles with hardcoded values. Ensure that these values are representative of the test cases and that any changes to the particle initialization still align with the test objectives.

188-194: The move method of the BorisPusher class is now called with two instances of selector. This change should be verified to ensure that it aligns with the new single-particle computation approach and that the selector is correctly implemented to work with the new method signature.

212-218: Similar to the previous comment, verify that the move method call with the selector instances is correct and that the test logic is consistent with the expected behavior of the updated BorisPusher class.

src/core/utilities/box/box.hpp (1)

25-28: The addition of a static constant dimension to the Box struct is a good practice for providing easy access to the dimensionality of the box, which can be useful in various parts of the code where the dimension is needed. This change is straightforward and does not seem to introduce any issues.

src/core/numerics/interpolator/interpolator.hpp (1)

485-520: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [461-514]

The refactoring of the Interpolator class to handle single particles is consistent with the pull request's goal of improving cache-friendliness and suitability for GPGPU computations. The use of tuples and structured bindings (lines 478-513) to handle the electric and magnetic fields is a modern C++ feature that enhances readability and maintainability. However, ensure that all calls to this function are updated to match the new signature and that the removal of the fields from the Particle struct does not affect other parts of the codebase that may rely on them.

tools/bench/core/numerics/pusher/pusher.cpp (1)

1-60: The refactoring of the push function and related types appears to be consistent with the pull request's goal of improving cache-friendliness and suitability for GPGPU computations. The use of GridLayout_t, Electromag_t, and ParticleRange types suggests a more modular and flexible design. The removal of the field function and the changes to the push function to accept Electromag_t object as an argument align with the centralization of electromagnetic field computation.

The inclusion of test_gridlayout.hpp (line 2) suggests that the GridLayout_t type is being tested within the benchmark, which might be intended for ensuring the correctness of the layout within the performance testing context.

The ParticleArray and ParticleRange types (lines 21-23) are used to handle particles, which is consistent with the new approach of computing fields on-the-fly.

The BorisPusher_t type (lines 25-26) is updated to work with the new ParticleRange and Electromag_t types, which should allow for more efficient particle pushing operations.

The pusher.move call (lines 50-57) reflects the updated interface, which now requires the Electromag_t object and Interpolator to be passed in, aligning with the centralized field computation approach.

The use of benchmark::State& state and state.KeepRunningBatch(1) (lines 10 and 48) is appropriate for benchmarking, allowing for the measurement of performance over a consistent workload.

The no_op lambda (line 47) is used as a placeholder for operations that might be performed before and after the particle move operation. This is a flexible way to allow for additional operations without changing the benchmarking loop structure.

Overall, the changes seem to be well-aligned with the stated goals of the pull request and follow good practices in terms of modularity and performance considerations.

src/core/data/particles/particle.hpp (2)

61-70: The comparison operator has been correctly updated to reflect the removal of the electromagnetic field components from the Particle struct. It now compares only the remaining members: weight, charge, iCell, delta, and v. This change is consistent with the refactoring goals mentioned in the pull request summary.

121-129: The templated equality operator for comparing different types of particles has been correctly updated to compare only the relevant members. This is a good example of maintaining consistency across the codebase after a significant refactoring.

src/core/numerics/pusher/boris.hpp (1)

186-250: The accelerate_ function has been updated to take a single particle and its associated electric and magnetic fields, which is a good change for modularity and clarity. The unpacking of the tuple returned by the interpolator using structured bindings is a modern C++ feature that enhances readability. However, the use of dto2m here assumes that it has been correctly initialized before accelerate_ is called. It is important to ensure that dto2m is set in the setMeshAndTimeStep method or constructor to guarantee that it is always initialized before use.

tests/core/numerics/interpolator/test_main.cpp

src/core/numerics/pusher/boris.hpp

nicolasaunai

for some reason you have commits here that already have been merged on master

src/core/numerics/interpolator/interpolator.hpp

src/core/numerics/pusher/boris.hpp

tools/bench/core/bench.hpp

nicolasaunai · 2023-11-19T17:49:43Z

for some reason you have commits here that already have been merged on master

ah not anymore

coderabbitai

Review Status

Actionable comments generated: 11

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between b112c3b and 7dd228c.

Files selected for processing (17)

src/core/data/particles/particle.hpp (3 hunks)
src/core/data/particles/particle_array.hpp (1 hunks)
src/core/numerics/interpolator/interpolator.hpp (5 hunks)
src/core/numerics/pusher/boris.hpp (5 hunks)
src/core/utilities/box/box.hpp (2 hunks)
src/core/utilities/point/point.hpp (1 hunks)
tests/amr/data/particles/copy/test_particledata_copyNd.cpp (2 hunks)
tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp (1 hunks)
tests/amr/data/particles/stream_pack/test_main.cpp (2 hunks)
tests/core/data/gridlayout/gridlayout_test.hpp (2 hunks)
tests/core/data/gridlayout/test_gridlayout.hpp (1 hunks)
tests/core/data/particles/test_main.cpp (1 hunks)
tests/core/numerics/interpolator/test_main.cpp (3 hunks)
tests/core/numerics/pusher/test_pusher.cpp (5 hunks)
tools/bench/core/bench.hpp (2 hunks)
tools/bench/core/numerics/pusher/pusher.cpp (1 hunks)
tools/bench/hi5/write_particles.cpp (1 hunks)

Files skipped from review due to trivial changes (6)

src/core/data/particles/particle_array.hpp
src/core/utilities/point/point.hpp
tests/amr/data/particles/stream_pack/test_main.cpp
tests/core/data/particles/test_main.cpp
tests/core/numerics/interpolator/test_main.cpp
tools/bench/hi5/write_particles.cpp

Additional comments: 16

tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp (1)

135-138: The removal of EXPECT_DOUBLE_EQ assertions for electromagnetic field components is consistent with the refactoring described in the summary. The tests have been updated to no longer check for these values, which are no longer stored in the Particle struct. This change is appropriate given the new design of on-the-fly field computation.

tests/amr/data/particles/copy/test_particledata_copyNd.cpp (2)

117-119: The removal of assertions for electromagnetic field components (Ex, Ey, Ez, Bx, By, Bz) is consistent with the summary provided, which indicates that these fields are no longer stored in the Particle struct. This change in the test reflects the updated logic in the codebase where fields are likely computed on-the-fly. It is important to ensure that the new approach is thoroughly tested, possibly by verifying the correctness of the on-the-fly field computation elsewhere in the test suite.

132-134: Similar to the previous comment, the removal of assertions for electromagnetic field components in the patchGhostParticles is consistent with the changes in the codebase. As long as the new logic for handling electromagnetic fields is covered by other tests, this change is appropriate.

tests/core/data/gridlayout/test_gridlayout.hpp (1)

24-24: The make function is a static member that returns a TestGridLayout object. It is a factory method, which is a good practice for creating objects. However, it should return an object by value to enable Return Value Optimization (RVO) and Named Return Value Optimization (NRVO), which are already being utilized correctly here.

src/core/utilities/box/box.hpp (1)

25-28: The addition of a static constant dimension to the Box struct is a good practice as it provides a clear and easily accessible way to retrieve the dimensionality of the box. This can be useful in various parts of the code where the dimension is needed without having to pass it as a separate parameter.

src/core/numerics/interpolator/interpolator.hpp (1)

485-520: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [461-514]

The Interpolator class has been refactored to operate on individual particles, as indicated by the change in the function signature and the removal of the loop over a particle range. The code is now more focused on the interpolation of electromagnetic fields for a single particle. This change should improve the flexibility of the class and potentially simplify its usage in contexts where individual particle processing is required. The use of structured bindings (e.g., auto& [pE, pB] = particle_EB;) enhances readability and maintainability.

src/core/data/particles/particle.hpp (2)

61-70: The operator== for Particle has been correctly updated to compare only the relevant members after the removal of the electromagnetic field components. This is consistent with the refactoring goal of calculating fields on-the-fly.

121-129: The template operator== for comparing different types of particles has been correctly updated to compare only the relevant members. This ensures that the comparison logic is consistent with the updated Particle struct.

tools/bench/core/numerics/pusher/pusher.cpp (2)

9-58: The push function has been refactored to use the new TestGridLayout, Electromag, and Interpolator classes. The ParticleArray is initialized with a fixed number of particles, and the BorisPusher is used to move these particles. The no_op lambda function is used as a placeholder for operations that are not needed in this benchmark. The use of KeepRunningBatch in the while loop is appropriate for batch processing in benchmarks. Ensure that the pusher.move function signature matches the expected parameters after the refactoring.

60-60: The benchmark registration is done for different dimensions and interpolation orders. This is a good practice to ensure that the performance of the push function is tested across a range of scenarios. Ensure that all necessary dimensions and interpolation orders are covered by the benchmarks.

tests/core/numerics/pusher/test_pusher.cpp (5)

20-26: The Trajectory struct has been updated to include velocity components. This change is consistent with the removal of field components from the Particle struct and the shift towards on-the-fly field computation. The Trajectory struct now holds position and velocity data, which is likely used for comparison with expected results in tests.

71-94: The Interpolator class has been refactored to return interpolated field values directly, which is consistent with the changes in the main codebase. The use of a tuple to return both electric and magnetic field components is a good approach to keep the interface clean and return multiple values. The hard-coded values for the fields are appropriate for a mock used in testing.

139-145: The APusher test class has been updated to initialize a particle with a specific position and velocity. This change is likely to set up the initial conditions for the test cases. The use of emplace_back to add a new Particle instance to particlesIn is efficient and avoids unnecessary copies.

188-194: The move method of the BorisPusher class is now called with two instances of selector instead of a lambda function. This change is likely part of the refactoring to accommodate the new approach to particle handling. The use of selector suggests a more flexible design that could potentially handle different selection criteria for particles.

212-217: Similar to the previous comment, the move method is called with two instances of selector. This consistency across different dimensions (1D, 2D, 3D) in the test cases is good practice and ensures that the pusher's behavior is tested under similar conditions.

tools/bench/core/bench.hpp (1)

202-212: The sort function is specialized for PHARE::core::ParticleArray<dim> and placed within the std namespace. This is a common practice for enabling Argument-Dependent Lookup (ADL) when using std::sort with custom types. However, it's important to ensure that this specialization does not conflict with any other potential specializations of std::sort in the standard library or other third-party libraries. If there's no conflict, this is acceptable.

tests/core/data/gridlayout/gridlayout_test.hpp

tests/core/data/gridlayout/test_gridlayout.hpp

tools/bench/core/bench.hpp

src/core/numerics/interpolator/interpolator.hpp

coderabbitai

Review Status

Actionable comments generated: 12

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between 43c40cc and bb34fe5.

Files selected for processing (17)

src/core/data/particles/particle.hpp (3 hunks)
src/core/data/particles/particle_array.hpp (1 hunks)
src/core/numerics/interpolator/interpolator.hpp (5 hunks)
src/core/numerics/pusher/boris.hpp (5 hunks)
src/core/utilities/box/box.hpp (2 hunks)
src/core/utilities/point/point.hpp (1 hunks)
tests/amr/data/particles/copy/test_particledata_copyNd.cpp (2 hunks)
tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp (1 hunks)
tests/amr/data/particles/stream_pack/test_main.cpp (2 hunks)
tests/core/data/gridlayout/gridlayout_test.hpp (2 hunks)
tests/core/data/gridlayout/test_gridlayout.hpp (1 hunks)
tests/core/data/particles/test_main.cpp (1 hunks)
tests/core/numerics/interpolator/test_main.cpp (3 hunks)
tests/core/numerics/pusher/test_pusher.cpp (5 hunks)
tools/bench/core/bench.hpp (2 hunks)
tools/bench/core/numerics/pusher/pusher.cpp (1 hunks)
tools/bench/hi5/write_particles.cpp (1 hunks)

Files skipped from review due to trivial changes (6)

src/core/data/particles/particle_array.hpp
tests/amr/data/particles/stream_pack/test_main.cpp
tests/core/data/gridlayout/gridlayout_test.hpp
tests/core/data/gridlayout/test_gridlayout.hpp
tools/bench/core/bench.hpp
tools/bench/hi5/write_particles.cpp

Additional comments: 15

tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp (1)

139-142: The removal of electromagnetic field components from the Particle struct is reflected here as the test no longer checks for these attributes. This change is consistent with the pull request's goal of simplifying the particle data structure. Ensure that the removal of these checks does not affect the test's ability to verify the correctness of particle copying in periodic boundary conditions.

src/core/utilities/point/point.hpp (1)

88-91: The change to the toArray method introduces a default template argument, which is a good practice for providing default behavior while still allowing customization. This change should be backward compatible as long as there are no existing calls to toArray that rely on implicit template argument deduction that would conflict with the new default. However, it's important to verify that this change does not introduce any unexpected behavior in existing code where toArray is used.

tests/amr/data/particles/copy/test_particledata_copyNd.cpp (2)

121-123: The removal of electromagnetic field components from the Particle struct is reflected here as the assertions for Ex, Ey, Ez, Bx, By, Bz have been removed. This change is consistent with the pull request's goal of simplifying the particle data structure. Ensure that the updated tests still cover all necessary attributes of the Particle struct to maintain thorough testing.

136-138: Similar to the previous hunk, the assertions for electromagnetic field components have been removed here as well. It is important to verify that the remaining attributes are sufficient for the test's purpose and that no additional attributes have been added to the Particle struct that require testing.

src/core/utilities/box/box.hpp (1)

25-28: The addition of a static constant dimension to the Box struct is a good practice as it provides a clear and easily accessible way to retrieve the dimensionality of the box. This can be useful in various parts of the code where operations might depend on the dimensionality of the space.

tests/core/numerics/pusher/test_pusher.cpp (4)

71-94: The Interpolator class has been refactored to return a tuple of arrays representing electric and magnetic field components for a single particle. This change aligns with the overall refactoring goal of improving cache efficiency and is beneficial for GPGPU computations.

139-145: The APusher test setup initializes particles with a fixed position and velocity. Ensure that this setup is representative of the scenarios that the pusher will encounter in production to avoid tests that pass due to unrealistic initial conditions.

188-194: The pusher->move method is now called with a single particle range instead of separate input and output ranges. This change simplifies the interface and reduces the potential for errors when managing particle data.

212-218: Similar to the previous comment, the pusher->move method is called with a single particle range, which is a positive change. However, ensure that the test cases cover a variety of scenarios to fully validate the pusher's behavior.

src/core/numerics/interpolator/interpolator.hpp (1)

485-520: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [461-514]

The Interpolator class has been refactored to operate on single particles instead of particle ranges, returning interpolated electromagnetic fields as a tuple. This change aligns with the pull request's goal of improving cache efficiency and is expected to benefit GPGPU computations. The code is well-structured, and the use of templates ensures that the interpolation order and dimensionality are flexible and determined at compile-time.

src/core/data/particles/particle.hpp (2)

61-70: The operator== for Particle has been updated to reflect the removal of electromagnetic field components. It now compares only the weight, charge, iCell, delta, and v members. This change is consistent with the refactoring goal of simplifying the particle data structure. Ensure that all usages of this operator in the codebase are consistent with the new definition.

121-129: The operator== template function has been updated to use enable_if_t to compare particles of different types. This change is part of the refactoring to support different particle types and is a good use of SFINAE (Substitution Failure Is Not An Error) to ensure type safety. Verify that this change does not affect any existing functionality where particles of different types are compared.

src/core/numerics/pusher/boris.hpp (3)

87-110: The loop in the move function iterates over the particles and calls accelerate_ for each one. This is a significant change from operating on ranges of particles to individual particles, which can be beneficial for cache efficiency and GPGPU computations. However, it's important to ensure that this change does not introduce any unintended side effects, such as race conditions or performance bottlenecks, especially if the code is intended to run in parallel environments. The developers should verify that the new implementation scales well with the number of particles and that it is thread-safe if used in a multi-threaded context.

167-255: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [154-187]

The prePushStep_ and postPushStep_ functions have been introduced to separate the pre-push and post-push steps. This is a good design choice as it modularizes the code and makes it clearer what each step is responsible for. However, it's important to ensure that the separation of these steps does not introduce any inconsistencies in the particle states between the steps. The developers should verify that the particle data remains consistent and that the separation does not affect the correctness of the simulation.

195-253: The accelerate_ function has been refactored to accept a single particle and its associated electromagnetic fields. This change is consistent with the overall refactoring towards handling individual particles. The function now directly updates the velocity of the given particle based on the provided electromagnetic fields. It's important to ensure that the electromagnetic fields are correctly interpolated and passed to this function, as any errors in field values could lead to incorrect particle acceleration and thus affect the simulation results.

tools/bench/core/numerics/pusher/pusher.cpp

coderabbitai · 2023-11-21T10:30:55Z

src/core/utilities/box/box.hpp

+template<typename Type, std::size_t dim, typename OType>
+Box<Type, dim> grow(Box<Type, dim> const& box, OType const& size)
 {
    auto copy{box};
    copy.grow(size);


The previous comment regarding the grow function within the Box class not being updated to match the new template signature with OType is still valid. The grow method should be updated to handle different arithmetic types for the size parameter, as suggested in the previous review. This will ensure consistency and flexibility in the Box class's interface.

template<typename OType> void Box::grow(OType const& size) { static_assert(std::is_arithmetic<OType>::value, "Size must be an arithmetic type"); assert(size >= 0); for (auto& c : lower) { c -= static_cast<Type>(size); } for (auto& c : upper) { c += static_cast<Type>(size); } }

tests/core/numerics/pusher/test_pusher.cpp

src/core/numerics/interpolator/interpolator.hpp

src/core/data/particles/particle.hpp

src/core/numerics/pusher/boris.hpp

tests/core/numerics/interpolator/test_main.cpp

coderabbitai

Review Status

Actionable comments generated: 3

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between bb34fe5 and 09746ff.

Files selected for processing (1)

tools/bench/core/bench.hpp (2 hunks)

tools/bench/core/bench.hpp

coderabbitai · 2023-11-23T10:17:12Z

tools/bench/core/bench.hpp

+
+template<std::size_t dim>
+auto& sort(PHARE::core::ParticleArray<dim>& particles)
+{
+    using box_t = typename PHARE::core::ParticleArray<dim>::box_t;
+    PHARE::core::LocalisedCellFlattener<box_t> cell_flattener{grow(particles.box(), 1)};
+    std::sort(particles.vector().begin(), particles.vector().end(),
+              [&](auto const& a, auto const& b) {
+                  return cell_flattener(a.iCell) < cell_flattener(b.iCell);
+              });
+    return particles;
+}
+
+template<std::size_t dim>
+auto sort(PHARE::core::ParticleArray<dim>&& particles)
+{
+    return sort(particles);
+}


The sort function is specialized for PHARE::core::ParticleArray within the std namespace. Extending the std namespace is not recommended unless specializing a function for a user-defined type that is intended to work with a standard library template. If this is not the case, consider moving the sort function to the PHARE::core namespace or another appropriate namespace within the project.

tools/bench/core/bench.hpp

…HAREHUB#781) push benchmark updates ebless particles, expect_true(abs)) -> expect_near

coderabbitai bot reviewed Nov 18, 2023

View reviewed changes

tests/core/numerics/interpolator/test_main.cpp Outdated Show resolved Hide resolved

tests/core/numerics/interpolator/test_main.cpp Outdated Show resolved Hide resolved

tests/core/numerics/interpolator/test_main.cpp Outdated Show resolved Hide resolved

PhilipDeegan changed the title ~~mesh to particle eb handled directory in boris~~ eb-less particles - mesh to particle eb handled directory in boris Nov 18, 2023

PhilipDeegan force-pushed the ebless branch from 4abed81 to 577caf6 Compare November 18, 2023 19:44

coderabbitai bot reviewed Nov 18, 2023

View reviewed changes

github-advanced-security bot found potential problems Nov 18, 2023

View reviewed changes

PhilipDeegan force-pushed the ebless branch from 577caf6 to d1d7268 Compare November 18, 2023 23:11

coderabbitai bot reviewed Nov 18, 2023

View reviewed changes

tools/bench/core/bench.hpp Show resolved Hide resolved

PhilipDeegan force-pushed the ebless branch from d1d7268 to 31ce3ba Compare November 18, 2023 23:29

coderabbitai bot reviewed Nov 18, 2023

View reviewed changes

PhilipDeegan force-pushed the ebless branch from 31ce3ba to fe2cb2a Compare November 18, 2023 23:49

coderabbitai bot reviewed Nov 18, 2023

View reviewed changes

PhilipDeegan force-pushed the ebless branch from fe2cb2a to 91d73a6 Compare November 19, 2023 13:34

coderabbitai bot reviewed Nov 19, 2023

View reviewed changes

nicolasaunai reviewed Nov 19, 2023

View reviewed changes

src/core/numerics/interpolator/interpolator.hpp Show resolved Hide resolved

src/core/numerics/interpolator/interpolator.hpp Show resolved Hide resolved

src/core/numerics/pusher/boris.hpp Outdated Show resolved Hide resolved

tools/bench/core/bench.hpp Show resolved Hide resolved

PhilipDeegan force-pushed the ebless branch from 91d73a6 to 7dd228c Compare November 19, 2023 19:44

coderabbitai bot reviewed Nov 19, 2023

View reviewed changes

tests/core/data/gridlayout/gridlayout_test.hpp Show resolved Hide resolved

tests/core/data/gridlayout/test_gridlayout.hpp Show resolved Hide resolved

tools/bench/core/bench.hpp Outdated Show resolved Hide resolved

src/core/numerics/interpolator/interpolator.hpp Show resolved Hide resolved

PhilipDeegan added 3 commits November 21, 2023 11:09

push benchmark updates

3e92405

ebless particles

523a5dd

expect_true(abs)) -> expect_near

bb34fe5

PhilipDeegan force-pushed the ebless branch from 7dd228c to bb34fe5 Compare November 21, 2023 10:25

coderabbitai bot reviewed Nov 21, 2023

View reviewed changes

pr comments /cleanup

09746ff

coderabbitai bot reviewed Nov 23, 2023

View reviewed changes

nicolasaunai approved these changes Nov 25, 2023

View reviewed changes

nicolasaunai merged commit ab52821 into PHAREHUB:master Nov 25, 2023
6 checks passed

PhilipDeegan added a commit to PhilipDeegan/PHARE that referenced this pull request Nov 30, 2023

eb-less particles - mesh to particle eb handled directory in boris (P…

b659cd9

…HAREHUB#781) push benchmark updates ebless particles, expect_true(abs)) -> expect_near

PhilipDeegan deleted the ebless branch February 26, 2024 14:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eb-less particles - mesh to particle eb handled directory in boris #781

eb-less particles - mesh to particle eb handled directory in boris #781

PhilipDeegan commented Nov 18, 2023 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 18, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

nicolasaunai left a comment

nicolasaunai commented Nov 19, 2023

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot Nov 21, 2023

coderabbitai bot left a comment

coderabbitai bot Nov 23, 2023

eb-less particles - mesh to particle eb handled directory in boris #781

eb-less particles - mesh to particle eb handled directory in boris #781

Conversation

PhilipDeegan commented Nov 18, 2023 • edited by coderabbitai bot Loading

Summary by CodeRabbit

coderabbitai bot commented Nov 18, 2023 • edited Loading

Chat with CodeRabbit Bot (@coderabbitai)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

nicolasaunai left a comment

Choose a reason for hiding this comment

nicolasaunai commented Nov 19, 2023

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Nov 21, 2023

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Nov 23, 2023

Choose a reason for hiding this comment

PhilipDeegan commented Nov 18, 2023 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 18, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Configration File (`.coderabbit.yaml`)