-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
try float32 #943
base: master
Are you sure you want to change the base?
try float32 #943
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a comprehensive type system overhaul across the PHARE project, focusing on introducing a new Changes
Sequence DiagramsequenceDiagram
participant Core as Core Library
participant FloaterType as Floater Type System
participant Particle as Particle Components
Core->>FloaterType: Define floater_t<N>
FloaterType-->>Core: Configurable precision types
Core->>Particle: Apply floater_t<N> to attributes
Particle-->>Core: Update weight, charge, delta types
Core->>Core: Propagate type changes across codebase
The sequence diagram illustrates how the new ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/core/data/particles/particle.hpp (2)
39-42
: Consider adding static_assert for floating-point type.The new template parameter
Float_
should be constrained to floating-point types to prevent potential misuse.template<size_t dim, typename Float_ = float> struct Particle { + static_assert(std::is_floating_point_v<Float_>, "Float_ must be a floating-point type"); using Float_t = Float_;
112-112
: Consider using Float_t in ParticleView.For consistency with the templated
Particle
struct, consider parameterizingParticleView
withFloat_
as well, instead of hardcodingfloat
.- std::array<float, dim>& delta; + std::array<Float_t, dim>& delta;src/core/data/particles/particle_array.hpp (2)
298-299
: Consider adding static_assert for Container_delta element type.To prevent type mismatches, consider adding a compile-time check to ensure Container_delta's value_type matches the expected floating-point type.
template<typename Container_int, typename Container_delta, typename Container_double> + static_assert(std::is_same_v<typename Container_delta::value_type, float>, + "Container_delta must contain float elements"); ContiguousParticles(Container_int&& _iCell, Container_delta&& _delta,
377-377
: Document memory layout and performance implications.The change from
container_t<double>
tocontainer_t<float>
fordelta
while keeping other containers asdouble
affects memory layout and might impact cache utilization. Consider documenting:
- The rationale for using mixed precision
- Expected performance benefits
- Any potential precision trade-offs
tests/core/numerics/pusher/test_pusher.cpp (1)
149-149
: Consider extracting particle initialization to a helper function.The particle initialization logic could be moved to a helper function to improve readability and reusability.
+ static Particle createParticle(ConstArray<int, dim> const& cell, + ConstArray<Float_t, dim> const& delta, + std::array<double, 3> const& velocity) + { + return Particle{1., 1., cell, delta, velocity}; + } + APusher() : expectedTrajectory{readExpectedTrajectory()} , particlesIn{layout.AMRBox()} , particlesOut(layout.AMRBox()) , pusher{std::make_unique<Pusher_>()} , mass{1} , dt{0.0001} , tstart{0} , tend{10} , nt{static_cast<std::size_t>((tend - tstart) / dt + 1)} { - particlesIn.emplace_back( - Particle{1., 1., ConstArray<int, dim>(5), ConstArray<Float_t, dim>(0.), {0., 10., 0.}}); + particlesIn.emplace_back(createParticle( + ConstArray<int, dim>(5), ConstArray<Float_t, dim>(0.), {0., 10., 0.}));src/core/numerics/pusher/boris.hpp (1)
Line range hint
137-141
: Consider adding range validation for particle movement.The error check for particle movement could be enhanced with a more descriptive error message and proper error handling.
if (std::abs(delta) > 2) { - PHARE_LOG_ERROR("Error, particle moves more than 1 cell, delta >2"); + std::string msg = "Particle movement exceeds maximum allowed distance: |delta| = " + + std::to_string(std::abs(delta)) + " > 2 cells"; + PHARE_LOG_ERROR(msg); + throw std::runtime_error(msg); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
res/cmake/def.cmake
(1 hunks)res/cmake/dep/highfive.cmake
(1 hunks)src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp
(3 hunks)src/core/data/particles/particle.hpp
(4 hunks)src/core/data/particles/particle_array.hpp
(2 hunks)src/core/hybrid/hybrid_quantities.hpp
(0 hunks)src/core/numerics/pusher/boris.hpp
(1 hunks)src/diagnostic/detail/h5writer.hpp
(1 hunks)src/python3/particles.hpp
(2 hunks)src/python3/pybind_def.hpp
(1 hunks)tests/core/data/particles/test_interop.cpp
(2 hunks)tests/core/numerics/interpolator/test_main.cpp
(2 hunks)tests/core/numerics/pusher/test_pusher.cpp
(6 hunks)
💤 Files with no reviewable changes (1)
- src/core/hybrid/hybrid_quantities.hpp
🧰 Additional context used
📓 Path-based instructions (7)
src/core/numerics/pusher/boris.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/data/particles/particle_array.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/data/particles/particle.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/python3/pybind_def.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/python3/particles.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/diagnostic/detail/h5writer.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze (python)
- GitHub Check: build (macos-13)
🔇 Additional comments (15)
tests/core/data/particles/test_interop.cpp (2)
24-24
: LGTM! Type alias enhances code maintainability.The addition of
Float_t
type alias improves code maintainability by centralizing the floating-point type definition.
36-36
: Verify float precision requirements.The change from
double
toFloat_t
for delta values might affect numerical precision in particle calculations.✅ Verification successful
Float precision for delta is appropriate ✅
The codebase consistently uses float precision for delta values, including in Python bindings and tests, while maintaining double precision for other critical particle attributes (weight, charge, velocity). The mathematical operations involving delta are simple position offsets and interpolation weights that don't require double precision.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential precision-sensitive calculations with delta # Search for mathematical operations involving delta rg -A 3 "delta\s*[+\-*/]|[+\-*/]\s*delta" --type cppLength of output: 8331
src/python3/pybind_def.hpp (2)
22-27
: LGTM! Well-documented type definitions.The tuple elements are clearly documented with their purposes, and the delta type change aligns with the float32 transition.
29-34
: LGTM! Consistent type changes in const reference tuple.The const reference version maintains consistency with the non-const version.
src/python3/particles.hpp (2)
20-20
: LGTM! Consistent type change in view creation.The change to
float
for delta values maintains consistency with the new type system.
29-33
: Verify Python-side type handling.The mixed use of float and double types in the tuple might need special handling in Python code.
✅ Verification successful
Mixed types are properly handled in Python code
The codebase shows explicit type awareness and proper handling of mixed float/double precision:
- Explicit float32/float64 type checking in particle comparisons
- Appropriate numerical tolerances (1e-6 for float32, 1e-12 for float64)
- Consistent use of numpy's type system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for Python code handling particle data types # Search for Python files using these particle arrays rg -l "makePyArrayTuple|contiguousViewFrom" --type py # Search for numpy dtype specifications rg "dtype\s*=" --type pyLength of output: 8827
Script:
#!/bin/bash # Search for particle-related Python files and type handling rg -l "particle" --type py # Get content of key particle-related files fd "particle.*\.py$" | xargs cat # Look for type conversion patterns rg "astype|dtype|float32|float64|np\.single" --type py | grep -i "particle"Length of output: 27509
src/core/data/particles/particle.hpp (1)
62-64
: Verify memory layout impact.The change from
double
toFloat_t
fordelta
while keeping other members asdouble
might affect memory layout and padding. Consider documenting the expected memory layout or aligning all floating-point members to the same type for consistency.✅ Verification successful
Memory layout is optimal, mixed precision is intentional.
The current memory layout is already optimal with natural alignment:
iCell
: Integer array for cell indicesdelta
: Single-precision array for normalized sub-cell position [0,1]v
: Double-precision array for particle velocity requiring higher accuracy🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check memory layout impact of mixed floating-point types. # Search for other mixed floating-point type usage rg -A 5 "std::array<(float|double)" # Search for potential alignment issues ast-grep --pattern 'struct $_ { $$$ std::array<double, $_> $_ std::array<float, $_> $_ $$$ }'Length of output: 47776
src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp (2)
141-142
: LGTM! Good use of type alias.The introduction of
Float_t
type alias improves code maintainability by centralizing the floating-point type definition.
Line range hint
153-181
: Verify numerical precision impact.The change from
double
toFloat_t
in particle initialization might affect numerical precision. Consider documenting the precision requirements and validating thatfloat
provides sufficient precision for the particle delta calculations.Consider adding a comment explaining the rationale for using lower precision and any performance benefits or trade-offs.
res/cmake/def.cmake (1)
72-72
: LGTM! Appropriate warning flag removal.Removing
-Wdouble-promotion
is consistent with the codebase's intentional use offloat
types for particle deltas.src/diagnostic/detail/h5writer.hpp (1)
42-42
:⚠️ Potential issueRemove redundant preprocessor check and verify data precision requirements.
The preprocessor check for
PHARE_DIAG_DOUBLES
at the top of the file is now redundant sinceFloatType
is fixed tofloat
. Additionally, this change might affect data precision and compatibility with existing HDF5 files.Run the following script to check for potential precision-dependent code:
✅ Verification successful
Precision change to float is safe and preprocessor check removal is correct
The change to fix
FloatType
asfloat
and remove thePHARE_DIAG_DOUBLES
preprocessor check is safe because:
- Production code uses tolerances compatible with float precision
- The change only affects diagnostic data storage precision
- The type is used consistently through the H5Writer template system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential precision-dependent code and HDF5 file format dependencies # Check for direct float/double comparisons or precision-dependent calculations rg -A 3 "1e-\d+" # Check for HDF5 file format version checks or compatibility code rg -A 3 "H5[A-Z][a-zA-Z_]+" # Check for other uses of PHARE_DIAG_DOUBLES rg "PHARE_DIAG_DOUBLES"Length of output: 55231
tests/core/numerics/pusher/test_pusher.cpp (2)
129-131
: LGTM! Type aliases improve type safety.The addition of type aliases enhances type safety and maintainability by using the particle's own float type.
255-257
: LGTM! Type aliases improve consistency.The addition of type aliases in
APusherWithLeavingParticles
maintains consistency with the changes inAPusher
.tests/core/numerics/interpolator/test_main.cpp (1)
674-674
: LGTM! Type alias and usage improve type safety.The addition of the
Float_t
type alias and its usage in particle initialization enhance type safety and maintainability.Also applies to: 695-695
src/core/numerics/pusher/boris.hpp (1)
130-131
: LGTM! Type alias improves type safety and consistency.The addition of the
Float_t
type alias and its usage in position calculations enhance type safety and maintain consistency with the particle's own type.Also applies to: 135-135
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tools/bench/core/bench.hpp
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tools/bench/core/bench.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
- GitHub Check: build (ubuntu-latest)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/core/def.hpp (1)
51-63
: Document the binary pattern's significance.The bitset initialization and its usage need documentation:
- The binary pattern
0b11110
determines which components use double precision.- The index mapping in the comments should explain why certain indices map to specific components.
Add a detailed comment block:
+/** + * Bitset controlling floating-point precision for different components: + * - bit 0 (rightmost): particle delta + * - bit 1: particle velocity + * - bit 2: particle charge + * - bit 3: particle weight + * - bit 4: fields + * + * 1 = double precision, 0 = single precision + */ constexpr std::bitset<5> doubles{0b11110};src/core/data/ions/particle_initializers/maxwellian_particle_initializer_base.hpp (1)
Line range hint
50-96
: Consider optimizing the vector normalization.The function performs multiple divisions by the same normalization factor.
Consider extracting the normalization into a separate step:
- basis[0][0] = B[0] / b2; - basis[0][1] = B[1] / b2; - basis[0][2] = B[2] / b2; + auto const b2_inv = T0{1} / b2; + basis[0][0] = B[0] * b2_inv; + basis[0][1] = B[1] * b2_inv; + basis[0][2] = B[2] * b2_inv;src/phare_core.hpp (1)
4-19
: Consider grouping related headers.The header includes could be better organized by grouping related functionality.
Consider reorganizing the includes:
+// Core data structures #include "core/data/ions/ions.hpp" #include "core/data/grid/grid.hpp" #include "core/data/grid/gridlayout.hpp" + +// Field-related headers #include "core/data/vecfield/vecfield.hpp" #include "core/data/electrons/electrons.hpp" #include "core/data/electromag/electromag.hpp" + +// Array and particle headers #include "core/data/ndarray/ndarray_vector.hpp" #include "core/data/grid/gridlayoutimplyee.hpp" #include "core/data/particles/particle_array.hpp"src/core/data/particles/particle.hpp (1)
Line range hint
45-62
: Document the meaning of floater_t template parameters.The different template parameters (0-3) used for
floater_t
are not self-explanatory. Please add documentation explaining the significance of each parameter value and why specific members use specific parameter values.src/core/numerics/pusher/boris.hpp (1)
Line range hint
132-144
: Enhance error message for better debugging.The error message "Error, particle moves more than 1 cell, delta >2" could be more descriptive by including:
- The actual delta value
- The dimension where the violation occurred
- The particle's current position
- PHARE_LOG_ERROR("Error, particle moves more than 1 cell, delta >2"); + PHARE_LOG_ERROR("Particle movement error: delta[{}] = {} exceeds maximum allowed distance (>2) at position {}", + iDim, delta, partIn.iCell[iDim]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/core/CMakeLists.txt
(0 hunks)src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp
(4 hunks)src/core/data/ions/particle_initializers/maxwellian_particle_initializer_base.hpp
(4 hunks)src/core/data/ndarray/ndarray_vector.hpp
(1 hunks)src/core/data/particles/particle.hpp
(4 hunks)src/core/data/particles/particle_array.hpp
(4 hunks)src/core/def.hpp
(2 hunks)src/core/numerics/pusher/boris.hpp
(2 hunks)src/core/utilities/logger/logger_defaults.hpp
(1 hunks)src/phare_core.hpp
(2 hunks)tests/core/data/particles/test_interop.cpp
(1 hunks)tests/core/numerics/interpolator/test_main.cpp
(1 hunks)tests/core/numerics/pusher/test_pusher.cpp
(6 hunks)tests/simulator/test_advance.py
(1 hunks)
💤 Files with no reviewable changes (1)
- src/core/CMakeLists.txt
✅ Files skipped from review due to trivial changes (2)
- src/core/data/ndarray/ndarray_vector.hpp
- src/core/utilities/logger/logger_defaults.hpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp
- tests/core/data/particles/test_interop.cpp
- tests/core/numerics/interpolator/test_main.cpp
🧰 Additional context used
📓 Path-based instructions (6)
src/core/def.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/numerics/pusher/boris.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/phare_core.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/data/ions/particle_initializers/maxwellian_particle_initializer_base.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/data/particles/particle_array.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/data/particles/particle.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-13)
🔇 Additional comments (11)
src/core/def.hpp (2)
4-5
: LGTM!The includes are appropriate for the functionality being added.
65-72
: LGTM!The template implementation is clean and efficient:
- Uses
std::conditional_t
for type selection- Provides a convenient type alias
src/core/data/ions/particle_initializers/maxwellian_particle_initializer_base.hpp (2)
Line range hint
34-46
: LGTM!The basis transform implementation is correct and efficiently templated.
17-25
: Consider adding parameter validation.The function uses
std::forward_as_tuple
to unpack arguments but doesn't validate the number of arguments.Add a static assertion to ensure the correct number of arguments:
template<typename T, typename... Args> void maxwellianVelocity(std::array<T, 3>& partVelocity, std::mt19937_64& generator, Args const... args) { + static_assert(sizeof...(Args) == 6, "Expected 6 arguments: V0, V1, V2, Vth0, Vth1, Vth2"); auto const& [V0, V1, V2, Vth0, Vth1, Vth2] = std::forward_as_tuple(args...);
✅ Verification successful
Consider adding parameter validation.
The suggested static assertion is a good addition as it provides compile-time validation and explicitly documents the requirements. While the current implementation always passes the correct number of arguments, the assertion will prevent potential misuse if the base class is used directly in the future.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for the maxwellian velocity function implementations and their context ast-grep --pattern 'void maxwellianVelocity' -A 10 # Search for static assertions in particle initializers rg "static_assert.*Args" src/ # Find files containing maxwellian in their name to understand the context better fd maxwellianLength of output: 491
Script:
#!/bin/bash # Check test file content cat tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cpp # Check implementation details cat src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hppLength of output: 10373
src/phare_core.hpp (1)
37-37
: LGTM!The array type now correctly uses
floater_t<4>
for field values.tests/simulator/test_advance.py (1)
291-292
: LGTM!The tolerance adjustment from
5.5e-15
to5e-15
is appropriate given the transition to the new floating-point type system.src/core/data/particles/particle.hpp (2)
107-111
: LGTM!The changes to ParticleView member variables are consistent with the Particle struct changes.
124-126
: LGTM!The formatting changes improve code readability while maintaining functionality.
src/core/data/particles/particle_array.hpp (1)
Line range hint
300-381
: LGTM!The changes to ContiguousParticles improve type safety while maintaining consistency with the Particle struct changes.
tests/core/numerics/pusher/test_pusher.cpp (2)
Line range hint
129-149
: LGTM!The addition of ParticleArray_t type alias and the use of floater_t<0> improve code clarity and maintain consistency with production code changes.
Line range hint
254-303
: LGTM!The changes in APusherWithLeavingParticles maintain consistency with the production code while preserving test coverage.
src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp
Fixed
Show resolved
Hide resolved
src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp
Fixed
Show resolved
Hide resolved
src/core/data/ions/particle_initializers/maxwellian_particle_initializer_base.hpp
Dismissed
Show resolved
Hide resolved
|
||
#include "core/utilities/types.hpp" | ||
|
||
// #include <iostream> |
Check notice
Code scanning / CodeQL
Commented-out code Note
} | ||
|
||
template<std::uint8_t i> | ||
struct Floater |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
placeholder name until future improvement
57fe7e7
to
8db371c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/core/numerics/pusher/boris.hpp (1)
4-14
:⚠️ Potential issueRestore commented-out includes and implementation.
The commented-out includes appear to be necessary for the code to function properly. Additionally, keeping commented-out implementation code reduces maintainability and clarity.
Please:
- Restore the commented-out includes:
- core/utilities/range/range.hpp
- core/data/particles/particle.hpp
- Either remove or restore the commented-out implementation of the
move
function.
🧹 Nitpick comments (5)
tests/core/numerics/pusher/test_pusher.cpp (1)
189-194
: Consider extracting position calculation into a helper method.The position calculation is repeated across multiple test cases. Consider extracting it into a helper method to improve maintainability and reduce code duplication.
+private: + floater_t<0> calculatePosition(size_t dim, const Particle& particle) const { + return (particle.iCell[dim] + particle.delta[dim]) * floater_t<0>(dxyz[dim]); + } + // In test: - actual[0][i] - = (particlesOut[0].iCell[0] + particlesOut[0].delta[0]) * floater_t<0>(dxyz[0]); - actual[1][i] - = (particlesOut[0].iCell[1] + particlesOut[0].delta[1]) * floater_t<0>(dxyz[1]); - actual[2][i] - = (particlesOut[0].iCell[2] + particlesOut[0].delta[2]) * floater_t<0>(dxyz[2]); + actual[0][i] = calculatePosition(0, particlesOut[0]); + actual[1][i] = calculatePosition(1, particlesOut[0]); + actual[2][i] = calculatePosition(2, particlesOut[0]);src/core/def.hpp (2)
49-54
: Add documentation for the bitset configuration.While the implementation is sound, consider adding a comment block explaining:
- The impact of each bit position
- How to configure PHARE_DOUBLE_BITSET
- The rationale behind the default configuration
56-66
: Consider using an enum for type indices.Replace magic numbers with an enum for better type safety and self-documentation:
+enum class FloaterIndex : std::uint8_t { + PARTICLE_DELTA = 0, + PARTICLE_V = 1, + PARTICLE_CHARGE = 2, + PARTICLE_WEIGHT = 3, + FIELDS = 4 +}; template<std::uint8_t i> bool constexpr is_double() { - // 0 = particle delta - // 1 = particle v - // 2 = particle charge - // 3 = particle weight - // 4 = fields return doubles[i] == 1; }src/core/data/ions/particle_initializers/maxwellian_particle_initializer_base.hpp (1)
Line range hint
34-46
: Consider making basis array type consistent.The basis array still uses
double
whilevec
is templated. Consider making both use the same precision type for consistency:- NO_DISCARD std::array<T, 3> basisTransform(std::array<std::array<double, 3>, 3> const& basis, + NO_DISCARD std::array<T, 3> basisTransform(std::array<std::array<T, 3>, 3> const& basis,src/core/numerics/pusher/boris.hpp (1)
Line range hint
130-142
: Enhance error message and consider boundary conditions.While the type changes look good, the error message could be more descriptive and include the actual delta value.
Consider this improvement:
- PHARE_LOG_ERROR("Error, particle moves more than 1 cell, delta >2"); + PHARE_LOG_ERROR("Particle movement exceeds maximum allowed distance: |delta| = {} > 2", std::abs(delta));Also, consider adding a debug log for tracking particle movements near the boundary:
+ if (std::abs(delta) > 1.5) { + PHARE_LOG_DEBUG("Warning: Particle approaching movement limit: |delta| = {}", std::abs(delta)); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
res/cmake/dep/highfive.cmake
(1 hunks)src/core/CMakeLists.txt
(0 hunks)src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp
(4 hunks)src/core/data/ions/particle_initializers/maxwellian_particle_initializer_base.hpp
(4 hunks)src/core/data/ndarray/ndarray_vector.hpp
(1 hunks)src/core/data/particles/particle.hpp
(5 hunks)src/core/data/particles/particle_array.hpp
(4 hunks)src/core/def.hpp
(2 hunks)src/core/hybrid/hybrid_quantities.hpp
(0 hunks)src/core/numerics/pusher/boris.hpp
(3 hunks)src/core/numerics/pusher/pusher.hpp
(1 hunks)src/core/utilities/logger/logger_defaults.hpp
(1 hunks)src/phare_core.hpp
(2 hunks)src/python3/particles.hpp
(2 hunks)src/python3/pybind_def.hpp
(2 hunks)tests/core/data/particles/test_interop.cpp
(1 hunks)tests/core/data/particles/test_main.cpp
(2 hunks)tests/core/numerics/interpolator/test_main.cpp
(1 hunks)tests/core/numerics/pusher/test_pusher.cpp
(9 hunks)tests/simulator/test_advance.py
(1 hunks)tools/bench/core/bench.hpp
(1 hunks)
💤 Files with no reviewable changes (2)
- src/core/CMakeLists.txt
- src/core/hybrid/hybrid_quantities.hpp
✅ Files skipped from review due to trivial changes (1)
- src/core/numerics/pusher/pusher.hpp
🚧 Files skipped from review as they are similar to previous changes (11)
- src/core/utilities/logger/logger_defaults.hpp
- tests/simulator/test_advance.py
- src/core/data/ndarray/ndarray_vector.hpp
- tools/bench/core/bench.hpp
- tests/core/data/particles/test_interop.cpp
- tests/core/numerics/interpolator/test_main.cpp
- res/cmake/dep/highfive.cmake
- src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp
- src/python3/particles.hpp
- src/python3/pybind_def.hpp
- src/core/data/particles/particle.hpp
🧰 Additional context used
📓 Path-based instructions (5)
src/phare_core.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/def.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/numerics/pusher/boris.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/data/particles/particle_array.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/data/ions/particle_initializers/maxwellian_particle_initializer_base.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (14)
tests/core/numerics/pusher/test_pusher.cpp (4)
129-130
: LGTM! Good type alias introduction.The addition of
ParticleArray_t
type alias improves code maintainability and readability by centralizing the particle array type definition.
281-281
: LGTM! Correct type update for random distribution.The random distribution has been properly updated to use
floater_t<0>
instead ofdouble
, maintaining type consistency.
307-309
: LGTM! Consistent use of type alias.Good use of the newly introduced
ParticleArray_t
type alias for member variables.
147-149
: Verify particle initialization values.The particle initialization uses hard-coded values that might need adjustment with the new
floater_t<0>
type. Ensure these values provide sufficient precision for the test cases.✅ Verification successful
Particle initialization values are appropriate and consistent with the codebase
The values used for particle initialization are consistent with other test cases in the codebase and provide appropriate precision for testing purposes. The
floater_t<0>
type is correctly used for position values, following the same pattern as other test files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other particle initializations to verify consistency rg --type cpp "emplace_back.*Particle.*floater_t" -A 2Length of output: 56
Script:
#!/bin/bash # Find Particle class definition ast-grep --pattern 'class Particle {$$$}' # Search for particle initializations in test files rg --type cpp "emplace_back.*Particle" tests/ # Find all files containing Particle initialization patterns rg --type cpp "Particle\{" tests/Length of output: 299
Script:
#!/bin/bash # Find Particle definition including struct ast-grep --pattern 'struct Particle {$$$}' # Find files containing Particle definition rg -l "struct Particle|class Particle" # Find floater_t definition or usage rg "floater_t" -A 2Length of output: 9817
tests/core/data/particles/test_main.cpp (2)
Line range hint
77-86
: LGTM! Type changes align with configurable precision goal.The change from
Point<double, 3>
toPoint<floater_t<0>, 3>
correctly implements the configurable precision type system for particle positions.
90-90
: LGTM! Explicit cast prevents precision issues.The explicit cast to float for
meshSize[i]
ensures consistent precision with particle delta calculations.src/core/def.hpp (2)
68-72
: LGTM! Clean type selection implementation.The
Floater
struct provides a clean way to select between double and float types at compile-time.
74-75
: LGTM! Convenient type alias.The
floater_t
alias provides a clean interface for using the configurable precision types.src/phare_core.hpp (1)
30-30
: LGTM! Consistent type usage for field arrays.The change to use
floater_t<4>
forArray_t
ensures consistent precision configuration for field data.src/core/data/ions/particle_initializers/maxwellian_particle_initializer_base.hpp (2)
17-25
: Consider precision implications of std::normal_distribution.While the function is now more flexible with templates, note that
std::normal_distribution
uses double internally. This might lead to unexpected precision behavior when used with float types.Consider testing with both float and double types to ensure the distribution behaves as expected.
50-54
: LGTM! Clean template implementation.The use of parameter pack for magnetic field components provides a flexible and type-safe implementation.
src/core/numerics/pusher/boris.hpp (1)
259-259
: LGTM: Type change is consistent.The change from
double
tofloater_t<0>
forhalfDtOverDl_
maintains precision requirements while aligning with the type system overhaul.src/core/data/particles/particle_array.hpp (2)
Line range hint
298-306
: LGTM: Improved type safety and performance.The constructor changes are well-implemented:
- Type-specific parameters improve type safety
- Perfect forwarding with
std::forward
optimizes performance by avoiding unnecessary copies
376-379
: Verify precision requirements for particle attributes.The type changes use different precision levels for different attributes:
delta
:floater_t<0>
weight
:floater_t<3>
charge
:floater_t<2>
v
:floater_t<1>
Please verify these precision levels meet the physics requirements for each attribute. Run this script to check usage patterns:
✅ Verification successful
Precision requirements appear appropriate for particle physics calculations
The precision levels follow a physically meaningful hierarchy:
delta
(0): Base precision suitable for position calculations, as evidenced by usage in Boris pusher and interpolationv
(1): Slightly higher precision for velocity, appropriate for momentum calculationscharge
(2): Higher precision for electromagnetic interactionsweight
(3): Highest precision for particle weight, crucial for conservation laws🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check how these attributes are used in calculations echo "Checking delta precision usage:" ast-grep --pattern 'delta = $$$' echo "---" echo "Checking weight precision usage:" ast-grep --pattern 'weight = $$$' echo "---" echo "Checking charge precision usage:" ast-grep --pattern 'charge = $$$' echo "---" echo "Checking velocity precision usage:" ast-grep --pattern 'v = $$$'Length of output: 1907
Script:
#!/bin/bash # Search for mathematical operations and documentation echo "=== Searching for delta usage in calculations ===" rg -l "delta" --type cpp --type hpp | xargs ast-grep --pattern '$$$delta$$$' echo -e "\n=== Searching for weight usage in calculations ===" rg -l "weight" --type cpp --type hpp | xargs ast-grep --pattern '$$$weight$$$' echo -e "\n=== Searching for charge usage in calculations ===" rg -l "charge" --type cpp --type hpp | xargs ast-grep --pattern '$$$charge$$$' echo -e "\n=== Searching for v usage in calculations ===" rg -l "\\bv\\b" --type cpp --type hpp | xargs ast-grep --pattern '$$$v$$$' echo -e "\n=== Searching for precision documentation ===" rg -A 5 "floater_t|precision" --type cpp --type hppLength of output: 1060
Script:
#!/bin/bash # Search for documentation and usage patterns echo "=== Searching for floater_t documentation ===" rg -B 2 -A 2 "floater_t" echo -e "\n=== Searching for precision-related comments ===" rg -B 2 -A 2 "precision|accuracy" "*.hpp" "*.cpp" "*.h" echo -e "\n=== Searching for mathematical operations ===" echo "Delta calculations:" rg "delta.*=|=.*delta" "*.hpp" "*.cpp" echo -e "\nWeight calculations:" rg "weight.*=|=.*weight" "*.hpp" "*.cpp" echo -e "\nCharge calculations:" rg "charge.*=|=.*charge" "*.hpp" "*.cpp" echo -e "\nVelocity calculations:" rg "\\bv\\b.*=|=.*\\bv\\b" "*.hpp" "*.cpp"Length of output: 13433
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/core/data/particles/test_main.cpp (1)
77-77
: Consider updating meshSize type for consistency.While changing
origin
to usefloater_t<0>
is good,meshSize
is still usingdouble
. This mixed precision could lead to unnecessary type conversions.Consider updating
meshSize
for type consistency:- std::array<double, 3> meshSize{{0.2, 0.05, 0.4}}; + std::array<floater_t<0>, 3> meshSize{{0.2, 0.05, 0.4}};
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/numerics/interpolator/interpolator.hpp
(3 hunks)tests/core/data/particles/test_main.cpp
(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/core/numerics/interpolator/interpolator.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
tests/core/data/particles/test_main.cpp (1)
86-91
: Verify precision requirements for particle position calculations.The explicit
float
cast ofmeshSize[i]
appears to be a workaround for mixed precision operations. Additionally, switching toEXPECT_FLOAT_EQ
reduces the precision of the comparison.Please confirm:
- Is single precision (
float
) sufficient for particle position calculations?- Could precision loss affect simulation accuracy?
Run this script to check precision usage across the codebase:
✅ Verification successful
Single precision for particle positions is sufficient and intentional
The codebase consistently uses single precision for spatial calculations (positions, deltas) while maintaining double precision for physics quantities (mass, charge, velocity). This design choice provides adequate precision for grid-based spatial calculations while optimizing performance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other position-related calculations and their precision rg -A 2 "positionAsPoint|expectedPosition" # Check for other float/double precision assertions rg "EXPECT_(FLOAT|DOUBLE)_EQ"Length of output: 24430
src/core/numerics/interpolator/interpolator.hpp (1)
426-426
: Verify the precision requirements for electromagnetic field components.The electromagnetic field components are using
floater_t<4>
while other variables usefloater_t<0>
. This difference in template parameters suggests different precision requirements.Let's verify the usage of these types across the codebase:
✅ Verification successful
Electromagnetic field precision requirement verified as appropriate.
The higher precision requirement (floater_t<4>) for electromagnetic field components is justified by:
- Complex mathematical operations in the Boris pusher algorithm
- Critical role in particle dynamics calculations
- Part of a deliberate precision hierarchy in the codebase where electromagnetic fields require the highest precision level
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other electromagnetic field-related code to verify precision requirements ast-grep --pattern 'floater_t<4>' # Search for any comments or documentation about precision requirements rg -i "precision|accuracy" --type cppLength of output: 1426
Script:
#!/bin/bash # Search for electromagnetic field-related code rg -l "std::array.*floater_t" --type cpp # Search for type definitions and electromagnetic field components ast-grep --pattern 'using.*floater_t' # Look for any documentation files fd -e md -e txt . | xargs rg -i "electromagnetic|field"Length of output: 6189
Script:
#!/bin/bash # Check boris.hpp and particle.hpp content cat src/core/numerics/pusher/boris.hpp cat src/core/data/particles/particle.hpp # Look for floater_t definition rg -A 5 "floater_t" src/core/utilities/types.hpp # Search for field-related constants rg "constexpr.*float" --type hppLength of output: 14075
@@ -399,7 +399,7 @@ class Interpolator : private Weighter<interpOrder> | |||
startIndex_[iDim] | |||
= iCell[iDim] - computeStartLeftShift<CenteringT, centering>(delta[iDim]); | |||
|
|||
double normalizedPos = iCell[iDim] + delta[iDim]; | |||
floater_t<0> normalizedPos = iCell[iDim] + delta[iDim]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Update computeWeight
method signature to match the new type system.
The normalizedPos
and dual_offset
variables are now using floater_t<0>
, but the computeWeight
method in the Weighter
class still expects double
parameters. This type mismatch could lead to unnecessary conversions or precision loss.
Consider updating the Weighter
class implementations to use floater_t<0>
instead of double
:
- inline void computeWeight(double normalizedPos, int startIndex,
- std::array<double, nbrPointsSupport(1)>& weights)
+ inline void computeWeight(floater_t<0> normalizedPos, int startIndex,
+ std::array<floater_t<0>, nbrPointsSupport(1)>& weights)
Also applies to: 387-387
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
tests/core/data/particles/test_main.cpp (1)
Line range hint
19-93
: Consider updating all tests for consistency with the new type system.Several tests still use
EXPECT_DOUBLE_EQ
and double literals whileCanBeReducedToAnAbsolutePositionPoint
has been updated to useEXPECT_FLOAT_EQ
. Consider:
- Updating all tests to use consistent precision expectations
- Updating the test fixture to use the new
floater_t
typessrc/core/data/particles/particle_utilities.hpp (1)
Line range hint
44-44
: Update debug utilities to use consistent types.The
checkDeltas
function still usesstd::vector<double>
while the main code has moved tofloater_t
. Consider updating the debug utilities to maintain type consistency:- std::vector<double> deltas; + std::vector<floater_t<4>> deltas;
🧹 Nitpick comments (8)
tests/core/data/particles/test_main.cpp (1)
86-86
: Review explicit float cast in position calculation.The explicit
float
cast onmeshSize[i]
might be unnecessary given the new type system:= origin[i] + float(meshSize[i]) * (iCell[i] - startIndexes[i] + part.delta[i]);
Consider removing the explicit cast and letting the
floater_t
type handle the precision requirements naturally.Also applies to: 90-91
src/core/data/particles/particle_utilities.hpp (1)
Line range hint
22-33
: Consider documenting precision requirements and performance implications.The position calculation combines integer and floating-point operations. With the transition to
floater_t<4>
, consider:
- Documenting the precision requirements for particle positions
- Adding a comment explaining the choice of precision level
- Validating that the precision is maintained throughout the calculation chain
Consider adding a comment above the position declaration:
// Level 4 precision used for particle positions to ensure sufficient accuracy // in physical space calculations while optimizing memory usagetests/core/data/gridlayout/gridlayout_cell_centered_coord.hpp (1)
113-114
: Update C++ version comment for new type systemThe comment about C++14/17 type deduction should be updated to reflect the new
floater_t<4>
type:- // NOTE: c++17 : Point{origin}, C++14 : Point<double, dim>{origin} + // NOTE: c++17 : Point{origin}, C++14 : Point<floater_t<4>, dim>{origin}tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cpp (1)
63-64
: Consider a more descriptive constant name.While using
floater_t<4>
for the zero constant improves type safety, the nameZRO
could be more descriptive, e.g.,ZERO
orDOMAIN_START
.src/core/data/grid/gridlayoutdefs.hpp (1)
30-30
: Consider documenting the precision requirements.Since this coefficient is using higher precision (floater_t<4>), it would be helpful to document why this specific precision level was chosen.
tests/core/numerics/ampere/test_main.cpp (1)
226-230
: Document the rationale for tolerance values.The test uses different tolerance levels (1e-12 for double vs 1e-5 for float) but lacks documentation explaining these choices.
Consider adding a comment explaining the tolerance selection:
else { + // Using wider tolerance for float type due to inherent precision limitations EXPECT_THAT(Jy(ix), ::testing::FloatNear((expectedJy[ix]), 1e-5)); EXPECT_THAT(Jz(ix), ::testing::FloatNear((expectedJz[ix]), 1e-5)); }
tests/core/numerics/faraday/test_main.cpp (1)
262-271
: LGTM! Consider adding a comment explaining the precision differences.The conditional precision handling based on the floating-point type is a good practice. The test uses stricter precision (1e-12) for double and relaxed precision (1e-5) for other types, which is appropriate.
Add a comment explaining why different precisions are used:
// Use stricter precision (1e-12) for double type and relaxed precision (1e-5) // for other types (e.g., float) due to their inherent precision differencessrc/core/def.hpp (1)
49-76
: LGTM: Well-designed type system implementation.The new type system is well-implemented with:
- Clear bit assignments for different particle properties
- Flexible compile-time type selection
- Good documentation of bit meanings
Consider adding a brief comment explaining why certain properties use different precision levels.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
src/amr/data/field/field_data.hpp
(9 hunks)src/core/data/grid/gridlayout.hpp
(4 hunks)src/core/data/grid/gridlayoutdefs.hpp
(1 hunks)src/core/data/particles/particle_utilities.hpp
(1 hunks)src/core/def.hpp
(2 hunks)src/core/numerics/faraday/faraday.hpp
(2 hunks)src/core/numerics/interpolator/interpolator.hpp
(16 hunks)src/phare_core.hpp
(2 hunks)tests/amr/data/field/coarsening/test_linear_coarsen.hpp
(1 hunks)tests/amr/data/field/refine/test_field_refine.cpp
(1 hunks)tests/amr/data/field/refine/test_field_refinement_on_hierarchy.cpp
(1 hunks)tests/amr/data/field/time_interpolate/test_field_data_time_interpolate.cpp
(1 hunks)tests/amr/data/field/variable/test_field_variable.cpp
(1 hunks)tests/amr/resources_manager/test_resources_manager.cpp
(1 hunks)tests/core/data/electrons/test_electrons.cpp
(1 hunks)tests/core/data/field/test_field_fixtures.hpp
(1 hunks)tests/core/data/gridlayout/gridlayout_allocsize.hpp
(1 hunks)tests/core/data/gridlayout/gridlayout_base_params.hpp
(1 hunks)tests/core/data/gridlayout/gridlayout_cell_centered_coord.hpp
(4 hunks)tests/core/data/gridlayout/gridlayout_field_centered_coord.hpp
(4 hunks)tests/core/data/gridlayout/gridlayout_indexing.hpp
(1 hunks)tests/core/data/gridlayout/gridlayout_params.hpp
(1 hunks)tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cpp
(1 hunks)tests/core/data/particles/test_main.cpp
(1 hunks)tests/core/data/tensorfield/test_tensorfield_fixtures.hpp
(2 hunks)tests/core/numerics/ampere/test_main.cpp
(1 hunks)tests/core/numerics/faraday/test_main.cpp
(1 hunks)tests/core/numerics/interpolator/test_main.cpp
(4 hunks)tests/core/numerics/ion_updater/test_updater.cpp
(2 hunks)tests/core/numerics/ohm/test_main.cpp
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/core/numerics/ion_updater/test_updater.cpp
🧰 Additional context used
📓 Path-based instructions (17)
src/core/data/particles/particle_utilities.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/core/data/field/test_field_fixtures.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/core/data/gridlayout/gridlayout_params.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/core/data/gridlayout/gridlayout_allocsize.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/core/data/gridlayout/gridlayout_indexing.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/data/grid/gridlayoutdefs.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/amr/data/field/coarsening/test_linear_coarsen.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/core/data/gridlayout/gridlayout_field_centered_coord.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/core/data/gridlayout/gridlayout_base_params.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/phare_core.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/core/data/tensorfield/test_tensorfield_fixtures.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/numerics/faraday/faraday.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/def.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/amr/data/field/field_data.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/data/grid/gridlayout.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/core/data/gridlayout/gridlayout_cell_centered_coord.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/numerics/interpolator/interpolator.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
🪛 GitHub Actions: CMake MacOS
tests/core/data/electrons/test_electrons.cpp
[error] 172-172: Cannot initialize a parameter of type 'Field<3, Scalar> *' with an rvalue of type 'field_type *'
[error] 173-173: Non-const lvalue reference to type 'TensorField<Field<[2 * ...], floater_t<4>>, [2 * ...]>' cannot bind to a value of unrelated type 'TensorField<Field<[2 * ...], double>, [2 * ...]>'
[error] 202-202: No matching constructor for initialization of 'Electrons'
🔇 Additional comments (40)
src/phare_core.hpp (2)
4-12
: LGTM! Well-organized include statements.The includes are logically grouped and follow a clear dependency hierarchy.
30-34
: Verify precision consistency across type definitions.Let's verify that the precision choice is consistent with other related types in the codebase.
✅ Verification successful
Precision choices are consistent and well-structured
The precision choice of
floater_t<4>
for Array_t and Field_t is consistent across the codebase, while other quantities use different precisions by design.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other floater_t usage to verify precision consistency echo "Searching for floater_t usage patterns..." rg "floater_t<\d+>" --type cpp echo -e "\nSearching for Array_t and Field_t usage..." ast-grep --pattern 'using $_t = $$$<$$$floater_t<$_>$$$>'Length of output: 14652
tests/core/data/particles/test_main.cpp (1)
77-78
: Verify precision requirements for different floater_t parameters.The type changes use different precision parameters:
floater_t<0>
fororigin
floater_t<4>
formeshSize
Please confirm if these specific precision choices are intentional and document the reasoning.
✅ Verification successful
Precision parameters are consistent with codebase patterns
The different precision parameters are following established patterns:
floater_t<0>
is consistently used for particle positionsfloater_t<4>
is the standard choice for grid-related measurements🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of these types to understand the precision pattern ast-grep --pattern 'floater_t<0>' ast-grep --pattern 'floater_t<4>'Length of output: 1643
src/core/data/particles/particle_utilities.hpp (1)
22-22
: Verify the precision level choice for position calculations.The change from
double
tofloater_t<4>
aligns with the type system overhaul. However, we should verify if level 4 precision is appropriate for position calculations, considering:
- Numerical accuracy requirements for particle positions
- Memory usage implications
- Consistency with other position-related calculations in the codebase
Let's check how this precision level compares to other position-related calculations:
✅ Verification successful
The precision level choice for position calculations is appropriate.
The use of
floater_t<4>
for position calculations is consistent with the codebase's type system design, where precision level 4 is systematically used for all spatial quantities including mesh sizes, field positions, and particle positions. This highest precision level is appropriate for position calculations which require maximum accuracy in particle simulations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other position-related floater_t usage ast-grep --pattern 'Point<floater_t<$_>, $_>'Length of output: 47
Script:
#!/bin/bash # Search for floater_t usage with different precisions rg "floater_t<" -A 2 -B 2 # Search for position-related type definitions rg "position|Position" --type cpp -A 2 -B 2 # Search for Point template usage rg "Point<" --type cpp -A 2 -B 2Length of output: 70879
tests/core/data/gridlayout/gridlayout_base_params.hpp (1)
25-25
: Type transition looks good but needs verification.The change from
double
tofloater_t<4>
is consistent across both members. However, we should verify that all dependent test cases handle this precision change correctly.Let's verify the test coverage:
Also applies to: 28-28
✅ Verification successful
Type transition is properly covered by tests ✅
The change from
double
tofloater_t<4>
is consistently implemented across all related test files, including cell-centered coordinates, field-centered coordinates, and indexing tests. The test suite properly handles the precision change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases using GridLayoutTestParam to ensure they handle floater_t rg -l "GridLayoutTestParam.*floater_t" tests/Length of output: 47
Script:
#!/bin/bash # Find all files containing GridLayoutTestParam rg -l "GridLayoutTestParam" tests/ # Find all test files related to gridlayout fd "gridlayout.*\.(hpp|cpp)" tests/ # Look for floater_t usage in gridlayout tests rg "floater_t" tests/core/data/gridlayout/Length of output: 4244
tests/core/data/gridlayout/gridlayout_params.hpp (1)
54-56
: Parameter type changes are consistent but need caller verification.The type changes in
createParam
match theGridLayoutTestParam
structure. However, we should verify all callers of this function have been updated accordingly.Let's check for any potential callers that might need updates:
✅ Verification successful
Parameter type changes are safe and consistent.
The
createParam
function is a template implementation used internally by the gridlayout test framework. All related test files consistently useGridLayoutTestParam
through the base member, ensuring type safety. No incompatible callers were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for createParam calls to ensure all callers are updated rg -A 3 "createParam.*double" tests/Length of output: 38
Script:
#!/bin/bash # Search for any createParam calls with surrounding context rg -B 2 -A 2 "createParam\(" tests/ # Backup search using ast-grep to find function calls ast-grep --pattern 'createParam($$$)'Length of output: 1047
Script:
#!/bin/bash # Search for files using GridLayoutTestParam rg "GridLayoutTestParam" tests/ # Search for potential test files related to gridlayout fd "test.*grid.*layout.*\.(hpp|cpp)" tests/Length of output: 1004
tests/core/data/gridlayout/gridlayout_allocsize.hpp (1)
79-79
: Verify input data file compatibility with new types.While the type changes are consistent, ensure that the input data files (e.g.,
allocSizes_*.txt
) maintain compatibility withfloater_t<4>
precision.Let's check the input files:
Also applies to: 86-86
✅ Verification successful
No compatibility issues with input data files
The test data files are generated programmatically by
allocSizes.py
using standard Python numeric types, which are compatible withfloater_t<4>
precision. No manual verification is needed as the data generation is controlled by the test framework.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # List all allocSizes test data files fd "allocSizes.*\.txt" tests/ # Check if any files contain scientific notation or high-precision values rg -l "[0-9]+\.[0-9]{10,}|[0-9]+e[+-][0-9]+" tests/allocSizes*.txtLength of output: 164
Script:
#!/bin/bash # First, let's find any .txt files under tests/ directory fd ".txt$" tests/ # Also search for references to test data files in the code rg -i "allocsizes.*\.txt" -g "!{*.txt,*.md}" # Look for file operations or paths in the test file rg "open|fstream|ifstream|read.*file" tests/core/data/gridlayout/Length of output: 9423
tests/core/data/gridlayout/gridlayout_indexing.hpp (1)
88-88
: Consistent type changes but verify test data compatibility.The type changes align with other files, but like
gridlayout_allocsize.hpp
, ensure that the input data files (e.g.,gridIndexing_*.txt
) are compatible withfloater_t<4>
precision.Let's check the input files:
Also applies to: 95-95
✅ Verification successful
Type change is safe, test data is precision-independent
The test data is generated programmatically during build time and consists mainly of grid indices and simple floating-point values (e.g., dx=0.1). The Python generator automatically adapts to the implementation's precision, making it compatible with the
floater_t<4>
change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # List all gridIndexing test data files fd "gridIndexing.*\.txt" tests/ # Check if any files contain scientific notation or high-precision values rg -l "[0-9]+\.[0-9]{10,}|[0-9]+e[+-][0-9]+" tests/gridIndexing*.txtLength of output: 170
Script:
#!/bin/bash # First, find all txt files in tests directory echo "=== All txt files in tests directory ===" fd -e txt . tests/ # Then search for any files containing "grid" and "index" (case-insensitive) echo -e "\n=== Files containing 'grid' and 'index' (case-insensitive) ===" fd -e txt . tests/ -X grep -il "grid.*index\|index.*grid" # If we find the files, examine their content for numerical values echo -e "\n=== Numerical content analysis ===" fd -e txt . tests/ -X grep -il "grid.*index\|index.*grid" | xargs -I {} sh -c 'echo "=== {} ==="; cat {}'Length of output: 5525
Script:
#!/bin/bash # Check the Python generator script echo "=== Generator Script Content ===" cat tests/core/data/gridlayout/gridlayout_generate_test_files.py # Look for any other related Python files echo -e "\n=== Related Python Files ===" fd -e py . tests/core/data/gridlayout/Length of output: 1286
Script:
#!/bin/bash # Check the gridIndexing implementation echo "=== gridIndexing.py Content ===" cat tests/core/data/gridlayout/gridIndexing.py # Also check if gridparams.py contains precision-related settings echo -e "\n=== gridparams.py Content ===" cat tests/core/data/gridlayout/gridparams.pyLength of output: 6065
tests/core/data/gridlayout/gridlayout_cell_centered_coord.hpp (4)
48-51
: Check for potential precision loss in coordinate calculationsThe coordinate calculations might involve mixed-precision operations. Consider auditing all mathematical operations involving
pos
andactualPos
to ensure no unintended precision loss occurs.Consider implementing precision validation tests to verify coordinate calculation accuracy with the new type system.
95-100
: Validate grid layout precision requirementsThe grid spacing (
dl
) and origin coordinates are fundamental parameters affecting all grid calculations. Ensure thatfloater_t<4>
meets the minimum precision requirements for your grid layout system.Consider documenting the precision requirements and tolerances for grid layout calculations in the codebase.
118-118
: Verify physical coordinate precision requirementsThe change to
floater_t<4>
forrealPosition
affects physical coordinate representation. Ensure this precision is sufficient for your physical domain requirements and coordinate transformations.Consider adding assertions or validation for physical coordinate ranges and precision requirements.
22-24
: Verify precision requirements for position calculationsThe change from
double
tofloater_t<4>
for position vectors may affect numerical precision. Ensure thatfloater_t<4>
provides sufficient precision for your grid coordinate calculations.✅ Verification successful
Precision requirements verified
The use of
floater_t<4>
is appropriate for the grid coordinate calculations:
- Tests use exact equality comparisons without requiring tolerance checks
- The same precision is used consistently for both cell and field centered coordinates
- The implementation successfully handles coordinates in 1D, 2D, and 3D cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any precision-dependent calculations or comparisons rg -A 3 "std::numeric_limits|std::abs|epsilon|tolerance" tests/core/data/gridlayout/Length of output: 86
Script:
#!/bin/bash # Find the implementation file and any related position comparisons fd gridlayout_cell_centered_coord --type f # Look for position comparison logic and test assertions rg -A 5 "expectedPosition|actualPosition" tests/core/data/gridlayout/ # Search for floater_t definition and usage ast-grep --pattern 'using floater_t = $_'Length of output: 12404
tests/core/data/gridlayout/gridlayout_field_centered_coord.hpp (2)
20-21
: Verify field node positioning precision requirementsThe change to
floater_t<4>
for field positions should maintain sufficient precision for field interpolation and gradient calculations. Ensure this precision meets electromagnetic field solver requirements.Consider documenting the minimum precision requirements for field solvers and interpolation schemes.
102-105
: Ensure consistent precision across grid typesThe grid spacing (
dl
) and origin coordinates usefloater_t<4>
, matching the cell-centered implementation. This consistency is good, but verify that all grid-related calculations maintain this precision.✅ Verification successful
Type consistency verified across grid implementations ✓
The use of
floater_t<4>
in the field-centered coordinates implementation is consistent with the core grid layout system, where it's uniformly used for mesh sizes, origins, and physical position calculations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent type usage across grid implementations rg -l "floater_t" src/core/data/grid/Length of output: 109
Script:
#!/bin/bash # Check the content of grid layout files for type usage echo "=== gridlayoutdefs.hpp ===" rg "floater_t" src/core/data/grid/gridlayoutdefs.hpp -B 2 -A 2 echo -e "\n=== gridlayout.hpp ===" rg "floater_t" src/core/data/grid/gridlayout.hpp -B 2 -A 2Length of output: 1875
tests/core/data/field/test_field_fixtures.hpp (1)
4-4
: Verify test coverage with the new floater_t type.The change from
double
tofloater_t<4>
inField_t
alias is part of the precision control effort. Since this is a test fixture, we should ensure all dependent tests properly handle the new type.Run this script to find affected test files:
Also applies to: 11-11
✅ Verification successful
No issues found with Field_t type change propagation
The change from
double
tofloater_t<4>
is properly propagated through the type system. The type is defined in test_field_fixtures.hpp with the correct include, and all dependent test files use Field_t through type aliases and templates, ensuring type consistency automatically.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find test files that depend on Field_t rg -l "Field_t" tests/Length of output: 419
Script:
#!/bin/bash # Check if files include core/def.hpp and show Field_t usage context echo "=== Checking for core/def.hpp includes ===" rg -l "core/def\.hpp" tests/core/numerics/ion_updater/test_updater.cpp tests/core/data/tensorfield/test_tensorfield_fixtures.hpp tests/core/data/vecfield/test_vecfield_fixtures.hpp tests/core/data/vecfield/test_main.cpp tests/core/data/ion_population/test_ion_population_fixtures.hpp tests/core/data/field/test_field_fixtures.hpp tests/core/data/electromag/test_electromag_fixtures.hpp tests/amr/models/test_models.cpp echo -e "\n=== Showing Field_t usage context ===" rg "Field_t" tests/core/numerics/ion_updater/test_updater.cpp tests/core/data/tensorfield/test_tensorfield_fixtures.hpp tests/core/data/vecfield/test_vecfield_fixtures.hpp tests/core/data/vecfield/test_main.cpp tests/core/data/ion_population/test_ion_population_fixtures.hpp tests/core/data/field/test_field_fixtures.hpp tests/core/data/electromag/test_electromag_fixtures.hpp tests/amr/models/test_models.cpp -C 2Length of output: 9872
tests/core/data/tensorfield/test_tensorfield_fixtures.hpp (2)
26-27
: LGTM! Type consistency maintained.The update to
Grid_t
usingfloater_t<4>
aligns with the project-wide precision control changes.
53-54
: LGTM! Improved lambda formatting.The lambda formatting changes enhance code readability while maintaining the same functionality.
tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cpp (1)
72-72
: LGTM! Consistent type usage.The update to use
ZRO
in comparisons ensures type consistency and prevents potential floating-point comparison issues.Also applies to: 74-74
src/core/numerics/faraday/faraday.hpp (2)
21-21
: LGTM! Improved parameter type safety.The change to
floater_t<4> const dt
improves type safety and ensures consistent precision in time evolution calculations.
52-52
: LGTM! Consistent member type.The update of
dt_
member tofloater_t<4>
maintains consistency with the parameter type.src/core/data/grid/gridlayoutdefs.hpp (1)
23-27
: LGTM! Constructor changes improve const-correctness.The addition of const qualifiers to parameters enhances safety by preventing accidental modifications.
tests/amr/data/field/refine/test_field_refinement_on_hierarchy.cpp (1)
23-23
: LGTM! Type consistency with production code.The GridND type now correctly matches the precision used in the production code.
tests/amr/data/field/variable/test_field_variable.cpp (1)
53-53
: LGTM! Consistent type system update.The FieldVar template alias now correctly uses the new floating-point type system.
tests/amr/resources_manager/test_resources_manager.cpp (2)
36-36
: LGTM! Grid type precision aligned with system requirements.The Grid1D type definition now correctly uses the new floating-point type system, maintaining consistency across the codebase.
36-36
: Verify complete migration to floater_t types.Let's check for any remaining double types that might need conversion:
✅ Verification successful
Double usage appears intentional, no missed conversions found
The remaining
double
types are appropriately used for physical constants (mass, charge, temperature) and simple computational variables, while precision-sensitive calculations consistently usefloater_t<N>
. This pattern appears to be an intentional design choice rather than incomplete migration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for double types in grid and particle-related code rg -g '*.{hpp,cpp}' 'double\s+\w+' src/core/data/Length of output: 1491
Script:
#!/bin/bash # Check for floater_t usage in the same files rg -g '*.{hpp,cpp}' 'floater_t' src/core/data/Length of output: 2981
tests/amr/data/field/refine/test_field_refine.cpp (1)
81-81
: Verify test stability with the new floating-point type.The change from implicit double precision to
floater_t<4>
may affect numerical accuracy in field refinement operations. Please ensure that all test assertions maintain their validity with the new precision level.Run the following script to check for any precision-sensitive test assertions:
✅ Verification successful
No precision-related test modifications needed
The change from implicit double to
floater_t<4>
is safe as the codebase confirms thatfloater_t<4>
is equivalent to double precision. The test assertions will maintain their validity since the numerical accuracy remains unchanged.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for floating-point assertions in the test file # Look for DoubleNear, FloatNear, or direct equality comparisons rg -A 2 'EXPECT_(DOUBLE|FLOAT)_EQ|EXPECT_THAT.*Near'Length of output: 44433
Script:
#!/bin/bash # Search for floater_t definition and usage rg -A 3 "floater_t"Length of output: 35953
tests/amr/data/field/coarsening/test_linear_coarsen.hpp (1)
58-58
: Ensure consistent floating-point type usage in test fixtures.The change to
floater_t<4>
inGrid_t
affects the EMData struct. However, there are other floating-point usages in the test fixtures that might need alignment.Run the following script to check for potential type inconsistencies:
✅ Verification successful
No floating-point type inconsistencies found in test fixtures.
The use of
floater_t<4>
inGrid_t
is consistent with the codebase's established pattern, wherefloater_t<4>
is systematically used for field data across all test fixtures and implementations. The EMData struct follows this convention correctly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find potential floating-point type mismatches # Look for double literals and other floating-point type usage rg -A 2 '\bdouble\b|\bfloat\b|\b\d+\.\d*f?\b'Length of output: 96017
Script:
#!/bin/bash # Get the test file content and context cat tests/amr/data/field/coarsening/test_linear_coarsen.hpp # Find floater_t usage patterns rg "floater_t" -A 2 # Find EMData references rg "EMData" -A 2 # List other files in the same directory ls -1 tests/amr/data/field/coarsening/Length of output: 45935
tests/amr/data/field/time_interpolate/test_field_data_time_interpolate.cpp (1)
51-51
: Verify template instantiation compatibility.The change to
floater_t<4>
inGridND
affects template instantiations. Ensure that all template specializations and type traits remain compatible with the new floating-point type.Run the following script to check for potential template-related issues:
✅ Verification successful
Template instantiation compatibility verified.
The use of
floater_t<4>
inGridND
is consistent with the codebase's type system, where different precision levels are used for different quantities (particle attributes, field values, etc.). All template instantiations are compatible with this type.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for template instantiations and type traits # Look for template specializations and type-dependent expressions ast-grep --pattern 'template<$$$> class $_ { $$$ using $_ = $_<$$$>; $$$ }'Length of output: 81
Script:
#!/bin/bash # Search for GridND usages and related template instantiations echo "=== GridND usages ===" rg "GridND" -A 2 -B 2 echo -e "\n=== floater_t definitions and usages ===" rg "floater_t" -A 2 -B 2 echo -e "\n=== Template instantiations with Grid ===" ast-grep --pattern 'Grid<$_>'Length of output: 47404
tests/core/numerics/ampere/test_main.cpp (1)
221-225
: LGTM! Precision-aware assertions for double type.The conditional compilation correctly handles double-precision comparisons with appropriate tolerance.
src/amr/data/field/field_data.hpp (4)
5-5
: LGTM! Include addition supports new type system.The addition of core/def.hpp provides necessary type definitions for the new floating-point system.
Also applies to: 8-8
91-100
: LGTM! Proper handling of different array types in restart functionality.The conditional logic correctly handles both double and float array types based on
floater_t<4>
, ensuring proper data restoration from the restart database.
501-502
: LGTM! Template changes improve type safety and reusability.The addition of template parameter T to pack/unpack methods makes them more generic and type-safe, allowing them to work with different buffer types while maintaining type consistency.
Also applies to: 515-516
483-488
: LGTM! Const correctness improvements.The addition of const qualifiers to variable declarations improves code clarity and prevents accidental modifications.
src/core/data/grid/gridlayout.hpp (4)
112-114
: LGTM: Constructor parameter types updated correctly.The change from
double
tofloater_t<4>
for mesh size and origin parameters aligns with the new type system and is appropriate for grid-related values.
158-176
: LGTM: Method return types updated appropriately.The change to
auto
return types is a good choice as it:
- Preserves type safety
- Maintains implementation flexibility
- Reduces maintenance burden when underlying types change
Line range hint
517-542
: LGTM: Method updated consistently with type system changes.The changes to
cellCenteredCoordinates
are well-implemented:
- Return type uses
auto
for flexibility- Internal variables correctly use
floater_t<4>
for physical positions
1494-1497
: LGTM: Member variables updated appropriately.The change to
floater_t<4>
for grid-related member variables is consistent with the type system and appropriate for these physical quantities.src/core/numerics/interpolator/interpolator.hpp (1)
403-403
:⚠️ Potential issueRemove unused variable
dual_offset
.The variable is declared but never used in the code.
- floater_t<0> constexpr dual_offset = .5;
Likely invalid or redundant comment.
tests/core/numerics/interpolator/test_main.cpp (2)
84-87
: LGTM: Test arrays updated correctly.The test arrays have been properly updated to use
floater_t<4>
, maintaining consistency with the production code changes.
Line range hint
679-696
: LGTM: Test fixture updated correctly.The test fixture has been properly updated to use appropriate floater_t types for grid layout and particle properties.
src/phare_core.hpp
Outdated
using Array_t = PHARE::core::NdArrayVector<dimension, floater_t<4>>; | ||
using ArrayView_t = PHARE::core::NdArrayView<dimension>; | ||
using Grid_t = PHARE::core::Grid<Array_t, PHARE::core::HybridQuantity::Scalar>; | ||
using Field_t = core::Field<dimension, PHARE::core::HybridQuantity::Scalar, floater_t<4>>; | ||
using VecField_t = PHARE::core::VecField<Field_t, PHARE::core::HybridQuantity>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Document the significance of floater_t<4>
.
The magic number 4
in floater_t<4>
needs documentation explaining its significance and impact on precision.
Consider:
- Adding a comment explaining what
floater_t<4>
represents - Using a named constant instead of the magic number
- Making the precision configurable via template parameter
+ // floater_t<4> represents ... precision for ...
+ static constexpr size_t ARRAY_PRECISION = 4;
- using Array_t = PHARE::core::NdArrayVector<dimension, floater_t<4>>;
+ using Array_t = PHARE::core::NdArrayVector<dimension, floater_t<ARRAY_PRECISION>>;
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
using Array_t = PHARE::core::NdArrayVector<dimension, floater_t<4>>; | |
using ArrayView_t = PHARE::core::NdArrayView<dimension>; | |
using Grid_t = PHARE::core::Grid<Array_t, PHARE::core::HybridQuantity::Scalar>; | |
using Field_t = core::Field<dimension, PHARE::core::HybridQuantity::Scalar, floater_t<4>>; | |
using VecField_t = PHARE::core::VecField<Field_t, PHARE::core::HybridQuantity>; | |
// floater_t<4> represents ... precision for ... | |
static constexpr size_t ARRAY_PRECISION = 4; | |
using Array_t = PHARE::core::NdArrayVector<dimension, floater_t<ARRAY_PRECISION>>; | |
using ArrayView_t = PHARE::core::NdArrayView<dimension>; | |
using Grid_t = PHARE::core::Grid<Array_t, PHARE::core::HybridQuantity::Scalar>; | |
using Field_t = core::Field<dimension, PHARE::core::HybridQuantity::Scalar, floater_t<4>>; | |
using VecField_t = PHARE::core::VecField<Field_t, PHARE::core::HybridQuantity>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🧹 Nitpick comments (15)
tests/amr/resources_manager/test_resources_manager.cpp (1)
36-36
: Consider adding precision-specific test cases.The switch to
floater_t<4>
warrants additional test coverage for precision-sensitive operations, especially for edge cases where numerical accuracy is critical.Consider adding test cases that:
- Verify numerical stability with the new precision
- Compare results between different precision levels
- Test boundary conditions and extreme values
src/core/def.hpp (4)
51-51
: Add documentation for the bitset pattern.The binary pattern
0b00000
controls precision for different particle properties. Consider adding a table in the comments that clearly documents what each bit position controls, similar to the comments in theis_double()
function.-constexpr std::bitset<5> doubles{0b00000}; // index 0 starts on the right in binary +constexpr std::bitset<5> doubles{0b00000}; // Controls precision (0=float, 1=double): + // bit 0: particle delta + // bit 1: particle velocity + // bit 2: particle charge + // bit 3: particle weight + // bit 4: fields
59-64
: Consider using an enum class for type-safe indices.Replace magic numbers with an enum class to improve type safety and make the code more maintainable.
+enum class ParticleProperty : std::uint8_t { + Delta = 0, + Velocity = 1, + Charge = 2, + Weight = 3, + Fields = 4 +}; template<std::uint8_t i> bool constexpr is_double() { - // 0 = particle delta - // 1 = particle v - // 2 = particle charge - // 3 = particle weight - // 4 = fields + static_assert(i <= static_cast<std::uint8_t>(ParticleProperty::Fields), + "Invalid particle property index");
68-72
: Improve struct documentation and consider a more descriptive name.While the implementation is correct, the name 'Floater' could be more descriptive. Consider adding documentation and renaming to better reflect its purpose.
+/** + * Provides compile-time floating-point type selection based on precision configuration. + * @tparam i The particle property index determining the precision. + */ -struct Floater +struct FloatingPointType // or PrecisionSelector { using value_type = std::conditional_t<is_double<i>(), double, float>; };
74-75
: Add documentation for the type alias.Add documentation to explain the usage and purpose of this type alias.
+/** + * Type alias for property-specific floating-point type. + * @tparam i The particle property index (0=delta, 1=velocity, 2=charge, 3=weight, 4=fields). + * @example floater_t<0> delta; // float or double based on configuration + */ template<std::uint8_t i> using floater_t = Floater<i>::value_type;src/amr/data/field/field_data.hpp (2)
91-100
: Add error handling for database operations.While the type handling is robust, the database operations (
getDoubleArray
/getFloatArray
) should include error handling to gracefully handle potential failures.if constexpr (std::is_same_v<core::floater_t<4>, double>) { - restart_db->getDoubleArray("field_" + field.name(), field.vector().data(), - field.vector().size()); + try { + restart_db->getDoubleArray("field_" + field.name(), field.vector().data(), + field.vector().size()); + } catch (const SAMRAI::tbox::DatabaseException& e) { + throw std::runtime_error("Failed to read double array from restart database: " + + std::string(e.what())); + } } else { - restart_db->getFloatArray("field_" + field.name(), field.vector().data(), - field.vector().size()); + try { + restart_db->getFloatArray("field_" + field.name(), field.vector().data(), + field.vector().size()); + } catch (const SAMRAI::tbox::DatabaseException& e) { + throw std::runtime_error("Failed to read float array from restart database: " + + std::string(e.what())); + } }
483-488
: Maintain consistency in const qualifiers.While the 1D implementation correctly uses
const
qualifiers for coordinate variables, the 2D and 3D implementations are missing them. Consider applying const qualifiers consistently across all dimension implementations.// In 2D implementation -int xStart = overlap.lower(0) - destination.lower(0); -int xEnd = overlap.upper(0) - destination.lower(0); -int yStart = overlap.lower(1) - destination.lower(1); -int yEnd = overlap.upper(1) - destination.lower(1); +int const xStart = overlap.lower(0) - destination.lower(0); +int const xEnd = overlap.upper(0) - destination.lower(0); +int const yStart = overlap.lower(1) - destination.lower(1); +int const yEnd = overlap.upper(1) - destination.lower(1);Also applies to: 577-581
src/core/data/grid/gridlayoutdefs.hpp (1)
23-28
: Remove unnecessarystd::move
on const parameter.The constructor uses
std::move
on a const parameter, which is unnecessary and potentially misleading. The move operation will be elided by the compiler since the parameter is marked as const.- constexpr WeightPoint(Point<int, dim> const point, floater_t<4> const _coef) - : indexes{std::move(point)} + constexpr WeightPoint(Point<int, dim> const point, floater_t<4> const _coef) + : indexes{point}tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cpp (1)
72-74
: Improve error handling and reduce code duplication.The position check is duplicated between the conditional and the assertion. Consider extracting the domain check into a helper method and improving the error message.
- if (!((pos[0] > ZRO) and (pos[0] < endDomain))) - std::cout << "position : " << pos[0] << " not in domain (0," << endDomain << ")\n"; - EXPECT_TRUE(pos[0] > ZRO && pos[0] < endDomain); + auto const isInDomain = [&](auto const& position) { + return position > ZRO && position < endDomain; + }; + if (!isInDomain(pos[0])) { + std::cout << "Particle at position " << pos[0] + << " is outside domain bounds [" << ZRO + << ", " << endDomain << "]\n"; + } + EXPECT_TRUE(isInDomain(pos[0]));tests/core/numerics/ohm/test_main.cpp (2)
Line range hint
4-9
: Consider adding test coverage for edge cases.The test suite focuses on standard cases, but could benefit from additional tests covering:
- Zero/negative values for n, V, P
- Edge cases for B and J fields
- Boundary conditions
71-71
: Review tolerance values across dimensions.The test uses different tolerance values:
- 1e-12 for 1D/2D cases
- 1e-10 for 3D cases
Consider documenting the reasoning behind these different tolerance levels and how they relate to the precision of
floater_t<4>
.Also applies to: 305-450
tests/core/numerics/ampere/test_main.cpp (1)
221-230
: Consider parameterizing the tolerance values.The tolerance values (1e-12 for double, 1e-5 for float) are hardcoded. Consider making these configurable to facilitate fine-tuning and maintenance.
+// At the top of the file +template <typename T> +constexpr T get_tolerance() { + if constexpr (std::is_same_v<T, double>) { + return 1e-12; + } else { + return 1e-5; + } +} // In the test -EXPECT_THAT(Jy(ix), ::testing::DoubleNear((expectedJy[ix]), 1e-12)); +EXPECT_THAT(Jy(ix), ::testing::DoubleNear((expectedJy[ix]), get_tolerance<floater_t<4>>()));src/core/data/particles/particle_utilities.hpp (2)
22-22
: Document the precision choice for position calculations.While changing to
floater_t<4>
aligns with the codebase's type system transition and could improve performance, the choice of precision level4
needs documentation. This is especially important for position calculations where precision requirements might affect particle physics simulations.Add a comment explaining:
- Why precision level 4 was chosen for position
- Any precision/performance trade-offs considered
- Whether this precision is sufficient for all use cases
Line range hint
42-89
: Update debug function to use consistent types.The
checkDeltas
debug function still usesdouble
type for deltas vector and comparisons. For consistency with the new type system:- std::vector<double> deltas; + std::vector<floater_t<4>> deltas;Also consider adding
[[maybe_unused]]
attribute to this debug-only function to suppress compiler warnings when not in use.tests/core/data/gridlayout/gridlayout_indexing.hpp (1)
Line range hint
25-95
: Consider documenting the floater_t precision requirementsThe transition to
floater_t<4>
across the grid layout system is a significant architectural change. Consider:
- Documenting the precision requirements and rationale for choosing template parameter 4
- Adding static assertions to validate precision requirements
- Updating the test data generation scripts to account for the new type system
This will help maintain consistency and prevent precision-related issues in the future.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
src/amr/data/field/field_data.hpp
(9 hunks)src/core/data/grid/gridlayout.hpp
(4 hunks)src/core/data/grid/gridlayoutdefs.hpp
(1 hunks)src/core/data/particles/particle_utilities.hpp
(1 hunks)src/core/def.hpp
(2 hunks)src/core/numerics/faraday/faraday.hpp
(2 hunks)src/core/numerics/interpolator/interpolator.hpp
(16 hunks)src/phare_core.hpp
(2 hunks)tests/amr/data/field/coarsening/test_linear_coarsen.hpp
(1 hunks)tests/amr/data/field/refine/test_field_refine.cpp
(1 hunks)tests/amr/data/field/refine/test_field_refinement_on_hierarchy.cpp
(1 hunks)tests/amr/data/field/time_interpolate/test_field_data_time_interpolate.cpp
(1 hunks)tests/amr/data/field/variable/test_field_variable.cpp
(1 hunks)tests/amr/resources_manager/test_resources_manager.cpp
(1 hunks)tests/core/data/electrons/test_electrons.cpp
(1 hunks)tests/core/data/field/test_field_fixtures.hpp
(1 hunks)tests/core/data/gridlayout/gridlayout_allocsize.hpp
(1 hunks)tests/core/data/gridlayout/gridlayout_base_params.hpp
(1 hunks)tests/core/data/gridlayout/gridlayout_cell_centered_coord.hpp
(4 hunks)tests/core/data/gridlayout/gridlayout_field_centered_coord.hpp
(4 hunks)tests/core/data/gridlayout/gridlayout_indexing.hpp
(1 hunks)tests/core/data/gridlayout/gridlayout_params.hpp
(1 hunks)tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cpp
(1 hunks)tests/core/data/particles/test_main.cpp
(1 hunks)tests/core/data/tensorfield/test_tensorfield_fixtures.hpp
(2 hunks)tests/core/numerics/ampere/test_main.cpp
(1 hunks)tests/core/numerics/faraday/test_main.cpp
(1 hunks)tests/core/numerics/interpolator/test_main.cpp
(4 hunks)tests/core/numerics/ion_updater/test_updater.cpp
(2 hunks)tests/core/numerics/ohm/test_main.cpp
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/core/data/particles/test_main.cpp
- src/phare_core.hpp
- tests/core/numerics/interpolator/test_main.cpp
🧰 Additional context used
📓 Path-based instructions (16)
src/core/data/particles/particle_utilities.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/core/data/gridlayout/gridlayout_params.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/core/data/gridlayout/gridlayout_indexing.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/core/data/gridlayout/gridlayout_base_params.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/core/data/field/test_field_fixtures.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/data/grid/gridlayoutdefs.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/core/data/gridlayout/gridlayout_allocsize.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/core/data/tensorfield/test_tensorfield_fixtures.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/amr/data/field/coarsening/test_linear_coarsen.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/numerics/faraday/faraday.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/def.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/amr/data/field/field_data.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/core/data/gridlayout/gridlayout_cell_centered_coord.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/data/grid/gridlayout.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/core/data/gridlayout/gridlayout_field_centered_coord.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/numerics/interpolator/interpolator.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (40)
tests/core/numerics/ion_updater/test_updater.cpp (2)
518-518
: LGTM!The indentation change improves code readability.
577-577
: Type change aligns with precision configuration initiative.The change from
double
tofloater_t<4>
is consistent with the broader effort to use configurable precision types throughout the codebase.However, verify that the precision level (4) matches the requirements for moment calculations:
✅ Verification successful
Precision level 4 is correctly used for field-related computations
The change from
double
tofloater_t<4>
for the sum variable is consistent with the codebase's precision hierarchy, where level 4 is specifically used for field and grid-related computations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other uses of floater_t to verify consistent precision levels # Test: Search for other floater_t declarations to ensure consistent precision rg "floater_t<" -A 2 -B 2Length of output: 42071
tests/amr/resources_manager/test_resources_manager.cpp (1)
36-36
: Verify integration with dependent components.The change to
Grid1D
type definition introducesfloater_t<4>
precision. Ensure this precision level is compatible with all components consuming Grid1D data.✅ Verification successful
Change is consistent with codebase precision patterns ✓
The use of
floater_t<4>
inGrid1D
aligns with the established precision type system used throughout the codebase, particularly in grid-based operations and field definitions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other Grid1D usages to verify compatibility rg "Grid1D" --type cpp # Look for other floater_t<4> usages to verify consistency rg "floater_t<4>" --type cppLength of output: 9649
src/amr/data/field/field_data.hpp (2)
5-5
: LGTM! Include addition is well-placed.The addition of
core/def.hpp
aligns with the type system changes and follows proper include ordering.
501-502
: LGTM! Well-designed template modifications.The addition of template parameter
T
to pack/unpack implementations provides type safety while maintaining flexibility. The changes are consistently applied across all dimension specializations with proper const correctness.Also applies to: 515-516, 572-573, 595-596, 672-673, 700-701
tests/core/data/field/test_field_fixtures.hpp (1)
11-11
: Document the meaning of precision level 4 infloater_t<4>
.The significance of the precision level
4
infloater_t<4>
is not immediately clear. Please add documentation explaining:
- The meaning of different precision levels
- Why level 4 was chosen for field values
Let's verify the consistency of precision levels across the codebase:
src/core/data/grid/gridlayoutdefs.hpp (1)
23-30
: Verify consistent precision level across particle-related types.The change from
double
tofloater_t<4>
for the coefficient suggests a specific precision requirement. Let's verify that this precision level (4) is consistent with other particle-related types in the codebase.✅ Verification successful
Precision level is consistent with the codebase's type system
The use of
floater_t<4>
for the coefficient in WeightPoint aligns with the codebase's systematic use of precision levels, where level 4 is consistently used for grid-related quantities and field operations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of floater_t to verify consistent precision levels rg "floater_t<" --type cppLength of output: 14411
tests/core/data/tensorfield/test_tensorfield_fixtures.hpp (2)
26-27
: LGTM: Type definition updated for configurable precision.The change from implicit double precision to explicit
floater_t<4>
aligns with the PR objective of introducing configurable precision across the codebase.
53-54
: LGTM: Lambda function formatting.The lambda function formatting changes improve readability while maintaining the same functionality.
tests/amr/data/field/refine/test_field_refinement_on_hierarchy.cpp (1)
23-23
: LGTM: Consistent type system update.The
GridND
type definition has been updated to usefloater_t<4>
, maintaining consistency with the project-wide precision configuration changes.tests/amr/data/field/variable/test_field_variable.cpp (1)
53-53
: LGTM: Template alias updated for precision configuration.The
FieldVar
template alias has been updated to usefloater_t<4>
, aligning with the project's move towards configurable precision types.tests/amr/data/field/refine/test_field_refine.cpp (1)
81-81
: LGTM: Type definition aligned with precision changes.The
GridT
type definition has been updated to usefloater_t<4>
, consistent with the project's transition to configurable precision types.tests/amr/data/field/coarsening/test_linear_coarsen.hpp (1)
58-58
: Verify numerical precision requirements.The change from the default floating-point type to
floater_t<4>
could affect numerical precision and memory usage in grid operations. Ensure that this precision level is necessary and consistent with the project's requirements.Run the following script to check for consistency in floating-point type usage across grid-related files:
✅ Verification successful
Precision requirements are appropriate for test scenarios
The use of
floater_t<4>
in this test file is consistent with other test implementations and appears to be intentionally testing different precision levels, as evidenced by the presence of both precision types in the same test file.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent use of floater_t<4> in grid definitions # Look for any inconsistencies in Grid type definitions rg -A 1 "using.*Grid.*=.*Grid<"Length of output: 3646
tests/amr/data/field/time_interpolate/test_field_data_time_interpolate.cpp (1)
51-51
: LGTM! Consistent type system update.The change to use
floater_t<4>
aligns with the project-wide type system update for grid operations.tests/core/data/electrons/test_electrons.cpp (1)
136-136
: LGTM! Consistent type system update.The change to use
floater_t<4>
maintains consistency with the project-wide type system update.src/core/numerics/faraday/faraday.hpp (2)
21-21
: LGTM! Good type safety improvement.The change from
double
tofloater_t<4>
with added const qualifier improves type safety and prevents accidental modifications.
52-52
: LGTM! Consistent type usage.The member variable type change maintains consistency with the operator() parameter type.
tests/core/numerics/faraday/test_main.cpp (1)
262-271
: LGTM! Well-handled precision differences.The conditional compilation correctly handles different precision requirements for float32 vs float64 comparisons.
src/core/numerics/interpolator/interpolator.hpp (8)
60-68
: LGTM! Good template generalization.The changes improve type flexibility by templating the computeWeight function and using consistent floating-point types for constants.
80-97
: LGTM! Consistent type system implementation.The changes maintain consistency with the type system overhaul while preserving the original logic.
110-137
: LGTM! Well-structured type conversions.The changes properly handle type conversions and maintain consistent floating-point precision throughout the calculations.
300-301
: LGTM! Consistent parameter type.The coef parameter type change aligns with the overall type system modifications.
326-327
: LGTM! Good const usage.The changes maintain type consistency and properly use const qualifiers for loop variables.
Also applies to: 339-340
358-359
: LGTM! Consistent implementation.The changes follow the same pattern as the 2D implementation, maintaining consistency across dimensions.
Also applies to: 373-375
403-403
: LGTM! Appropriate precision levels.The changes use appropriate precision levels: floater_t<0> for interpolation calculations and floater_t<4> for field components.
Also applies to: 418-418, 428-428, 443-443
490-490
: LGTM! Complete type system implementation.The changes complete the type system overhaul with consistent usage of floater_t types throughout the interpolation system.
Also applies to: 523-523, 572-572
tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cpp (1)
63-63
: Clarify the choice of precision level for ZRO constant.The use of
floater_t<4>
seems arbitrary. Other files in the codebase use different precision levels (e.g.,floater_t<3>
for weight,floater_t<2>
for charge). Please explain the reasoning behind choosing precision level 4 for this zero constant.Let's check for consistency across the codebase:
✅ Verification successful
The precision level for ZRO constant is correct and consistent.
The use of
floater_t<4>
follows the established pattern in the codebase where precision level 4 is consistently used for grid-related operations, field types, and coordinate systems. This includes similar usages inGrid
,Field
, and other grid-layout components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other floater_t zero constants rg "floater_t<\d+>\s+(?:const|constexpr)\s+.*=\s*0" --type cppLength of output: 196
Script:
#!/bin/bash # Search for all floater_t precision levels in test files echo "=== Floater type usage in tests ===" rg "floater_t<\d+>" tests/ --type cpp # Search for any documentation about precision levels echo -e "\n=== Documentation about precision levels ===" rg "precision|floater_t" README.md || true # Look for other constants in the same file echo -e "\n=== Constants in the test file ===" rg "(?:const|constexpr)\s+static" tests/core/data/maxwellian_particle_initializer/test_maxwellian_particle_initializer.cppLength of output: 7386
tests/core/numerics/ohm/test_main.cpp (1)
71-71
: Verify numerical precision requirements withfloater_t<4>
.The change to
floater_t<4>
for the grid data type might affect the numerical precision of the tests. The current test tolerances (1e-12 for 1D/2D and 1e-10 for 3D) should be reviewed to ensure they align with the precision capabilities offloater_t<4>
.✅ Verification successful
Numerical precision requirements are correctly aligned with
floater_t<4>
The test tolerances (1e-12 for 1D/2D and 1e-10 for 3D) are appropriate for the precision capabilities of
floater_t<4>
. The codebase consistently handles both float and double precision cases with proper tolerance adjustments.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other uses of floater_t to verify consistent precision levels ast-grep --pattern 'floater_t<$_>'Length of output: 36
Script:
#!/bin/bash # Search for floater_t mentions rg "floater_t" -A 2 -B 2 # Search for type definitions ast-grep --pattern 'using $_= $_floater_t' # Search for numerical tolerances in test files rg "1e-\d+" tests/Length of output: 52926
tests/core/numerics/ampere/test_main.cpp (1)
221-230
: Verify precision of test data files.The test reads expected values from files (e.g.,
jy_yee_1D_order1.txt
), but there's no verification that these files contain values with appropriate precision for float32 testing.src/core/data/particles/particle_utilities.hpp (1)
Line range hint
22-33
: Verify type consistency in position calculations.The position calculation mixes different types (floater_t<4>, origin type, meshSize type), which could lead to implicit conversions. This might affect numerical accuracy.
Let's verify the types of all components used in the position calculation:
src/core/data/grid/gridlayout.hpp (1)
1494-1497
: Ensure consistency with updated floating-point typesThe member variables
meshSize_
,origin_
, andinverseMeshSize_
have been updated tofloater_t<4>
. Verify that all computations and usages of these variables are compatible with the new type to prevent precision or compatibility issues.tests/core/data/gridlayout/gridlayout_base_params.hpp (1)
25-25
: Verify the choice of template parameter 4 for floater_tThe change from
double
tofloater_t<4>
fordxdydz
andorigin
is consistent with the type system overhaul. However, we should verify if4
is the appropriate template parameter value for these grid layout parameters.Also applies to: 28-28
✅ Verification successful
The use of floater_t<4> is verified and correct
The template parameter choice is consistent with the project-wide standard, used uniformly across all grid-related operations, fields, and numerical solvers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of floater_t to verify consistency rg "floater_t<4>" -A 2 -B 2Length of output: 23453
tests/core/data/gridlayout/gridlayout_params.hpp (1)
54-56
: LGTM! Type changes are consistentThe parameter type changes in
createParam
correctly align with the modifications inGridLayoutTestParam
, maintaining type consistency across the grid layout implementation.tests/core/data/gridlayout/gridlayout_allocsize.hpp (1)
79-79
: Consider precision loss when reading test dataThe change to
floater_t<4>
affects how floating-point values are read from the test data file. Ensure that the test data files maintain sufficient precision for the new type.Also applies to: 86-86
tests/core/data/gridlayout/gridlayout_indexing.hpp (1)
88-88
: Similar precision concerns as in allocsize.hppThe changes to use
floater_t<4>
here raise the same precision considerations when reading test data.Also applies to: 95-95
tests/core/data/gridlayout/gridlayout_cell_centered_coord.hpp (3)
48-51
: Consider precision impact on coordinate calculations.The coordinate calculations now use
floater_t<4>
instead ofdouble
. While this aligns with the PR's objective, verify that:
- The cell-centered coordinate calculations maintain sufficient accuracy
- No precision-related issues occur during type conversion in
cellCenteredCoord
95-95
: Validate grid parameter precision requirements.Grid spacing (
dl
) and origin coordinates are fundamental parameters that affect all grid-based calculations. Ensure thatfloater_t<4>
provides adequate precision for:
- Small-scale grid spacing
- Large-scale origin coordinates
- Accumulated position calculations
Also applies to: 100-100
22-24
: Verify precision requirements for position vectors.The change from
double
tofloater_t<4>
for position vectors could impact numerical precision in test calculations. Ensure that float32 precision is sufficient for these test cases.✅ Verification successful
Type change to floater_t<4> is safe for position vectors
The coordinate calculations in these tests involve simple linear transformations with no complex mathematical operations that could be affected by reduced precision. Since test data will be regenerated with matching precision, the exact equality comparisons will remain valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing precision-related test failures or comments rg -i "precision|accuracy" tests/core/data/gridlayout/Length of output: 56
Script:
#!/bin/bash # Get the complete test file content cat tests/core/data/gridlayout/gridlayout_cell_centered_coord.hpp # Search for floating-point comparison patterns in grid layout tests rg -A 2 "EXPECT_|ASSERT_" tests/core/data/gridlayout/gridlayout_cell_centered_coord.* # Look for epsilon or tolerance definitions rg "epsilon|tolerance|precision" tests/core/data/Length of output: 7021
Script:
#!/bin/bash # Check the implementation of writeToArray and related utilities rg -A 5 "writeToArray" tests/core/data/gridlayout/gridlayout_utilities.hpp # Look for the data file generation code fd -e cpp -e hpp -x rg -l "centeredCoords_.*\.txt"Length of output: 127
tests/core/data/gridlayout/gridlayout_field_centered_coord.hpp (2)
20-21
: LGTM! Type changes are consistent with cell-centered coordinates.The change to
floater_t<4>
maintains consistency with the changes ingridlayout_cell_centered_coord.hpp
.
102-102
: LGTM! Grid parameter types are consistent.The change to
floater_t<4>
fordl
andorigin
maintains consistency with the cell-centered coordinate implementation.Also applies to: 105-105
if constexpr (std::is_same_v<floater_t<4>, double>) | ||
{ | ||
EXPECT_THAT(Jy(ix), ::testing::DoubleNear((expectedJy[ix]), 1e-12)); | ||
EXPECT_THAT(Jz(ix), ::testing::DoubleNear((expectedJz[ix]), 1e-12)); | ||
} | ||
else | ||
{ | ||
EXPECT_THAT(Jy(ix), ::testing::FloatNear((expectedJy[ix]), 1e-5)); | ||
EXPECT_THAT(Jz(ix), ::testing::FloatNear((expectedJz[ix]), 1e-5)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure consistent floating-point comparison across all test cases.
While the 1D test has been updated to handle both float32 and double precision, the 2D and 3D tests still exclusively use DoubleNear
. This inconsistency could lead to test failures when using float32.
Apply similar conditional checks to 2D and 3D test cases. Example for 2D:
-EXPECT_THAT(Jx(ix, iy), ::testing::DoubleNear((expectedJx[index_]), 1e-12));
+if constexpr (std::is_same_v<floater_t<4>, double>)
+{
+ EXPECT_THAT(Jx(ix, iy), ::testing::DoubleNear((expectedJx[index_]), 1e-12));
+}
+else
+{
+ EXPECT_THAT(Jx(ix, iy), ::testing::FloatNear((expectedJx[index_]), 1e-5));
+}
Committable suggestion skipped: line range outside the PR's diff.
params.back().base = createParam<GridLayoutImpl>( | ||
dl, nbCell, Point<floater_t<4>, GridLayoutImpl::dimension>{origin}); | ||
|
||
|
||
std::array<std::uint32_t, GridLayoutImpl::dimension> icell; | ||
std::array<double, GridLayoutImpl::dimension> realPosition; | ||
std::array<floater_t<4>, GridLayoutImpl::dimension> realPosition; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Update comment to reflect new type and verify consistency.
- The comment on line 113 mentions
Point<double, dim>
but the code now usesPoint<floater_t<4>, dim>
. Update the comment for accuracy. - Ensure consistent precision across all position calculations, including
realPosition
.
- // NOTE: c++17 : Point{origin}, C++14 : Point<double, dim>{origin}
+ // NOTE: c++17 : Point{origin}, C++14 : Point<floater_t<4>, dim>{origin}
Committable suggestion skipped: line range outside the PR's diff.
@@ -49,7 +49,7 @@ struct GridLayoutFieldCenteringParam | |||
Point<double, GridLayoutImpl::dimension> pos; | |||
pos = fieldCoord(iCell); | |||
|
|||
std::array<double, GridLayoutImpl::dimension> actualPos; | |||
std::array<floater_t<4>, GridLayoutImpl::dimension> actualPos; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix type inconsistency in coordinate conversion.
There's a type mismatch in the coordinate conversion:
pos
is declared asPoint<double, GridLayoutImpl::dimension>
actualPos
is declared asstd::array<floater_t<4>, GridLayoutImpl::dimension>
This could lead to unnecessary type conversion. Consider updating pos
to use floater_t<4>
for consistency.
- Point<double, GridLayoutImpl::dimension> pos;
+ Point<floater_t<4>, GridLayoutImpl::dimension> pos;
Committable suggestion skipped: line range outside the PR's diff.
@@ -123,7 +123,7 @@ | |||
|
|||
|
|||
std::array<std::uint32_t, GridLayoutImpl::dimension> icell; | |||
std::array<double, GridLayoutImpl::dimension> realPosition; | |||
std::array<floater_t<4>, GridLayoutImpl::dimension> realPosition; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix type inconsistency in Point initialization.
Similar to the previous issue, there's a type mismatch in the createParam
call where Point<double, GridLayoutImpl::dimension>
is used instead of Point<floater_t<4>, GridLayoutImpl::dimension>
. Update this for consistency with the rest of the changes.
- dl, nbCell, Point<double, GridLayoutImpl::dimension>{origin});
+ dl, nbCell, Point<floater_t<4>, GridLayoutImpl::dimension>{origin});
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tests/core/numerics/ohm/test_main.cpp (1)
Line range hint
371-529
: Improve test coverage for non-double precision.The test assertions are only executed when
floater_t<4>
is the same asdouble
, leaving a gap in test coverage for other precision types.Consider the following improvements:
- Generate test data for non-double precision types.
- Add assertions with appropriate tolerances for different precision types.
- Document the expected behavior and precision requirements.
Example approach for handling different precision types:
if constexpr (std::is_same_v<floater_t<4>, double>) { EXPECT_THAT(Exnew(ix), ::testing::DoubleNear(expected_ohmX[ix], 1e-12)); } else if constexpr (std::is_same_v<floater_t<4>, float>) { EXPECT_THAT(Exnew(ix), ::testing::FloatNear(expected_ohmX[ix], 1e-6f)); } else { // Add assertions for other precision types static_assert(false, "Unsupported precision type"); }
🧹 Nitpick comments (5)
src/initializer/data_provider.hpp (1)
24-27
: Consider Templating Over Dimension to Reduce Code DuplicationThe specializations of
InitFunctionHelper
for dimensions 1, 2, and 3 have similar structures. To enhance maintainability and reduce code duplication, consider templating over the dimension:template<std::size_t dim> struct InitFunctionHelper<core::floater_t<4>, dim> { using return_type = std::shared_ptr<core::Span<core::floater_t<4>>>; using param_type = std::vector<core::floater_t<4>> const&; using type = /* define type based on dim */; };This approach allows you to define the
type
member based on the dimensiondim
dynamically.Also applies to: 32-35, 40-43
tests/initializer/init_functions.hpp (1)
137-154
: SimplifymakeSharedPtr
Function to Reduce Code DuplicationThe
makeSharedPtr
function contains repetitive code for different dimensions. Consider templating over the parameter types to reduce duplication:template<std::size_t dim> auto makeSharedPtr() { using Param = std::vector<PHARE::core::floater_t<4>> const&; if constexpr (dim >= 1 && dim <= 3) { return [](auto&&... params) { return std::make_shared<PHARE::core::VectorSpan<PHARE::core::floater_t<4>>>(params...); }; } }This change enhances readability and maintainability.
src/core/data/ions/particle_initializers/particle_initializer_factory.hpp (1)
50-55
: Optimize type-based branching usingconstexpr
.The lambda introduces unnecessary runtime overhead for a compile-time decision. Consider moving the
if constexpr
outside the lambda for better performance.- auto const densityCutOff = [&]() { - if constexpr (std::is_same_v<floater_t<4>, double>) - return cppdict::get_value(dict, "density_cut_off", double{1e-16}); - else - return cppdict::get_value(dict, "density_cut_off", float{1e-6}); - }(); + auto const densityCutOff = std::is_same_v<floater_t<4>, double> + ? cppdict::get_value(dict, "density_cut_off", double{1e-16}) + : cppdict::get_value(dict, "density_cut_off", float{1e-6});src/core/data/ions/particle_initializers/maxwellian_particle_initializer_base.hpp (2)
17-25
: Add type safety constraints to variadic template.The current implementation lacks compile-time validation of argument types and count. Consider adding constraints to ensure type safety.
+ template<typename T> + concept FloatingPoint = std::is_floating_point_v<T>; + - template<typename T, typename... Args> + template<typename T, typename... Args> + requires (sizeof...(Args) == 6) && + (FloatingPoint<std::remove_cvref_t<Args>> && ...) void maxwellianVelocity(std::array<T, 3>& partVelocity, std::mt19937_64& generator, Args const... args)
50-56
: Add type safety constraints to magnetic basis calculation.Similar to maxwellianVelocity, add compile-time validation for argument types and count.
- template<typename T0, typename... Bargs> + template<typename T0, typename... Bargs> + requires (sizeof...(Bargs) == 3) && + (std::is_same_v<std::remove_cvref_t<Bargs>, T0> && ...) void localMagneticBasis(std::array<std::array<T0, 3>, 3>& basis, Bargs const... bargs)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/core/data/grid/gridlayout.hpp
(5 hunks)src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp
(7 hunks)src/core/data/ions/particle_initializers/maxwellian_particle_initializer_base.hpp
(4 hunks)src/core/data/ions/particle_initializers/particle_initializer_factory.hpp
(1 hunks)src/initializer/data_provider.hpp
(1 hunks)src/initializer/dictator.cpp
(1 hunks)tests/core/data/electrons/test_electrons.cpp
(1 hunks)tests/core/numerics/ampere/test_main.cpp
(7 hunks)tests/core/numerics/faraday/test_main.cpp
(7 hunks)tests/core/numerics/interpolator/test_main.cpp
(4 hunks)tests/core/numerics/ohm/test_main.cpp
(8 hunks)tests/initializer/init_functions.hpp
(3 hunks)tests/initializer/test_initializer.cpp
(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/core/numerics/faraday/test_main.cpp
- tests/core/numerics/ampere/test_main.cpp
- tests/core/numerics/interpolator/test_main.cpp
- src/core/data/grid/gridlayout.hpp
🧰 Additional context used
📓 Path-based instructions (5)
src/core/data/ions/particle_initializers/particle_initializer_factory.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/initializer/data_provider.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/data/ions/particle_initializers/maxwellian_particle_initializer_base.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/initializer/init_functions.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (macos-latest)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-13)
🔇 Additional comments (10)
src/initializer/data_provider.hpp (1)
51-53
: Verify the Necessity of Including Bothfloat
anddouble
inPHAREDict
With the transition to
core::floater_t<4>
, which seems to standardize floating-point types, is it necessary to include bothfloat
anddouble
inPHAREDict
? Keeping only the necessary types could prevent potential type mismatches and simplify the dictionary.src/initializer/dictator.cpp (1)
59-59
: Ensure Proper Overloading ofadd_array_as_vector
in Pybind11Defining
add_array_as_vector
for bothfloat
anddouble
types may cause ambiguity in Python bindings. Verify that Pybind11 handles this overloading correctly and that each function is accessible without conflicts.tests/initializer/init_functions.hpp (1)
15-16
: Consistent Update tofloater_t<4>
for TypesThe change from
double
tofloater_t<4>
forParam
andReturn
types is correctly implemented, ensuring consistency with the updated type system.tests/initializer/test_initializer.cpp (1)
94-96
: LGTM! Test assertions correctly updated.The test assertions have been properly updated from
EXPECT_DOUBLE_EQ
toEXPECT_FLOAT_EQ
to match the newfloater_t<4>
type system.Also applies to: 105-105, 107-115
src/core/data/ions/particle_initializers/maxwellian_particle_initializer_base.hpp (1)
Line range hint
34-46
: LGTM! Clean template conversion.The function has been properly templated while maintaining its core functionality.
src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp (2)
35-39
: LGTM! Consistent type system update.The constructor parameters and member variables have been properly updated to use
floater_t<4>
while maintaining the existing default values.Also applies to: 82-83
181-194
: LGTM! Particle initialization logic preserved.The type system updates have been properly applied while maintaining the core particle initialization logic and velocity calculations.
tests/core/data/electrons/test_electrons.cpp (2)
136-136
: Update Electrons constructor to handle floater_t<4>.The type alias change is correct, but ensure that the Electrons constructor is updated to handle the new floating-point type.
137-137
: LGTM!The type alias change is consistent with the overall transition to using
floater_t<4>
.tests/core/numerics/ohm/test_main.cpp (1)
71-71
: Update dependent types to handle floater_t<4>.The type alias change is correct, but ensure that dependent types like
Field
andTensorField
are updated to handle the new floating-point type.
@@ -49,12 +49,14 @@ PYBIND11_MODULE(dictator, m) | |||
m.def("add_int", add<int>, "add"); | |||
m.def("add_vector_int", add<std::vector<int>>, "add"); | |||
m.def("add_double", add<double>, "add"); | |||
m.def("add_double", add<float>, "add"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect Function Name: Rename add_double
to add_float
The function add_double
is bound to add<float>
, which is misleading. To maintain clarity and consistency, the function should be renamed to add_float
:
-m.def("add_double", add<float>, "add");
+m.def("add_float", add<float>, "add");
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
m.def("add_double", add<float>, "add"); | |
m.def("add_float", add<float>, "add"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 14
♻️ Duplicate comments (1)
src/core/numerics/pusher/boris.hpp (1)
4-14
:⚠️ Potential issueMissing critical include directives
The code still requires the previously identified include directives for proper functionality.
Please uncomment these essential includes:
#include "core/utilities/range/range.hpp" #include "core/data/particles/particle.hpp"
🧹 Nitpick comments (23)
src/core/data/field/initializers/field_user_initializer.hpp (1)
25-27
: LGTM! Consider improving type readability.The type changes from
double
tofloater_t<4>
and the addition ofconst
qualifier look good. The use ofauto&
for grid is appropriate as the type is clearly deduced.Consider using a type alias to improve readability:
+ using GridDataType = Span<floater_t<4>>; - std::shared_ptr<Span<floater_t<4>>> const gridPtr // keep grid data alive + std::shared_ptr<GridDataType> const gridPtr // keep grid data alive = std::apply([&](auto&... args) { return init(args...); }, coords); auto& grid = *gridPtr;src/amr/data/field/time_interpolate/field_linear_time_interpolate.hpp (2)
55-58
: Document the significance of template parameter<4>
infloater_t
.The meaning of the template parameter
<4>
incore::floater_t<4>
is not immediately clear. Consider adding documentation to explain the significance of this value and its impact on precision.
134-135
: LGTM! Consider vectorization for performance.The 3D interpolation implementation is correct. However, since this is a compute-intensive loop nest, consider adding OpenMP SIMD directives or compiler-specific vectorization hints to potentially improve performance.
Example optimization:
#pragma omp simd for (auto iz = iDestStartZ, izSrc = iSrcStartZ; iz <= iDestEndZ; ++iz, ++izSrc) { fieldDest(ix, iy, iz) = (1.f - alpha) * fieldSrcOld(ixSrc, iySrc, izSrc) + alpha * fieldSrcNew(ixSrc, iySrc, izSrc); }tests/core/numerics/faraday/test_main.cpp (1)
26-26
: Avoid leading/trailing underscores in identifier names.The identifier
_PI_
could conflict with reserved identifiers. Consider renaming it toPI
orPHARE_PI
to follow safer naming conventions.-PHARE::core::floater_t<4> constexpr static _PI_ = std::numbers::pi_v<PHARE::core::floater_t<4>>; +PHARE::core::floater_t<4> constexpr static PHARE_PI = std::numbers::pi_v<PHARE::core::floater_t<4>>;src/amr/data/field/field_data.hpp (4)
67-79
: Consider removing deprecated code.The deprecated constructor is commented out but still present in the codebase. Since it's marked as deprecated, it should be removed to maintain code cleanliness.
- // [[deprecated]] FieldData(SAMRAI::hier::Box const& domain, - // SAMRAI::hier::IntVector const& ghost, std::string name, - // std::array<double, dimension> const& dl, - // std::array<std::uint32_t, dimension> const& nbrCells, - // core::Point<double, dimension> const& origin, PhysicalQuantity - // qty) - - // : SAMRAI::hier::PatchData(domain, ghost) - // , gridLayout{dl, nbrCells, origin} - // , field(name, qty, gridLayout.allocSize(qty)) - // , quantity_{qty} - // { - // }
485-490
: Consider adding const qualifiers to local variables in 1D implementation.Local variables in the 1D implementation should be marked as
const
for better const-correctness.- int xStart = overlap.lower(0) - destination.lower(0); - int xEnd = overlap.upper(0) - destination.lower(0); + int const xStart = overlap.lower(0) - destination.lower(0); + int const xEnd = overlap.upper(0) - destination.lower(0);Also applies to: 503-508, 517-523
574-583
: Consider adding const qualifiers to local variables in 2D implementation.Local variables in the 2D implementation should be marked as
const
for better const-correctness.- int xStart = overlap.lower(0) - destination.lower(0); - int xEnd = overlap.upper(0) - destination.lower(0); - int yStart = overlap.lower(1) - destination.lower(1); - int yEnd = overlap.upper(1) - destination.lower(1); + int const xStart = overlap.lower(0) - destination.lower(0); + int const xEnd = overlap.upper(0) - destination.lower(0); + int const yStart = overlap.lower(1) - destination.lower(1); + int const yEnd = overlap.upper(1) - destination.lower(1);Also applies to: 597-606
674-675
: Consider adding const qualifiers to local variables in 3D implementation.Local variables in the 3D implementation should be marked as
const
for better const-correctness.- int xStart = overlap.lower(0) - destination.lower(0); - int xEnd = overlap.upper(0) - destination.lower(0); - int yStart = overlap.lower(1) - destination.lower(1); - int yEnd = overlap.upper(1) - destination.lower(1); - int zStart = overlap.lower(2) - destination.lower(2); - int zEnd = overlap.upper(2) - destination.lower(2); + int const xStart = overlap.lower(0) - destination.lower(0); + int const xEnd = overlap.upper(0) - destination.lower(0); + int const yStart = overlap.lower(1) - destination.lower(1); + int const yEnd = overlap.upper(1) - destination.lower(1); + int const zStart = overlap.lower(2) - destination.lower(2); + int const zEnd = overlap.upper(2) - destination.lower(2);Also applies to: 702-703
src/amr/data/field/coarsening/default_field_coarsener.hpp (2)
103-103
: Consider extracting precision as a template parameter.The type change to
core::floater_t<4>
is consistent with the overall precision control. Consider making the precision level a template parameter of the class to allow for flexible precision control.template<std::size_t dimension, std::size_t precision = 4> class DefaultFieldCoarsener { using float_type = core::floater_t<precision>; // ... };
129-133
: Consider refactoring to reduce code duplication.The type changes to
core::floater_t<4>
are consistent. However, the interpolation logic is similar across dimensions. Consider extracting the common pattern into a dimension-agnostic helper function.Example approach:
template<typename FieldT, typename WeightT> core::floater_t<4> interpolateAxis( const FieldT& field, const WeightT& weights, const auto& startIndex, const auto& getFieldValue) { core::floater_t<4> result = 0; for (std::size_t shift = 0; shift < weights.size(); ++shift) { result += getFieldValue(startIndex + shift) * weights[shift]; } return result; }src/core/data/electrons/electrons.hpp (1)
221-221
: Consider using 0.0f for explicit floating-point initialization.While
0
works due to implicit conversion, using0.0f
would be more explicit and consistent with floating-point initialization conventions.- floater_t<4> const Te_ = 0; + floater_t<4> const Te_ = 0.0f;tests/core/data/gridlayout/gridlayout_deriv.cpp (2)
12-31
: Consider simplifying the read function using std::transform.The function correctly handles both double and floater_t<4> cases, but could be more concise.
Here's a more idiomatic C++ implementation:
auto read(std::string filename) { std::ifstream readFile(filename); assert(readFile.is_open()); std::vector<double> x; std::copy(std::istream_iterator<double>(readFile), std::istream_iterator<double>(), std::back_inserter(x)); if constexpr (std::is_same_v<floater_t<4>, double>) { return x; } else { - std::vector<floater_t<4>> fx(x.size()); - for (std::size_t i = 0; i < x.size(); i++) - fx[i] = x[i]; - return fx; + std::vector<floater_t<4>> fx; + fx.reserve(x.size()); + std::transform(x.begin(), x.end(), std::back_inserter(fx), + [](double d) { return static_cast<floater_t<4>>(d); }); + return fx; } }
59-60
: Consider using constexpr for trigonometric constants.The trigonometric calculations use magic numbers (5.f, 6.f, 12.f) directly in the expressions. These could be made more maintainable and self-documenting.
Consider defining these as named constants:
+constexpr auto X_WAVELENGTH = floater_t<4>{5.f}; +constexpr auto Y_WAVELENGTH = floater_t<4>{6.f}; +constexpr auto Z_WAVELENGTH = floater_t<4>{12.f}; auto point = this->layout.fieldNodeCoordinates(this->By, {0., 0., 0.}, ix, iy, iz); -this->By(ix, iy, iz) = std::sin(2 * _PI_ / 5.f * point[0]) - * std::cos(2 * _PI_ / 6.f * point[1]) - * std::sin(2 * _PI_ / 12.f * point[2]); +this->By(ix, iy, iz) = std::sin(2 * _PI_ / X_WAVELENGTH * point[0]) + * std::cos(2 * _PI_ / Y_WAVELENGTH * point[1]) + * std::sin(2 * _PI_ / Z_WAVELENGTH * point[2]);Also applies to: 88-89, 132-134, 174-176, 215-217, 313-315, 365-367, 418-420, 470-472, 522-524, 574-576
tests/amr/tagging/test_tagging.cpp (1)
28-30
: Add 'f' suffix to floating-point literal for type consistency.The threshold value should use the
f
suffix to maintain type consistency withfloater_t<4>
.Apply this diff:
- dict["threshold"] = 0.2; + dict["threshold"] = 0.2f;src/core/data/grid/gridlayout.hpp (1)
688-688
: Avoid using reserved identifiers and improve variable namingThe identifier
_2
starts with an underscore followed by a lowercase letter, which is reserved for the implementation in certain contexts. Additionally, naming the constant_2
may reduce code readability. Consider renaming the constant to a more descriptive name.Apply this diff to rename the constant:
-floater_t<4> constexpr static _2 = 2.0; +floater_t<4> constexpr static TWO = 2.0;And update its usage accordingly.
tests/amr/models/test_models.cpp (1)
24-24
: Consider including only necessary headers to reduce compilation timeIncluding the broad header
<phare_core.hpp>
may introduce unnecessary dependencies and increase compilation times. If possible, include only the specific headers required by this file to improve build performance.tests/core/numerics/ohm/test_main.cpp (1)
324-341
: Optimize vector type conversion.The current implementation creates a temporary vector and copies elements one by one. Consider using std::transform for better performance.
- std::vector<floater_t<4>> fx(x.size()); - for (std::size_t i = 0; i < x.size(); ++i) - fx[i] = x[i]; - return fx; + std::vector<floater_t<4>> fx; + fx.reserve(x.size()); + std::transform(x.begin(), x.end(), std::back_inserter(fx), + [](double d) { return static_cast<floater_t<4>>(d); }); + return fx;src/core/numerics/pusher/boris.hpp (1)
115-120
: LGTM! Consider naming improvement for clarityThe type changes to
floater_t<4>
are well-implemented. However, the constant namep5
could be more descriptive.Consider renaming for better readability:
- floater_t<4> constexpr static p5 = .5; + floater_t<4> constexpr static HALF = 0.5;src/amr/data/field/field_data_factory.hpp (1)
91-93
: LGTM: Type changes align with project-wide precision standardization.The change from
double
tocore::floater_t<4>
maintains consistency with the project's type system overhaul.Consider using named constants for initialization values.
Even though these values are temporary and unused, using named constants would improve code clarity.
+ static constexpr core::floater_t<4> DEFAULT_CELL_SIZE = 0.01; std::array<core::floater_t<4>, dimension> dl; std::array<std::uint32_t, dimension> nbCell; core::Point<core::floater_t<4>, dimension> origin;
src/amr/tagging/default_hybrid_tagger_strategy.hpp (1)
19-19
: Add documentation for the threshold value and type.While the type change to
core::floater_t<4>
aligns with the codebase standardization:
- Consider documenting the significance of the
0.1
threshold value.- Add a comment explaining what the template parameter
<4>
represents in the type system.Also applies to: 23-23, 29-29
tests/amr/data/particles/copy/test_particledata_copyNd.cpp (1)
Line range hint
118-148
: Consider adopting a more type-agnostic testing approach.The current implementation uses type-specific conditional blocks, which could lead to maintenance challenges as more floating-point types are supported. Consider these architectural improvements:
- Use type-parameterized tests to run the same test cases with different floating-point types
- Create type-specific matcher utilities that abstract away the comparison details
- Consider property-based testing to verify numerical properties across different precisions
Example approach:
template<typename T> class ParticlesDataTypeTest : public testing::Test { // ... test fixture for type T }; using TestedTypes = testing::Types<float, double>; TYPED_TEST_SUITE(ParticlesDataTypeTest, TestedTypes);src/python3/cpp_simulator.hpp (2)
34-45
: Restore PatchData template with floater_t support.The PatchData template is a core component that appears to be needed for data management. Consider:
- Updating it to support
floater_t
instead of removing it- Adding type traits to handle both
double
andfloater_t
types
Line range hint
24-124
: Consider a gradual migration strategy.Instead of commenting out large sections of code, consider:
- Create a type alias for the numeric type:
using numeric_t = core::floater_t<4>;
- Update existing implementations to use this alias
- Add temporary compatibility layer for existing code
- Deprecate old interfaces with compiler warnings
- Remove deprecated code in a future PR
This approach would allow for a smoother transition while maintaining functionality.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
src/amr/data/field/coarsening/default_field_coarsener.hpp
(4 hunks)src/amr/data/field/coarsening/magnetic_field_coarsener.hpp
(2 hunks)src/amr/data/field/field_data.hpp
(11 hunks)src/amr/data/field/field_data_factory.hpp
(3 hunks)src/amr/data/field/time_interpolate/field_linear_time_interpolate.hpp
(4 hunks)src/amr/resources_manager/amr_utils.hpp
(1 hunks)src/amr/tagging/default_hybrid_tagger_strategy.hpp
(2 hunks)src/core/data/electrons/electrons.hpp
(2 hunks)src/core/data/field/initializers/field_user_initializer.hpp
(1 hunks)src/core/data/grid/gridlayout.hpp
(12 hunks)src/core/def/phare_mpi.hpp
(1 hunks)src/core/numerics/interpolator/interpolator.hpp
(19 hunks)src/core/numerics/ohm/ohm.hpp
(3 hunks)src/core/numerics/pusher/boris.hpp
(4 hunks)src/core/numerics/pusher/pusher.hpp
(2 hunks)src/diagnostic/diagnostic_model_view.hpp
(1 hunks)src/phare_core.hpp
(2 hunks)src/python3/cpp_simulator.cpp
(1 hunks)src/python3/cpp_simulator.hpp
(3 hunks)src/python3/particles.hpp
(1 hunks)tests/amr/data/field/coarsening/test_linear_coarsen.hpp
(5 hunks)tests/amr/data/field/copy_pack/field_data_test_param.hpp
(1 hunks)tests/amr/data/field/geometry/test_field_geometry.cpp
(2 hunks)tests/amr/data/field/time_interpolate/test_field_data_time_interpolate.cpp
(5 hunks)tests/amr/data/particles/copy/test_particledata_copyNd.cpp
(2 hunks)tests/amr/models/test_models.cpp
(2 hunks)tests/amr/resources_manager/test_resources_manager.cpp
(1 hunks)tests/amr/resources_manager/test_resources_manager.hpp
(3 hunks)tests/amr/tagging/test_tagging.cpp
(2 hunks)tests/core/data/electrons/test_electrons.cpp
(10 hunks)tests/core/data/gridlayout/gridlayout_deriv.cpp
(23 hunks)tests/core/data/gridlayout/gridlayout_deriv.hpp
(1 hunks)tests/core/data/gridlayout/gridlayout_laplacian.cpp
(15 hunks)tests/core/data/gridlayout/test_gridlayout.hpp
(1 hunks)tests/core/numerics/ampere/test_main.cpp
(15 hunks)tests/core/numerics/faraday/test_main.cpp
(22 hunks)tests/core/numerics/interpolator/test_main.cpp
(4 hunks)tests/core/numerics/ion_updater/test_updater.cpp
(6 hunks)tests/core/numerics/ohm/test_main.cpp
(16 hunks)tests/core/numerics/pusher/test_pusher.cpp
(10 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/core/def/phare_mpi.hpp
- src/amr/data/field/coarsening/magnetic_field_coarsener.hpp
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/core/data/electrons/test_electrons.cpp
- tests/core/numerics/ampere/test_main.cpp
- src/core/numerics/pusher/pusher.hpp
- src/phare_core.hpp
- src/python3/particles.hpp
- tests/amr/resources_manager/test_resources_manager.cpp
- tests/amr/data/field/time_interpolate/test_field_data_time_interpolate.cpp
🧰 Additional context used
📓 Path-based instructions (19)
tests/core/data/gridlayout/gridlayout_deriv.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/amr/data/field/copy_pack/field_data_test_param.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/core/data/gridlayout/test_gridlayout.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/amr/data/field/time_interpolate/field_linear_time_interpolate.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/amr/data/field/coarsening/default_field_coarsener.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/data/field/initializers/field_user_initializer.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/diagnostic/diagnostic_model_view.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/amr/resources_manager/amr_utils.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/data/electrons/electrons.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/amr/data/field/field_data_factory.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/python3/cpp_simulator.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/amr/tagging/default_hybrid_tagger_strategy.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/amr/resources_manager/test_resources_manager.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/numerics/ohm/ohm.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/numerics/pusher/boris.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/amr/data/field/field_data.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
tests/amr/data/field/coarsening/test_linear_coarsen.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/data/grid/gridlayout.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/numerics/interpolator/interpolator.hpp (1)
Pattern **/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (macos-latest)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (46)
src/amr/resources_manager/amr_utils.hpp (1)
144-144
: Verify the precision requirements for spatial coordinates.The change from
double
tocore::floater_t<4>
for spatial coordinates (origin
anddl
) might affect numerical precision. While this aligns with the PR objective to try float32, spatial coordinates often require higher precision to maintain numerical stability, especially in AMR calculations.Let's verify the usage of these coordinates in calculations:
Also applies to: 146-146
✅ Verification successful
The precision change to
core::floater_t<4>
for spatial coordinates is appropriateThe codebase shows consistent usage of
floater_t<4>
for spatial calculations, primarily involving linear operations (additions and multiplications) that are well-suited for the chosen precision level. This change aligns with the PR's objective to try float32 particle delta.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for mathematical operations involving origin and dl rg -A 5 "origin\[.*\].*[+\-*/]|dl\[.*\].*[+\-*/]" # Search for other floater_t<4> usage with spatial coordinates ast-grep --pattern 'floater_t<4>'Length of output: 3533
src/amr/data/field/time_interpolate/field_linear_time_interpolate.hpp (2)
93-93
: LGTM! Correct linear interpolation formula with appropriate precision.The interpolation formula and the use of
.f
suffix for float literals are correct.
110-111
: LGTM! Correct 2D interpolation with proper alignment.The interpolation formula, precision, and code formatting are correct.
tests/core/numerics/faraday/test_main.cpp (1)
268-277
: Complete the float32 precision test implementation.The test has proper assertions for double precision with 1e-12 tolerance but the float32 case uses a significantly lower precision (1e-5). Consider documenting why this tolerance was chosen or adjust it based on numerical analysis.
src/amr/data/field/field_data.hpp (2)
5-5
: LGTM! Type system enhancement with floater_t.The addition of
core/def.hpp
and the static assertion forfloater_t<4>
type ensures consistent floating-point precision across the codebase.Also applies to: 44-44
92-102
: LGTM! Type-aware restart data handling.The conditional compilation using
std::is_same_v
ensures proper type handling when reading from restart database, correctly choosing betweendouble
andfloat
arrays based on thefloater_t<4>
type.tests/amr/data/field/copy_pack/field_data_test_param.hpp (1)
281-281
: 🛠️ Refactor suggestionEnsure consistent precision types across test fixtures.
The Grid1D type now uses
floater_t<4>
precision, but the test functionssourceFill()
anddestinationFill()
still returndouble
. This precision mismatch could lead to subtle numerical differences in tests.Consider updating the test functions to use consistent precision:
- double sourceFill(int iCell) + floater_t<4> sourceFill(int iCell) { auto& sourceLayout = sourceFieldData->gridLayout; auto& sourceField = sourceFieldData->field; auto origin = sourceLayout.origin(); auto position = sourceLayout.fieldNodeCoordinates(sourceField, origin, iCell); - return std::cos(position[0]); + return static_cast<floater_t<4>>(std::cos(position[0])); } - double destinationFill(int iCell) + floater_t<4> destinationFill(int iCell) { auto& destinationLayout = destinationFieldData->gridLayout; auto& destinationField = destinationFieldData->field; auto origin = destinationLayout.origin(); auto position = destinationLayout.fieldNodeCoordinates(destinationField, origin, iCell); - return std::sin(position[0]); + return static_cast<floater_t<4>>(std::sin(position[0])); }Also verify if
CellData<double>
andNodeData<double>
should be updated to usefloater_t<4>
for consistency.src/amr/data/field/coarsening/default_field_coarsener.hpp (2)
14-14
: LGTM: Include placement is correct.The new include for types.hpp is properly placed after external headers and before the implementation.
71-71
: Verify field data type compatibility.The change to
core::floater_t<4>
for intermediate calculations should match the precision of the input/output field data types to avoid unnecessary precision loss or conversion overhead.✅ Verification successful
Type compatibility verified for core::floater_t<4>
The coarseValue type matches the field data type (floater_t<4>) used consistently throughout the codebase for all field operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for field type declarations to verify precision compatibility ast-grep --pattern 'template<$_> class Field { $$$ }' rg -A 5 'using\s+.*Field.*='Length of output: 34807
src/core/data/electrons/electrons.hpp (1)
158-158
: LGTM! Verify template instantiation compatibility.The type conversion using template method is correct and type-safe.
Run the following to verify template instantiation compatibility across the codebase:
✅ Verification successful
Template instantiation is compatible with codebase patterns
The use of
floater_t<4>
with the template conversion methodto<>()
follows established patterns throughout the codebase, particularly in physics calculations and grid-based operations where precision control is critical.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of the template conversion with floater_t ast-grep --pattern 'to<floater_t<$_>>()'Length of output: 42
Script:
#!/bin/bash # Find definition of floater_t rg "floater_t" -A 2 -B 2 # Find all files containing floater_t rg --files-with-matches "floater_t" # Find template instantiations ast-grep --pattern 'floater_t<$_>'Length of output: 69286
tests/core/data/gridlayout/gridlayout_deriv.cpp (1)
2-2
: Good modernization of PI constant usage!The switch from
<math.h>
to<numbers>
and usingstd::numbers::pi_v
is a good C++ modernization. The_PI_
constant definition withfloater_t<4>
aligns well with the type system overhaul.Also applies to: 8-8, 10-10
tests/amr/tagging/test_tagging.cpp (1)
47-55
: LGTM!The function correctly uses
floater_t<4>
for type consistency and properly handles floating-point literals withf
suffix.tests/core/numerics/ion_updater/test_updater.cpp (2)
Line range hint
577-593
: LGTM!The function correctly uses
floater_t<4>
for type consistency and properly handles floating-point literals withf
suffix.
Line range hint
627-647
: LGTM!The function correctly uses
floater_t<4>
for type consistency and properly handles floating-point literals withf
suffix.tests/core/numerics/pusher/test_pusher.cpp (3)
Line range hint
129-173
: LGTM!The class correctly uses type aliases and floating-point types for consistency.
189-194
: LGTM!The test cases correctly handle type conversions and floating-point calculations.
Also applies to: 217-220, 241-242
Line range hint
260-309
: LGTM!The class correctly uses type aliases and floating-point types for consistency, including proper handling of random distributions.
tests/core/numerics/interpolator/test_main.cpp (3)
84-87
: LGTM! Type system enhancement for numerical precision.The change from
double
tofloater_t<4>
for weight-related arrays improves type safety and precision control.
101-112
: Good addition of precision-aware comparisons.The test now correctly handles different floating-point types by adjusting the comparison threshold based on the precision (1e-10 for double vs 1e-6 for float).
689-690
: Verify the precision requirements for particle attributes.The code uses different precision levels:
floater_t<4>
for layout dimensionsfloater_t<0>
for particle deltaPlease confirm if these precision levels are intentionally different and meet the numerical requirements for particle physics calculations.
Run this script to check precision usage patterns:
Also applies to: 706-706
✅ Verification successful
Precision levels are intentionally differentiated for numerical stability
The different precision levels are part of the design pattern consistently used throughout the codebase:
floater_t<4>
for grid-level quantities (layout dimensions, fields)floater_t<0>
for particle sub-grid positions (deltas)floater_t<1,2,3>
for other particle attributes (velocities, charges, weights)This separation helps maintain numerical stability while optimizing memory usage for particle-in-cell calculations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other floater_t usage patterns in particle-related code ast-grep --pattern 'floater_t<$_> $$$'Length of output: 40
Script:
#!/bin/bash # Search for floater_t template usage with proper C++ syntax ast-grep --pattern 'floater_t<$num>' # Fallback to ripgrep for a broader search echo "=== Searching for floater_t definitions and usage ===" rg "floater_t" -t cpp --no-heading # Look for any precision-related documentation echo "=== Searching for precision-related documentation ===" rg "precision|floater_t" -g "*.md" -g "*.h" -g "*.hpp" --no-headingLength of output: 62170
tests/amr/data/field/coarsening/test_linear_coarsen.hpp (3)
4-5
: Appropriate inclusion of"phare_core.hpp"
The inclusion of
"phare_core.hpp"
consolidates necessary headers, improving code maintenance and readability.
11-13
: Removal of redundant includesCommenting out the individual grid headers avoids redundancy since they are likely included within
"phare_core.hpp"
. This streamlines the code and reduces potential conflicts.
93-93
: 🛠️ Refactor suggestionAdjust
absError
for floating-point precisionWith
Real_t
defined asfloater_t<4>
, which may befloat
, the currentabsError
of1.e-8
might be too strict due tofloat
precision limits. Consider adjustingabsError
to accommodatefloat
precision.Apply this change:
- static constexpr double absError = 1.e-8; + static constexpr double absError = std::is_same_v<Real_t, float> ? 1.e-5 : 1.e-8;Likely invalid or redundant comment.
tests/amr/resources_manager/test_resources_manager.hpp (3)
6-7
: Verify include reordering for dependency issuesReordering includes by moving
#include <memory>
below#include "phare_core.hpp"
might introduce dependency problems if"phare_core.hpp"
relies on<memory>
. Ensure that all dependencies are correctly resolved and that this change does not cause compilation issues.Also applies to: 23-24
35-38
: Consistent type definitions usingPHARE_Types
Defining
Grid_t
andGridLayout_t
usingPHARE_Types
enhances type consistency throughout the codebase, which is beneficial for maintainability and clarity.
40-40
: VerifyResourcesManager
template parameter updatesUpdating
ResourcesManager
to useGridLayout_t
andGrid_t
fromPHARE_Types
changes its template parameters. Confirm that these changes are compatible with existing code and do not introduce any unintended side effects.src/core/data/grid/gridlayout.hpp (4)
112-114
: Consider the impact of changing constructor parameter typesChanging the constructor parameters from
double
tofloater_t<4>
may affect code that instantiatesGridLayout
objects. Ensure that all constructor calls are updated accordingly, and consider providing overloads or type conversions if backward compatibility is desired.
158-158
: Specify return types explicitly in public member functionsUsing
auto
as a return type in public member functions can reduce API clarity and hinder code expressiveness. It's recommended to explicitly specify return types to improve readability and maintainability.Apply this diff to specify the return types explicitly:
-NO_DISCARD auto origin() const noexcept { return origin_; } +NO_DISCARD Point<floater_t<4>, dimension> origin() const noexcept { return origin_; } -NO_DISCARD auto const& meshSize() const noexcept { return meshSize_; } +NO_DISCARD std::array<floater_t<4>, dimension> const& meshSize() const noexcept { return meshSize_; } -NO_DISCARD auto inverseMeshSize(Direction direction) const noexcept +NO_DISCARD floater_t<4> inverseMeshSize(Direction direction) const noexcept -NO_DISCARD auto inverseMeshSize() const noexcept { return inverseMeshSize_; } +NO_DISCARD std::array<floater_t<4>, dimension> const& inverseMeshSize() const noexcept { return inverseMeshSize_; }Also applies to: 165-165, 169-169, 176-176
511-511
: Specify return type explicitly in public member functionUsing
auto
as a return type in a public member function can reduce API clarity. It's recommended to explicitly specify the return type to improve readability and maintainability.Apply this diff to specify the return type explicitly:
-NO_DISCARD auto cellCenteredCoordinates(Indexes... index) const +NO_DISCARD Point<floater_t<4>, dimension> cellCenteredCoordinates(Indexes... index) const
464-464
: 🛠️ Refactor suggestionUpdate parameter type to match
floater_t<4>
usageThe parameter
origin
is still usingPoint<double, dimension>
while the rest of the code has shifted toPoint<floater_t<4>, dimension>
. To maintain consistency and prevent potential type-related issues, consider changing the parameter type toPoint<floater_t<4>, dimension> const&
.Apply this diff to update the parameter type:
-NO_DISCARD auto fieldNodeCoordinates(Field_t const& field, - Point<double, dimension> const& origin, +NO_DISCARD auto fieldNodeCoordinates(Field_t const& field, + Point<floater_t<4>, dimension> const& origin, Indexes... index) constLikely invalid or redundant comment.
tests/core/data/gridlayout/test_gridlayout.hpp (1)
18-21
: Ensure consistency with updatedfloater_t<4>
typesThe constructor now uses
floater_t<4>
types, aligning with the changes in theGridLayout
base class. Verify that all instances ofTestGridLayout
are updated accordingly to prevent any type mismatches.src/core/numerics/ohm/ohm.hpp (2)
25-26
: LGTM: Type system update for resistivity parameters.The change from
double
tofloater_t<4>
foreta_
andnu_
is consistent with the PR's objective of introducing a configurable precision type system.Also applies to: 60-61
363-375
: Good: Added type safety checks and consistent floating-point types.The changes improve type safety by:
- Using 'f' suffix for floating-point literals
- Adding static type assertion for the return value
- Consistently using floater_t<4> for variables
tests/core/numerics/ohm/test_main.cpp (2)
71-71
: Fix type compatibility issues with Field and TensorField.The change to use
floater_t<4>
inGrid_t
causes compilation errors due to type mismatches with dependent types.
105-112
: LGTM: Consistent floating-point literal usage.Changed floating-point literals to use 'f' suffix, maintaining consistency with the new type system.
src/core/numerics/interpolator/interpolator.hpp (3)
60-68
: LGTM: Enhanced type flexibility in Weighter class.Good improvements:
- Template parameter T allows flexible numeric types
- Consistent use of floater_t<0> for constants
- Proper static casting for index calculations
Also applies to: 80-97, 110-138
185-186
: LGTM: Consistent field interpolation precision.Changed field interpolation variables to floater_t<4>, maintaining consistent precision throughout the interpolation process.
Also applies to: 220-224, 263-269
404-404
: Verify if dual_offset is needed.The variable is marked as [[maybe_unused]] but static analysis indicates it's never used. Consider removing it if it's not needed for future use.
src/diagnostic/diagnostic_model_view.hpp (1)
40-41
: Verify precision requirements for mixed float/double usage.The change to
std::vector<float>
aligns with the codebase's transition to lower precision types, but the mixed use of float and double vectors warrants attention:
- Consider documenting the rationale for using different precisions
- Verify that float precision is sufficient for the stored data
- Be cautious of potential precision loss in operations between these vectors
Run this script to analyze the usage context:
src/core/numerics/pusher/boris.hpp (1)
Line range hint
131-144
: Verify precision compatibility between floater_t typesThe method uses
floater_t<0>
for internal calculations while the class methods usefloater_t<4>
. This mixing of precision levels could lead to unexpected behavior.Please verify that:
- The precision conversion between
floater_t<4>
andfloater_t<0>
is intentional- There are no precision loss issues in critical calculations
Run this script to analyze precision usage:
✅ Verification successful
Precision usage between floater_t types is intentional and correct
The mixing of precision levels is a deliberate design choice where:
- floater_t<0> is used for sub-grid particle operations
- floater_t<4> is used for field quantities and physical parameters
This is a common pattern in particle-in-cell codes and does not introduce precision issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for mixed precision usage across the codebase rg "floater_t<0>|floater_t<4>" --type cppLength of output: 35529
src/amr/data/field/field_data_factory.hpp (2)
38-38
: LGTM: Base class initialization is correct.The initialization properly sets up the ghost cell dimensions using SAMRAI's dimension system.
119-121
: LGTM: Memory calculation type changes are consistent.The change to
core::floater_t<4>
maintains type consistency throughout the memory allocation calculations.Verify the TODO comment about memory calculation assumptions.
The comment suggests potential improvements to memory calculations. Consider verifying if these assumptions still hold with the new type system.
tests/amr/data/field/geometry/test_field_geometry.cpp (2)
19-19
: Include added to support configurable precision types.The addition of
core/def.hpp
provides access to thefloater_t
type definition, which is essential for the precision control changes introduced in this PR.
36-36
: Verify test coverage with new floating-point precision.The Grid1D type now uses
floater_t<4>
for precision control. While the change aligns with the PR's objective, we should ensure that:
- The tests remain valid with the new precision type
- The comparison with SAMRAI's geometry implementations is not affected by potential floating-point differences
Let's verify the test coverage and potential precision-related issues:
✅ Verification successful
Precision change in Grid1D is properly covered by tests
The change to
floater_t<4>
in Grid1D is safe because:
- The precision type is only used in 1D geometry tests
- Test coverage includes multiple orders (1,2,3)
- SAMRAI geometry comparisons are not affected as they don't handle Yee layouts in higher dimensions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other precision-sensitive tests and potential floating-point comparisons # Look for floating-point comparisons in tests rg -A 2 "EXPECT_.*float|EXPECT_.*double" tests/ # Check for other uses of floater_t to ensure consistent precision ast-grep --pattern 'floater_t<$_>' tests/Length of output: 93
Script:
#!/bin/bash # Search for test assertions and floating-point operations rg -t cpp "EXPECT_|ASSERT_" tests/ rg -t cpp "Grid1D" tests/ # Look for numerical operations and comparisons rg -t cpp "float|double|floater_t" tests/ # Check for SAMRAI related comparisons rg -t cpp "SAMRAI" tests/Length of output: 94744
Script:
#!/bin/bash # Search for Grid1D usage in geometry tests rg -t cpp "Grid1D" tests/amr/data/field/geometry/ # Look for floater_t precision tests in geometry context rg -t cpp "floater_t" tests/amr/data/field/geometry/ # Check for SAMRAI geometry comparisons rg -t cpp -A 3 "SAMRAI.*geometry|geometry.*SAMRAI" tests/Length of output: 1177
src/python3/cpp_simulator.cpp (1)
22-24
:⚠️ Potential issueAddress commented-out patch data declarations.
The patch data declarations have been commented out without providing alternative functionality. This could lead to missing features in the Python bindings. Either:
- Remove the commented code if it's no longer needed, or
- Implement the new declarations using
floater_t<4>
if the functionality is still required.Run this script to verify the impact:
✅ Verification successful
Remove unused PatchDataVectorDouble declarations
These Python binding declarations appear to be unused legacy code. No references to these types were found in the Python codebase or tests. You can safely remove these commented-out lines as they are not being used and appear to be remnants of an older implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if these Python bindings are used in tests or examples rg -l "PatchDataVectorDouble_[123]D"Length of output: 68
Script:
#!/bin/bash # Search for Python files using these types fd -e py -x rg -l "PatchDataVectorDouble_[123]D" {} # Check for potential alternative implementations or newer versions rg -l "PatchData.*Vector.*Double" --type cpp --type hpp # Look for any documentation or migration notes fd -e md -e rst -x rg -l "PatchDataVector" {}Length of output: 189
Script:
#!/bin/bash # Search for any PatchData implementations in C++ files rg -l "PatchData" --type cpp # Look for Vector-related implementations rg -l "std::vector.*floater_t" --type cpp # Check the specific file for context around the commented code rg -C 5 "PatchDataVectorDouble" src/python3/cpp_simulator.cppLength of output: 2235
src/python3/cpp_simulator.hpp (1)
24-26
: Remove or restore commented header includes.These header files appear to be required for the commented-out functionality below. Either:
- Remove these includes if the functionality is permanently removed, or
- Restore them if you plan to reimplement the functionality with
floater_t
.✅ Verification successful
Remove the commented header includes
The commented-out headers are part of the old implementation that has been commented out throughout the file. Since you're transitioning to
floater_t
and the old implementation is no longer in use, these includes should be removed to maintain clean code.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if these headers are used elsewhere rg -l "patch_data\.hpp|patch_level\.hpp|data_wrangler\.hpp"Length of output: 121
Script:
#!/bin/bash # Check the content of data_wrangler.hpp cat src/python3/data_wrangler.hppLength of output: 6856
@@ -17,7 +17,7 @@ | |||
|
|||
using namespace PHARE::core; | |||
|
|||
std::vector<double> read(std::string filename); | |||
auto read(std::string filename); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider improving the function declaration.
- Pass string by const reference to avoid unnecessary copying
- Add documentation to clarify the function's purpose and return type
- Consider moving the function into a namespace to prevent global namespace pollution
Apply this diff to improve the function declaration:
-auto read(std::string filename);
+/**
+ * Reads data from the specified file.
+ * @param filename Path to the input file
+ * @return std::vector of values (type matches PHARE::core::floater_t<4>)
+ */
+namespace PHARE::core::testing {
+auto read(std::string const& filename);
+}
Committable suggestion skipped: line range outside the PR's diff.
// todo | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implement the missing float32 test cases.
The float32 test cases are marked with TODO comments. These should be implemented to ensure proper validation of float32 precision calculations.
Apply this pattern for both By and Bz components:
else
{
- // todo
+ EXPECT_THAT(Bynew(ix, iy), ::testing::FloatNear((expected_dbydt[index_]), 1e-5));
}
Also applies to: 425-426
if constexpr (std::is_same_v<floater_t<4>, double>) | ||
{ | ||
EXPECT_THAT(Bxnew(ix, iy, iz), | ||
::testing::DoubleNear((expected_dbxdt[index_]), 1e-12)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add float32 precision test cases for 3D calculations.
The 3D test case only handles double precision. Add float32 test cases to maintain consistency with 1D and 2D implementations.
Apply this pattern for all components (Bx, By, Bz):
if constexpr (std::is_same_v<floater_t<4>, double>)
{
EXPECT_THAT(Bxnew(ix, iy, iz),
::testing::DoubleNear((expected_dbxdt[index_]), 1e-12));
+}
+else
+{
+ EXPECT_THAT(Bxnew(ix, iy, iz),
+ ::testing::FloatNear((expected_dbxdt[index_]), 1e-5));
}
Also applies to: 593-597, 611-615
if constexpr (std::is_same_v<floater_t<4>, double>) | ||
{ | ||
EXPECT_THAT(localDerivative, ::testing::DoubleNear(expDerValue[ix], 1e-12)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Test coverage is indeed missing for non-double floater_t types
The codebase only tests the derivative calculations with double precision (floater_t<4> = double), using a precision of 1e-12. Consider:
- Adding test cases for other floating-point types (e.g., float)
- Using appropriate precision values for each type (e.g., 1e-6 for float)
- Using type-specific test matchers or templated test helpers
🔗 Analysis chain
Verify test coverage for non-double floater_t cases.
The tests are currently only executed when floater_t<4>
is double
. This means we're missing test coverage for other floating-point types.
Consider adding test cases for other floating-point precisions or verifying that the existing test data is valid for different precisions. Run this script to check test coverage:
Also applies to: 95-98, 147-150, 188-191, 229-232, 269-272, 331-334, 383-386, 436-439, 488-491, 540-543, 592-595
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any tests for non-double floater_t cases
# Search for test cases that don't depend on floater_t being double
rg "EXPECT_THAT.*localDerivative" -B 5 -A 5
# Search for any existing test coverage for different floater_t types
ast-grep --pattern 'if constexpr (!std::is_same_v<floater_t<$_>, double>)'
Length of output: 11263
if constexpr (std::is_same_v<floater_t<4>, double>) | ||
{ | ||
EXPECT_THAT(localLaplacian, ::testing::DoubleNear(expLapValue[ix], 1e-12)); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add missing 'else' branches for non-double types.
The test cases for LaplacianJy1D
and LaplacianJz1D
are missing the else
branch for handling non-double types, unlike LaplacianJx1D
. This could lead to missing test coverage for floater_t<4>
when it's not a double.
Apply this diff to add the missing branches:
if constexpr (std::is_same_v<floater_t<4>, double>)
{
EXPECT_THAT(localLaplacian, ::testing::DoubleNear(expLapValue[ix], 1e-12));
}
+ else
+ {
+ EXPECT_THAT(localLaplacian, ::testing::FloatNear(expLapValue[ix], 1e-6));
+ }
Also applies to: 107-111
std::array<floater_t<0>, dim> halfDtOverDl_; | ||
double dt_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Ensure type consistency for time-related variables
The member variables use inconsistent types:
halfDtOverDl_
usesfloater_t<0>
dt_
remainsdouble
Consider updating for consistency:
std::array<floater_t<0>, dim> halfDtOverDl_;
- double dt_;
+ floater_t<0> dt_;
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
std::array<floater_t<0>, dim> halfDtOverDl_; | |
double dt_; | |
std::array<floater_t<0>, dim> halfDtOverDl_; | |
floater_t<0> dt_; |
auto Byavg = 0.2f * (By(ix - 2) + By(ix - 1) + By(ix) + By(ix + 1) + By(ix + 2)); | ||
auto Bzavg = 0.2f * (Bz(ix - 2) + Bz(ix - 1) + Bz(ix) + Bz(ix + 1) + Bz(ix + 2)); | ||
auto Byavgp1 = 0.2f * (By(ix - 1) + By(ix) + By(ix + 1) + By(ix + 2) + By(ix + 3)); | ||
auto Bzavgp1 = 0.2f * (Bz(ix - 1) + Bz(ix) + Bz(ix + 1) + Bz(ix + 2) + Bz(ix + 3)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Revert float literals to maintain consistent precision
The use of 0.2f
is inconsistent with the codebase's precision model. The magnetic field calculations should maintain double precision as:
- The field type uses
floater_t<4>
as its base type - Field averaging operations explicitly use double precision
- Reducing precision to float could affect numerical stability
🔗 Analysis chain
Verify precision requirements for magnetic field averaging.
The change from 0.2
to 0.2f
reduces precision from double to float. Please verify that:
- This precision reduction aligns with the requirements for magnetic field calculations.
- The variables
Byavg
,Bzavg
, etc. are explicitly typed to match the intended precision.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other magnetic field calculations to verify consistent precision
rg --type cpp "By\(|Bz\(" src/
Length of output: 3358
Script:
#!/bin/bash
# Search for field type definitions and declarations
rg --type cpp -A 3 "class.*Field|struct.*Field|Field\s+[A-Z][a-z]|Field<" src/
Length of output: 14615
|
||
if constexpr (std::is_same_v<floater_t<0>, double>) | ||
{ | ||
EXPECT_THAT(this->destData.domainParticles[0].v, Pointwise(DoubleEq(), this->particle.v)); | ||
EXPECT_THAT(this->destData.domainParticles[0].iCell, Eq(this->particle.iCell)); | ||
EXPECT_THAT(this->destData.domainParticles[0].delta, | ||
Pointwise(DoubleEq(), this->particle.delta)); | ||
|
||
|
||
EXPECT_THAT(this->destData.domainParticles[0].weight, DoubleEq(this->particle.weight)); | ||
EXPECT_THAT(this->destData.domainParticles[0].charge, DoubleEq(this->particle.charge)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
The review comment correctly identifies a gap in test coverage
The test file only verifies particle data copying for double-precision floating-point values, leaving other potential floater_t<N>
types untested. The suggested addition of equivalent assertions for non-double types would improve test coverage and ensure correct behavior across all supported floating-point types.
🔗 Analysis chain
Ensure comprehensive testing for non-double floating-point types.
The test assertions are now conditionally executed only when floater_t<0>
is double
. This creates a potential gap in test coverage for other floating-point types that might be used in the system.
Consider adding equivalent assertions for non-double types using appropriate comparison functions. For example:
if constexpr (std::is_same_v<floater_t<0>, double>)
{
EXPECT_THAT(this->destData.domainParticles[0].v, Pointwise(DoubleEq(), this->particle.v));
// ... other double-specific assertions ...
}
+else
+{
+ // Add appropriate assertions for float32 or other types
+ EXPECT_THAT(this->destData.domainParticles[0].v, Pointwise(FloatEq(), this->particle.v));
+ EXPECT_THAT(this->destData.domainParticles[0].iCell, Eq(this->particle.iCell));
+ // ... other type-specific assertions ...
+}
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# First, let's look at the complete test file
cat tests/amr/data/particles/copy/test_particledata_copyNd.cpp
# Then search for floater_t definition and usage
rg "floater_t" -t cpp -A 2 -B 2
# Look for similar test patterns with if constexpr and floating-point comparisons
rg "if\s+constexpr.*float" -t cpp
Length of output: 65668
if constexpr (std::is_same_v<floater_t<0>, double>) | ||
{ | ||
EXPECT_THAT(this->destData.patchGhostParticles[0].v, | ||
Pointwise(DoubleEq(), this->particle.v)); | ||
EXPECT_THAT(this->destData.patchGhostParticles[0].iCell, Eq(this->particle.iCell)); | ||
EXPECT_THAT(this->destData.patchGhostParticles[0].delta, | ||
Pointwise(DoubleEq(), this->particle.delta)); | ||
EXPECT_THAT(this->destData.patchGhostParticles[0].weight, DoubleEq(this->particle.weight)); | ||
EXPECT_THAT(this->destData.patchGhostParticles[0].charge, DoubleEq(this->particle.charge)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Add test coverage for ghost particles with non-double types
The review comment correctly identifies a gap in test coverage. The test file only verifies ghost particle attributes for double types, leaving non-double types untested. This is evident in both the original file and related test files:
- Current tests use
if constexpr (std::is_same_v<floater_t<0>, double>)
to conditionally test only double types - Similar pattern exists in domain particle tests
- Related test file (test_particledata_copy_periodicNd.cpp) shows that particle attributes should be verified regardless of type
The suggested test helper template would be a good solution to ensure consistent verification across all supported types and improve test maintainability.
🔗 Analysis chain
Add test coverage for ghost particles with non-double types.
Similar to the domain particles, the ghost particle assertions are also conditionally executed only for double
types, leaving potential gaps in test coverage.
Consider adding a test helper template that can handle different floating-point types:
template<typename T>
void verifyParticleAttributes(const Particle<T>& actual, const Particle<T>& expected) {
if constexpr (std::is_same_v<T, double>) {
EXPECT_THAT(actual.v, Pointwise(DoubleEq(), expected.v));
// ... other double-specific assertions ...
} else {
EXPECT_THAT(actual.v, Pointwise(FloatEq(), expected.v));
// ... other type-specific assertions ...
}
}
This would make the tests more maintainable and ensure consistent verification across different types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for Particle template usage and floating-point type definitions
rg -g '*.{cpp,hpp,h}' "Particle\s*<\s*\w+\s*>" -B 2 -A 2
# Search for ghost particle related test files
fd "test.*ghost.*particle.*\.(cpp|hpp|h)$"
# Search for floater_t usage to understand type variations
rg -g '*.{cpp,hpp,h}' "floater_t\s*<" -B 2 -A 2
# Look for test patterns with FloatEq
rg -g '*test*.{cpp,hpp,h}' "FloatEq\(\)" -B 2 -A 2
Length of output: 78307
Script:
#!/bin/bash
# Search for particle test helpers and similar patterns
rg -g 'test_*.{cpp,hpp,h}' "void\s+verify.*Particle" -B 2 -A 2
# Look for other particle test files that might have type-specific testing patterns
fd "test.*particle.*\.(cpp|hpp|h)$"
# Search for FloatEq usage in particle tests
rg -g 'test_*particle*.{cpp,hpp,h}' "FloatEq\(\)" -B 2 -A 2
Length of output: 656
Script:
#!/bin/bash
# Check the content of related test files
echo "=== test_particledata_copy_periodicNd.cpp ==="
cat tests/amr/data/particles/copy_overlap/test_particledata_copy_periodicNd.cpp
echo -e "\n=== test_particles_fixtures.hpp ==="
cat tests/core/data/particles/test_particles_fixtures.hpp
# Search for type-specific test patterns in these files
echo -e "\n=== Searching for type-specific patterns ==="
rg -g 'test_particledata_copy*.cpp' "is_same.*double|is_same.*float" -B 2 -A 2
Length of output: 8048
// using Sim = Simulator<dim, interp, nbRefinedPart>; | ||
|
||
std::string name; | ||
|
||
// using DW = DataWrangler<dim, interp, nbRefinedPart>; | ||
// py::class_<DW, std::shared_ptr<DW>>(m, name.c_str()) | ||
// .def(py::init<std::shared_ptr<Sim> const&, std::shared_ptr<amr::Hierarchy> const&>()) | ||
// .def(py::init<std::shared_ptr<ISimulator> const&, std::shared_ptr<amr::Hierarchy> | ||
// const&>()) .def("sync_merge", &DW::sync_merge) .def("getPatchLevel", &DW::getPatchLevel) | ||
// .def("getNumberOfLevels", &DW::getNumberOfLevels); | ||
|
||
// using PL = PatchLevel<dim, interp, nbRefinedPart>; | ||
// name = "PatchLevel_" + type_string; | ||
|
||
// py::class_<PL, std::shared_ptr<PL>>(m, name.c_str()) | ||
// .def("getEM", &PL::getEM) | ||
// .def("getE", &PL::getE) | ||
// .def("getB", &PL::getB) | ||
// .def("getBx", &PL::getBx) | ||
// .def("getBy", &PL::getBy) | ||
// .def("getBz", &PL::getBz) | ||
// .def("getEx", &PL::getEx) | ||
// .def("getEy", &PL::getEy) | ||
// .def("getEz", &PL::getEz) | ||
// .def("getVix", &PL::getVix) | ||
// .def("getViy", &PL::getViy) | ||
// .def("getViz", &PL::getViz) | ||
// .def("getDensity", &PL::getDensity) | ||
// .def("getBulkVelocity", &PL::getBulkVelocity) | ||
// .def("getPopDensities", &PL::getPopDensities) | ||
// .def("getPopFluxes", &PL::getPopFlux) | ||
// .def("getFx", &PL::getFx) | ||
// .def("getFy", &PL::getFy) | ||
// .def("getFz", &PL::getFz) | ||
// .def("getParticles", &PL::getParticles, py::arg("userPopName") = "all"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Critical Python bindings have been removed without replacement
The verification confirms that the commented-out DataWrangler and PatchLevel bindings are not defined elsewhere in the codebase. This is concerning because:
- These methods are actively used across multiple Python and C++ files
- No alternative bindings or replacement functionality is provided
- The removal affects core data access functionality (EM fields, densities, particle data)
🔗 Analysis chain
Critical functionality has been commented out.
A significant portion of the data access layer has been commented out, including:
- DataWrangler initialization and synchronization
- PatchLevel access to EM fields, densities, and particle data
- Essential getters for physical quantities
This could break dependent code that relies on these Python bindings.
Run this script to check for dependencies:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of the commented out methods
rg -l "getEM|getE|getB|getDensity|getBulkVelocity|getParticles"
Length of output: 1951
Script:
#!/bin/bash
# Check for DataWrangler and PatchLevel bindings in other files
ast-grep --pattern 'py::class_<DataWrangler'
ast-grep --pattern 'py::class_<PatchLevel'
# Check recent changes in related files
fd -e hpp -e cpp -e py | xargs git log -n 1 --format="%h %ad %s" --
Length of output: 91
Script:
#!/bin/bash
# Search for Python module initialization and potential alternative bindings
rg "PYBIND11_MODULE" -A 5
rg "class_<" -A 5 src/python3/
# Check recent changes in cpp_simulator.hpp
git log -p src/python3/cpp_simulator.hpp
Length of output: 6102
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Summary by CodeRabbit
Based on the comprehensive changes across multiple files, here are the updated release notes:
New Features
floater_t
for enhanced precision control.Type System Improvements
double
with customfloater_t
types across particle and numeric components.Library Updates
BlueBrain/HighFive
tohighfive-devs/highfive
.master
tomain
.Code Refactoring
Testing
These changes represent a significant architectural update focusing on type system flexibility and precision management.