-
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
3d C++ / python #591
base: master
Are you sure you want to change the base?
3d C++ / python #591
Conversation
4bcae8e
to
84dd60a
Compare
eb507bb
to
c963ec0
Compare
7bb1041
to
263ecd6
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.
some comments and questions.
pyphare/pyphare/core/gridlayout.py
Outdated
@@ -366,6 +366,20 @@ def yeeCoords(self, knode, iStart, centering, direction, ds, origin, derivOrder) | |||
|
|||
return x | |||
|
|||
def meshCoords(self, qty): |
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.
seems this function is not used anywhere
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.
cause it's be obsoleted by FieldData::meshgrid
@@ -556,11 +556,44 @@ def plot2d(self, **kwargs): | |||
|
|||
return fig, ax | |||
|
|||
def plot3d(self, **kwargs): | |||
"""!HAX!""" |
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.
yeah.... not sure exactly this function should stay there. If I understand, it's basically hacking the 3D hierarchy into a 2D one that is the slice at the middle Z index.
I think this should not be called plot3d. Rather, we should do allow for making a slice in a n-dimensional hierarchy at a certain index and then use the normal plot2d.
But this is not as simple. Already, making a 1D slice in a 2D hierarchy is complicated since fine indexes are not aligned with coarse one. So making a 2D slice in 3D has the same issue.
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.
I was using this just to check the 3d run wasn't total nonsense
# -DhighResourceTests=ON | ||
option(highResourceTests "Enable heavy tests for CI (3d/etc" OFF) | ||
|
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.
I agree with the rabbit, no?
src/core/utilities/box/box.hpp
Outdated
@@ -146,6 +147,26 @@ struct Box | |||
else | |||
return 6; | |||
} | |||
|
|||
auto as_points() const |
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.
I don't see this function used anywhere
@@ -0,0 +1,51 @@ | |||
CartesianGridGeometry |
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.
this input files seem to be used nowhere. So is the test_particledata_refine_basic_hierarchy... why
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.
I added this to be consistent, but I'm not sure the other versions are even used
|
||
TYPED_TEST_P(ACollectionOfParticles_3d, DepositCorrectlyTheirWeight_3d) | ||
{ | ||
// auto const& [vx, vy, vz] = this->v(); |
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.
I tried to uncomment these and they fail I guess this is why they are commented out :)
tests/simulator/__init__.py
Outdated
@@ -308,3 +313,33 @@ def clean_up_diags_dirs(self): | |||
if os.path.exists(diag_dir): | |||
shutil.rmtree(diag_dir) | |||
cpp_lib().mpi_barrier() | |||
|
|||
|
|||
def debug_tracer(): |
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.
this does not seem to be used anywhere
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.
I was using it for debug things, you only use it when you need it
self._test_overlaped_fields_are_equal(datahier, time_step_nbr, time_step) | ||
|
||
# @data( | ||
# *per_interp(({"L0": {"B0": Box3D(10, 14)}})), |
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.
TODO
f"\n{self._testMethodName}_{ndim}d took {self.datetime_diff(now)} seconds" | ||
) | ||
|
||
# @data({"cells": 40, "smallest_patch_size": 20, "largest_patch_size": 20, "nbr_part_per_cell" : ppc}) |
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.
TODO
splitter_t = splitter_type(dim, interp, refined_particle_nbr) | ||
np.testing.assert_allclose(yaml_delta, splitter_t.delta) | ||
np.testing.assert_allclose(yaml_weight, splitter_t.weight) | ||
return dim2 |
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.
I don't see if dim ==3 although self._do_dim(3, This.PRIOR_MIN_DIFF_3d, This.PRIOR_MAX_DIFF_3d)
is called
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.
I think the very next line is what you're looking for ;)
res/cmake/funcs.cmake
Outdated
@@ -0,0 +1,192 @@ | |||
# public test functions |
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.
I do not understand this file. It seems to be a copy of what's already in def.cmake
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.
I think the idea was to remove all the functions in "def.cmake" cause that file was getting a little hectic
|
||
if constexpr (dimension == 1) | ||
{ | ||
// in 1D div(B) is automatically satisfied so using this coarsening |
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.
TODO: keep comments
|
||
// X = 0 = primal | ||
// Y = 1 = dual | ||
std::array<A0_arr, 2> const fns{ |
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.
this function is not coarsening correctly the magnetic field.
Magnetic components are on faces. in 3D coarse B compoennts should always take 4 fine cells divided by 1/4. There are only 6 faces and nothing is pure dual.
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.
I tried to extrapolate the 2d function without really knowing what I was doing (how could I?)
669224a
to
2af0e65
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: 5
♻️ Duplicate comments (5)
tests/core/numerics/interpolator/test_main.cpp (1)
741-741
: Fix the grid layout initialization for 3D.The grid layout initialization is missing the
nz
dimension in the mesh size array.Apply this diff to fix the grid layout initialization:
- GridLayout_t layout{ConstArray<double, dim>(.1), {nx, ny}, ConstArray<double, dim>(0)}; + GridLayout_t layout{ConstArray<double, dim>(.1), {nx, ny, nz}, ConstArray<double, dim>(0)};📝 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.GridLayout_t layout{ConstArray<double, dim>(.1), {nx, ny, nz}, ConstArray<double, dim>(0)};
tests/functional/harris/harris_3d.py (2)
29-31
: Fix potential floating-point to integer conversion issue.The variable
nt
is calculated as a float but used innp.arange()
which expects an integer.- dt = 100 * time_step - nt = final_time / dt + 1 - timestamps = dt * np.arange(nt) + dt = 100 * time_step + nt = int(final_time / dt) + 1 + timestamps = dt * np.arange(nt)📝 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.dt = 100 * time_step nt = int(final_time / dt) + 1 timestamps = dt * np.arange(nt)
106-108
: Replace assertion with proper error handling.Using assertions for runtime checks is not recommended as they can be disabled with optimization flags.
- temp = 1.0 / density(x, y, z) * (K - b2(x, y, z) * 0.5) - assert np.all(temp > 0) - return temp + temp = 1.0 / density(x, y, z) * (K - b2(x, y, z) * 0.5) + if not np.all(temp > 0): + raise ValueError("Temperature must be positive") + return temp📝 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.temp = 1.0 / density(x, y, z) * (K - b2(x, y, z) * 0.5) if not np.all(temp > 0): raise ValueError("Temperature must be positive") return temp
tests/amr/data/field/refine/test_refine_field.py (1)
105-107
: Replace placeholder with proper 3D electric field refinement implementation.The current placeholder needs to be replaced with a proper implementation for 3D fields. Based on the existing 1D and 2D implementations, here's the pattern to follow:
- Handle each dimension separately based on the field's primal directions
- For each dimension:
- Handle even indices where fine edges are collocated with coarse edges
- Handle odd indices where fine edges fall between coarse edges
- Use proper interpolation weights based on the field's primal directions
🧰 Tools
🪛 Ruff (0.8.2)
106-106: Do not
assert False
(python -O
removes these calls), raiseAssertionError()
Replace
assert False
(B011)
tests/simulator/test_initialization.py (1)
392-398
: Fix scope issues in inner function.The inner
domain
function uses variables from the outer scope (select
,patch
,nbrGhosts
,dim
) which can lead to bugs and makes the code harder to maintain.Apply this diff:
-def domain(patch_data): +def domain(patch_data, select, patch_box, nbrGhosts, dim): if dim == 1: return patch_data.dataset[select] return patch_data.dataset[:].reshape( - patch.box.shape + (nbrGhosts * 2) + 1 + patch_box.shape + (nbrGhosts * 2) + 1 )[select]🧰 Tools
🪛 Ruff (0.8.2)
394-394: Function definition does not bind loop variable
select
(B023)
396-396: Function definition does not bind loop variable
patch
(B023)
396-396: Function definition does not bind loop variable
nbrGhosts
(B023)
397-397: Function definition does not bind loop variable
select
(B023)
🧹 Nitpick comments (16)
src/amr/data/particles/refine/split_2d.hpp (2)
1-4
: Enhance documentation with brief overview and examples.While the GitHub wiki reference is helpful, consider adding:
- A brief overview of the splitting patterns
- Example usage
- Description of the pattern naming convention (Purple, Brown, Pink, Black)
86-87
: Document the significance of delta and weight values.The numerical values for
delta
andweight
arrays appear to be carefully chosen, but their significance and derivation are not documented. Consider adding:
- Mathematical explanation for these values
- References to papers or documentation explaining their derivation
- Impact of these values on the splitting patterns
Also applies to: 105-106, 124-125, 144-145, 162-163, 181-182, 200-201, 220-221, 241-242, 259-260, 278-279, 297-298, 317-318, 340-342
src/core/utilities/meta/meta_utilities.hpp (2)
83-98
: Document the relationship between dimensions and particle counts.The particle counts (e.g., 6, 12 for 3D) appear to be carefully chosen values, but their significance isn't clear. Consider:
- Adding comments explaining why these specific numbers were chosen
- Documenting how these values relate to the interpolation order
- Using named constants to make the relationship between dimension and particle count more explicit
Example:
// Constants to clarify particle counts per dimension constexpr size_t MIN_3D_PARTICLES = 6; // Minimum particles needed for 3D stability constexpr size_t OPT_3D_PARTICLES = 12; // Optimal particle count for 3D accuracy
90-91
: Track TODOs in the issue system.The TODO comments and commented-out value (
27
) indicate incomplete implementation of 3D particle permutations. Consider:
- Creating issues to track the remaining 3D particle permutations
- Documenting the criteria for "uncommon cases" that need compile-time activation
- Adding a link to the tracking issue in the code comments
Would you like me to help create an issue to track these TODOs?
Also applies to: 94-94
tests/simulator/refined_particle_nbr.py (2)
44-47
: Consider improving readability with better variable naming and documentation.While the logic is correct, consider these improvements:
- Rename
dim2
to something more descriptive likeborder_cells_2d
- Add a docstring explaining the purpose and parameters
def _less_per_dim(self, dim, refined_particle_nbr, patch): + """Calculate the number of particles to remove from border cells. + + Args: + dim: dimension (1, 2, or 3) + refined_particle_nbr: number of particles per cell + patch: patch object containing upper and lower bounds + + Returns: + Number of particles to remove from border cells + """ if dim == 1: return refined_particle_nbr * 2 cellNbr = patch.upper - patch.lower + 1 - dim2 = refined_particle_nbr * ((cellNbr[0] * 2 + (cellNbr[1] * 2))) + border_cells_2d = refined_particle_nbr * ((cellNbr[0] * 2 + (cellNbr[1] * 2))) if dim == 2: - return dim2 - return dim2 * (cellNbr[2] * 2) + return border_cells_2d + return border_cells_2d * (cellNbr[2] * 2)
117-135
: Add error handling for YAML key access.The validation logic is good, but consider adding error handling for YAML key access to provide better error messages when configuration is missing or invalid.
def _check_deltas_and_weights(self, dim, interp, refined_particle_nbr): + try: yaml_dim = self.yaml_root["dimension_" + str(dim)] yaml_interp = yaml_dim["interp_" + str(interp)] yaml_n_particles = yaml_interp["N_particles_" + str(refined_particle_nbr)] yaml_delta = [float(s) for s in str(yaml_n_particles["delta"]).split(" ")] yaml_weight = [float(s) for s in str(yaml_n_particles["weight"]).split(" ")] + except KeyError as e: + raise KeyError(f"Missing configuration for dim={dim}, interp={interp}, " \ + f"refined_particle_nbr={refined_particle_nbr}: {str(e)}") + except ValueError as e: + raise ValueError(f"Invalid delta/weight values in configuration: {str(e)}")tests/core/numerics/interpolator/test_main.cpp (1)
752-756
: Consider extracting the weight calculation into a named function.The lambda for weight calculation could be moved to a named function for better readability and reusability.
Apply this diff to extract the weight calculation:
+ static double calculateWeight(const auto& meshSize) { + return std::accumulate(meshSize.begin(), meshSize.end(), 1.0, + std::multiplies<double>()); + } + ACollectionOfParticles_3d() : particles{grow(layout.AMRBox(), safeLayer)} , rho{"field", HybridQuantity::Scalar::rho, nx, ny, nz} , v{"v", layout, HybridQuantity::Vector::V} { - double weight = [](auto const& meshSize) { - return std::accumulate(meshSize.begin(), meshSize.end(), 1.0, - std::multiplies<double>()); - }(layout.meshSize()); + double weight = calculateWeight(layout.meshSize());tests/diagnostic/test-diagnostics_3d.cpp (1)
30-35
: Add error handling in main function.The main function should handle potential exceptions from SAMRAI initialization and finalization.
int main(int argc, char** argv) { + int testResult = 1; // Default to error ::testing::InitGoogleTest(&argc, argv); - - SAMRAI::tbox::SAMRAI_MPI::init(&argc, &argv); - SAMRAI::tbox::SAMRAIManager::initialize(); - SAMRAI::tbox::SAMRAIManager::startup(); - - int testResult = RUN_ALL_TESTS(); + try { + SAMRAI::tbox::SAMRAI_MPI::init(&argc, &argv); + SAMRAI::tbox::SAMRAIManager::initialize(); + SAMRAI::tbox::SAMRAIManager::startup(); + + testResult = RUN_ALL_TESTS(); + } + catch (const std::exception& e) { + std::cerr << "Error: " << e.what() << std::endl; + }tests/simulator/initialize/test_fields_init_3d.py (1)
40-45
: Optimize memory usage in density decrease test.The test is marked as using too much RAM. Consider:
- Reducing the number of test points or using a logarithmic scale.
- Adding memory cleanup between test iterations.
- Using generators instead of arrays where possible.
def test_density_decreases_as_1overSqrtN(self, interp_order): print(f"\n{self._testMethodName}_{ndim}d") + # Use fewer test points with logarithmic scale + test_points = np.logspace(1, 2, 4, dtype=int) # [10, 18, 32, 56] self._test_density_decreases_as_1overSqrtN( - ndim, interp_order, np.asarray([10, 25, 50, 75]), cells=cells + ndim, interp_order, test_points, cells=cells )tests/amr/data/particles/copy/test_particles_data_copy.cpp (1)
56-68
: Add test cases for edge conditions.The current test only covers basic copying functionality. Consider adding tests for:
- Empty source data
- Maximum particle count
- Invalid cell indices
- Overlapping domains
- Error conditions
TYPED_TEST(AParticlesDataND, copy_empty_source) { this->destData.copy(this->sourceData); ASSERT_THAT(this->destData.domainParticles.size(), Eq(0)); } TYPED_TEST(AParticlesDataND, copy_invalid_cell) { static constexpr auto dim = TypeParam{}(); this->particle.iCell = ConstArray<int, dim>(-1); // Invalid cell this->sourceData.patchGhostParticles.push_back(this->particle); EXPECT_THROW(this->destData.copy(this->sourceData), std::out_of_range); }tests/simulator/initialize/test_particles_init_3d.py (1)
77-94
: Complete the implementation of commented-out test cases.The commented-out test cases for patch ghost handling should be implemented to ensure comprehensive testing coverage.
Would you like me to help implement these test cases or create an issue to track this task?
tests/functional/harris/harris_3d.py (2)
19-21
: Consider using named constants for grid parameters.The grid parameters are defined as magic numbers. Consider extracting these into named constants for better maintainability and clarity.
-cells = (40, 40, 40) -dl = (0.2, 0.2, 0.2) +GRID_CELLS = (40, 40, 40) # Number of cells in each dimension +GRID_SPACING = (0.2, 0.2, 0.2) # Grid spacing in each dimension + +cells = GRID_CELLS +dl = GRID_SPACING
178-184
: Uncomment and implement error handling for plotting tools.The commented code for plotting tools should be properly implemented with error handling.
Would you like me to help implement proper error handling for the plotting functionality?
pyphare/pyphare/core/box.py (1)
206-207
: Improve readability of theselect
function.The function uses ambiguous variable names and a complex list comprehension.
-def select(data, box): - return data[tuple([slice(l, u + 1) for l,u in zip(box.lower, box.upper)])] +def select(data, box): + """Select a subset of data using box dimensions. + + Args: + data: n-dimensional array-like object + box: Box instance defining the selection boundaries + + Returns: + Selected subset of data + """ + slices = [ + slice(lower, upper + 1) + for lower, upper in zip(box.lower, box.upper) + ] + return data[tuple(slices)]🧰 Tools
🪛 Ruff (0.8.2)
207-207: Ambiguous variable name:
l
(E741)
tests/amr/data/particles/stream_pack/test_no_ghost.cpp (1)
81-86
: Initialize all particle attributes in constructor.The particle initialization in the constructor is incomplete. Consider initializing all attributes including
iCell
anddelta
.AParticlesData() { particle.weight = 1.0; particle.charge = 1.0; particle.v = {1.0, 1.0, 1.0}; + particle.delta = {0.0, 0.0, 0.0}; + particle.iCell = {0, 0, 0}; }src/amr/data/particles/refine/split_3d.hpp (1)
25-40
: Optimize PinkPattern initialization.The constructor uses a loop for initialization which can be simplified.
constexpr PinkPattern(float const weight, float const delta) : Super{weight} - { - constexpr float zero = 0; - - for (std::size_t i = 0; i < 2; i++) - { - std::size_t offset = i * 3; - float sign = i % 2 ? -1 : 1; - auto mode = delta * sign; - - Super::deltas_[0 + offset] = {mode, zero, zero}; - Super::deltas_[1 + offset] = {zero, zero, mode}; - Super::deltas_[2 + offset] = {zero, mode, zero}; - } - } + { + Super::deltas_ = { + {delta, 0, 0}, {0, 0, delta}, {0, delta, 0}, + {-delta, 0, 0}, {0, 0, -delta}, {0, -delta, 0} + }; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
.github/workflows/cmake_macos.yml
(1 hunks)pyphare/pyphare/core/box.py
(2 hunks)pyphare/pyphare/pharesee/geometry.py
(7 hunks)pyphare/pyphare/pharesee/hierarchy/hierarchy.py
(1 hunks)pyphare/pyphare/pharesee/run/utils.py
(1 hunks)res/amr/splitting.yml
(1 hunks)res/cmake/options.cmake
(2 hunks)src/amr/data/field/coarsening/magnetic_field_coarsener.hpp
(1 hunks)src/amr/data/particles/refine/split.hpp
(1 hunks)src/amr/data/particles/refine/split_1d.hpp
(10 hunks)src/amr/data/particles/refine/split_2d.hpp
(17 hunks)src/amr/data/particles/refine/split_3d.hpp
(1 hunks)src/amr/data/particles/refine/splitter.hpp
(2 hunks)src/core/data/grid/gridlayoutdefs.hpp
(2 hunks)src/core/data/ndarray/ndarray_vector.hpp
(8 hunks)src/core/utilities/box/box.hpp
(1 hunks)src/core/utilities/meta/meta_utilities.hpp
(2 hunks)tests/amr/data/field/refine/test_refine_field.py
(1 hunks)tests/amr/data/particles/copy/test_particles_data_copy.cpp
(1 hunks)tests/amr/data/particles/refine/input/input_3d_ratio_2.txt
(1 hunks)tests/amr/data/particles/refine/test_split.cpp
(1 hunks)tests/amr/data/particles/stream_pack/test_no_ghost.cpp
(1 hunks)tests/core/data/ndarray/test_main.cpp
(1 hunks)tests/core/numerics/interpolator/test_main.cpp
(1 hunks)tests/diagnostic/CMakeLists.txt
(1 hunks)tests/diagnostic/job_3d.py.in
(1 hunks)tests/diagnostic/test-diagnostics_3d.cpp
(1 hunks)tests/functional/harris/harris_3d.py
(1 hunks)tests/simulator/__init__.py
(1 hunks)tests/simulator/advance/CMakeLists.txt
(1 hunks)tests/simulator/advance/test_fields_advance_3d.py
(1 hunks)tests/simulator/advance/test_particles_advance_3d.py
(1 hunks)tests/simulator/initialize/CMakeLists.txt
(1 hunks)tests/simulator/initialize/test_fields_init_2d.py
(1 hunks)tests/simulator/initialize/test_fields_init_3d.py
(1 hunks)tests/simulator/initialize/test_particles_init_1d.py
(0 hunks)tests/simulator/initialize/test_particles_init_2d.py
(1 hunks)tests/simulator/initialize/test_particles_init_3d.py
(1 hunks)tests/simulator/per_test.hpp
(1 hunks)tests/simulator/refined_particle_nbr.py
(2 hunks)tests/simulator/test_advance.py
(6 hunks)tests/simulator/test_initialization.py
(13 hunks)
💤 Files with no reviewable changes (1)
- tests/simulator/initialize/test_particles_init_1d.py
🚧 Files skipped from review as they are similar to previous changes (23)
- src/amr/data/particles/refine/split.hpp
- .github/workflows/cmake_macos.yml
- tests/simulator/advance/CMakeLists.txt
- tests/simulator/init.py
- tests/diagnostic/CMakeLists.txt
- tests/simulator/initialize/test_fields_init_2d.py
- res/cmake/options.cmake
- tests/simulator/initialize/test_particles_init_2d.py
- tests/core/data/ndarray/test_main.cpp
- tests/simulator/per_test.hpp
- tests/diagnostic/job_3d.py.in
- src/core/utilities/box/box.hpp
- tests/amr/data/particles/refine/test_split.cpp
- pyphare/pyphare/pharesee/run/utils.py
- src/amr/data/particles/refine/splitter.hpp
- src/core/data/grid/gridlayoutdefs.hpp
- tests/simulator/initialize/CMakeLists.txt
- tests/simulator/advance/test_fields_advance_3d.py
- tests/amr/data/particles/refine/input/input_3d_ratio_2.txt
- res/amr/splitting.yml
- src/amr/data/field/coarsening/magnetic_field_coarsener.hpp
- src/amr/data/particles/refine/split_1d.hpp
- tests/simulator/advance/test_particles_advance_3d.py
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative t...
**/*.hpp
: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/core/utilities/meta/meta_utilities.hpp
src/core/data/ndarray/ndarray_vector.hpp
src/amr/data/particles/refine/split_3d.hpp
src/amr/data/particles/refine/split_2d.hpp
🪛 Ruff (0.8.2)
pyphare/pyphare/core/box.py
207-207: Ambiguous variable name: l
(E741)
pyphare/pyphare/pharesee/geometry.py
161-161: Ambiguous variable name: l
(E741)
164-164: Ambiguous variable name: l
(E741)
167-167: Ambiguous variable name: l
(E741)
188-188: Ambiguous variable name: l
(E741)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py
567-567: Loop control variable lvl
not used within loop body
Rename unused lvl
to _lvl
(B007)
tests/amr/data/field/refine/test_refine_field.py
106-106: Do not assert False
(python -O
removes these calls), raise AssertionError()
Replace assert False
(B011)
tests/functional/harris/harris_3d.py
62-62: Ambiguous variable name: l
(E741)
tests/simulator/test_advance.py
89-89: Ambiguous variable name: l
(E741)
tests/simulator/test_initialization.py
394-394: Function definition does not bind loop variable select
(B023)
396-396: Function definition does not bind loop variable patch
(B023)
396-396: Function definition does not bind loop variable nbrGhosts
(B023)
397-397: Function definition does not bind loop variable select
(B023)
582-582: Loop control variable pi
not used within loop body
Rename unused pi
to _pi
(B007)
586-586: Yoda condition detected
Rewrite as ppc == H
(SIM300)
664-664: Local variable out
is assigned to but never used
Remove assignment to unused variable out
(F841)
🪛 Cppcheck (2.10-2)
tests/diagnostic/test-diagnostics_3d.cpp
[performance] 24-24: Function parameter 'path' should be passed by const reference.
(passedByValue)
⏰ 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 (18)
src/amr/data/particles/refine/split_2d.hpp (1)
21-70
: Well-designed pattern structures with compile-time optimizations.The pattern structures make good use of:
- Constexpr constructors for compile-time evaluation
- Template specialization for 2D cases
- Clear delta initialization patterns
src/core/utilities/meta/meta_utilities.hpp (1)
9-11
: Document the purpose and impact ofPHARE_SIMULATORS
.The macro's value determines which simulation dimensions are enabled during compilation. Please add documentation explaining:
- The meaning of each value (1=1D, 2=2D, 3=3D)
- The impact on compilation and runtime behavior
- The relationship with CMake configuration
tests/simulator/refined_particle_nbr.py (3)
33-37
: LGTM! Good practice for test cleanup.The tearDown method ensures proper cleanup of simulator resources even when tests fail, preventing resource leaks and test interference.
105-116
: LGTM! Well-documented 3D support.The 3D constants and test method are well-implemented with clear documentation showing the calculations for the min/max difference thresholds.
139-144
: LGTM! Good test organization.The TestSuite implementation ensures proper test execution order, with delta/weights verification running first. This is a good practice when tests have dependencies.
tests/core/numerics/interpolator/test_main.cpp (1)
788-788
: Remove unused static variable warning.The static analysis tool indicates that
gtest_testInterpolator_ACollectionOfParticles_3d
is unused. This is a false positive as it's used by the Google Test framework internally.tests/diagnostic/test-diagnostics_3d.cpp (1)
9-27
: Refactor repeated code in test cases.Each test case follows the same pattern. Consider refactoring to reduce code duplication.
🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 24-24: Function parameter 'path' should be passed by const reference.
(passedByValue)
tests/simulator/initialize/test_particles_init_3d.py (1)
34-38
: Remove extraneous parentheses in per_interp calls.The static analysis tool has flagged unnecessary parentheses around the dictionary in the
per_interp
function calls.Also applies to: 59-63
tests/functional/harris/harris_3d.py (1)
62-62
: Rename ambiguous variable 'l' to 'length'.The variable name 'l' is ambiguous and can be mistaken for the number '1'.
-def S(y, y0, l): +def S(y, y0, length):🧰 Tools
🪛 Ruff (0.8.2)
62-62: Ambiguous variable name:
l
(E741)
pyphare/pyphare/core/box.py (1)
185-198
: Add unit tests forremove_all
function.The
remove_all
function handles complex box removal operations but lacks comprehensive test coverage.Would you like me to help create unit tests covering various scenarios such as:
- Empty list of boxes to remove
- Single box removal
- Multiple overlapping boxes
- Non-overlapping boxes
src/amr/data/particles/refine/split_3d.hpp (1)
133-134
: Consider using higher precision for critical calculations.The delta and weight values are defined as float but might benefit from double precision for better accuracy.
Also applies to: 151-152, 172-173, 193-194, 211-212, 232-233, 252-253, 270-271, 291-292
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check for precision-related issues in the codebase # Search for float usage in critical calculations rg --type cpp 'float.*delta' rg --type cpp 'float.*weight' # Search for any existing precision-related issues or bugs rg --type cpp -i 'precision|accuracy'Length of output: 11291
Consider using double precision for critical particle computations
The code consistently defines delta and weight as arrays of floats (e.g. in split_3d.hpp and related files). If these values are used in calculations where rounding errors are critical, switching to double precision (and updating related function parameters) may help improve numerical accuracy. However, note that using float appears to be an intentional design choice across the particle refinement modules, so any change should be evaluated carefully for consistency and performance trade-offs.pyphare/pyphare/pharesee/geometry.py (1)
161-161
: Rename ambiguous variable names for clarity.The variable
l
is used in dictionary comprehensions but is not descriptive. Consider renaming it to a more meaningful name that indicates its purpose or contents.Apply this diff:
-f"{k}front": [(v[0], v[1], shape_z) for v in l] for k, l in shifts.items() +f"{k}front": [(v[0], v[1], shape_z) for v in values] for k, values in shifts.items() -f"{k}back": [([v[0], v[1], -shape_z]) for v in l] for k, l in shifts.items() +f"{k}back": [([v[0], v[1], -shape_z]) for v in values] for k, values in shifts.items() -shifts = {k: [([v[0], v[1], 0]) for v in l] for k, l in shifts.items()} +shifts = {k: [([v[0], v[1], 0]) for v in values] for k, values in shifts.items()} -shifts = {"".join(sorted(k)): l for k, l in shifts.items()} +shifts = {"".join(sorted(k)): values for k, values in shifts.items()}Also applies to: 164-164, 167-167, 188-188
🧰 Tools
🪛 Ruff (0.8.2)
161-161: Ambiguous variable name:
l
(E741)
src/core/data/ndarray/ndarray_vector.hpp (1)
392-428
: LGTM! The 3D array implementation is well-structured.The implementation correctly handles the filling of 3D arrays by:
- Properly handling array bounds for each dimension
- Correctly implementing the border filling logic for all six faces
- Using consistent indexing patterns across all dimensions
tests/simulator/test_initialization.py (2)
582-586
: Fix unused loop variable.The loop variable
pi
is not used in the loop body. Either use it or prefix it with an underscore to indicate it's intentionally unused.Apply this diff:
-for pi, patch in enumerate(datahier.level(0).patches): +for _pi, patch in enumerate(datahier.level(0).patches):🧰 Tools
🪛 Ruff (0.8.2)
582-582: Loop control variable
pi
not used within loop bodyRename unused
pi
to_pi
(B007)
586-586: Yoda condition detected
Rewrite as
ppc == H
(SIM300)
664-664
: Remove unused variable.The variable
out
is assigned but never used in the function.Apply this diff:
-out = "phare_outputs"
🧰 Tools
🪛 Ruff (0.8.2)
664-664: Local variable
out
is assigned to but never usedRemove assignment to unused variable
out
(F841)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
590-597
: LGTM!The changes to handle 3D plotting follow the existing pattern for 1D and 2D plotting.
tests/simulator/test_advance.py (2)
370-372
: LGTM!The addition of the
cells
parameter with a default value maintains backward compatibility while adding flexibility.
398-400
: LGTM!The changes improve test organization by:
- Adding the
cells
parameter with a sensible default- Using a structured path for diagnostic outputs
Also applies to: 413-421
// auto const& [vx, vy, vz] = this->v(); | ||
// EXPECT_DOUBLE_EQ(this->rho(7, 7, 7), 1.0); | ||
// EXPECT_DOUBLE_EQ(vx(7, 7, 7), 2.0); | ||
// EXPECT_DOUBLE_EQ(vy(7, 7, 7), -1.0); | ||
// EXPECT_DOUBLE_EQ(vz(7, 7, 7), 1.0); |
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.
💡 Verification agent
🧩 Analysis chain
Uncomment and fix the failing test case.
The test case is commented out due to failures. Based on the user's comment, this needs investigation and fixing rather than being left commented out.
Let's verify the test case implementation:
🏁 Script executed:
#!/bin/bash
# Description: Search for similar test cases in 1D and 2D to understand the expected behavior
# Search for test assertions in 1D and 2D test cases
rg -A 5 'EXPECT_DOUBLE_EQ.*rho.*1\.0' tests/core/numerics/interpolator/
Length of output: 1699
3D Test Case Implementation Requires Fixing
- While 1D and 2D tests in
tests/core/numerics/interpolator/test_main.cpp
use a computed index (e.g.,idx
) and are active and passing, the analogous 3D test remains commented out (using hard-coded indices(7, 7, 7)
). - This discrepancy suggests that the 3D test is known to be failing. It should be uncommented and updated—ideally to follow the same pattern as the other dimensions (e.g., using a dynamic index if appropriate) or ensuring that the expected values accurately reflect the 3D interpolator's behavior.
Please address the 3D case to bring it in line with the 1D/2D tests.
int main(int argc, char** argv) | ||
{ | ||
::testing::InitGoogleTest(&argc, argv); | ||
|
||
|
||
|
||
SAMRAI::tbox::SAMRAI_MPI::init(&argc, &argv); | ||
|
||
SAMRAI::tbox::SAMRAIManager::initialize(); | ||
|
||
SAMRAI::tbox::SAMRAIManager::startup(); | ||
|
||
|
||
int testResult = RUN_ALL_TESTS(); | ||
|
||
|
||
// Finalize | ||
|
||
SAMRAI::tbox::SAMRAIManager::shutdown(); | ||
|
||
SAMRAI::tbox::SAMRAIManager::finalize(); | ||
|
||
SAMRAI::tbox::SAMRAI_MPI::finalize(); | ||
|
||
return testResult; | ||
} |
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
Add error handling for MPI initialization.
The main function should handle potential MPI initialization failures.
int main(int argc, char** argv)
{
::testing::InitGoogleTest(&argc, argv);
-
-
-
SAMRAI::tbox::SAMRAI_MPI::init(&argc, &argv);
+ if (SAMRAI::tbox::SAMRAI_MPI::getErrorCode() != 0) {
+ std::cerr << "Failed to initialize MPI" << std::endl;
+ return 1;
+ }
SAMRAI::tbox::SAMRAIManager::initialize();
SAMRAI::tbox::SAMRAIManager::startup();
-
int testResult = RUN_ALL_TESTS();
-
// Finalize
-
SAMRAI::tbox::SAMRAIManager::shutdown();
SAMRAI::tbox::SAMRAIManager::finalize();
SAMRAI::tbox::SAMRAI_MPI::finalize();
return testResult;
}
📝 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.
int main(int argc, char** argv) | |
{ | |
::testing::InitGoogleTest(&argc, argv); | |
SAMRAI::tbox::SAMRAI_MPI::init(&argc, &argv); | |
SAMRAI::tbox::SAMRAIManager::initialize(); | |
SAMRAI::tbox::SAMRAIManager::startup(); | |
int testResult = RUN_ALL_TESTS(); | |
// Finalize | |
SAMRAI::tbox::SAMRAIManager::shutdown(); | |
SAMRAI::tbox::SAMRAIManager::finalize(); | |
SAMRAI::tbox::SAMRAI_MPI::finalize(); | |
return testResult; | |
} | |
int main(int argc, char** argv) | |
{ | |
::testing::InitGoogleTest(&argc, argv); | |
SAMRAI::tbox::SAMRAI_MPI::init(&argc, &argv); | |
if (SAMRAI::tbox::SAMRAI_MPI::getErrorCode() != 0) { | |
std::cerr << "Failed to initialize MPI" << std::endl; | |
return 1; | |
} | |
SAMRAI::tbox::SAMRAIManager::initialize(); | |
SAMRAI::tbox::SAMRAIManager::startup(); | |
int testResult = RUN_ALL_TESTS(); | |
// Finalize | |
SAMRAI::tbox::SAMRAIManager::shutdown(); | |
SAMRAI::tbox::SAMRAIManager::finalize(); | |
SAMRAI::tbox::SAMRAI_MPI::finalize(); | |
return testResult; | |
} |
def plot3d(self, **kwargs): | ||
"""!HAX!""" | ||
time = kwargs.get("time", self._default_time()) |
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
Improve documentation and remove "!HAX!" comment.
The method lacks proper documentation. Replace the "!HAX!" comment with a proper docstring explaining the method's purpose, parameters, and behavior.
- def plot3d(self, **kwargs):
- """!HAX!"""
+ def plot3d(self, **kwargs):
+ """Plot 3D data by taking a 2D slice at the middle Z index.
+
+ Args:
+ **kwargs: Keyword arguments passed to plot2d.
+
+ Returns:
+ The result of plot2d.
+
+ Note:
+ This is a temporary solution that modifies the input data in-place.
+ TODO: Implement proper 3D visualization or slicing without data modification.
+ """
📝 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.
def plot3d(self, **kwargs): | |
"""!HAX!""" | |
time = kwargs.get("time", self._default_time()) | |
def plot3d(self, **kwargs): | |
"""Plot 3D data by taking a 2D slice at the middle Z index. | |
Args: | |
**kwargs: Keyword arguments passed to plot2d. | |
Returns: | |
The result of plot2d. | |
Note: | |
This is a temporary solution that modifies the input data in-place. | |
TODO: Implement proper 3D visualization or slicing without data modification. | |
""" | |
time = kwargs.get("time", self._default_time()) |
pdat = patch.patch_datas[qty] | ||
primals = pdat.primal_directions() | ||
if primals[0]: | ||
pdat._x = pdat.x[:-1] | ||
if primals[1]: | ||
pdat._y = pdat.y[:-1] | ||
pdat.dataset = pdat.dataset[:, :, int(pdat.ghost_box.shape[2] / 2)] | ||
patch.box.lower = patch.box.lower[:-1] | ||
patch.box.upper = patch.box.upper[:-1] | ||
patch.box.ndim = 2 | ||
|
||
pdat.ghost_box.lower = pdat.ghost_box.lower[:-1] | ||
pdat.ghost_box.upper = pdat.ghost_box.upper[:-1] | ||
pdat.ghost_box.ndim = 2 | ||
pdat.size = np.copy(pdat.ghost_box.shape) | ||
pdat.layout.dl = pdat.layout.dl[:-1] | ||
|
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.
Avoid in-place data modification and hardcoded slice selection.
The method modifies the input data in-place and uses a hardcoded middle Z index for slicing. This can lead to side effects and limits flexibility.
Consider creating a new method that:
- Creates a copy of the data
- Allows configurable slice selection
- Preserves the original data
- pdat = patch.patch_datas[qty]
- primals = pdat.primal_directions()
- if primals[0]:
- pdat._x = pdat.x[:-1]
- if primals[1]:
- pdat._y = pdat.y[:-1]
- pdat.dataset = pdat.dataset[:, :, int(pdat.ghost_box.shape[2] / 2)]
- patch.box.lower = patch.box.lower[:-1]
- patch.box.upper = patch.box.upper[:-1]
- patch.box.ndim = 2
-
- pdat.ghost_box.lower = pdat.ghost_box.lower[:-1]
- pdat.ghost_box.upper = pdat.ghost_box.upper[:-1]
- pdat.ghost_box.ndim = 2
- pdat.size = np.copy(pdat.ghost_box.shape)
- pdat.layout.dl = pdat.layout.dl[:-1]
+ pdat = patch.patch_datas[qty]
+ slice_data = self._create_2d_slice(pdat, kwargs.get('z_index', None))
+ patch_2d = self._create_2d_patch(patch, slice_data)
Would you like me to generate the implementation for the helper methods _create_2d_slice
and _create_2d_patch
?
Committable suggestion skipped: line range outside the PR's diff.
def S(x, x0, l): | ||
return 0.5 * (1 + np.tanh((x - x0) / l)) | ||
|
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
Improve function documentation and parameter naming.
The function uses an ambiguous parameter name 'l' and lacks documentation.
-def S(x, x0, l):
- return 0.5 * (1 + np.tanh((x - x0) / l))
+def S(x, x0, length):
+ """Compute a smooth transition using hyperbolic tangent.
+
+ Args:
+ x: Input value.
+ x0: Center point of the transition.
+ length: Length scale of the transition.
+
+ Returns:
+ float: Smooth transition value between 0 and 1.
+ """
+ return 0.5 * (1 + np.tanh((x - x0) / length))
📝 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.
def S(x, x0, l): | |
return 0.5 * (1 + np.tanh((x - x0) / l)) | |
def S(x, x0, length): | |
"""Compute a smooth transition using hyperbolic tangent. | |
Args: | |
x: Input value. | |
x0: Center point of the transition. | |
length: Length scale of the transition. | |
Returns: | |
float: Smooth transition value between 0 and 1. | |
""" | |
return 0.5 * (1 + np.tanh((x - x0) / length)) |
🧰 Tools
🪛 Ruff (0.8.2)
89-89: Ambiguous variable name: l
(E741)
mostly there, some coarsening tests missing
only some 3d split patterns, the rest can come afterwards
Summary by CodeRabbit
New Features
Enhancements
Tests
CI & Configuration