Update to 3D #960
Update to 3D #960
Conversation
📝 WalkthroughWalkthroughThe changes span core build configurations, library functionality, AMR and particle refinement, and test infrastructure. Updates include modifications to CMake workflows, additions of new methods and classes to support 3D operations (e.g., mesh coordinate generation, ghost box handling, and dispatcher-to-pattern renaming), and enhanced error handling. Several new configuration files and test suites have been added to validate 3D simulations, while existing tests have been updated (e.g., removal of skip decorators). Minor documentation and formatting improvements are also introduced throughout the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Caller
participant Dispatcher as PatternDispatcher
participant Pattern as Pattern Object
Client->>Dispatcher: Invoke operator() with coarse particle info
Dispatcher->>Dispatcher: Call dispatch_ method
loop For each pattern
Dispatcher->>Pattern: Retrieve weight and delta values
Pattern-->>Dispatcher: Return pattern details
end
Dispatcher-->>Client: Return refined particle attributes
sequenceDiagram
participant User
participant PH as PatchHierarchy
User->>PH: Call plot()
alt ndim equals 3
PH-->>User: Raise NotImplementedError("cannot make 3D plots")
else
PH-->>User: Proceed with plotting operations
end
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 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
|
|
on top of #591 |
| @@ -0,0 +1,48 @@ | |||
| from tools.python3 import decode_bytes, run | |||
There was a problem hiding this comment.
this file doesn't exist on my branch anymore
https://github.com/PhilipDeegan/PHARE/tree/3d_base/tools/python3
There was a problem hiding this comment.
ok good to know, I think I will use meld to see the diff more easily.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
tools/python3/cmake.py (1)
47-49:⚠️ Potential issueFix undefined variable in run_test function.
The
run_testfunction uses an undefined variablecmd. This will cause a runtime error.Apply this diff to fix the issue:
-def run_test(test, verbose=False, capture_output=False): - run(test_cmd(cmd, verbose=verbose), capture_output=capture_output) +def run_test(test, verbose=False, capture_output=False): + run(test_cmd(test, verbose=verbose), capture_output=capture_output)🧰 Tools
🪛 Ruff (0.8.2)
48-48: Undefined name
cmd(F821)
🧹 Nitpick comments (34)
src/amr/data/particles/refine/split_3d.hpp (3)
43-63: Leverage descriptive function names or inline comments for clarity.In
PurplePattern<DimConst<3>>, each constructor loop sets corner-based offsets for the deltas. Consider more explicit naming for variables or comments to describe how each coordinate is derived. This helps maintainers quickly grasp the connection betweeni % 2 ? -1 : 1and corner offsets, especially in 3D splits.
90-114: Check class vs. struct consistency for pattern types.
WhitePattern<DimConst<3>>is declared as a class, whereas other patterns (PinkPattern,PurplePattern,LimePattern) are declared as structs. Functionally, this is not an error because everything is public, but for consistency and readability, you might consider usingstructhere or explaining whyclassis intended.
164-174: Consider consolidating repeated pattern dispatchers.
Splitter<DimConst<3>, InterpConst<1>, RefinedParticlesConst<27>>references four pattern types (BlackPattern,PinkPattern,PurplePattern,LimePattern). If multipleSplitterspecializations frequently combine the same patterns, you could introduce a shared alias or central definition to reduce duplication and ease maintenance if new patterns or logic are introduced later.src/amr/data/field/coarsening/magnetic_field_coarsener.hpp (1)
226-333: Keep 3D coarsening logic maintainable.The 3D coarsen method uses multiple nested conditionals to handle primal/dual layout combinations for each B-component. While comprehensive, it can become a maintenance burden if new layouts or partial-edge cases appear. Extracting these computations into well-named helper functions or a small lookup table can aid clarity and reduce future duplication across the 1D/2D/3D variants.
tests/amr/data/field/coarsening/coarsening_utils.py (2)
25-37: Enhance variable naming for self-documentation.In lines 25-37, the usage of
is_primaland simple helper functions (coarse_indices,fineLocal,coarseLocal) is clear, but variable names likeindex,indexX, andindexYcould be more descriptive. For instance, consider naming themcoarseX,fineX, etc., to highlight the difference between coarse and fine indexing.
181-182: Flatten nested conditionals per static analysis (SIM102).Ruff suggests merging these nested checks:
elif not is_primal[0] and is_primal[1] and not is_primal[2]: if qty == "By": ...into a single condition:
- elif not is_primal[0] and is_primal[1] and not is_primal[2]: - if qty == "By": + elif (not is_primal[0] + and is_primal[1] + and not is_primal[2] + and qty == "By"):This eliminates a layer of nesting and aligns with
SIM102.🧰 Tools
🪛 Ruff (0.8.2)
181-182: Use a single
ifstatement instead of nestedifstatements(SIM102)
pyphare/pyphare/pharesee/geometry.py (5)
63-74: Improve clarity for naming 3D faces.
These lines add left, right, bottom, top, front, and back boxes for 3D domains. It might be helpful to add comments explaining which physical dimension corresponds to "front" and "back" to avoid confusion when reading or extending the code.
87-88: Clarify the re-initialization ofshifts.
Re-initializingshiftsto an empty dictionary for all dimensions is fine, but consider adding a brief comment describing why it's reset here for better maintainability.
100-102: Check dimension branching logic.
Explicitly handlingndim == 3and then groupingndim > 1in the subsequent block can be confusing if future expansions occur. Consider restructuring or adding comments to differentiate 2D vs. 3D handling more distinctly.
150-150: Consider naming consistency for combined shift keys.
The new key"leftrightbottomtop"may be difficult to parse. A more structured or descriptive naming strategy (e.g.,"composite_shifts_lr_bt") might be clearer for maintainers.
161-180: Rename loop variables for readability.
The static analysis hints mention ambiguous variable names (e.g.,l). Variable naming such as(key, shiftList)instead of(k, l)helps readability. Also, ensure that the newly addedfront,back, and combined shift logic is well-documented to avoid confusion for new contributors.🧰 Tools
🪛 Ruff (0.8.2)
161-161: Ambiguous variable name:
l(E741)
164-164: Ambiguous variable name:
l(E741)
167-167: Ambiguous variable name:
l(E741)
src/core/data/ndarray/ndarray_vector.hpp (2)
386-425: Evaluate performance in triple-nested fill3D loops.
This expanded logic for 3D borders is straightforward but potentially costly. If performance becomes critical, consider batched or SIMD approaches. Otherwise, the clarity of straightforward nested loops is acceptable.
529-595: Comprehensive 3D copy logic inoperator>>.
The new block handles 3D corner and edge copying. The approach is thorough but quite extensive. Consider extracting it into a helper function for readability and easier testing.tests/diagnostic/job_3d.py.in (1)
3-14: Validate 3D diagnostic workflow.
This new script sets up a 3D domain, defines an isothermal electron model, and callsdump_all_diags. It's a solid start for 3D diagnostic tests. Consider adding assertions or checks in the script to confirm that the output data is generated as expected, boosting test reliability.tools/python3/git.py (1)
19-24: Remove unused exception variable.The function logic is correct, but the exception variable is unused.
Apply this diff to fix the issue:
def branch_exists(branch): try: run(f"git show-branch {branch}", check=True) - except subprocess.CalledProcessError as e: + except subprocess.CalledProcessError: return False # exit failure means branch does not exist return True🧰 Tools
🪛 Ruff (0.8.2)
22-22: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
tools/python3/cmake.py (1)
4-5: Implement the version() function.The
version()function is currently empty. Consider implementing it to return the CMake version for compatibility checks.Would you like me to help implement the version function?
tests/simulator/initialize/test_fields_init_3d.py (1)
40-45: Address high RAM usage in density test.The comment indicates that this test uses excessive RAM. Consider:
- Reducing the test matrix size
- Adding memory usage checks
- Making it an optional heavy test
You could add memory usage checks using this script:
#!/usr/bin/env python3 import psutil import numpy as np def estimate_memory(cells, ppc_array): # Estimate memory usage for 3D grid with particles max_ppc = np.max(ppc_array) grid_points = cells ** 3 particles = grid_points * max_ppc memory_bytes = particles * (8 * 6) # 6 doubles per particle (position + velocity) return memory_bytes / (1024 * 1024) # Convert to MB cells = 20 ppc_array = np.asarray([10, 25, 50, 75]) estimated_mb = estimate_memory(cells, ppc_array) print(f"Estimated memory usage: {estimated_mb:.2f} MB")src/core/data/grid/gridlayoutdefs.hpp (1)
5-5: LGTM! Enhanced type safety with fixed-width integer.The changes improve type safety and ABI stability by:
- Including the necessary header for fixed-width integers
- Specifying a fixed-width underlying type for the enum class
Consider adding static assertions to validate the enum's size requirements:
enum class QtyCentering : std::uint16_t { primal = 0, dual = 1 }; +static_assert(sizeof(QtyCentering) == sizeof(std::uint16_t), "QtyCentering must be 16 bits");Also applies to: 18-18
tests/simulator/initialize/test_particles_init_3d.py (1)
77-84: Implement the missing patch ghost particle tests.The TODO3D comment indicates that patch ghost particle tests need to be implemented for 3D scenarios.
Would you like me to help implement these tests or create an issue to track this task?
tests/simulator/per_test.hpp (1)
111-117: Consider expanding 3D test parameters for better coverage.The 3D test parameters are currently limited to 3 configurations with a fixed
nbRefinePart=6, while 1D and 2D tests have more varied configurations. Consider adding more test cases with differentnbRefinePartvalues for better coverage.Example expansion:
using Simulator3d = testing::Types<SimulatorTestParam<3, 1, 6>, SimulatorTestParam<3, 2, 6>, - SimulatorTestParam<3, 3, 6>>; + SimulatorTestParam<3, 3, 6>, + SimulatorTestParam<3, 1, 8>, SimulatorTestParam<3, 2, 8>, + SimulatorTestParam<3, 3, 8>>;tests/simulator/advance/test_fields_advance_3d.py (1)
18-18: Consider increasing particles per cell for better accuracy.The
ppcvalue is set to 10, which is lower than the 2D tests (25). Consider increasing this value for better statistical accuracy in particle-based calculations.-ppc, cells = 10, 30 +ppc, cells = 25, 30 # Match 2D test parameters for consistencysrc/core/utilities/meta/meta_utilities.hpp (1)
90-97: Consider completing the TODO for 3D particle permutations.The TODO comment indicates that additional 3D particle permutations need to be added.
Would you like me to help implement the remaining 3D particle permutations or create an issue to track this task?
src/amr/data/particles/refine/splitter.hpp (1)
145-163: Consider implementing empty pattern structs.The pattern structs are currently empty. They should be implemented to define their specific splitting behaviors.
Would you like me to help implement these pattern structs or create an issue to track this task?
tests/functional/harris/harris_3d.py (3)
62-63: Rename ambiguous variablelfor better readability.The single-letter variable name
lis ambiguous and could be confused with the number 1 or uppercase I.-def S(y, y0, l): - return 0.5 * (1.0 + np.tanh((y - y0) / l)) +def S(y, y0, length): + return 0.5 * (1.0 + np.tanh((y - y0) / length))🧰 Tools
🪛 Ruff (0.8.2)
62-62: Ambiguous variable name:
l(E741)
177-188: Address TODO comments regarding 3D plotting.The plotting functionality needs to be reimplemented after the removal of hierarchy plot3d.
Would you like me to help implement an alternative 3D plotting solution or create an issue to track this task?
106-108: Consider adding error handling for temperature calculation.The temperature calculation uses an assertion to check for positive values, but it might be better to handle this case more gracefully.
- 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 calculation resulted in non-positive values") + return temptests/simulator/refined_particle_nbr.py (1)
140-145: Consider adding test suite ordering documentation.The test suite execution order is enforced but not documented. Consider adding a comment explaining why test_values must run first.
- # the following ensures the order of execution so delta/weights are verified first + # test_values must run first to verify delta/weights before dimension-specific tests + # because it validates the core splitting parameters used by all other testspyphare/pyphare/core/box.py (1)
206-207: Use more descriptive variable names.The variable name 'l' is ambiguous and could be confused with the number '1'.
-def select(data, box): - return data[tuple([slice(l, u + 1) for l,u in zip(box.lower, box.upper)])] +def select(data, box): + return data[tuple([slice(lower, upper + 1) for lower, upper in zip(box.lower, box.upper)])]🧰 Tools
🪛 Ruff (0.8.2)
207-207: Ambiguous variable name:
l(E741)
res/cmake/funcs.cmake (1)
107-108: Consider documenting empty function purpose.The empty add_phare_build_test function lacks documentation explaining its purpose or future use.
+ # Placeholder for future build-time test functionality function(add_phare_build_test binary directory) endfunction(add_phare_build_test)pyphare/pyphare/core/gridlayout.py (1)
369-382: Add docstring to themeshCoordsmethod.The method implementation is correct, but it lacks documentation explaining its purpose, parameters, and return values.
Add a docstring to improve code maintainability:
def meshCoords(self, qty): + """ + Compute mesh coordinates for a given quantity in 1D, 2D, or 3D. + + Args: + qty: The quantity for which to compute coordinates + + Returns: + For 1D: array of x coordinates + For 2D: array of shape (nx, ny, 2) containing (x,y) coordinates + For 3D: array of shape (nx, ny, nz, 3) containing (x,y,z) coordinates + """tests/core/numerics/interpolator/test_main.cpp (1)
722-787: Test suite for 3D particle weight deposition needs to be uncommented.The test case
DepositCorrectlyTheirWeight_3dis currently commented out. Consider implementing and enabling the test to ensure proper 3D functionality.Would you like me to help implement the test case? The implementation would be similar to the existing 1D and 2D test cases but adapted for 3D coordinates.
tests/simulator/test_initialization.py (1)
427-428: Consider adjusting particle numbers for better 3D performance.The number of particles for 3D (20) seems quite low compared to 1D (10000) and 2D (1000). This might affect the accuracy of the simulation.
Consider increasing the number of particles for 3D cases if performance allows:
- nbParts = {1: 10000, 2: 1000, 3: 20} + nbParts = {1: 10000, 2: 1000, 3: 100} # Increased 3D particlestests/simulator/test_advance.py (1)
290-290: Consider renaming unused loop variable.The loop control variable
ilvlis not used within the loop body.- for ilvl, overlaps in hierarchy_overlaps(datahier, coarsest_time).items(): + for _ilvl, overlaps in hierarchy_overlaps(datahier, coarsest_time).items():🧰 Tools
🪛 Ruff (0.8.2)
290-290: Loop control variable
ilvlnot used within loop bodyRename unused
ilvlto_ilvl(B007)
res/amr/splitting.yml (1)
102-142: Introducing 3D Particle Splitting ParametersThe new
dimension_3section is well structured and mirrors the formatting used in the existing dimensions. It clearly defines three interpolations (interp_1,interp_2, andinterp_3) with specificdeltaandweightvalues for each particle configuration.A few points to consider:
- YAML list representation: The
weightfields are specified as space-separated values (e.g.,.125 .0625). Ensure that the YAML parser or the custom configuration loader interprets these values correctly. If any ambiguity is encountered, consider using a standard YAML list format (e.g.,[.125, .0625]) for enhanced clarity and maintainability.- Value precision: Double-check that the differing precisions on
weight(for example,.099995 .055301ininterp_2versus.104047 .05564ininterp_3) are intentional and calibrated for your 3D simulation requirements.- Integration Validation: Please verify that all downstream modules (e.g., particle refinement in 3D, ghost field computations) are updated to correctly consume these new parameters and that relevant tests are in place to confirm the expected behavior.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (68)
.github/workflows/cmake_macos.yml(1 hunks)pyphare/pyphare/core/box.py(2 hunks)pyphare/pyphare/core/gridlayout.py(1 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/funcs.cmake(1 hunks)res/cmake/options.cmake(2 hunks)src/amr/data/field/coarsening/coarsen_weighter.hpp(1 hunks)src/amr/data/field/coarsening/magnetic_field_coarsener.hpp(1 hunks)src/amr/data/particles/refine/particles_data_split.hpp(2 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(5 hunks)src/core/data/grid/grid.hpp(1 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/coarsening/CMakeLists.txt(0 hunks)tests/amr/data/field/coarsening/coarsening_utils.py(1 hunks)tests/amr/data/field/coarsening/input/input_1d_ratio_10.txt(0 hunks)tests/amr/data/field/coarsening/input/input_1d_ratio_2.txt(0 hunks)tests/amr/data/field/coarsening/input/input_1d_ratio_3.txt(0 hunks)tests/amr/data/field/coarsening/input/input_1d_ratio_4.txt(0 hunks)tests/amr/data/field/coarsening/input/input_1d_ratio_6.txt(0 hunks)tests/amr/data/field/coarsening/input/input_1d_ratio_7.txt(0 hunks)tests/amr/data/field/coarsening/input/input_1d_ratio_8.txt(0 hunks)tests/amr/data/field/coarsening/input/input_1d_ratio_9.txt(0 hunks)tests/amr/data/field/coarsening/input_config.h.in(0 hunks)tests/amr/data/field/coarsening/test_coarsening_main.cpp(0 hunks)tests/amr/data/field/coarsening/test_linear_coarsen.hpp(0 hunks)tests/amr/data/field/coarsening/test_weighter_coarsen.hpp(1 hunks)tests/amr/data/field/refine/CMakeLists.txt(1 hunks)tests/amr/data/field/refine/test_default_field_refinement.cpp(4 hunks)tests/amr/data/field/refine/test_field_refinement_on_hierarchy.cpp(1 hunks)tests/amr/data/particles/refine/input/input_3d_ratio_2.txt(2 hunks)tests/amr/data/particles/refine/test_split.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/CMakeLists.txt(1 hunks)tests/simulator/__init__.py(3 hunks)tests/simulator/advance/CMakeLists.txt(1 hunks)tests/simulator/advance/test_fields_advance_1d.py(1 hunks)tests/simulator/advance/test_fields_advance_2d.py(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(15 hunks)tests/simulator/test_initialization.py(13 hunks)tests/simulator/utilities/field_coarsening.py(0 hunks)tests/simulator/utilities/test_refine_field.py(2 hunks)tools/python3/cmake.py(1 hunks)tools/python3/git.py(1 hunks)
💤 Files with no reviewable changes (14)
- tests/amr/data/field/coarsening/input_config.h.in
- tests/amr/data/field/coarsening/input/input_1d_ratio_4.txt
- tests/amr/data/field/coarsening/input/input_1d_ratio_3.txt
- tests/amr/data/field/coarsening/test_coarsening_main.cpp
- tests/amr/data/field/coarsening/input/input_1d_ratio_7.txt
- tests/amr/data/field/coarsening/input/input_1d_ratio_9.txt
- tests/simulator/initialize/test_particles_init_1d.py
- tests/amr/data/field/coarsening/input/input_1d_ratio_6.txt
- tests/amr/data/field/coarsening/input/input_1d_ratio_10.txt
- tests/amr/data/field/coarsening/input/input_1d_ratio_2.txt
- tests/amr/data/field/coarsening/CMakeLists.txt
- tests/amr/data/field/coarsening/input/input_1d_ratio_8.txt
- tests/simulator/utilities/field_coarsening.py
- tests/amr/data/field/coarsening/test_linear_coarsen.hpp
✅ Files skipped from review due to trivial changes (6)
- src/amr/data/particles/refine/split.hpp
- src/core/data/grid/grid.hpp
- src/amr/data/field/coarsening/coarsen_weighter.hpp
- src/core/utilities/box/box.hpp
- src/amr/data/particles/refine/particles_data_split.hpp
- src/amr/data/particles/refine/split_2d.hpp
🧰 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/data/grid/gridlayoutdefs.hppsrc/core/utilities/meta/meta_utilities.hpptests/simulator/per_test.hppsrc/core/data/ndarray/ndarray_vector.hpptests/amr/data/field/coarsening/test_weighter_coarsen.hppsrc/amr/data/particles/refine/split_3d.hppsrc/amr/data/particles/refine/splitter.hppsrc/amr/data/field/coarsening/magnetic_field_coarsener.hppsrc/amr/data/particles/refine/split_1d.hpp
🪛 Ruff (0.8.2)
tools/python3/git.py
22-22: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
29-29: Undefined name delete_branch
(F821)
tests/simulator/__init__.py
326-326: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
pyphare/pyphare/core/box.py
207-207: Ambiguous variable name: l
(E741)
tests/functional/harris/harris_3d.py
62-62: Ambiguous variable name: l
(E741)
tools/python3/cmake.py
48-48: Undefined name cmd
(F821)
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)
tests/simulator/test_advance.py
290-290: Loop control variable ilvl not used within loop body
Rename unused ilvl to _ilvl
(B007)
tests/amr/data/field/coarsening/coarsening_utils.py
181-182: Use a single if statement instead of nested if statements
(SIM102)
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: Analyze (cpp)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-13)
🔇 Additional comments (65)
src/amr/data/particles/refine/split_3d.hpp (1)
19-41: Ensure index usage remains within bounds.In
PinkPattern<DimConst<3>>, you are usingfor (std::size_t i = 0; i < 2; i++)and then calculatingstd::size_t offset = i * 3. SinceRefinedParticlesConst<6>makes exactly six elements, the usage of offsets[0..5]should be safe. If any future refactor changes the base class template parameters (e.g., if you introduce more or fewer refined particles), remember to revisit these loops to prevent out-of-range writes.src/amr/data/field/coarsening/magnetic_field_coarsener.hpp (2)
33-36: Clarify assumptions about Yee layout.Lines 33-36 explicitly warn about relying on Yee layout centering. Though this is correct for typical magnetic field allocations, consider providing a safeguard check or static assertion to confirm that
centering_matches the expected pattern. This ensures that future developers don’t mistakenly pass other layouts to this class.
65-79: Validate indexing infine_start_index().The function multiplies
coarseIndexbyratio_and then performs an AMR index transformation. Ensure that each dimension ofsourceBox_truly accommodates these multiplied indices before usage, especially for corner or boundary cells wherecoarseIndexmight push the resulting index out of range.pyphare/pyphare/pharesee/geometry.py (3)
59-60: Ensure domain boundaries for 2D dimensions.
These lines correctly add bottom and top Box definitions for 2D. Make sure that (0, 0) as a lower bound is intended for all 2D scenarios, as any negative or shifted domain might require a different approach.
184-188: Evaluate the dictionary key transformation.
The assertion ensures no duplication in sorted shift keys, and the subsequent line replaces keys with"".join(sorted(k)). While this is functionally clever, it can reduce clarity if keys become cryptic. Document this transformation or consider an alternative, more explicit approach.🧰 Tools
🪛 Ruff (0.8.2)
188-188: Ambiguous variable name:
l(E741)
290-290: Confirm sorted usage of patch border keys.
Constructing thein_sidesstring by sorting the patch border keys is consistent with the shift dictionary logic. However, watch out for subtle discrepancies if border naming conventions change in the future.src/core/data/ndarray/ndarray_vector.hpp (3)
123-125: Document new 3D boundary helpers.
The newly addedzstart()andzend()methods improve consistency with the x and y boundaries. Adding short docstrings can help future contributors understand how these methods interact with the 3D fill operations.
348-353: Ensure validity for 1D fill loops.
The loops correctly fill the left and right masks in a 1D array frommin_tomax_and fromshape[0] - 1 - max_toshape[0] - 1 - min_. Double-check thatmin_ <= max_is always guaranteed to avoid an empty or unintended range.
469-470: Double-check assertion coverage for boundary offsets.
These new asserts verifyinneris strictly withinouterin x and y dimensions. Confirm that the rest of the code cannot pass invalid or equal boundary values.Also applies to: 477-480
tests/diagnostic/job_3d.py.in (1)
1-2: Confirm environment for Python 3.
Using a shebang with/usr/bin/env python3is a best practice. Ensure that your CI or local environment consistently has Python 3 available under this path.tests/diagnostic/test-diagnostics_3d.cpp (3)
1-7: LGTM!The includes and static constants are well-defined and follow the naming convention.
9-27: LGTM!The test functions are well-organized, consistent, and properly typed. Each test has a clear purpose and follows the same pattern.
🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 24-24: Function parameter 'path' should be passed by const reference.
(passedByValue)
30-35: LGTM!The main function properly initializes both Google Test and PHARE lifecycle.
tests/amr/data/particles/refine/test_split.cpp (1)
25-25: LGTM!The addition of
Splitter<3, 1, 27>aligns with the PR objective of transitioning to 3D and follows the established pattern for splitter parameters.tools/python3/git.py (3)
5-6: LGTM!The function correctly retrieves the current Git branch name.
9-10: LGTM!The function correctly retrieves commit hashes with a sensible default parameter.
13-16: LGTM!The function correctly retrieves Git log entries with helpful documentation and sensible defaults.
tests/simulator/advance/test_particles_advance_3d.py (3)
1-19: LGTM!The file is well-documented with clear imports and well-defined constants.
21-22: LGTM!The helper function correctly generates test cases for different interpolation orders.
25-51: LGTM!The test class is well-structured with appropriate test methods and parameters. The use of data-driven testing enhances test coverage.
tests/simulator/initialize/test_fields_init_2d.py (1)
26-26: LGTM! Parameter renaming improves consistency.The change from
nbr_part_per_celltoppcaligns with the parameter naming used in the new 3D test files, making the codebase more consistent.tests/amr/data/field/coarsening/test_weighter_coarsen.hpp (1)
6-6: LGTM! Simplified header dependencies.Consolidating multiple includes into a single
coarsen_weighter.hppheader improves maintainability and reduces potential include-order dependencies.tests/simulator/initialize/test_fields_init_3d.py (1)
16-19: LGTM! Well-structured test configuration.The test configuration with
ndim=3and reasonable defaults forppcandcellsis well-thought-out for 3D testing.tests/simulator/advance/test_fields_advance_1d.py (1)
96-96:✅ Verification successful
LGTM! Test has been re-enabled.
The test for field level ghosts via subcycles and coarser interpolation is now active, aligning with issue #748.
Let's verify the test coverage for this functionality across all dimensions:
🏁 Script executed:
#!/bin/bash # Description: Check test coverage for field level ghosts across dimensions rg -l "test_field_level_ghosts_via_subcycles_and_coarser_interpolation" tests/simulator/advance/Length of output: 246
LGTM! The previously skipped test for field level ghosts via subcycles and coarser interpolation is now re-enabled and verified to be active across 1D, 2D, and 3D (see files:
tests/simulator/advance/test_fields_advance_1d.py,tests/simulator/advance/test_fields_advance_2d.py, andtests/simulator/advance/test_fields_advance_3d.py). This change aligns with the objectives of issue #748.tests/simulator/initialize/test_particles_init_2d.py (1)
26-26: LGTM! Class name now correctly reflects 2D context.The class name has been updated to accurately represent the two-dimensional nature of these tests.
tests/simulator/initialize/test_particles_init_3d.py (1)
1-100: LGTM! Well-structured test suite for 3D particle initialization.The test implementation follows the established patterns from 1D and 2D tests, providing good coverage for:
- Particle count verification
- Level ghost particles
- Domain particles
tests/simulator/advance/test_fields_advance_2d.py (1)
101-101:✅ Verification successful
Verify test readiness for moments.
The commented-out skip decorator suggests that this test should now work with moments. Please verify that the necessary changes have been made to support moments in this test.
🏁 Script executed:
#!/bin/bash # Description: Check if moments-related changes have been implemented. # Search for moment-related code in the test file and its base class. echo "Checking for moment-related code in test files..." rg -A 5 "moment" tests/simulator/advance/test_fields_advance_2d.py tests/simulator/test_advance.pyLength of output: 1824
Test Ready for Moments Integration
The execution confirms that moment-related functionality is implemented in the base tests (see
tests/simulator/test_advance.py), and the removal of the skip decorator intests/simulator/advance/test_fields_advance_2d.pyappears appropriate. Please proceed with these changes.tests/simulator/advance/test_fields_advance_3d.py (1)
102-102:❓ Verification inconclusive
Verify test readiness for moments in 3D.
Similar to the 2D test, this test has a commented skip decorator about moments. Please verify that the necessary changes have been made to support moments in 3D.
🏁 Script executed:
#!/bin/bash # Description: Check if moments-related changes have been implemented for 3D. # Search for moment-related code in the test file and its base class. echo "Checking for moment-related code in 3D test files..." rg -A 5 "moment" tests/simulator/advance/test_fields_advance_3d.py tests/simulator/test_advance.pyLength of output: 1828
Verify moments readiness in 3D tests
- In
tests/simulator/advance/test_fields_advance_3d.py, the skip decorator remains commented-out (# @unittest.skip("should change to work on moments")), which suggests that the test may not be fully updated for 3D moments support.- Meanwhile,
tests/simulator/test_advance.pycontains moments-related logic (e.g., checking for"moments"and processing hierarchies), indicating that part of the moments support is in place.- Action: Please confirm that the 3D test now fully exercises the moments functionality. If the implementation is complete, remove the outdated skip comment and ensure the test validations cover the intended moments behavior in 3D. Otherwise, update the test accordingly.
res/cmake/options.cmake (1)
71-74: Clarify interaction between lowResourceTests and highResourceTests options.Having both
lowResourceTestsandhighResourceTestsoptions might lead to confusion. Consider documenting how these options interact or consolidating them into a single option with multiple values.Please clarify:
- What happens if both options are ON?
- What happens if both options are OFF?
- Should these be mutually exclusive?
src/core/utilities/meta/meta_utilities.hpp (2)
9-11: LGTM! Good use of preprocessor directive for configuration.The default value of 3 for PHARE_SIMULATORS allows for maximum dimensionality while providing flexibility to reduce it during compilation.
83-88: LGTM! Well-structured conditional compilation for 2D simulators.The code effectively uses preprocessor directives to conditionally include 2D simulator options, improving compilation efficiency.
src/amr/data/particles/refine/splitter.hpp (3)
18-40: LGTM! Well-documented and well-structured pattern definition.The SplitPattern struct provides a clear geometric description of refined particle placement with comprehensive documentation.
43-104: LGTM! Excellent implementation of pattern combination.The PatternDispatcher class effectively manages multiple patterns with proper error handling and type safety.
124-132: LGTM! Good use of template specialization prevention.The deleted constructor effectively prevents instantiation of unspecialized templates, enforcing proper usage.
tests/amr/data/field/refine/test_field_refinement_on_hierarchy.cpp (2)
7-8: LGTM! Good cleanup of includes.Replacing multiple specific includes with the comprehensive grid.hpp improves maintainability.
11-15: LGTM! Clear and informative test documentation.The comment block effectively explains the test's purpose and scope, particularly regarding the DEFAULT field refinement operator.
tests/simulator/refined_particle_nbr.py (2)
33-37: LGTM! Good cleanup practice.The tearDown method ensures proper cleanup of simulator resources after each test, which is essential for test isolation.
44-48:❓ Verification inconclusive
Verify the 3D case implementation.
The TODO comment indicates that the 3D case might need verification. The current implementation assumes a simple multiplication by cellNbr[2] * 2.
Please verify if this calculation is correct for 3D particle refinement by running the test suite:
🏁 Script executed:
#!/bin/bash # Run the test suite focusing on 3D cases python3 -m pytest tests/simulator/refined_particle_nbr.py -v -k "test_3d"Length of output: 114
Re-check 3D Calculation in Particle Refinement
The current 3D implementation still multiplies by
cellNbr[2] * 2as a simple extension of the 2D calculation. However, the test suite did not run because thepytestmodule is missing. Please ensure thatpytestis installed in your environment so that tests for the 3D case can be executed correctly. Until the tests run successfully, a manual verification of the 3D calculation is recommended.
- File:
tests/simulator/refined_particle_nbr.py(Lines 44-48)src/amr/data/particles/refine/split_1d.hpp (1)
20-21: LGTM! Improved naming clarity.The renaming from *Dispatcher to *Pattern better reflects the purpose of these structures, as they define splitting patterns rather than just dispatching logic.
Also applies to: 33-34, 52-53, 71-72, 89-90, 108-109, 127-128, 145-146, 164-165, 183-185
pyphare/pyphare/core/box.py (2)
185-198: LGTM! Efficient implementation of multiple box removal.The remove_all function efficiently handles multiple box removals by iteratively processing each box in the to_remove list.
209-224: LGTM! Well-documented data selection interface.The DataSelector class provides a clean interface for data selection and modification using box objects.
res/cmake/funcs.cmake (2)
23-27: LGTM! Important developer note.Clear documentation warning about not using CMake variable function arguments for Python unit test executions.
37-42: LGTM! Proper test environment setup.The set_exe_paths_ function correctly sets up PYTHONPATH and ASAN options for test execution.
tests/simulator/__init__.py (1)
98-108: LGTM! The 3D density function follows the established pattern.The implementation correctly extends the 1D and 2D density calculations to 3D, maintaining consistency with the existing code structure.
tests/core/data/ndarray/test_main.cpp (1)
395-446: LGTM! Comprehensive test coverage for 3D masked views.The test case thoroughly validates 3D array masking operations with:
- Clear initialization and boundary testing
- Well-documented cell count calculations
- Multiple mask operation scenarios
- Proper verification of expected results
tests/amr/data/field/refine/test_default_field_refinement.cpp (1)
11-11: LGTM! Improved code organization and documentation.The changes enhance code readability and maintainability through:
- Addition of necessary Grid header
- Clear documentation of test scope
- Better visual separation of test sections
Also applies to: 14-17, 25-26
pyphare/pyphare/core/gridlayout.py (1)
369-382: LGTM! Well-structured coordinate generation.The implementation correctly handles all dimensionality cases and uses appropriate numpy functions for coordinate generation.
pyphare/pyphare/pharesee/run/utils.py (3)
362-362: LGTM! Shape calculation updated for multi-dimensional support.The shape calculation now correctly uses
nbrGhostsmultiplied byndimto accommodate different dimensions.
365-365: LGTM! Improved error handling for 1D case.The error message now clearly indicates that the functionality is not used in 1D.
371-371: LGTM! Clear error message for unsupported dimensions.The error message now clearly indicates that the functionality is not implemented and questions its usefulness.
tests/simulator/utilities/test_refine_field.py (2)
105-107: LGTM! Added 3D electric field refinement handling.The code now explicitly handles 3D electric field refinement by raising a
NotImplementedError, making it clear that this functionality is not yet implemented.
178-181: LGTM! Added 3D magnetic field refinement handling.The code now explicitly handles 3D magnetic field refinement by raising a
NotImplementedError, and includes a clear error message for impossible layouts.tests/simulator/test_initialization.py (3)
254-272: LGTM! Added performance measurements and improved parameter handling.The code now includes timing measurements for initialization and updates parameter handling to support 3D cases.
329-352: LGTM! Added comprehensive 3D magnetic field testing.The code now properly tests magnetic field components in 3D using appropriate mesh grid calculations and assertions.
399-416: Verify the bulk velocity calculation in 3D.The bulk velocity calculation has been updated for 3D support, but there might be potential division by zero if
ni(ion density) is zero.# Add a safety check for zero density if np.any(ni == 0): raise ValueError("Zero ion density detected, cannot calculate bulk velocity")pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
564-565: LGTM! Clear handling of 3D plots.The addition of an explicit error for 3D plots is a good practice, making it clear to users that this functionality is not yet supported.
tests/simulator/test_advance.py (1)
285-340: LGTM! Improved error handling and code clarity.The refactoring enhances readability and provides better error diagnostics with detailed information when assertions fail.
🧰 Tools
🪛 Ruff (0.8.2)
290-290: Loop control variable
ilvlnot used within loop bodyRename unused
ilvlto_ilvl(B007)
tests/amr/data/particles/refine/input/input_3d_ratio_2.txt (1)
3-6: LGTM! Consistent 3D configuration.The configuration has been properly updated to support 3D operations with appropriate domain boxes, coordinates, and ratios.
Also applies to: 19-19, 23-23, 41-41
tests/simulator/initialize/CMakeLists.txt (1)
21-24: LGTM! Well-structured test configuration.The addition of 3D tests under the
highResourceTestscondition follows the established pattern and maintains good organization of test resources.tests/simulator/advance/CMakeLists.txt (1)
21-24: Enable 3D Tests with High Resource Flag: The new conditional block that invokes tests for 3D fields and particles whenhighResourceTestsis enabled is well integrated. This addition clearly delineates 3D tests from lower-resource scenarios. Please ensure that the corresponding 3D test scripts (e.g.,test_fields_advance_3d.pyandtest_particles_advance_3d.py) are updated and documented regarding their resource requirements.tests/diagnostic/CMakeLists.txt (2)
37-37: Add 3D Diagnostics Test: Introducing_add_diagnostics_test(test-diagnostics_3d)expands the diagnostic test coverage into 3D scenarios. Confirm that the new 3D diagnostic test (and its corresponding C++ test implementation) adheres to the framework’s expectations.
41-41: Configure 3D Job File: The added configuration step usingconfigure_fileforjob_3d.py.into generatejob_3d.pyis a solid move to extend the test setup into 3D. Please verify thatjob_3d.py.inis properly set up to reflect the parameters required for 3D simulations.tests/amr/data/field/refine/CMakeLists.txt (1)
49-49: Update Test Naming for Field Refinement: Updating the test call from the old naming convention to_add_serial_amr_field_refine_test(test_default_field_refinement)clarifies the test’s intent. Ensure that any accompanying documentation or references to the old test name are updated accordingly.tests/simulator/CMakeLists.txt (1)
8-8: Include Utilities Directory in Build: Adding the command to copy the "utilities" directory to the binary directory ensures that necessary helper scripts or resources are available during runtime. Verify that the "utilities" folder remains up-to-date and is correctly referenced by other build or test scripts..github/workflows/cmake_macos.yml (1)
81-81: Extend CMake CXX Flags for Simulator Support: Appending the flag-DPHARE_SIMULATORS=2alongside-DPHARE_DIAG_DOUBLES=1in the CMake configuration expands the build's configurability to support multiple simulator options. Please confirm that this change aligns with other modules relying on these flags and that the associated documentation is updated.
|
|
||
| if not is_primal[0] and not is_primal[1] and is_primal[2]: | ||
| if qty == "Bz": | ||
| coarseData[ | ||
| coarseLocalIndexX, coarseLocalIndexY, coarseLocalIndexZ | ||
| ] = 0.25 * ( | ||
| fineData[fineIndexX, fineIndexY, fineIndexZ] | ||
| + fineData[fineIndexX + 1, fineIndexY, fineIndexZ] | ||
| + fineData[fineIndexX, fineIndexY + 1, fineIndexZ] | ||
| + fineData[fineIndexX + 1, fineIndexY + 1, fineIndexZ] | ||
| ) | ||
|
|
||
| elif not is_primal[0] and is_primal[1] and is_primal[2]: # Ex | ||
| value = 0.0 | ||
| # dual in X means taking the two dual fine nodes | ||
| # around the coarse one we want the value for. | ||
| ishift = (0, 1) | ||
| iweights = (0.5, 0.5) | ||
|
|
||
| jshift = (-1, 0, 1) | ||
| jweights = (0.25, 0.5, 0.25) | ||
|
|
||
| kshift = (-1, 0, 1) | ||
| kweights = (0.25, 0.5, 0.25) | ||
| for i, iweight in zip(ishift, iweights): | ||
| yvalue = 0.0 | ||
| for j, jweight in zip(jshift, jweights): | ||
| zvalue = 0.0 | ||
| for k, kweight in zip(kshift, kweights): | ||
| zvalue += ( | ||
| fineData[fineIndexX + i][fineIndexY + j][ | ||
| fineIndexZ + k | ||
| ] | ||
| * kweight | ||
| ) | ||
| yvalue += zvalue * jweight | ||
| value += yvalue * iweight | ||
| coarseData[ | ||
| coarseLocalIndexX, coarseLocalIndexY, coarseLocalIndexZ | ||
| ] = value | ||
|
|
||
| elif ( | ||
| is_primal[0] and not is_primal[1] and is_primal[2] | ||
| ): # Ey is PDP | ||
| value = 0.0 | ||
| ishift = (-1, 0, 1) | ||
| iweights = (0.25, 0.5, 0.25) | ||
|
|
||
| # dual in Y means taking the two dual fine nodes | ||
| # around the coarse one we want the value for. | ||
| jshift = (0, 1) | ||
| jweights = (0.5, 0.5) | ||
|
|
||
| kshift = (-1, 0, 1) | ||
| kweights = (0.25, 0.5, 0.25) | ||
| for i, iweight in zip(ishift, iweights): | ||
| yvalue = 0.0 | ||
| for j, jweight in zip(jshift, jweights): | ||
| zvalue = 0.0 | ||
| for k, kweight in zip(kshift, kweights): | ||
| zvalue += ( | ||
| fineData[fineIndexX + i][fineIndexY + j][ | ||
| fineIndexZ + k | ||
| ] | ||
| * kweight | ||
| ) | ||
| yvalue += zvalue * jweight | ||
| value += yvalue * iweight | ||
| coarseData[ | ||
| coarseLocalIndexX, coarseLocalIndexY, coarseLocalIndexZ | ||
| ] = value | ||
|
|
||
| elif ( | ||
| is_primal[0] and is_primal[1] and not is_primal[2] | ||
| ): # Ez is PPD | ||
| value = 0.0 | ||
| ishift = (-1, 0, 1) | ||
| iweights = (0.25, 0.5, 0.25) | ||
|
|
||
| jshift = (-1, 0, 1) | ||
| jweights = (0.25, 0.5, 0.25) | ||
|
|
||
| # dual in Z means taking the two dual fine nodes | ||
| # around the coarse one we want the value for. | ||
| kshift = (0, 1) | ||
| kweights = (0.5, 0.5) | ||
| for i, iweight in zip(ishift, iweights): | ||
| yvalue = 0.0 | ||
| for j, jweight in zip(jshift, jweights): | ||
| zvalue = 0.0 | ||
| for k, kweight in zip(kshift, kweights): | ||
| zvalue += ( | ||
| fineData[fineIndexX + i][fineIndexY + j][ | ||
| fineIndexZ + k | ||
| ] | ||
| * kweight | ||
| ) | ||
| yvalue += zvalue * jweight | ||
| value += yvalue * iweight | ||
| coarseData[ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider systematically generating coarsening weights.
Between lines 144 and 290, various blocks compute weighted averages depending on primal/dual states. Repetitive patterns, such as ishift, jshift, and kshift arrays, could be encapsulated in a function or dictionary keyed by dimension and primal/dual flags. Doing so can reduce code duplication, centralize the weighting logic, and simplify future expansions or bug fixes.
🧰 Tools
🪛 Ruff (0.8.2)
181-182: Use a single if statement instead of nested if statements
(SIM102)
| for (auto i = min_; i <= max_; ++i) | ||
| for (auto j = min_; j <= shape[1] - 1 - max_; ++j) | ||
| array(i, j) = val; | ||
|
|
||
| // right border | ||
| for (std::size_t i = shape[0] - 1 - max_; i <= shape[0] - 1 - min_; ++i) | ||
| for (std::size_t j = min_; j <= shape[1] - 1 - max_; ++j) | ||
| for (auto i = shape[0] - 1 - max_; i <= shape[0] - 1 - min_; ++i) | ||
| for (auto j = min_; j <= shape[1] - 1 - max_; ++j) | ||
| array(i, j) = val; | ||
|
|
||
|
|
||
| for (std::size_t i = min_; i <= shape[0] - 1 - min_; ++i) | ||
| for (auto i = min_; i <= shape[0] - 1 - min_; ++i) | ||
| { | ||
| // bottom border | ||
| for (std::size_t j = min_; j <= max_; ++j) | ||
| for (auto j = min_; j <= max_; ++j) | ||
| array(i, j) = val; | ||
|
|
||
| // top border | ||
| for (std::size_t j = shape[1] - 1 - max_; j <= shape[1] - 1 - min_; ++j) | ||
| for (auto j = shape[1] - 1 - max_; j <= shape[1] - 1 - min_; ++j) | ||
| array(i, j) = val; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid potential off-by-one in 2D borders.
These lines fill the 2D boundary loops (left, right, bottom, top). It appears correct for inclusive boundaries. If min_ or max_ become larger than expected, you risk overlaps or out-of-range indexing. Consider adding boundary checks.
| def checkout(branch, create=False, recreate=False): | ||
| if recreate: | ||
| delete_branch(branch) | ||
| create = True | ||
|
|
||
| if create and not branch_exists(branch): | ||
| run(f"git checkout -b {branch}") | ||
| else: | ||
| run(f"git checkout {branch}") |
There was a problem hiding this comment.
Add missing delete_branch function.
The function references an undefined delete_branch function.
Please implement the missing delete_branch function before using it in the checkout function. Example implementation:
def delete_branch(branch):
"""Delete a Git branch if it exists."""
if branch_exists(branch):
run(f"git branch -D {branch}")🧰 Tools
🪛 Ruff (0.8.2)
29-29: Undefined name delete_branch
(F821)
| # -DhighResourceTests=ON | ||
| option(highResourceTests "Enable heavy tests for CI (3d/etc" OFF) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing print statement for highResourceTests option.
The new option is not included in the print_phare_options function output. Add it for consistency with other options.
Add this line in the print_phare_options function:
message("Exclude heavier tests : " ${lowResourceTests})
+ message("Include heavy tests (3D/etc) : " ${highResourceTests})
message("PHARE_MPI_PROCS : " ${PHARE_MPI_PROCS})Committable suggestion skipped: line range outside the PR's diff.
| def debug_tracer(): | ||
| """ | ||
| print live stack trace during execution | ||
| """ | ||
|
|
||
| import sys, os | ||
|
|
||
| def tracefunc(frame, event, arg, indent=[0]): | ||
| filename = os.path.basename(frame.f_code.co_filename) | ||
| line_number = frame.f_lineno | ||
| if event == "call": | ||
| indent[0] += 2 | ||
| print( | ||
| "-" * indent[0] + "> enter function", | ||
| frame.f_code.co_name, | ||
| f"{filename} {line_number}", | ||
| ) | ||
| elif event == "return": | ||
| print( | ||
| "<" + "-" * indent[0], | ||
| "exit function", | ||
| frame.f_code.co_name, | ||
| f"{filename} {line_number}", | ||
| ) | ||
| indent[0] -= 2 | ||
| return tracefunc | ||
|
|
||
| sys.setprofile(tracefunc) |
There was a problem hiding this comment.
Fix mutable default argument in debug_tracer function.
The function uses a mutable list as a default argument (indent=[0]), which is a Python anti-pattern that can lead to unexpected behavior when the function is reused.
Apply this diff to fix the issue:
-def tracefunc(frame, event, arg, indent=[0]):
+def tracefunc(frame, event, arg, indent=None):
+ if indent is None:
+ indent = [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.
| def debug_tracer(): | |
| """ | |
| print live stack trace during execution | |
| """ | |
| import sys, os | |
| def tracefunc(frame, event, arg, indent=[0]): | |
| filename = os.path.basename(frame.f_code.co_filename) | |
| line_number = frame.f_lineno | |
| if event == "call": | |
| indent[0] += 2 | |
| print( | |
| "-" * indent[0] + "> enter function", | |
| frame.f_code.co_name, | |
| f"{filename} {line_number}", | |
| ) | |
| elif event == "return": | |
| print( | |
| "<" + "-" * indent[0], | |
| "exit function", | |
| frame.f_code.co_name, | |
| f"{filename} {line_number}", | |
| ) | |
| indent[0] -= 2 | |
| return tracefunc | |
| sys.setprofile(tracefunc) | |
| def debug_tracer(): | |
| """ | |
| print live stack trace during execution | |
| """ | |
| import sys, os | |
| def tracefunc(frame, event, arg, indent=None): | |
| if indent is None: | |
| indent = [0] | |
| filename = os.path.basename(frame.f_code.co_filename) | |
| line_number = frame.f_lineno | |
| if event == "call": | |
| indent[0] += 2 | |
| print( | |
| "-" * indent[0] + "> enter function", | |
| frame.f_code.co_name, | |
| f"{filename} {line_number}", | |
| ) | |
| elif event == "return": | |
| print( | |
| "<" + "-" * indent[0], | |
| "exit function", | |
| frame.f_code.co_name, | |
| f"{filename} {line_number}", | |
| ) | |
| indent[0] -= 2 | |
| return tracefunc | |
| sys.setprofile(tracefunc) |
🧰 Tools
🪛 Ruff (0.8.2)
326-326: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -67,6 +79,7 @@ | |||
| using FineParticle = decltype(particles[0]); // may be a reference | |||
|
|
|||
| core::apply(patterns, [&](auto const& pattern) { | |||
| auto weight = static_cast<Weight_t>(pattern.weight_); | |||
Check notice
Code scanning / CodeQL
Unused local variable
| @@ -392,6 +392,60 @@ | |||
| + Mask{0u}.nCells(array)); | |||
| } | |||
|
|
|||
| TEST(MaskedView3d, maskOps3) | |||
| { | |||
| constexpr std::size_t dim = 3; | |||
Check notice
Code scanning / CodeQL
Unused local variable
| TYPED_TEST_SUITE_P(ACollectionOfParticles_3d); | ||
|
|
||
|
|
||
| TYPED_TEST_P(ACollectionOfParticles_3d, DepositCorrectlyTheirWeight_3d) |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| // 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); |
Check notice
Code scanning / CodeQL
Unused local variable Note test
|
|
||
|
|
||
| using My3dTypes = ::testing::Types<Interpolator<3, 1>, Interpolator<3, 2>, Interpolator<3, 3>>; | ||
| INSTANTIATE_TYPED_TEST_SUITE_P(testInterpolator, ACollectionOfParticles_3d, My3dTypes); |
Check notice
Code scanning / CodeQL
Unused local variable Note test
|
|
||
| #include "core/def/phare_mpi.hpp" | ||
|
|
||
| #include "test_diagnostics.ipp" |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| static std::string const job_file = "job_3d"; | ||
| static std::string const out_dir = "phare_outputs/diags_3d/"; | ||
|
|
||
| TYPED_TEST(Simulator3dTest, fluid) |
Check warning
Code scanning / CodeQL
Poorly documented large function Warning test
| fluid_test(TypeParam{job_file}, out_dir); | ||
| } | ||
|
|
||
| TYPED_TEST(Simulator3dTest, particles) |
Check warning
Code scanning / CodeQL
Poorly documented large function Warning test
| particles_test(TypeParam{job_file}, out_dir); | ||
| } | ||
|
|
||
| TYPED_TEST(Simulator3dTest, electromag) |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| electromag_test(TypeParam{job_file}, out_dir); | ||
| } | ||
|
|
||
| TYPED_TEST(Simulator3dTest, allFromPython) |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| @@ -392,6 +392,60 @@ | |||
| + Mask{0u}.nCells(array)); | |||
| } | |||
|
|
|||
| TEST(MaskedView3d, maskOps3) | |||
| { | |||
| constexpr std::size_t dim = 3; | |||
Check notice
Code scanning / CodeQL
Unused local variable Note test
|
|
||
| TYPED_TEST_P(ACollectionOfParticles_3d, DepositCorrectlyTheirWeight_3d) | ||
| { | ||
| // auto const& [vx, vy, vz] = this->v(); |
Check notice
Code scanning / CodeQL
Unused static variable Note test
| // 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); | ||
| } | ||
| REGISTER_TYPED_TEST_SUITE_P(ACollectionOfParticles_3d, DepositCorrectlyTheirWeight_3d); |
Check notice
Code scanning / CodeQL
Commented-out code Note test
| using My3dTypes = ::testing::Types<Interpolator<3, 1>, Interpolator<3, 2>, Interpolator<3, 3>>; | ||
| INSTANTIATE_TYPED_TEST_SUITE_P(testInterpolator, ACollectionOfParticles_3d, My3dTypes); | ||
| /*********************************************************************************************/ | ||
|
|
Check notice
Code scanning / CodeQL
Unused static variable Note test
|
|
||
| #include "core/def/phare_mpi.hpp" | ||
|
|
||
| #include "test_diagnostics.ipp" |
Check notice
Code scanning / CodeQL
Include header files only Note test
| static std::string const job_file = "job_3d"; | ||
| static std::string const out_dir = "phare_outputs/diags_3d/"; | ||
|
|
||
| TYPED_TEST(Simulator3dTest, fluid) |
Check notice
Code scanning / CodeQL
Unused static variable Note test
| fluid_test(TypeParam{job_file}, out_dir); | ||
| } | ||
|
|
||
| TYPED_TEST(Simulator3dTest, particles) |
Check notice
Code scanning / CodeQL
Unused static variable Note test
| particles_test(TypeParam{job_file}, out_dir); | ||
| } | ||
|
|
||
| TYPED_TEST(Simulator3dTest, electromag) |
Check notice
Code scanning / CodeQL
Unused static variable Note test
| electromag_test(TypeParam{job_file}, out_dir); | ||
| } | ||
|
|
||
| TYPED_TEST(Simulator3dTest, allFromPython) |
Check notice
Code scanning / CodeQL
Unused static variable Note test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/simulator/test_advance.py (1)
290-290: Rename unused loop variable.The loop control variable
ilvlis not used within the loop body.- for ilvl, overlaps in hierarchy_overlaps(datahier, coarsest_time).items(): + for _ilvl, overlaps in hierarchy_overlaps(datahier, coarsest_time).items():🧰 Tools
🪛 Ruff (0.8.2)
290-290: Loop control variable
ilvlnot used within loop bodyRename unused
ilvlto_ilvl(B007)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/simulator/test_advance.py(15 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/simulator/test_advance.py
290-290: Loop control variable ilvl not used within loop body
Rename unused ilvl to _ilvl
(B007)
⏰ 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 (3)
tests/simulator/test_advance.py (3)
9-9: LGTM! Documentation improvements enhance code clarity.The updated docstring clearly explains the class's role in dimensionality-agnostic testing and its relationship with dimension-specific test files.
Also applies to: 32-42
285-340: LGTM! Improved error handling and debugging capabilities.The changes enhance the test's maintainability and debuggability through:
- More descriptive variable naming (
success_test_nbr)- Detailed error information for debugging
- Well-documented empirical tolerance value (5.5e-15)
🧰 Tools
🪛 Ruff (0.8.2)
290-290: Loop control variable
ilvlnot used within loop bodyRename unused
ilvlto_ilvl(B007)
415-417: LGTM! Enhanced method signature with default value.The addition of a default value for the
cellsparameter improves the method's usability while maintaining backward compatibility.
nicolasaunai
force-pushed
the
3d_base
branch
from
0e85fa8 to
a8973ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/simulator/test_advance.py (1)
286-340: Improve variable naming and error handling in overlap test.The changes improve error reporting and test logic, but there's an unused loop variable that should be addressed.
Apply this diff to fix the unused variable:
- for ilvl, overlaps in hierarchy_overlaps(datahier, coarsest_time).items(): + for _ilvl, overlaps in hierarchy_overlaps(datahier, coarsest_time).items():🧰 Tools
🪛 Ruff (0.8.2)
290-290: Loop control variable
ilvlnot used within loop bodyRename unused
ilvlto_ilvl(B007)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/simulator/test_advance.py(16 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/simulator/test_advance.py
290-290: Loop control variable ilvl not used within loop body
Rename unused ilvl to _ilvl
(B007)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-latest)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze (python)
- GitHub Check: build (macos-13)
🔇 Additional comments (4)
tests/simulator/test_advance.py (4)
32-42: LGTM! Documentation improvements enhance code clarity.The added docstring clearly explains the class purpose and its relationship with dimension-dependent tests.
79-82: LGTM! Improved code organization with clear section markers.The added section markers and docstrings improve code readability by clearly separating test setup from test definitions.
Also applies to: 98-100, 210-213, 277-283
415-417: LGTM! Improved function signature with default value.The addition of a default value for
cellsparameter improves function usability.
444-444: LGTM! Enhanced error reporting in field coarsening test.The changes improve error reporting by:
- Adding detailed comments explaining index calculations
- Including more context in error messages
- Using empirically determined tolerance values
Also applies to: 458-459, 464-465, 526-535, 555-587
nicolasaunai
force-pushed
the
3d_base
branch
2 times, most recently
from
a0e526d to
3735984
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
tools/python3/git.py (1)
29-29:⚠️ Potential issueDefine or import the missing
delete_branchfunction.
This function call is undefined, causing a name error. This issue was also flagged in a previous review.+def delete_branch(branch): + """Delete a Git branch if it exists.""" + if branch_exists(branch): + run(f"git branch -D {branch}") def checkout(branch, create=False, recreate=False): if recreate: delete_branch(branch)🧰 Tools
🪛 Ruff (0.8.2)
29-29: Undefined name
delete_branch(F821)
src/amr/data/particles/refine/splitter.hpp (1)
82-82:⚠️ Potential issueRemove unused variable.
The variable
weightis declared but never used.Apply this diff to remove the unused variable:
- auto weight = static_cast<Weight_t>(pattern.weight_);tests/simulator/__init__.py (1)
319-346:⚠️ Potential issueFix mutable default argument in
debug_tracerfunction.The function uses a mutable list as a default argument (
indent=[0]), which is a Python anti-pattern that can lead to unexpected behavior when the function is reused.Apply this diff to fix the issue:
-def tracefunc(frame, event, arg, indent=[0]): +def tracefunc(frame, event, arg, indent=None): + if indent is None: + indent = [0]🧰 Tools
🪛 Ruff (0.8.2)
326-326: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
🧹 Nitpick comments (25)
src/core/data/ndarray/ndarray_vector.hpp (3)
383-424: Consider optimizing the 3D border fill implementation.The current implementation has nested loops that could be optimized for better cache utilization and readability:
- Consider extracting common loop patterns into helper functions
- The border fill logic could benefit from SIMD optimizations for large arrays
template<typename Array> void fill3D(Array& array, typename Array::type val) const { auto shape = array.shape(); + auto fillPlane = [&](auto i_start, auto i_end, auto j_start, auto j_end, auto k_start, auto k_end) { + for (auto i = i_start; i <= i_end; ++i) + for (auto j = j_start; j <= j_end; ++j) + for (auto k = k_start; k <= k_end; ++k) + array(i, j, k) = val; + }; - // left border - for (auto i = min_; i <= shape[0] - 1 - max_; ++i) - for (auto j = min_; j <= shape[1] - 1 - max_; ++j) - for (auto k = min_; k <= max_; ++k) - array(i, j, k) = val; + // Fill left and right borders + fillPlane(min_, shape[0] - 1 - max_, min_, shape[1] - 1 - max_, min_, max_); + fillPlane(min_, shape[0] - 1 - max_, min_, shape[1] - 1 - max_, shape[2] - 1 - max_, shape[2] - 1 - min_); // ... similar refactoring for other borders
433-449: Improve readability of 3D cell count calculation.The nested logic for calculating cells in 3D is complex and could benefit from better documentation and variable naming:
if constexpr (Array::dimension == 3) { auto [x, y, z] = shape; x -= i * 2; y -= i * 2; z -= i * 2; - cells += (x * y * 2) + (y * (z - 2) * 2) + ((z - 2) * (x - 2) * 2); + // Calculate cells for each face pair (front/back, left/right, top/bottom) + const auto frontBackFaces = x * y * 2; + const auto leftRightFaces = y * (z - 2) * 2; + const auto topBottomFaces = (z - 2) * (x - 2) * 2; + cells += frontBackFaces + leftRightFaces + topBottomFaces; }
527-595: Consider extracting common patterns in 3D view operator.The 3D implementation of operator>> has repeated patterns that could be extracted into helper functions for better maintainability:
if constexpr (MaskedView_t::dimension == 3) { assert(inner.zstart() > outer.zstart() and inner.zend() < outer.zend()); + auto fillFace = [&](auto i_range, auto j_range, auto k_range, auto getValue) { + for (auto i : i_range) + for (auto j : j_range) + for (auto k : k_range) + outer(i, j, k) = getValue(i, j, k); + }; + // ... use fillFace for repeated patterns }Additionally, the corner lambda could be moved outside the if constexpr block to improve code organization.
tests/core/numerics/interpolator/test_main.cpp (1)
750-753: Consider extracting the weight calculation into a named function.The lambda function for weight calculation could be moved to a named function for better readability and reusability.
Apply this diff to extract the weight calculation:
- double weight = [](auto const& meshSize) { - return std::accumulate(meshSize.begin(), meshSize.end(), 1.0, - std::multiplies<double>()); - }(layout.meshSize()); + double weight = computeMeshWeight(layout.meshSize());Add this function above the test class:
template<typename MeshSize> double computeMeshWeight(MeshSize const& meshSize) { return std::accumulate(meshSize.begin(), meshSize.end(), 1.0, std::multiplies<double>()); }src/core/utilities/meta/meta_utilities.hpp (1)
94-96: Consider refactoring 3D simulator options for better maintainability.The commented out value
27and limited particle numbers for 3D suggest potential validation gaps.Consider:
- Using a constexpr function to generate valid particle numbers for each dimension
- Adding validation for the commented out value
27- Documenting the rationale for the chosen particle numbers
Example structure:
template<typename DimConstant> constexpr auto getValidParticleNumbers() { if constexpr (std::is_same_v<DimConstant, DimConst<3>>) { return std::tuple< std::integral_constant<std::size_t, 6>, std::integral_constant<std::size_t, 12>, std::integral_constant<std::size_t, 27> // Add after validation >{}; } // ... handle other dimensions }res/cmake/funcs.cmake (4)
28-35: Consider makingPHARE_MPI_PROCSconfigurable via an external variable or option.
Currently,PHARE_MPI_PROCSis set to 2 iftestMPIis enabled. For more flexibility, you could allow users (or CI) to override this in different testing environments (e.g., large clusters vs. local machines).
44-57: Ensure that the test target is not re-run unnecessarily if tests are auto-executed during the build.
You are callingadd_custom_commandto run tests post-build. If you have incremental builds or repeated builds, this might cause repeated executions. You may want to conditionally skip re-executing if nothing changes or consider turning off this post-build test run for normal development.
139-156: Standardize MPI arguments for Python tests.
The functionphare_mpi_python3_execuses a hard-codedmpirun. Consider making the MPI launcher configurable (e.g.,mpiexec,srun) if your code will run on various HPC systems with different MPI launchers.
184-191: Consider adding filtering options tophare_print_all_vars().
Printing all variables can be overwhelming. A filtering mechanism (e.g., by prefix) might help developers find relevant variables in large builds.src/amr/data/particles/refine/split_3d.hpp (1)
223-235: Confirm or document the usage of magic numbers fordeltaandweight.
Hardcoded floating-point values like.888184and.083333might need referencing or comments explaining how they were derived.tools/python3/git.py (1)
22-22: Remove unused variable or rename if necessary.
The variableeis assigned in theexceptblock but never used, triggering a static analysis warning.- except subprocess.CalledProcessError as e: + except subprocess.CalledProcessError:🧰 Tools
🪛 Ruff (0.8.2)
22-22: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
tools/python3/cmake.py (1)
4-5: Implement the version function.The
version()function is empty and needs to be implemented.Would you like me to generate an implementation that returns the CMake version using
cmake --version?tests/functional/harris/harris_3d.py (2)
142-172: Document or remove commented-out plot calls.Several plot calls are commented out without explanation. Consider either:
- Adding comments explaining why these plots are disabled and under what conditions they should be re-enabled.
- Removing the commented code if it's no longer needed.
177-178: Address the TODO comment for 3D plotting.The TODO comment indicates that a solution for 3D plotting is needed after the removal of the hierarchy plot3d feature.
Would you like me to help implement a new 3D plotting solution using libraries like matplotlib or plotly?
tests/simulator/refined_particle_nbr.py (1)
47-48: Remove TODO comment and document 3D case.The TODO comment can be removed as the 3D case is now implemented. Consider adding a comment explaining the calculation for the 3D case.
Apply this diff to improve documentation:
- # TODO3D if dim==3? + # For 3D, multiply 2D result by the number of cells in z dimension * 2 + # to account for particles leaving through z-faces return dim2 * (cellNbr[2] * 2)pyphare/pyphare/core/box.py (3)
185-198: Improve list modification safety.The function modifies the list while iterating, which could be error-prone. Consider using a more explicit approach.
Apply this diff to improve safety:
def remove_all(box, to_remove): if len(to_remove) > 0: remaining = box - to_remove[0] for to_rm in to_remove[1:]: - tmp, remove = [], [] - for i, rem in enumerate(remaining): - if rem * to_rm is not None: - remove.append(i) - tmp += rem - to_rm - for rm in reversed(remove): - del remaining[rm] - remaining += tmp + new_remaining = [] + for rem in remaining: + if rem * to_rm is not None: + new_remaining.extend(rem - to_rm) + else: + new_remaining.append(rem) + remaining = new_remaining return remaining return box
206-207: Improve variable naming clarity.The variable name 'l' is ambiguous and could be confused with the number '1'.
Apply this diff to improve readability:
-def select(data, box): - return data[tuple([slice(l, u + 1) for l,u in zip(box.lower, box.upper)])] +def select(data, box): + return data[tuple([slice(lower, upper + 1) for lower, upper in zip(box.lower, box.upper)])]🧰 Tools
🪛 Ruff (0.8.2)
207-207: Ambiguous variable name:
l(E741)
209-223: Enhance class documentation.The class documentation could be more descriptive and include examples.
Apply this diff to improve documentation:
class DataSelector: """ - can't assign return to function unless [] - usage - DataSelector(data)[box] = val + A utility class for selecting and modifying data using Box instances or slices. + + This class provides a convenient interface for data access and modification + using Box instances or standard Python slices. + + Examples: + >>> selector = DataSelector(data) + >>> # Using a Box instance + >>> box = Box([0, 0], [2, 2]) + >>> selector[box] = new_values + >>> # Using a slice + >>> selector[1:3, 1:3] = other_values """pyphare/pyphare/core/gridlayout.py (1)
369-382: Consider improving variable naming and error handling.The implementation could benefit from:
- More descriptive variable names (e.g.,
X,Y,Z→x_mesh, y_mesh, z_mesh)- Error handling for invalid dimensions
Here's a suggested improvement:
def meshCoords(self, qty): ndim = self.ndim - assert ndim > 0 and ndim < 4 + if not (0 < ndim < 4): + raise ValueError(f"Invalid number of dimensions: {ndim}. Expected 1, 2, or 3.") x = self.yeeCoordsFor(qty, "x") if ndim == 1: return x y = self.yeeCoordsFor(qty, "y") if ndim == 2: - X,Y = np.meshgrid(x,y,indexing="ij") - return np.array([X.flatten(),Y.flatten()]).T.reshape((len(x), len(y), ndim)) + x_mesh, y_mesh = np.meshgrid(x, y, indexing="ij") + return np.array([x_mesh.flatten(), y_mesh.flatten()]).T.reshape((len(x), len(y), ndim)) z = self.yeeCoordsFor(qty, "z") - X ,Y, Z = np.meshgrid(x,y,z,indexing="ij") - return np.array([X.flatten(), Y.flatten(), Z.flatten()]).T.reshape((len(x), len(y), len(z), ndim)) + x_mesh, y_mesh, z_mesh = np.meshgrid(x, y, z, indexing="ij") + return np.array([x_mesh.flatten(), y_mesh.flatten(), z_mesh.flatten()]).T.reshape((len(x), len(y), len(z), ndim))tests/amr/data/field/coarsening/coarsening_utils.py (2)
8-21: Enhance docstring with parameter descriptions.The docstring should include descriptions of all parameters and their types for better clarity.
def coarsen(qty, coarseField, fineField, coarseBox, fineData, coarseData): """ + Parameters + ---------- + qty : str + The quantity to coarsen (e.g., "Bx", "By", "Bz", etc.) + coarseField : Field + The coarse field object + fineField : Field + The fine field object + coarseBox : Box + The box defining the coarse grid region + fineData : ndarray + The dataset from the fine field + coarseData : ndarray + The dataset to store coarsened values + + Notes + ----- this function takes data in fineData (the dataset from the fineFied FieldData object) and put it in the coarseData dataset The coarsening method is the equivalent (and union) of the C++ coarsening methods, namely: - DefaultFieldCoarsener, used for all fields (moments and E) but the magnetic field B - MagneticFieldCoarsener, used for the magnetic field B as in the C++ counterpart, we hard-code here that the layout of B follows the yee layout. That is Bx is PDD, By is DPD and Bz is DDP """
181-182: Simplify nested if statements.Combine the nested if statements into a single condition for better readability.
- elif not is_primal[0] and is_primal[1] and not is_primal[2]: - if qty == "By": + elif not is_primal[0] and is_primal[1] and not is_primal[2] and qty == "By":🧰 Tools
🪛 Ruff (0.8.2)
181-182: Use a single
ifstatement instead of nestedifstatements(SIM102)
pyphare/pyphare/pharesee/geometry.py (1)
161-165: Improve variable naming in list comprehensions.Replace the ambiguous variable name
lwith a more descriptive name likeshift_listto enhance code readability.- 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 shift_list] for k, shift_list 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 shift_list] for k, shift_list in shifts.items()🧰 Tools
🪛 Ruff (0.8.2)
161-161: Ambiguous variable name:
l(E741)
164-164: Ambiguous variable name:
l(E741)
tests/simulator/test_initialization.py (1)
355-416: Clean up unused loop variable.The loop variable
piin the patch iteration is not used within the loop body.- for pi, patch in enumerate(level.patches): + for _pi, patch in enumerate(level.patches):🧰 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)
tests/simulator/test_advance.py (2)
285-340: Clean up unused loop variable and enhance error reporting.
- The loop variable
ilvlin the overlaps iteration is not used within the loop body.- The error reporting includes comprehensive debug information.
- for ilvl, overlaps in hierarchy_overlaps(datahier, coarsest_time).items(): + for _ilvl, overlaps in hierarchy_overlaps(datahier, coarsest_time).items():🧰 Tools
🪛 Ruff (0.8.2)
290-290: Loop control variable
ilvlnot used within loop bodyRename unused
ilvlto_ilvl(B007)
443-587: Remove unused variable.The variable
outis assigned but never used in the function.- out = "domain_particles"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (69)
.github/workflows/cmake_macos.yml(1 hunks)pyphare/pyphare/core/box.py(2 hunks)pyphare/pyphare/core/gridlayout.py(1 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/funcs.cmake(1 hunks)res/cmake/options.cmake(2 hunks)src/amr/data/field/coarsening/coarsen_weighter.hpp(1 hunks)src/amr/data/field/coarsening/magnetic_field_coarsener.hpp(1 hunks)src/amr/data/particles/refine/particles_data_split.hpp(2 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(5 hunks)src/core/data/grid/grid.hpp(1 hunks)src/core/data/grid/gridlayoutdefs.hpp(2 hunks)src/core/data/ndarray/ndarray_vector.hpp(8 hunks)src/core/data/particles/particle.hpp(3 hunks)src/core/utilities/box/box.hpp(1 hunks)src/core/utilities/meta/meta_utilities.hpp(2 hunks)tests/amr/data/field/coarsening/CMakeLists.txt(0 hunks)tests/amr/data/field/coarsening/coarsening_utils.py(1 hunks)tests/amr/data/field/coarsening/input/input_1d_ratio_10.txt(0 hunks)tests/amr/data/field/coarsening/input/input_1d_ratio_2.txt(0 hunks)tests/amr/data/field/coarsening/input/input_1d_ratio_3.txt(0 hunks)tests/amr/data/field/coarsening/input/input_1d_ratio_4.txt(0 hunks)tests/amr/data/field/coarsening/input/input_1d_ratio_6.txt(0 hunks)tests/amr/data/field/coarsening/input/input_1d_ratio_7.txt(0 hunks)tests/amr/data/field/coarsening/input/input_1d_ratio_8.txt(0 hunks)tests/amr/data/field/coarsening/input/input_1d_ratio_9.txt(0 hunks)tests/amr/data/field/coarsening/input_config.h.in(0 hunks)tests/amr/data/field/coarsening/test_coarsening_main.cpp(0 hunks)tests/amr/data/field/coarsening/test_linear_coarsen.hpp(0 hunks)tests/amr/data/field/coarsening/test_weighter_coarsen.hpp(1 hunks)tests/amr/data/field/refine/CMakeLists.txt(1 hunks)tests/amr/data/field/refine/test_default_field_refinement.cpp(4 hunks)tests/amr/data/field/refine/test_field_refinement_on_hierarchy.cpp(1 hunks)tests/amr/data/particles/refine/input/input_3d_ratio_2.txt(2 hunks)tests/amr/data/particles/refine/test_split.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/CMakeLists.txt(1 hunks)tests/simulator/__init__.py(3 hunks)tests/simulator/advance/CMakeLists.txt(1 hunks)tests/simulator/advance/test_fields_advance_1d.py(1 hunks)tests/simulator/advance/test_fields_advance_2d.py(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(16 hunks)tests/simulator/test_initialization.py(13 hunks)tests/simulator/utilities/field_coarsening.py(0 hunks)tests/simulator/utilities/test_refine_field.py(2 hunks)tools/python3/cmake.py(1 hunks)tools/python3/git.py(1 hunks)
💤 Files with no reviewable changes (14)
- tests/amr/data/field/coarsening/input_config.h.in
- tests/amr/data/field/coarsening/test_coarsening_main.cpp
- tests/amr/data/field/coarsening/input/input_1d_ratio_8.txt
- tests/amr/data/field/coarsening/input/input_1d_ratio_6.txt
- tests/simulator/initialize/test_particles_init_1d.py
- tests/amr/data/field/coarsening/input/input_1d_ratio_9.txt
- tests/amr/data/field/coarsening/input/input_1d_ratio_7.txt
- tests/amr/data/field/coarsening/input/input_1d_ratio_3.txt
- tests/amr/data/field/coarsening/input/input_1d_ratio_4.txt
- tests/amr/data/field/coarsening/input/input_1d_ratio_10.txt
- tests/amr/data/field/coarsening/CMakeLists.txt
- tests/simulator/utilities/field_coarsening.py
- tests/amr/data/field/coarsening/input/input_1d_ratio_2.txt
- tests/amr/data/field/coarsening/test_linear_coarsen.hpp
🚧 Files skipped from review as they are similar to previous changes (33)
- src/amr/data/field/coarsening/coarsen_weighter.hpp
- src/core/data/grid/grid.hpp
- tests/simulator/CMakeLists.txt
- src/amr/data/particles/refine/split.hpp
- tests/amr/data/field/refine/CMakeLists.txt
- src/core/utilities/box/box.hpp
- .github/workflows/cmake_macos.yml
- res/cmake/options.cmake
- tests/simulator/advance/CMakeLists.txt
- tests/simulator/initialize/test_fields_init_2d.py
- tests/simulator/advance/test_fields_advance_2d.py
- pyphare/pyphare/pharesee/run/utils.py
- tests/diagnostic/job_3d.py.in
- tests/diagnostic/CMakeLists.txt
- src/amr/data/particles/refine/particles_data_split.hpp
- tests/simulator/initialize/CMakeLists.txt
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py
- tests/core/data/ndarray/test_main.cpp
- src/core/data/grid/gridlayoutdefs.hpp
- tests/amr/data/field/refine/test_default_field_refinement.cpp
- tests/amr/data/particles/refine/test_split.cpp
- tests/simulator/advance/test_particles_advance_3d.py
- tests/simulator/initialize/test_particles_init_3d.py
- tests/simulator/utilities/test_refine_field.py
- tests/amr/data/field/coarsening/test_weighter_coarsen.hpp
- tests/simulator/initialize/test_particles_init_2d.py
- res/amr/splitting.yml
- tests/simulator/initialize/test_fields_init_3d.py
- src/core/data/particles/particle.hpp
- tests/amr/data/field/refine/test_field_refinement_on_hierarchy.cpp
- tests/simulator/advance/test_fields_advance_1d.py
- tests/amr/data/particles/refine/input/input_3d_ratio_2.txt
- src/amr/data/particles/refine/split_1d.hpp
🧰 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.
tests/simulator/per_test.hppsrc/core/utilities/meta/meta_utilities.hppsrc/amr/data/particles/refine/splitter.hppsrc/core/data/ndarray/ndarray_vector.hppsrc/amr/data/particles/refine/split_3d.hppsrc/amr/data/field/coarsening/magnetic_field_coarsener.hppsrc/amr/data/particles/refine/split_2d.hpp
🪛 Ruff (0.8.2)
tests/simulator/__init__.py
326-326: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
pyphare/pyphare/core/box.py
207-207: Ambiguous variable name: l
(E741)
tests/functional/harris/harris_3d.py
62-62: Ambiguous variable name: l
(E741)
tests/amr/data/field/coarsening/coarsening_utils.py
181-182: Use a single if statement instead of nested if statements
(SIM102)
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)
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)
tools/python3/git.py
22-22: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
29-29: Undefined name delete_branch
(F821)
tests/simulator/test_advance.py
290-290: Loop control variable ilvl not used within loop body
Rename unused ilvl to _ilvl
(B007)
tools/python3/cmake.py
48-48: Undefined name cmd
(F821)
🪛 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 (27)
tests/diagnostic/test-diagnostics_3d.cpp (3)
4-4: Consider moving test implementation to a header file.The
.ippextension typically indicates an implementation file. Consider moving the implementation to a proper header file (.hpp) to follow standard C++ practices.
9-27: LGTM! Well-structured test cases.The test cases follow a consistent pattern and cover different aspects of the 3D simulator (fluid dynamics, particle behavior, electromagnetic fields, and Python integration).
🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 24-24: Function parameter 'path' should be passed by const reference.
(passedByValue)
30-35: LGTM! Standard test initialization.The main function properly initializes both Google Test and PHARE lifecycle management.
src/core/data/ndarray/ndarray_vector.hpp (1)
123-124: LGTM! The new z-dimension accessor methods are consistent with existing patterns.The implementation follows the same pattern as xstart/xend and ystart/yend methods, maintaining code consistency.
src/core/utilities/meta/meta_utilities.hpp (2)
9-11: LGTM! Well-structured preprocessor directive.The directive follows C++ best practices by checking if the macro is already defined and providing a sensible default value that enables all simulator dimensions.
90-91: Address TODO comments before merging.The TODO comments indicate incomplete implementation for 3D particle number permutations. This could affect the functionality of 3D simulations.
Please clarify:
- What are the missing permutations?
- Are these permutations critical for 3D functionality?
- What is the timeline for implementing these permutations?
res/cmake/funcs.cmake (1)
59-67: Validate forced serial tests whentestMPIis enabled.
The logic here excludes tests from being built ifforceSerialTestsis not set. If the developer mistakenly forgets to set it, tests may get silently skipped. Consider adding a warning message to help troubleshoot.src/amr/data/particles/refine/split_3d.hpp (2)
90-116: Make constructor ofWhitePattern<DimConst<3>>publicly accessible or confirm it’s intended to be private.
Classes default to private members. If you actually instantiateWhitePattern<DimConst<3>>outside this file, a private constructor may cause compilation issues.
156-165: Validate the inclusion ofBlackPattern<DimConst<3>>.
TheBlackPatternappears in the dispatcher for 27-particle splits but isn’t defined here. Ensure it's included or defined in another header to avoid linkage or compilation errors.tests/simulator/per_test.hpp (1)
111-117: LGTM! Well-structured 3D test setup.The implementation follows the established pattern from 1D and 2D test structures, maintaining consistency across dimensions.
tests/simulator/advance/test_fields_advance_3d.py (1)
1-4: LGTM! Well-documented and comprehensive test suite.The test file is well-structured with clear documentation, good test parameterization, and comprehensive coverage of 3D field advancement scenarios.
src/amr/data/particles/refine/splitter.hpp (1)
18-30: LGTM! Well-documented pattern structure.The documentation clearly explains the purpose and behavior of the
SplitPatternstruct.tests/functional/harris/harris_3d.py (1)
138-139: LGTM!The function correctly generates plot filenames with appropriate parameters.
tests/simulator/refined_particle_nbr.py (3)
33-36: LGTM!The tearDown method correctly handles simulator cleanup with appropriate null checks.
106-116: LGTM!The test_3d method and its constants are well-documented with clear explanations of the expected ratios.
130-136: LGTM!The test_values method provides comprehensive coverage across dimensions and interpolation orders.
tests/simulator/__init__.py (1)
98-108: LGTM!The function correctly implements 3D density calculation following the established pattern from 1D and 2D implementations.
src/amr/data/particles/refine/split_2d.hpp (1)
21-327: LGTM! The renaming improves code clarity.The changes consistently rename
DispatchertoPatternacross all structures and type aliases, which better reflects their purpose in the codebase. This standardization of naming conventions enhances code readability and maintainability.src/amr/data/field/coarsening/magnetic_field_coarsener.hpp (2)
38-42: LGTM! The template parameter renaming improves consistency.The change from
dimensiontodimmakes the code more concise while maintaining clarity.
82-333: Consider unifying duplicate logic across dimension-specific coarsening methods.The coarsening methods for 1D, 2D, and 3D contain similar patterns of checking primal vs. dual centering and applying weighted averages. Consider extracting these common patterns into helper functions to reduce code duplication and improve maintainability.
For example, create a helper function to handle the centering checks and weight calculations:
template<std::size_t dim> struct CoarseningWeights { static constexpr float getPrimalWeight() { return 0.5f; } static constexpr float getDualWeight() { return 0.25f; } static std::vector<float> getWeights(bool isPrimal) { return isPrimal ? std::vector<float>{0.25f, 0.5f, 0.25f} : std::vector<float>{0.5f, 0.5f}; } };tests/amr/data/field/coarsening/coarsening_utils.py (1)
144-290: Consider systematically generating coarsening weights.The code contains repetitive patterns for computing weighted averages based on primal/dual states. Consider encapsulating these patterns in a helper function or dictionary.
Example:
def get_weights(is_primal): """Return shift and weight arrays based on primal/dual state.""" if is_primal: return [-1, 0, 1], [0.25, 0.5, 0.25] return [0, 1], [0.5, 0.5] # Usage ishift, iweights = get_weights(is_primal[0]) jshift, jweights = get_weights(is_primal[1]) kshift, kweights = get_weights(is_primal[2])🧰 Tools
🪛 Ruff (0.8.2)
181-182: Use a single
ifstatement instead of nestedifstatements(SIM102)
pyphare/pyphare/pharesee/geometry.py (2)
63-74: Well-structured 3D ghost box implementation!The extension to 3D maintains consistency with existing 1D and 2D implementations while following standard naming conventions (front/back) for the new dimension.
519-520: LGTM!The list comprehension effectively handles the box removal operation.
tests/simulator/test_initialization.py (3)
254-353: Well-structured 3D magnetic field testing with performance tracking!The implementation effectively extends magnetic field testing to 3D while adding useful timing measurements for performance analysis.
417-480: Well-calibrated 3D density testing!The implementation effectively extends density testing to 3D with appropriate empirical deviation values for each dimension.
🧰 Tools
🪛 Ruff (0.8.2)
452-452: Local variable
xis assigned to but never usedRemove assignment to unused variable
x(F841)
415-417: LGTM!The addition of the
cellsparameter with a sensible default value enhances the function's flexibility while maintaining backward compatibility.tests/simulator/test_advance.py (1)
342-354: LGTM!The implementation effectively tracks successful tests and includes appropriate assertions to validate test execution.
| part.v[2] = +1.; | ||
| } | ||
|
|
||
| interpolator(particles, rho, v, layout); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the interpolator call signature.
The interpolator call signature differs from the 1D and 2D implementations, which use makeIndexRange.
Apply this diff to maintain consistency with other dimensions:
- interpolator(particles, rho, v, layout);
+ interpolator(makeIndexRange(particles), rho, v, layout);📝 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.
| interpolator(particles, rho, v, layout); | |
| interpolator(makeIndexRange(particles), rho, v, layout); |
| using Grid_t = typename PHARE_TYPES::Grid_t; | ||
| using UsableVecFieldND = UsableVecField<dim>; | ||
|
|
||
| GridLayout_t layout{ConstArray<double, dim>(.1), {nx, ny}, ConstArray<double, dim>(0)}; |
There was a problem hiding this comment.
Fix the grid layout initialization.
The grid layout initialization is incorrect as it only specifies two dimensions ({nx, ny}) for a 3D grid.
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}, ConstArray<double, dim>(0)}; | |
| GridLayout_t layout{ConstArray<double, dim>(.1), {nx, ny, nz}, ConstArray<double, dim>(0)}; |
| TYPED_TEST_P(ACollectionOfParticles_3d, DepositCorrectlyTheirWeight_3d) | ||
| { | ||
| // 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.
Uncomment and fix the test assertions.
The test case is currently commented out, which is flagged by static analysis. The test should verify the correct deposition of weights and velocities.
Apply this diff to restore and fix the test assertions:
- // 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);
+ auto idx = 2 + this->layout.nbrGhosts(QtyCentering::dual);
+ auto const& [vx, vy, vz] = this->v();
+ EXPECT_DOUBLE_EQ(this->rho(idx, idx, idx), 1.0);
+ EXPECT_DOUBLE_EQ(vx(idx, idx, idx), 2.0);
+ EXPECT_DOUBLE_EQ(vy(idx, idx, idx), -1.0);
+ EXPECT_DOUBLE_EQ(vz(idx, idx, idx), 1.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.
| TYPED_TEST_P(ACollectionOfParticles_3d, DepositCorrectlyTheirWeight_3d) | |
| { | |
| // 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); | |
| } | |
| TYPED_TEST_P(ACollectionOfParticles_3d, DepositCorrectlyTheirWeight_3d) | |
| { | |
| auto idx = 2 + this->layout.nbrGhosts(QtyCentering::dual); | |
| auto const& [vx, vy, vz] = this->v(); | |
| EXPECT_DOUBLE_EQ(this->rho(idx, idx, idx), 1.0); | |
| EXPECT_DOUBLE_EQ(vx(idx, idx, idx), 2.0); | |
| EXPECT_DOUBLE_EQ(vy(idx, idx, idx), -1.0); | |
| EXPECT_DOUBLE_EQ(vz(idx, idx, idx), 1.0); | |
| } |
| def run_test(test, verbose=False, capture_output=False): | ||
| run(test_cmd(cmd, verbose=verbose), capture_output=capture_output) |
There was a problem hiding this comment.
Fix undefined variable in run_test function.
The function uses an undefined variable cmd instead of the test parameter.
Apply this diff to fix the variable name:
- run(test_cmd(cmd, verbose=verbose), capture_output=capture_output)
+ run(test_cmd(test, verbose=verbose), capture_output=capture_output)📝 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 run_test(test, verbose=False, capture_output=False): | |
| run(test_cmd(cmd, verbose=verbose), capture_output=capture_output) | |
| def run_test(test, verbose=False, capture_output=False): | |
| run(test_cmd(test, verbose=verbose), capture_output=capture_output) |
🧰 Tools
🪛 Ruff (0.8.2)
48-48: Undefined name cmd
(F821)
| def config( | ||
| path, samrai_dir=None, cxx_flags=None, use_ninja=False, use_ccache=False, extra="" | ||
| ): | ||
| cmd = make_config_str(path, samrai_dir, cxx_flags, use_ninja, extra) | ||
| run(cmd, capture_output=False) |
There was a problem hiding this comment.
Fix missing parameter in config function.
The use_ccache parameter is not being passed to make_config_str(), which could prevent ccache from being enabled.
Apply this diff to fix the parameter passing:
- cmd = make_config_str(path, samrai_dir, cxx_flags, use_ninja, extra)
+ cmd = make_config_str(path, samrai_dir, cxx_flags, use_ninja, use_ccache, extra)📝 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 config( | |
| path, samrai_dir=None, cxx_flags=None, use_ninja=False, use_ccache=False, extra="" | |
| ): | |
| cmd = make_config_str(path, samrai_dir, cxx_flags, use_ninja, extra) | |
| run(cmd, capture_output=False) | |
| def config( | |
| path, samrai_dir=None, cxx_flags=None, use_ninja=False, use_ccache=False, extra="" | |
| ): | |
| cmd = make_config_str(path, samrai_dir, cxx_flags, use_ninja, use_ccache, extra) | |
| run(cmd, capture_output=False) |
| def T(x, y, z): | ||
| K = 1 | ||
| temp = 1.0 / density(x, y, z) * (K - b2(x, y, z) * 0.5) | ||
| assert np.all(temp > 0) | ||
| return temp |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent potential assertion errors in temperature calculation.
The temperature function could raise an assertion error if b2(x, y, z) * 0.5 exceeds K = 1. Consider adding a safety margin to K or implementing bounds checking for the magnetic field pressure.
Apply this diff to add bounds checking:
- K = 1
+ K = 1.5 # Increased to provide safety margin
temp = 1.0 / density(x, y, z) * (K - b2(x, y, z) * 0.5)
- assert np.all(temp > 0)
+ if not np.all(temp > 0):
+ raise ValueError("Temperature calculation resulted in negative values. "
+ "Consider adjusting K or magnetic field components.")
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.
| def T(x, y, z): | |
| K = 1 | |
| temp = 1.0 / density(x, y, z) * (K - b2(x, y, z) * 0.5) | |
| assert np.all(temp > 0) | |
| return temp | |
| def T(x, y, z): | |
| - K = 1 | |
| + K = 1.5 # Increased to provide safety margin | |
| temp = 1.0 / density(x, y, z) * (K - b2(x, y, z) * 0.5) | |
| - assert np.all(temp > 0) | |
| + if not np.all(temp > 0): | |
| + raise ValueError("Temperature calculation resulted in negative values. " | |
| + "Consider adjusting K or magnetic field components.") | |
| return temp |
nicolasaunai
force-pushed
the
3d_base
branch
from
72ab7cc to
5a4071c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/simulator/test_advance.py (1)
290-290: Rename unused loop variable.The loop control variable
ilvlis not used within the loop body.-for ilvl, overlaps in hierarchy_overlaps(datahier, coarsest_time).items(): +for _ilvl, overlaps in hierarchy_overlaps(datahier, coarsest_time).items():🧰 Tools
🪛 Ruff (0.8.2)
290-290: Loop control variable
ilvlnot used within loop bodyRename unused
ilvlto_ilvl(B007)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/simulator/test_advance.py(16 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/simulator/test_advance.py
290-290: Loop control variable ilvl not used within loop body
Rename unused ilvl to _ilvl
(B007)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-latest)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze (python)
- GitHub Check: build (macos-13)
🔇 Additional comments (4)
tests/simulator/test_advance.py (4)
9-9: LGTM! Clear documentation and well-organized imports.The added import and expanded class documentation effectively clarify the purpose of the class and its relationship with dimension-specific tests.
Also applies to: 32-42
285-340: Improved error handling and test success tracking.The changes enhance debugging capabilities and make the test results more accurate by:
- Using a more descriptive variable name
success_test_nbr- Adding detailed error information for debugging
- Simplifying box transformation logic with
amr_to_local🧰 Tools
🪛 Ruff (0.8.2)
290-290: Loop control variable
ilvlnot used within loop bodyRename unused
ilvlto_ilvl(B007)
415-417: LGTM! Enhanced test parameterization.The addition of the
cellsparameter to both test methods improves test flexibility by allowing customization of the grid size.Also applies to: 443-445
555-587: Improved error reporting in coarsening tests.The enhanced error reporting now includes detailed information about the failing indices and box configurations, making it easier to debug test failures.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (8)
tools/python3/git.py (1)
27-35:⚠️ Potential issueMissing
delete_branchfunction referenced incheckout.The
checkoutfunction references adelete_branchfunction that is not defined in this file, causing a runtime error whenrecreate=True. This issue was previously flagged in a past review comment.Add the missing
delete_branchfunction:+def delete_branch(branch): + """Delete a Git branch if it exists.""" + if branch_exists(branch): + run(f"git branch -D {branch}") + def checkout(branch, create=False, recreate=False): if recreate: delete_branch(branch) create = True if create and not branch_exists(branch): run(f"git checkout -b {branch}") else: run(f"git checkout {branch}")🧰 Tools
🪛 Ruff (0.8.2)
29-29: Undefined name
delete_branch(F821)
tests/amr/data/field/coarsening/coarsening_utils.py (1)
144-168: 🛠️ Refactor suggestionRefactor repeated weight computation patterns into a reusable function.
This section and several similar blocks below use the same pattern of nested loops with shifts and weights to compute weighted averages. This pattern should be extracted into a helper function to reduce code duplication.
Create a helper function for the triple-nested loop pattern:
+def compute_weighted_value(fineData, fineIndexX, fineIndexY, fineIndexZ, + ishift, iweights, jshift, jweights, kshift, kweights): + value = 0.0 + for i, iweight in zip(ishift, iweights): + yvalue = 0.0 + for j, jweight in zip(jshift, jweights): + zvalue = 0.0 + for k, kweight in zip(kshift, kweights): + zvalue += ( + fineData[fineIndexX + i][fineIndexY + j][ + fineIndexZ + k + ] + * kweight + ) + yvalue += zvalue * jweight + value += yvalue * iweight + return value # all primal is for moments if all(is_primal): - value = 0.0 ishift = (-1, 0, 1) iweights = (0.25, 0.5, 0.25) jshift = (-1, 0, 1) jweights = (0.25, 0.5, 0.25) kshift = (-1, 0, 1) kweights = (0.25, 0.5, 0.25) - for i, iweight in zip(ishift, iweights): - yvalue = 0.0 - for j, jweight in zip(jshift, jweights): - zvalue = 0.0 - for k, kweight in zip(kshift, kweights): - zvalue += ( - fineData[fineIndexX + i][fineIndexY + j][ - fineIndexZ + k - ] - * kweight - ) - yvalue += zvalue * jweight - value += yvalue * iweight + value = compute_weighted_value(fineData, fineIndexX, fineIndexY, fineIndexZ, + ishift, iweights, jshift, jweights, kshift, kweights) coarseData[ coarseLocalIndexX, coarseLocalIndexY, coarseLocalIndexZ ] = valueThis pattern can then be reused for the other similar blocks at lines 203-230, 232-261, and 263-292.
tests/core/numerics/interpolator/test_main.cpp (3)
739-739:⚠️ Potential issueFix the grid layout initialization to include all three dimensions.
The grid layout initialization is incorrect as it only specifies two dimensions (
{nx, ny}) for a 3D grid.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)};
768-768:⚠️ Potential issueFix the interpolator call signature to match 1D and 2D implementations.
The interpolator call signature is inconsistent with the 1D and 2D implementations, which use
makeIndexRange.Apply this diff to maintain consistency with other dimensions:
- interpolator(particles, rho, v, layout); + interpolator(makeIndexRange(particles), rho, v, layout);
773-781:⚠️ Potential issueUncomment and fix the test assertions to properly test 3D interpolation.
The test case is currently commented out, which means it's not performing any actual verification. This test should be uncommented and updated to test the correct deposition of weights and velocities at the appropriate index.
Apply this diff to restore and fix the test assertions:
- // 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); + auto idx = 2 + this->layout.nbrGhosts(QtyCentering::dual); + auto const& [vx, vy, vz] = this->v(); + EXPECT_DOUBLE_EQ(this->rho(idx, idx, idx), 1.0); + EXPECT_DOUBLE_EQ(vx(idx, idx, idx), 2.0); + EXPECT_DOUBLE_EQ(vy(idx, idx, idx), -1.0); + EXPECT_DOUBLE_EQ(vz(idx, idx, idx), 1.0);tests/functional/harris/harris_3d.py (1)
104-108:⚠️ Potential issuePrevent potential assertion errors in temperature calculation.
The temperature function could raise an assertion error if
b2(x, y, z) * 0.5exceedsK = 1. This would happen if the magnetic field pressure becomes too high, leading to failed simulations.Apply this diff to add bounds checking:
- K = 1 + K = 1.5 # Increased to provide safety margin temp = 1.0 / density(x, y, z) * (K - b2(x, y, z) * 0.5) - assert np.all(temp > 0) + if not np.all(temp > 0): + raise ValueError("Temperature calculation resulted in negative values. " + "Consider adjusting K or magnetic field components.") return temptools/python3/cmake.py (2)
47-48:⚠️ Potential issueFix undefined variable in run_test function.
The function uses an undefined variable
cmdinstead of thetestparameter.- run(test_cmd(cmd, verbose=verbose), capture_output=capture_output) + run(test_cmd(test, verbose=verbose), capture_output=capture_output)🧰 Tools
🪛 Ruff (0.8.2)
48-48: Undefined name
cmd(F821)
23-27:⚠️ Potential issueFix missing parameter in config function.
The
use_ccacheparameter is not being passed tomake_config_str(), which will prevent ccache from being enabled when specified.- cmd = make_config_str(path, samrai_dir, cxx_flags, use_ninja, extra) + cmd = make_config_str(path, samrai_dir, cxx_flags, use_ninja, use_ccache, extra)
🧹 Nitpick comments (37)
src/core/utilities/meta/meta_utilities.hpp (1)
90-97:❓ Verification inconclusive
Consider optimizing 3D particle permutations.
The implementation of 3D simulator options is solid, but note the TODO comment about adding more 3D particle permutations. In line 94, there's a commented-out value
27that might be needed in the future.For 3D simulations, particle counts can grow substantially. Consider implementing more efficient data structures or algorithms to handle larger particle counts, especially when working with
InterpConst<3>which likely requires more computational resources.
🏁 Script executed:
#!/bin/bash # Description: Check if there are any performance optimizations or memory concerns addressed elsewhere # in relation to 3D particle handling echo "Searching for related performance optimizations for 3D particles..." rg -A 3 "particle.*performance|memory.*optimization|3D.*optimization" --type cppLength of output: 227
Attention: Revisit 3D Simulator Options for Future Scalability
The implementation for 3D simulator options appears solid; however, the TODO comment highlights that additional 3D particle permutations (including the commented-out value
27) might be needed to manage larger particle counts. No further performance or memory optimization measures specific to 3D particles were found elsewhere in the code. Given the potential for scalability issues—especially withInterpConst<3>—please verify through manual benchmarks or further static analysis that the current design will not introduce performance bottlenecks when these additional permutations are later enabled.src/core/data/ndarray/ndarray_vector.hpp (4)
123-124: Good implementation of zstart() and zend() methods.The implementation correctly extends the existing pattern for 2D operations to support 3D operations. Consider whether having dimension-specific min/max values might be more flexible for future use cases instead of using the same min/max values for all dimensions.
- NO_DISCARD auto zstart() const { return mask_.min(); } - NO_DISCARD auto zend() const { return shape_[2] - 1 - mask_.max(); } + NO_DISCARD auto zstart() const { return mask_.minZ(); } + NO_DISCARD auto zend() const { return shape_[2] - 1 - mask_.maxZ(); }
383-424: Implement boundary checks in 3D fill method to improve robustness.The 3D fill implementation could benefit from boundary validations before execution to prevent potential out-of-bounds access. Also, there's a commented line
// // right borderon line 394 that has a double comment character, which should be fixed.template<typename Array> void fill3D(Array& array, typename Array::type val) const { auto shape = array.shape(); + + // Validate that mask fits within the array dimensions + assert(max_ < shape[0]/2 && max_ < shape[1]/2 && max_ < shape[2]/2); // left border for (auto i = min_; i <= shape[0] - 1 - max_; ++i) for (auto j = min_; j <= shape[1] - 1 - max_; ++j) for (auto k = min_; k <= max_; ++k) array(i, j, k) = val; - // // right border + // right border for (auto i = min_; i <= shape[0] - 1 - max_; ++i) for (auto j = min_; j <= shape[1] - 1 - max_; ++j) for (auto k = shape[2] - 1 - max_; k <= shape[2] - 1 - min_; ++k) array(i, j, k) = val;
433-450: Add comments explaining the 3D cells calculation for better maintainability.The 3D calculation in nCells is complex and would benefit from a comment explaining the formula and reasoning. This would make future maintenance easier.
if constexpr (Array::dimension == 3) { auto [x, y, z] = shape; x -= i * 2; y -= i * 2; z -= i * 2; + // Calculate total number of cells in 3D ghost layer: + // (x*y*2) -> front and back face cells + // (y*(z-2)*2) -> left and right face cells (excluding corners already counted) + // ((z-2)*(x-2)*2) -> bottom and top face cells (excluding corners and edges already counted) cells += (x * y * 2) + (y * (z - 2) * 2) + ((z - 2) * (x - 2) * 2); }
527-595: Well-structured implementation of 3D operator>> with good use of a lambda function.The implementation for 3D MaskedView operator>> is thorough and follows the pattern established for 2D. The use of a lambda function for corner handling is a good approach to reduce code duplication. Consider further refactoring opportunities to make the code more maintainable:
- The edge handling could be factored into helper functions similar to the corner handling.
- Consider adding debug assertions to verify that values are properly transferred.
You could refactor the edge handling similar to the corner handling:
if constexpr (MaskedView_t::dimension == 3) { assert(inner.zstart() > outer.zstart() and inner.zend() < outer.zend()); + // Helper function for handling edges along x axis + auto edgeX = [&](auto i) { + for (auto k = inner.zstart(); k <= inner.zend(); ++k) + { + outer(i, outer.ystart(), k) = inner(i, inner.ystart(), k); + outer(i, outer.yend(), k) = inner(i, inner.yend(), k); + } + for (auto j = inner.ystart(); j <= inner.yend(); ++j) + { + outer(i, j, outer.zstart()) = inner(i, j, inner.zstart()); + outer(i, j, outer.zend()) = inner(i, j, inner.zend()); + } + }; for (auto i = inner.xstart(); i <= inner.xend(); ++i) { - for (auto k = inner.zstart(); k <= inner.zend(); ++k) - { - outer(i, outer.ystart(), k) = inner(i, inner.ystart(), k); - outer(i, outer.yend(), k) = inner(i, inner.yend(), k); - } - for (auto j = inner.ystart(); j <= inner.yend(); ++j) - { - outer(i, j, outer.zstart()) = inner(i, j, inner.zstart()); - outer(i, j, outer.zend()) = inner(i, j, inner.zend()); - } + edgeX(i); }src/amr/data/particles/refine/split_3d.hpp (2)
15-18: Avoidusing namespacein a header file.Inclusion of
using namespace PHARE::core;within a header file can cause unexpected namespace pollutions across the codebase. It’s usually best to qualify symbols or use namespace aliases in source files rather than in headers.
119-294: Consider refactoring repeated splitter definitions.Many
Splitterspecializations differ only by small constants or pattern combinations. Centralizing these repetitive definitions via macros or more generic templates could reduce boilerplate and promote easier maintenance.tests/simulator/__init__.py (1)
98-108: Optional docstring for 3D density function.Although consistent with the 1D/2D variants, it may be helpful to provide a docstring explaining the 3D density calculation logic and any assumptions or domain constraints. This helps clarify its usage and maintainability.
tests/simulator/advance/test_particles_advance_3d.py (1)
25-43: Method naming inconsistency with actual implementation.The method name
test_overlapped_particledatas_have_identical_particlesdoesn't match the actual implementation method_test_overlapped_particledatas_have_identical_particles. This mismatch can cause confusion for maintainers.Consider renaming for consistency:
-def test_overlapped_particledatas_have_identical_particles( +def test_overlapped_particledatas_have_identical_particles( self, interp_order, refinement_boxes ): self._test_overlapped_particledatas_have_identical_particles( ndim, interp_order, refinement_boxes, ppc=ppc, cells=40, largest_patch_size=20, )pyphare/pyphare/core/box.py (2)
206-207: Ambiguous variable names in select function.The single-letter variable names
landuin the list comprehension make the code less readable and maintainable. This was also flagged by static analysis.-def select(data, box): - return data[tuple([slice(l, u + 1) for l,u in zip(box.lower, box.upper)])] +def select(data, box): + return data[tuple([slice(lower, upper + 1) for lower, upper in zip(box.lower, box.upper)])]🧰 Tools
🪛 Ruff (0.8.2)
207-207: Ambiguous variable name:
l(E741)
222-223: Consider adding docstring to the setitem method.While the implementation is concise, adding a docstring would improve clarity about what's happening - the method gets the array view via getitem and then assigns values to it using slice assignment.
def __setitem__(self, box_or_slice, val): + """Assign values to the selected region of data. + + Gets a view of the data via __getitem__ and then uses slice assignment to modify it. + """ self.__getitem__(box_or_slice)[:] = valtools/python3/git.py (2)
5-16: Add docstrings to Git utility functions.The
current_branch,hashes, andlogfunctions lack proper docstrings. Adding documentation would improve code maintainability and help users understand the purpose and expected behavior of these functions.def current_branch(): + """Return the name of the current Git branch.""" return decode_bytes(run("git branch --show-current").stdout) def hashes(N=1): + """Return a list of the last N commit hashes. + + Args: + N: Number of commit hashes to return + + Returns: + List of commit hash strings + """ return decode_bytes(run(f"git log -{N} --pretty=format:%h").stdout).splitlines() def log(N=10, use_short=False): + """Return the Git log for the last N commits. + + Args: + N: Number of commits to include in the log + use_short: When True, uses a compact format for log output + + Returns: + String containing the git log output + """ # https://git-scm.com/docs/pretty-formats short = '--pretty=format:"%h%x09%an%x09%ad%x09%s"' if use_short else "" return decode_bytes(run(f"git log -{N} {short}").stdout)
19-24: Unused exception variable.The exception variable
eis assigned but never used. This was flagged by static analysis.def branch_exists(branch): try: run(f"git show-branch {branch}", check=True) - except subprocess.CalledProcessError as e: + except subprocess.CalledProcessError: return False # exit failure means branch does not exist return True🧰 Tools
🪛 Ruff (0.8.2)
22-22: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
tests/amr/data/field/coarsening/coarsening_utils.py (4)
8-22: Implementation looks good but docstring could be improved.The function's docstring explains the purpose clearly, but could be enhanced by adding parameter descriptions and return value information.
def coarsen(qty, coarseField, fineField, coarseBox, fineData, coarseData): """ this function takes data in fineData (the dataset from the fineFied FieldData object) and put it in the coarseData dataset The coarsening method is the equivalent (and union) of the C++ coarsening methods, namely: - DefaultFieldCoarsener, used for all fields (moments and E) but the magnetic field B - MagneticFieldCoarsener, used for the magnetic field B as in the C++ counterpart, we hard-code here that the layout of B follows the yee layout. That is Bx is PDD, By is DPD and Bz is DDP + + Parameters: + ----------- + qty : str + The field quantity name ("Bx", "By", "Bz", etc.) + coarseField : Field + The coarse field object + fineField : Field + The fine field object + coarseBox : Box + The coarse box defining the region to fill + fineData : ndarray + The source data from the fine field + coarseData : ndarray + The target data array for the coarse field """
80-95: Boolean condition should be checked first before further processing.The code performs field-specific calculations after checking the primality, but for clarity and efficiency, it should check if the quantity is "Bx" first before calculating temporary values that might not be used.
if is_primal[0] and not is_primal[1]: - if qty == "Bx": - coarseData[coarseLocalIndexX, coarseLocalIndexY] = 0.5 * ( - fineData[fineIndexX, fineIndexY] - + fineData[fineIndexX, fineIndexY + 1] - ) - else: + if qty != "Bx": left += fineData[fineIndexX - 1][fineIndexY] * 0.5 left += fineData[fineIndexX - 1][fineIndexY + 1] * 0.5 middle += fineData[fineIndexX][fineIndexY] * 0.5 middle += fineData[fineIndexX][fineIndexY + 1] * 0.5 right += fineData[fineIndexX + 1][fineIndexY] * 0.5 right += fineData[fineIndexX + 1][fineIndexY + 1] * 0.5 coarseData[coarseLocalIndexX][coarseLocalIndexY] = ( left * 0.25 + middle * 0.5 + right * 0.25 ) + else: + coarseData[coarseLocalIndexX, coarseLocalIndexY] = 0.5 * ( + fineData[fineIndexX, fineIndexY] + + fineData[fineIndexX, fineIndexY + 1] + )
181-182: Use a singleifstatement instead of nested statements.The code uses an
elifstatement after checking conditions in the previousifblock, but these conditions are mutually exclusive. Converting this to a separateifstatement improves readability.-elif not is_primal[0] and is_primal[1] and not is_primal[2]: +if not is_primal[0] and is_primal[1] and not is_primal[2]: if qty == "By": coarseData[ coarseLocalIndexX, coarseLocalIndexY, coarseLocalIndexZ ] = 0.25 * (🧰 Tools
🪛 Ruff (0.8.2)
181-182: Use a single
ifstatement instead of nestedifstatements(SIM102)
170-190: Improve consistency in conditional checks for magnetic field components.The code structure varies between different conditional blocks. For consistent code structure, use the same pattern for all magnetic field components.
if is_primal[0] and not is_primal[1] and not is_primal[2]: - if qty == "Bx": - coarseData[ - coarseLocalIndexX, coarseLocalIndexY, coarseLocalIndexZ - ] = 0.25 * ( - fineData[fineIndexX, fineIndexY, fineIndexZ] - + fineData[fineIndexX, fineIndexY + 1, fineIndexZ] - + fineData[fineIndexX, fineIndexY, fineIndexZ + 1] - + fineData[fineIndexX, fineIndexY + 1, fineIndexZ + 1] - ) + # Check for Bx component with primality PDD + if qty == "Bx": + coarseData[ + coarseLocalIndexX, coarseLocalIndexY, coarseLocalIndexZ + ] = 0.25 * ( + fineData[fineIndexX, fineIndexY, fineIndexZ] + + fineData[fineIndexX, fineIndexY + 1, fineIndexZ] + + fineData[fineIndexX, fineIndexY, fineIndexZ + 1] + + fineData[fineIndexX, fineIndexY + 1, fineIndexZ + 1] + ) + # For other fields, implement the appropriate coarsening algorithm + else: + # Add implementation for other fields with PDD primality + passApply similar structure to the other magnetic field component blocks.
🧰 Tools
🪛 Ruff (0.8.2)
181-182: Use a single
ifstatement instead of nestedifstatements(SIM102)
tests/functional/harris/harris_3d.py (3)
62-62: Replace ambiguous variable name 'l' with more descriptive name.The single-letter variable name 'l' is ambiguous and can be easily confused with the number '1' or capital letter 'I'. Use a more descriptive name like 'scale_length' for better readability.
-def S(y, y0, l): - return 0.5 * (1.0 + np.tanh((y - y0) / l)) +def S(y, y0, scale_length): + return 0.5 * (1.0 + np.tanh((y - y0) / scale_length))🧰 Tools
🪛 Ruff (0.8.2)
62-62: Ambiguous variable name:
l(E741)
177-182: Address TODOs for 3D plotting functionality.The TODOs indicate that 3D plotting functionality has been removed but no alternative has been implemented. This should be addressed to ensure proper visualization of 3D simulation results.
Would you like me to help implement a solution for 3D plotting? I could suggest approaches such as:
- Using libraries like Matplotlib's 3D plotting capabilities
- Creating slice-based 2D visualizations of 3D data
- Integrating with visualization tools like ParaView or VisIt
Let me know which approach you'd prefer and I can provide code implementation suggestions.
142-172: Clean up commented-out plotting code and improve active plotting functionality.The plotting function contains several commented-out sections. Either remove these completely or implement them properly for 3D data. The current mixture of active and inactive code makes maintenance difficult.
Consider refactoring the plotting function to use a more modular approach:
def plot(diag_dir): run = Run(diag_dir) + # Define available plot types and their configurations + plot_configs = { + "N": {"pop_name": "protons", "plot_patches": True}, + "B": {"components": ["x", "y", "z"], "plot_patches": True} + # Add more plot types as needed + } + for time in timestamps: - # run.GetDivB(time).plot( - # filename=plot_file_for_qty("divb", time), - # plot_patches=True, - # vmin=1e-11, - # vmax=2e-10, - # ) - # run.GetRanks(time).plot( - # filename=plot_file_for_qty("Ranks", time), - # plot_patches=True, - # ) - run.GetN(time, pop_name="protons").plot( - filename=plot_file_for_qty("N", time), - plot_patches=True, - ) - for c in ["x", "y", "z"]: - run.GetB(time).plot( - filename=plot_file_for_qty(f"b{c}", time), - qty=f"B{c}", - plot_patches=True, - ) - # run.GetJ(time).plot( - # filename=plot_file_for_qty("jz", time), - # qty="Jz", - # plot_patches=True, - # vmin=-2, - # vmax=2, - # ) + # Plot density + if "N" in plot_configs: + cfg = plot_configs["N"] + run.GetN(time, pop_name=cfg["pop_name"]).plot( + filename=plot_file_for_qty("N", time), + plot_patches=cfg["plot_patches"], + ) + + # Plot magnetic field components + if "B" in plot_configs: + cfg = plot_configs["B"] + for c in cfg["components"]: + run.GetB(time).plot( + filename=plot_file_for_qty(f"b{c}", time), + qty=f"B{c}", + plot_patches=cfg["plot_patches"], + )tests/simulator/refined_particle_nbr.py (1)
44-48: Remove the TODO comment and clarify the 3D calculation logic.The TODO comment on line 47 asks about handling dim==3, but the code already implements this case on line 48. The logic for calculating the particle numbers in 3D needs better documentation to explain the formula.
- dim2 = refined_particle_nbr * ((cellNbr[0] * 2 + (cellNbr[1] * 2))) + # Calculate particles on x-y surface boundaries + dim2 = refined_particle_nbr * ((cellNbr[0] * 2 + (cellNbr[1] * 2))) if dim == 2: return dim2 - # TODO3D if dim==3? + # For 3D, also consider particles on z surface boundaries return dim2 * (cellNbr[2] * 2)tools/python3/cmake.py (1)
4-5: Implement or remove the empty version function.The
version()function is defined but has no implementation (it just containspass). This should either be implemented to return the CMake version or removed if not needed.-def version(): - pass +def version(): + """Return the CMake version.""" + proc = run("cmake --version", capture_output=True) + output = decode_bytes(proc.stdout) + # Extract version from first line which is typically: + # cmake version X.Y.Z + return output.splitlines()[0].split(' ')[2]pyphare/pyphare/pharesee/geometry.py (2)
159-182: Improve variable naming for better readability.The code uses single-letter variable name
lwhich is ambiguous and can be confused with the number 1 or capital I in some fonts. Replace it with a more descriptive name.- front = { - f"{k}front": [(v[0], v[1], shape_z) for v in l] for k, l in shifts.items() - } - back = { - f"{k}back": [([v[0], v[1], -shape_z]) for v in l] for k, l in shifts.items() - } - - shifts = {k: [([v[0], v[1], 0]) for v in l] for k, l in shifts.items()} + front = { + f"{k}front": [(v[0], v[1], shape_z) for v in shift_list] for k, shift_list in shifts.items() + } + back = { + f"{k}back": [([v[0], v[1], -shape_z]) for v in shift_list] for k, shift_list in shifts.items() + } + + shifts = {k: [([v[0], v[1], 0]) for v in shift_list] for k, shift_list in shifts.items()}🧰 Tools
🪛 Ruff (0.8.2)
161-161: Ambiguous variable name:
l(E741)
164-164: Ambiguous variable name:
l(E741)
167-167: Ambiguous variable name:
l(E741)
188-188: Improve variable naming consistency.Similar to the previous comment, the variable
lis used here as well. Replace it with a more descriptive name for better readability.- shifts = {"".join(sorted(k)): l for k, l in shifts.items()} + shifts = {"".join(sorted(k)): shift_list for k, shift_list in shifts.items()}🧰 Tools
🪛 Ruff (0.8.2)
188-188: Ambiguous variable name:
l(E741)
res/cmake/funcs.cmake (3)
44-57: Clarify usage ofadd_phare_test_versus top-level test functions.Currently,
add_phare_test_is invoked by bothadd_phare_testandadd_no_mpi_phare_test. For new contributors, it may be unclear under which conditions each function should be used. Consider adding a small comment block or docstring toadd_phare_test_explaining its role (e.g., environment configuration vs. user-facing usage).
107-109: Remove or implement the placeholder functionadd_phare_build_test.This function is currently empty, which might be confusing. If it is intentionally a no-op, document why. Otherwise, remove or implement it to avoid confusion.
185-191: Consider scoping or grouping the outputs ofphare_print_all_vars.While printing all CMake variables can help in debugging, it may generate noisy logs. Consider adding an optional parameter or filter (e.g., a prefix) to only print PHARE-specific variables, reducing excessive output during normal development.
src/amr/data/field/coarsening/magnetic_field_coarsener.hpp (1)
226-333: Consider merging 3D patterns using a dimension-agnostic approach.Each 3D component block (Bx, By, Bz) follows the same multi-point averaging pattern. A dimension-agnostic loop or partial template specialization could reduce repetitive code and risk of bugs.
tests/simulator/test_advance.py (4)
289-290: Rename the unused loop variableilvl.In the loop “for ilvl, overlaps in hierarchy_overlaps(...):”,
ilvlis not used within the body. Rename it to_ilvlor remove it to address potential linter warnings and clarify that it’s unused.🧰 Tools
🪛 Ruff (0.8.2)
290-290: Loop control variable
ilvlnot used within loop bodyRename unused
ilvlto_ilvl(B007)
293-315: Check performance impact of repeated indexing in overlap slices.Multiple transformations and slices (
amr_to_local,boxm.shift, etc.) may introduce moderate overhead if the dataset is large and repeated frequently. If performance becomes critical, consider caching intermediate results to reduce repeated computations in tight loops.
545-556: Hardcoded sentinel value-144123.Here, a sentinel is used to detect uninitialized or overridden array data. Document in code comments the meaning of
-144123or consider using a self-explanatory constant (e.g.,INVALID_DATA_MARKER = -144123) to improve readability.
713-735: Enhance error reporting for sub-cycle interpolation tests.In case of failures, the printed debug info is valuable but could be more structured (e.g., JSON or multiline formatting). Clear formatting often speeds up debugging.
tests/simulator/test_initialization.py (5)
417-420: Parameter naming inconsistency between methodsThe parameter name has changed from
dimtondimin this method, while some other methods still usedim. Consider standardizing all parameter names tondimfor consistency.- def _test_B_is_as_provided_by_user(self, dim, interp_order, ppc=100, **kwargs): + def _test_B_is_as_provided_by_user(self, ndim, interp_order, ppc=100, **kwargs):Similar changes should be made throughout the file for consistency.
Also applies to: 425-425
582-582: Unused loop control variableThe loop control variable
piis not used within the loop body. Consider renaming it to_to indicate it's intentionally unused.- for pi, patch in enumerate(datahier.level(0).patches): + for _, patch in enumerate(datahier.level(0).patches):🧰 Tools
🪛 Ruff (0.8.2)
582-582: Loop control variable
pinot used within loop bodyRename unused
pito_pi(B007)
585-586: Consider using NumPy's array testing methods for histogram verificationInstead of using a boolean assertion with
.all(), consider using NumPy's testing utilities which provide better error messages on failure.- self.assertTrue((H == ppc).all()) + np.testing.assert_array_equal(H, ppc * np.ones_like(H), + err_msg="Not all cells have exactly ppc particles")🧰 Tools
🪛 Ruff (0.8.2)
586-586: Yoda condition detected
Rewrite as
ppc == H(SIM300)
664-664: Unused variableoutThe variable
outis assigned but never used. Consider removing it if it's not needed.- out = "phare_outputs"🧰 Tools
🪛 Ruff (0.8.2)
664-664: Local variable
outis assigned to but never usedRemove assignment to unused variable
out(F841)
394-398: Closure variable binding issue in nested functionThe nested function
domain()references variables from the outer scope (dim,nbrGhosts,patch) which could lead to unexpected behavior if these variables change. Consider passing these as arguments to the function instead.- def domain(patch_data): - if dim == 1: - return patch_data.dataset[select] - return patch_data.dataset[:].reshape( - patch.box.shape + (nbrGhosts * 2) + 1 - )[select] + def domain(patch_data, dim_value, nb_ghosts, patch_box): + if dim_value == 1: + return patch_data.dataset[select] + return patch_data.dataset[:].reshape( + patch_box.shape + (nb_ghosts * 2) + 1 + )[select]Then call it with:
domain(pdatas["rho"], dim, nbrGhosts, patch.box)🧰 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)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (56)
.github/workflows/cmake_macos.yml(1 hunks)pyphare/pyphare/core/box.py(2 hunks)pyphare/pyphare/core/gridlayout.py(1 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/funcs.cmake(1 hunks)res/cmake/options.cmake(2 hunks)src/amr/data/field/coarsening/coarsen_weighter.hpp(1 hunks)src/amr/data/field/coarsening/magnetic_field_coarsener.hpp(1 hunks)src/amr/data/particles/refine/particles_data_split.hpp(2 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(5 hunks)src/core/data/grid/grid.hpp(1 hunks)src/core/data/grid/gridlayoutdefs.hpp(2 hunks)src/core/data/ndarray/ndarray_vector.hpp(8 hunks)src/core/data/particles/particle.hpp(3 hunks)src/core/utilities/box/box.hpp(1 hunks)src/core/utilities/meta/meta_utilities.hpp(2 hunks)tests/amr/data/field/coarsening/coarsening_utils.py(1 hunks)tests/amr/data/field/coarsening/test_weighter_coarsen.hpp(1 hunks)tests/amr/data/field/refine/CMakeLists.txt(1 hunks)tests/amr/data/field/refine/test_default_field_refinement.cpp(4 hunks)tests/amr/data/field/refine/test_field_refinement_on_hierarchy.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/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(3 hunks)tests/simulator/advance/CMakeLists.txt(1 hunks)tests/simulator/advance/test_fields_advance_1d.py(1 hunks)tests/simulator/advance/test_fields_advance_2d.py(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(16 hunks)tests/simulator/test_initialization.py(13 hunks)tests/simulator/utilities/field_coarsening.py(0 hunks)tests/simulator/utilities/test_refine_field.py(2 hunks)tools/python3/cmake.py(1 hunks)tools/python3/git.py(1 hunks)
💤 Files with no reviewable changes (2)
- tests/simulator/utilities/field_coarsening.py
- tests/simulator/initialize/test_particles_init_1d.py
🚧 Files skipped from review as they are similar to previous changes (33)
- src/amr/data/field/coarsening/coarsen_weighter.hpp
- tests/simulator/advance/CMakeLists.txt
- .github/workflows/cmake_macos.yml
- src/core/utilities/box/box.hpp
- tests/diagnostic/job_3d.py.in
- src/amr/data/particles/refine/particles_data_split.hpp
- src/amr/data/particles/refine/split.hpp
- tests/simulator/initialize/test_fields_init_2d.py
- src/core/data/grid/grid.hpp
- tests/amr/data/field/refine/CMakeLists.txt
- res/cmake/options.cmake
- tests/simulator/initialize/CMakeLists.txt
- pyphare/pyphare/pharesee/run/utils.py
- tests/simulator/initialize/test_particles_init_2d.py
- tests/core/data/ndarray/test_main.cpp
- src/core/data/grid/gridlayoutdefs.hpp
- pyphare/pyphare/core/gridlayout.py
- tests/diagnostic/CMakeLists.txt
- tests/simulator/initialize/test_particles_init_3d.py
- tests/amr/data/field/refine/test_default_field_refinement.cpp
- tests/simulator/per_test.hpp
- tests/simulator/advance/test_fields_advance_1d.py
- tests/simulator/advance/test_fields_advance_2d.py
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py
- tests/amr/data/particles/refine/test_split.cpp
- tests/simulator/utilities/test_refine_field.py
- tests/amr/data/field/coarsening/test_weighter_coarsen.hpp
- tests/amr/data/field/refine/test_field_refinement_on_hierarchy.cpp
- src/core/data/particles/particle.hpp
- src/amr/data/particles/refine/splitter.hpp
- tests/simulator/initialize/test_fields_init_3d.py
- src/amr/data/particles/refine/split_1d.hpp
- src/amr/data/particles/refine/split_2d.hpp
🧰 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.hppsrc/core/data/ndarray/ndarray_vector.hppsrc/amr/data/field/coarsening/magnetic_field_coarsener.hppsrc/amr/data/particles/refine/split_3d.hpp
🪛 Ruff (0.8.2)
tests/simulator/__init__.py
326-326: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
tools/python3/git.py
22-22: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
29-29: Undefined name delete_branch
(F821)
pyphare/pyphare/core/box.py
207-207: Ambiguous variable name: l
(E741)
tools/python3/cmake.py
48-48: Undefined name cmd
(F821)
tests/functional/harris/harris_3d.py
62-62: 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)
tests/amr/data/field/coarsening/coarsening_utils.py
181-182: Use a single if statement instead of nested if statements
(SIM102)
tests/simulator/test_advance.py
290-290: Loop control variable ilvl not used within loop body
Rename unused ilvl to _ilvl
(B007)
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 (ubuntu-latest)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-14)
🔇 Additional comments (52)
src/core/utilities/meta/meta_utilities.hpp (3)
9-11: Good approach for configurable simulator dimensionality.The preprocessor directive with a default value for
PHARE_SIMULATORSprovides flexibility, allowing users to override the maximum dimensions supported at compile time. This approach helps control build size and compilation time while enabling 3D simulations by default.
81-88: Clean transition to conditional dimension support.The conditional compilation approach for 2D simulators is well-implemented, maintaining backward compatibility while enabling the transition to 3D. The code structure keeps the tuple format consistent.
98-98: Ensure consistent tuple formatting.The closing bracket with semicolon
>{};is correctly placed, maintaining the formatting consistency of the function. The conditional compilation blocks are properly structured to ensure valid C++ syntax regardless of which preprocessor branches are taken.src/core/data/ndarray/ndarray_vector.hpp (5)
6-6: Good addition of the<algorithm>include.The addition of the algorithm header is appropriate as the code uses std::fill, std::copy, and other algorithm functions.
348-352: Good use of auto for loop variables.Changing from explicit
std::size_ttoautois a good practice in modern C++ for loop variables, making the code more flexible and maintainable.
361-381: Consider boundary checks to prevent off-by-one issues in 2D borders.While the code appears correct for inclusive boundaries, if
min_ormax_become larger than expected, you risk overlaps or out-of-range indexing. Consider adding boundary checks or assertions to ensure the indices remain valid.
469-470: Good update to include x dimension assertions.The assertion is correctly updated to ensure proper nesting of the masked views in the x dimension.
477-480: Enhanced dimension checking for multi-dimensional arrays.The check properly ensures that dimensions greater than 1 have the appropriate y-dimension assertions.
src/amr/data/particles/refine/split_3d.hpp (1)
90-116: Verify the use of the templated constructor inWhitePattern.The constructor is templated on
Delta, but it is not immediately visible how or where this is instantiated. If unused, consider removing or confirming its necessity.tests/simulator/__init__.py (1)
326-345: Address mutable default argument indebug_tracer.This was previously flagged. Python recommends against using mutable objects (
[0]) as default parameter values, as they can lead to unexpected behavior when reusing the function.Apply this diff to fix:
-def tracefunc(frame, event, arg, indent=[0]): +def tracefunc(frame, event, arg, indent=None): + if indent is None: + indent = [0]🧰 Tools
🪛 Ruff (0.8.2)
326-326: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
tests/diagnostic/test-diagnostics_3d.cpp (1)
1-36: Looks good!The diagnostic tests for 3D scenarios appear well-structured and consistent with the workflow for the fluid, particles, electromag, and Python-driven tests. The main function is straightforward and correctly invokes the PHARE lifecycle before running Google Tests.
🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 24-24: Function parameter 'path' should be passed by const reference.
(passedByValue)
[performance] 36-36: Function parameter 'path' should be passed by const reference.
(passedByValue)
tests/simulator/advance/test_particles_advance_3d.py (8)
1-4: Clear docstring explaining the purpose of this module.Good documentation that clearly explains why this file exists separately from
test_advance.py- to isolate dimension-specific test cases.
6-14: Import organization is appropriate.The imports are well-organized, with standard library imports first, followed by third-party libraries, and then project-specific imports.
16-19: Good use of constants for dimensionality and test parameters.Defining constants like
ndim,interp_orders, andppcat the module level makes the code more maintainable and easier to modify for different testing scenarios.
21-22: Helper function enhances test readability.The
per_interpfunction nicely abstracts the creation of test cases for different interpolation orders, making the test data definition cleaner.
33-43: Good use of parameterized testing with refinement boxes.The test effectively uses data-driven testing with multiple box configurations to ensure the 3D functionality works across different scenarios.
45-47: Test method properly leverages base class functionality.Good reuse of the base test logic while providing 3D-specific parameters.
50-51: Standard unittest entrypoint in place.Properly implemented main block for standalone test execution.
1-51:❌ Incorrect review comment
Verify coverage of 3D test scenarios.
Based on the AI summary, this file should include tests for field coarsening and ghost level handling, but these methods appear to be missing:
test_field_coarsening_via_subcyclestest_field_level_ghosts_via_subcycles_and_coarser_interpolationEither the summary is incorrect or the implementation is incomplete.
🏁 Script executed:
#!/bin/bash # Check if these test methods exist in the PR branch grep -r "test_field_coarsening_via_subcycles\|test_field_level_ghosts_via_subcycles_and_coarser_interpolation" tests/Length of output: 2707
Clarification on 3D Particle Test Coverage
The tests for field coarsening and ghost level handling are not missing—instead, they are implemented in a separate file (e.g.,tests/simulator/advance/test_fields_advance_3d.py). The intent behind having this file (tests/simulator/advance/test_particles_advance_3d.py) is to isolate 3D particle test cases from field-specific tests, which aligns with the established test organization.Likely an incorrect or invalid review comment.
res/amr/splitting.yml (2)
103-141: Well-structured 3D configuration maintained consistency with existing patterns.The
dimension_3section follows the same logical pattern as the existingdimension_1anddimension_2sections, with appropriate parameters for 3D operations:
- Each interpolation order (1-3) is properly defined
- Particle types (N_particles_6, N_particles_12, N_particles_27) mirror the design pattern of lower dimensions
- Delta and weight values are carefully specified with appropriate precision
This consistency makes maintenance easier and ensures a cohesive configuration system across dimensions.
103-115:❓ Verification inconclusive
Verify 3D delta and weight values with physics equations.
The delta and weight values for 3D operations appear to be carefully calculated, but it would be beneficial to verify they are derived from the correct physical equations for 3D particle interactions.
🌐 Web query:
What are the typical delta and weight values for 3D particle interpolation in particle-in-cell (PIC) simulations?💡 Result:
In 3D particle-in-cell (PIC) simulations, delta functions (shape functions) and particle weights are critical for balancing numerical accuracy and computational efficiency. Here's a breakdown of typical implementations:
1. Delta Functions (Shape Functions)
These determine how particle charge/current densities are interpolated to grid points and vice versa. Common choices include:
Nearest Grid Point (NGP):
- Zero-order shape function (Dirac delta approximation).
- Rarely used due to high noise but computationally simple.
Cloud-in-Cell (CIC):
- First-order linear interpolation (triangular-shaped cloud in 1D, trilinear in 3D).
- Most common choice for its balance of accuracy and simplicity [6][8].
- Mathematically:
[
S(x - X) = \prod_{d=1}^3 \begin{cases}
1 - \frac{|x_d - X_d|}{\Delta x_d}, & |x_d - X_d| \leq \Delta x_d \
0, & \text{otherwise}
\end{cases}
]Triangular-Shaped Cloud (TSC):
- Second-order quadratic interpolation for smoother fields [3][8].
- Reduces numerical noise compared to CIC but increases computational cost.
Higher-Order B-Splines:
- Used in advanced codes (e.g., WarpX) to suppress numerical instabilities [2][5].
- Example: Third-order B-splines for improved field solver accuracy [3][5].
2. Particle Weights
Weights represent the contribution of each macro-particle to physical quantities like charge density. Key considerations:
Standard PIC (Full-(f)):
- Weight (w_j = q_j / q_0), where (q_j) is the charge of the macro-particle and (q_0) is the charge of a physical particle.
- Typical values depend on the plasma density and computational constraints (e.g., (10^6)–(10^{12}) physical particles per macro-particle) [10][8].
(\delta f)-PIC Method:
- Weights track perturbations from a background distribution (f_0):
[
w_j = \frac{\delta f}{g(Z_j)} \propto \frac{f - f_0}{f_0}
]
where (g) is the marker distribution (often Maxwellian) [1][7].- Normalized to ensure (\sum w_j \approx 0) for small perturbations [1][7].
Adaptive Weighting:
- Weights adjust dynamically in phase-space remapping schemes to minimize noise [3][7].
3. Trade-offs and Best Practices
- Noise vs. Resolution: Lower-order methods (CIC) introduce more noise but are faster; higher-order methods (TSC, B-splines) improve accuracy at higher cost [2][6].
- Energy Conservation: Codes like WarpX use energy-conserving schemes (e.g., Esirkepov current deposition) to mitigate numerical heating [5][9].
- Velocity-Space Smoothing: For (\delta f) methods, weights are often initialized with a Maxwellian (g) to reduce statistical variance [1][7].
Example Workflow in 3D
- Charge Deposition:
Particles deposit charge on the grid using CIC/TSC delta functions:
[
\rho(\mathbf{x}_g) = \sum_j q_j S(\mathbf{x}_g - \mathbf{x}_j)
]- Field Solve:
Compute (\mathbf{E}) and (\mathbf{B}) on the grid via Maxwell’s equations [4][5].- Force Interpolation:
Fields are interpolated back to particles using the same shape function:
[
\mathbf{F}_j = q_j \sum_g \mathbf{E}(\mathbf{x}_g) S(\mathbf{x}_g - \mathbf{x}_j)
]- Weight Evolution:
In (\delta f)-PIC, weights are updated via:
[
\dot{w}j = -\frac{d}{dt} \ln f_0 \big|{\mathbf{Z}_j} \cdot \mathbf{\dot{Z}}_j
] [1][7].
Key References
- CIC/TSC: [8][3][6]
- (\delta f)-PIC weights: [1][7]
- High-order shape functions: [2][5][3]
- Energy conservation: [5][9]
This framework ensures self-consistent, noise-controlled simulations while maintaining scalability for large 3D domains.
Citations:
- 1: https://youjunhu.github.io/research_notes/particle_simulation/particle_simulation.pdf
- 2: https://pubs.aip.org/aip/pop/article/27/5/052106/290439/Is-it-necessary-to-resolve-the-Debye-length-in
- 3: https://extremescaleglobalpic.princeton.edu/sites/g/files/toruqf1001/files/media/100811805.pdf
- 4: https://www.particleincell.com/2010/es-pic-method/
- 5: https://warpx.readthedocs.io/en/22.11/theory/picsar_theory.html
- 6: https://static.ias.edu/pitp/2016/sites/pitp/files/pictutorialas2016.1.pdf
- 7: https://www.iter.org/sites/default/files/education/Bottino_lec.pdf
- 8: https://en.wikipedia.org/wiki/Particle-in-cell
- 9: https://pubs.aip.org/aip/pop/article/21/5/052703/319041/Enhanced-stopping-of-macro-particles-in-particle
- 10: https://ipag.osug.fr/~ceruttbe/Documentation/Bcerutti_Astrosim2019_PartI.pdf
Action: Verify 3D Interpolation Constants and Weight Format
- The delta and weight values for N_particles_6 and N_particles_12 (i.e. 0.966431/0.166666 and 0.74823/0.083333) look reminiscent of fractional distributions (e.g. 1/6 and 1/12) that one might expect from a linear interpolation (CIC); however, please ensure that these numbers are indeed derived from the correct 3D PIC physics equations.
- For N_particles_27, the snippet shows a delta of 1 accompanied by two weight values (.125 and .0625). Please verify whether these are intended to be combined (e.g. as an array or summed components) or if one of them is erroneous. Confirm that their derivation is consistent with the targeted interpolation method (e.g. if using a higher-order scheme like TSC or B-splines).
Ensure that the underlying equations used in your model are reflected accurately in these parameters.
pyphare/pyphare/core/box.py (3)
65-68: Good enhancement to Box subtraction operator to handle multiple boxes.The update to the
__sub__method extends functionality to handle lists or tuples of boxes, which improves the flexibility of the Box class. The type checking and assertion that all items are Box instances is a good safety measure.
185-198: New remove_all function handles multiple box removals efficiently.The implementation correctly manages the removal of multiple boxes from a target box. The algorithm:
- Processes boxes sequentially
- Keeps track of which boxes need to be removed
- Handles overlapping cases appropriately
This implementation is robust and addresses a common use case in AMR operations.
209-224: DataSelector class provides a clean interface for box-based data access.The DataSelector class is well designed with clear documentation and implements both
__getitem__and__setitem__for intuitive data access and modification. The implementation is concise while maintaining clarity.tests/core/numerics/interpolator/test_main.cpp (2)
774-774: Remove unused static variable warning.The static variable
gtest_DepositCorrectlyTheirWeight_3d_defined_is generated by the test registration macro but is not read. This is a common pattern in GTest and doesn't represent an actual issue.
786-786: Remove unused static variable warning.The static variable
gtest_testInterpolator_ACollectionOfParticles_3dis generated by the test registration macro but is not read. This is a common pattern in GTest and doesn't represent an actual issue.tests/amr/data/particles/refine/input/input_3d_ratio_2.txt (1)
46-51:✅ Verification successful
Consider adding required configuration for TileClustering and GriddingAlgorithm sections.
The TileClustering and GriddingAlgorithm sections are currently empty. If these sections require configuration parameters for a valid simulation, they should be added.
🏁 Script executed:
#!/bin/bash # Search for other 3D configuration files to check if empty sections are typical # Find other 3D input files and check their TileClustering and GriddingAlgorithm sections echo "Checking other 3D configuration files for reference..." fd "input.*3d.*\.txt" tests/ -x grep -A 10 "TileClustering" {}Length of output: 265
Confirmation: Empty Config Sections Are Intentional
Based on the verification, the empty configuration blocks for both the TileClustering and GriddingAlgorithm sections are consistent with other 3D configuration files. It appears these sections are intentionally left blank as part of the established pattern. No additional configuration is required at this time.
tests/simulator/refined_particle_nbr.py (4)
33-36: Good addition of a tearDown method.Adding a tearDown method ensures that the simulator is properly reset after each test, preventing state from one test affecting others. This is especially important if exceptions are raised during tests.
106-116: LGTM! Good 3D test case implementation.The 3D test case is well-defined with appropriate constants for minimum and maximum differences. The code correctly extends the testing framework to handle 3D scenarios.
118-136: Well-structured testing helper method.The
_check_deltas_and_weightsmethod is well organized, breaking down the validation process into clear steps. This method ensures the correct calculation of delta and weight values for particles across different dimensions.
140-145: Good test execution ordering.The explicit ordering of tests ensures that delta/weights are verified first, followed by dimension-specific tests. This ordered test execution provides a more systematic validation approach.
tests/simulator/advance/test_fields_advance_3d.py (3)
1-15: LGTM! Well-structured 3D test file header.The file is well-organized with clear imports and setup for 3D testing. The docstring explains the purpose of separating dimension-specific test cases, which is good practice for maintainability.
21-22: LGTM! Effective helper function.The
per_interpfunction elegantly generates test cases for different interpolation orders, promoting code reuse throughout the test file.
102-117: Good test implementation for level ghost fields.The implementation of the
test_field_level_ghosts_via_subcycles_and_coarser_interpolationmethod is well-done. I notice there's a commented-out@unittest.skipline which suggests this test was previously skipped but is now enabled, which is good for expanding test coverage to 3D.pyphare/pyphare/pharesee/geometry.py (4)
63-74: LGTM! Good implementation of 3D ghost boxes.The function now correctly handles 3D ghost boxes by defining them for all six faces of the 3D volume (left, right, bottom, top, front, back). This is a necessary extension for 3D functionality.
392-400: LGTM! Well-defined 3D sides dictionary.The addition of 3D sides (front and back) in the dictionary is well-implemented, correctly handling the domain boundaries in all three dimensions.
407-415: LGTM! Effective periodic patch handling.The implementation of periodic boundary conditions for patches is well-done, creating copies of patches with appropriate shifts based on their positioning relative to domain boundaries.
519-519: LGTM! Efficient box subtraction operation.The operation to subtract patch boxes from ghost area boxes is efficient and correctly handles the computation of level ghost boxes.
src/amr/data/field/coarsening/magnetic_field_coarsener.hpp (2)
65-79: Validate indexing logic infine_start_index.The function calculates the fine-level starting index by multiplying the coarse index by the ratio. Ensure that boundary conditions are checked upstream to avoid unexpected out-of-bounds access when
coarseIndexlies near domain extremes, especially for higher dimensions or custom ratio values.
126-224: Consolidate repeated averaging formulas for 2D components.The logic for Bx, By, and the dual-dual combination repeats in structure. Consider extracting the averaging step into a helper utility, passing the relevant fine indices. This can simplify future maintenance or expansions (e.g., if new weighting factors are introduced).
tests/simulator/test_advance.py (3)
415-417: Ensure the newcells=120parameter is tested for all relevant scenarios.With
_test_L0_particle_number_conservationnow supporting acells=120default, confirm other test paths also pass. Large cell counts may expose boundary corner cases or performance constraints.
444-465: Parameterizingcells=60in_test_field_coarsening_via_subcycles.This refactoring adds flexibility for different domain sizes. Double-check that the newly introduced parameter interacts properly with any user overrides and with
largest_patch_size=30to avoid mismatch or off-by-one indexing errors.
802-809: Validate the threshold inbase_test_field_level_ghosts_via_subcycles_and_coarser_interpolation.The assertion
self.assertGreater(successful_test_nbr, ...)indicates a minimal threshold for success. Ensure it covers all refined boxes or corner cases where a refinement might carry zero overlap.tests/simulator/test_initialization.py (9)
254-254: Added flexible particle count configuration - good improvement!Adding the
ppcparameter with a default value of 100 improves configurability of the test, allowing for different particle densities to be tested.Also applies to: 266-267
260-261: Excellent addition of timing diagnosticsAdding timing measurements is valuable for tracking test performance, especially when dealing with computationally intensive 3D simulation tests.
Also applies to: 270-274
329-351: 3D support correctly implemented for magnetic field testingThe implementation properly handles z-dimension coordinates and correctly applies the 3D meshgrid for testing magnetic field components. Good use of dimension-specific code blocks.
384-414: Well-structured domain selection and mesh handling for vector field testingThe refactored code provides cleaner dimension-agnostic handling of domain selection and mesh management. The nested function approach improves code organization.
🧰 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)
427-428: Adjustment of particle numbers for 3D testingGood adaptation of particle counts for 3D testing (20 particles per cell), which balances computational cost with testing accuracy. The empirical deviation tolerance of 2e-1 for 3D seems reasonable.
692-693: Consistent dimensionality checkingGood adaptation for extracting dimensionality from the patch box's
ndimattribute and verifying that simulation dimensions match. This improves robustness when dealing with multi-dimensional simulations.Also applies to: 697-698
528-536: Conditional plotting code properly updated for 3D supportThe plotting code is correctly updated to handle 3D data by checking the dimension and adjusting plot parameters accordingly. The filename now includes the dimensionality parameter, which is useful for result tracking.
458-467: Clean dimension-agnostic mesh grid and data selectionThe code now properly handles data selection and mesh grid generation for arbitrary dimensions, making it more maintainable and ready for 3D testing.
621-623: Early return for non-primary MPI rankGood addition of MPI rank check to avoid redundant test execution in parallel environments. This prevents unnecessary test duplication and potential issues with concurrent access.
| } | ||
| Main | ||
| { | ||
| dim = 1 |
There was a problem hiding this comment.
Fix dimensionality in Main section for 3D simulation.
The dim parameter is set to 1 despite this being a 3D configuration file. This appears to be inconsistent with the three-dimensional domain defined in the CartesianGridGeometry section.
- dim = 1
+ dim = 3📝 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.
| dim = 1 | |
| dim = 3 |
| def test_overlaped_fields_are_equal_with_min_max_patch_size_of_max_ghosts( | ||
| self, interp_order, refinement_boxes | ||
| ): | ||
| print(f"{self._testMethodName}_{ndim}d") | ||
| time_step_nbr = 3 | ||
| time_step = 0.001 | ||
| from pyphare.pharein.simulation import check_patch_size | ||
|
|
||
| largest_patch_size, smallest_patch_size = check_patch_size( | ||
| ndim, interp_order=interp_order, cells=[cells] * ndim | ||
| ) | ||
| datahier = self.getHierarchy( | ||
| ndim, | ||
| interp_order, | ||
| refinement_boxes, | ||
| "eb", | ||
| cells=cells, | ||
| smallest_patch_size=smallest_patch_size, | ||
| largest_patch_size=smallest_patch_size, | ||
| time_step=time_step, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix parameter inconsistency in patch size test.
There's an inconsistency in the patch size parameters. The function retrieves both largest_patch_size and smallest_patch_size, but then uses smallest_patch_size for both parameters in the getHierarchy call.
datahier = self.getHierarchy(
ndim,
interp_order,
refinement_boxes,
"eb",
cells=cells,
smallest_patch_size=smallest_patch_size,
- largest_patch_size=smallest_patch_size,
+ largest_patch_size=largest_patch_size,
time_step=time_step,
time_step_nbr=time_step_nbr,
nbr_part_per_cell=ppc,
)📝 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 test_overlaped_fields_are_equal_with_min_max_patch_size_of_max_ghosts( | |
| self, interp_order, refinement_boxes | |
| ): | |
| print(f"{self._testMethodName}_{ndim}d") | |
| time_step_nbr = 3 | |
| time_step = 0.001 | |
| from pyphare.pharein.simulation import check_patch_size | |
| largest_patch_size, smallest_patch_size = check_patch_size( | |
| ndim, interp_order=interp_order, cells=[cells] * ndim | |
| ) | |
| datahier = self.getHierarchy( | |
| ndim, | |
| interp_order, | |
| refinement_boxes, | |
| "eb", | |
| cells=cells, | |
| smallest_patch_size=smallest_patch_size, | |
| largest_patch_size=smallest_patch_size, | |
| time_step=time_step, | |
| def test_overlaped_fields_are_equal_with_min_max_patch_size_of_max_ghosts( | |
| self, interp_order, refinement_boxes | |
| ): | |
| print(f"{self._testMethodName}_{ndim}d") | |
| time_step_nbr = 3 | |
| time_step = 0.001 | |
| from pyphare.pharein.simulation import check_patch_size | |
| largest_patch_size, smallest_patch_size = check_patch_size( | |
| ndim, interp_order=interp_order, cells=[cells] * ndim | |
| ) | |
| datahier = self.getHierarchy( | |
| ndim, | |
| interp_order, | |
| refinement_boxes, | |
| "eb", | |
| cells=cells, | |
| smallest_patch_size=smallest_patch_size, | |
| largest_patch_size=largest_patch_size, | |
| time_step=time_step, | |
| time_step_nbr=time_step_nbr, | |
| nbr_part_per_cell=ppc, | |
| ) |
| template<std::size_t D, typename FieldT> | ||
| typename std::enable_if<D == 1, void>::type | ||
| coarsen(Point_t const fineStartIndex, FieldT const& fineField, FieldT& coarseField, | ||
| Point_t const coarseIndex); | ||
|
|
||
| if constexpr (dimension == 1) | ||
| { | ||
| // in 1D div(B) is automatically satisfied so using this coarsening | ||
| // opertor is probably not better than the default one, but we do that | ||
| // for a kind of consistency... | ||
| // coarse flux is equal to fine flux and we're 1D so there is flux partitioned | ||
| // only for By and Bz, Bx is equal to the fine value | ||
| template<std::size_t D, typename FieldT> | ||
| typename std::enable_if<D == 2, void>::type | ||
| coarsen(Point_t const fineStartIndex, FieldT const& fineField, FieldT& coarseField, | ||
| Point_t const coarseIndex); | ||
|
|
||
| if (centering_[dirX] == core::QtyCentering::primal) // bx | ||
| { | ||
| coarseField(coarseIndex[dirX]) = fineField(fineStartIndex[dirX]); | ||
| } | ||
| else if (centering_[dirX] == core::QtyCentering::dual) // by and bz | ||
| { | ||
| coarseField(coarseIndex[dirX]) | ||
| = 0.5 * (fineField(fineStartIndex[dirX] + 1) + fineField(fineStartIndex[dirX])); | ||
| } | ||
| } | ||
| template<std::size_t D, typename FieldT> | ||
| typename std::enable_if<D == 3, void>::type | ||
| coarsen(Point_t const fineStartIndex, FieldT const& fineField, FieldT& coarseField, | ||
| Point_t const coarseIndex); | ||
|
|
||
| if constexpr (dimension == 2) | ||
| { | ||
| if (centering_[dirX] == core::QtyCentering::primal | ||
| and centering_[dirY] == core::QtyCentering::dual) | ||
| { | ||
| coarseField(coarseIndex[dirX], coarseIndex[dirY]) | ||
| = 0.5 | ||
| * (fineField(fineStartIndex[dirX], fineStartIndex[dirY]) | ||
| + fineField(fineStartIndex[dirX], fineStartIndex[dirY] + 1)); | ||
| } | ||
| else if (centering_[dirX] == core::QtyCentering::dual | ||
| and centering_[dirY] == core::QtyCentering::primal) | ||
| { | ||
| coarseField(coarseIndex[dirX], coarseIndex[dirY]) | ||
| = 0.5 | ||
| * (fineField(fineStartIndex[dirX], fineStartIndex[dirY]) | ||
| + fineField(fineStartIndex[dirX] + 1, fineStartIndex[dirY])); | ||
| } | ||
| else if (centering_[dirX] == core::QtyCentering::dual | ||
| and centering_[dirY] == core::QtyCentering::dual) | ||
| { | ||
| coarseField(coarseIndex[dirX], coarseIndex[dirY]) | ||
| = 0.25 | ||
| * (fineField(fineStartIndex[dirX], fineStartIndex[dirY]) | ||
| + fineField(fineStartIndex[dirX] + 1, fineStartIndex[dirY]) | ||
| + fineField(fineStartIndex[dirX], fineStartIndex[dirY] + 1) | ||
| + fineField(fineStartIndex[dirX] + 1, fineStartIndex[dirY] + 1)); | ||
| } | ||
| else | ||
| { | ||
| throw std::runtime_error("no magnetic field should end up here"); | ||
| } | ||
| } | ||
| else if constexpr (dimension == 3) | ||
| { | ||
| throw std::runtime_error("Not Implemented yet"); | ||
| } | ||
| } | ||
|
|
||
| private: | ||
| std::array<core::QtyCentering, dimension> const centering_; | ||
| std::array<core::QtyCentering, dim> const centering_; | ||
| SAMRAI::hier::Box const sourceBox_; | ||
| SAMRAI::hier::Box const destinationBox_; | ||
| static int constexpr ratio_ = 2; | ||
| }; | ||
|
|
||
| template<std::size_t dim> | ||
| template<std::size_t D, typename FieldT> | ||
| typename std::enable_if<D == 1, void>::type | ||
| MagneticFieldCoarsener<dim>::coarsen(Point_t const fineStartIndex, FieldT const& fineField, | ||
| FieldT& coarseField, Point_t const coarseIndex) | ||
| { | ||
| // in 1D div(B) is automatically satisfied so using this coarsening | ||
| // opertor is probably not better than the default one, but we do that | ||
| // for some constistency | ||
| // coarse flux is equal to fine flux and we're 1D so tehre is flux partitionned | ||
| // only for By and Bz, Bx is equal to the fine value | ||
| if (centering_[dirX] == core::QtyCentering::primal) // bx | ||
| { | ||
| coarseField(coarseIndex[dirX]) = fineField(fineStartIndex[dirX]); | ||
| } | ||
| else if (centering_[dirX] == core::QtyCentering::dual) // by and bz | ||
| { | ||
| coarseField(coarseIndex[dirX]) | ||
| = 0.5 * (fineField(fineStartIndex[dirX] + 1) + fineField(fineStartIndex[dirX])); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unclear fallback behavior for 1D coarsening logic.
While 1D automatically satisfies div(B), the code lumps By/Bz under the dual centering case without additional checks. Ensure that the function cannot receive an unexpected combination of centerings. If there's a possibility, consider throwing an error or handling that case explicitly.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
tools/python3/git.py (1)
27-35:⚠️ Potential issueAdd missing delete_branch function.
The function references an undefined
delete_branchfunction.Please implement the missing
delete_branchfunction before using it in thecheckoutfunction. Example implementation:def delete_branch(branch): """Delete a Git branch if it exists.""" if branch_exists(branch): run(f"git branch -D {branch}")🧰 Tools
🪛 Ruff (0.8.2)
29-29: Undefined name
delete_branch(F821)
tests/simulator/__init__.py (1)
319-346:⚠️ Potential issueFix mutable default argument in
debug_tracerfunction.The function uses a mutable list as a default argument (
indent=[0]), which is a Python anti-pattern that can lead to unexpected behavior when the function is reused.Apply this diff to fix the issue:
-def tracefunc(frame, event, arg, indent=[0]): +def tracefunc(frame, event, arg, indent=None): + if indent is None: + indent = [0]🧰 Tools
🪛 Ruff (0.8.2)
326-326: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
tests/core/numerics/interpolator/test_main.cpp (3)
739-739:⚠️ Potential issueFix the grid layout initialization.
The grid layout initialization is incorrect as it only specifies two dimensions (
{nx, ny}) for a 3D grid.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)};
768-768:⚠️ Potential issueFix the interpolator call signature.
The interpolator call signature differs from the 1D and 2D implementations, which use
makeIndexRange.Apply this diff to maintain consistency with other dimensions:
- interpolator(particles, rho, v, layout); + interpolator(makeIndexRange(particles), rho, v, layout);
774-781:⚠️ Potential issueUncomment and fix the test assertions.
The test case is currently commented out, which is flagged by static analysis. The test should verify the correct deposition of weights and velocities.
Apply this diff to restore and fix the test assertions:
- // 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); + auto idx = 2 + this->layout.nbrGhosts(QtyCentering::dual); + auto const& [vx, vy, vz] = this->v(); + EXPECT_DOUBLE_EQ(this->rho(idx, idx, idx), 1.0); + EXPECT_DOUBLE_EQ(vx(idx, idx, idx), 2.0); + EXPECT_DOUBLE_EQ(vy(idx, idx, idx), -1.0); + EXPECT_DOUBLE_EQ(vz(idx, idx, idx), 1.0);
🧹 Nitpick comments (21)
tools/python3/cmake.py (3)
4-5: Complete the implementation of the version() function.The
version()function is currently a placeholder with no implementation. Either implement the function to return the CMake version or add a TODO comment explaining why it's empty.def version(): - pass + """Returns the CMake version.""" + proc = run("cmake --version", capture_output=True) + out = decode_bytes(proc.stdout) + # First line contains version like "cmake version 3.x.x" + version_str = out.splitlines()[0].split(" ")[2] + return version_str
30-31: Add error handling for build command failures.The
buildfunction doesn't handle potential failures in the build process. Consider adding error handling to provide more informative messages when builds fail.def build(use_ninja=False, threads=1): - run("ninja" if use_ninja else f"make -j{threads}", capture_output=False) + try: + proc = run("ninja" if use_ninja else f"make -j{threads}", capture_output=False) + if proc.returncode != 0: + print(f"Build failed with return code {proc.returncode}") + return False + return True + except Exception as e: + print(f"Build failed with exception: {e}") + return False
34-37: Add docstrings to remaining functions for better code documentation.The
list_tests(),test_cmd(), andrun_test()functions lack docstrings explaining their purpose and usage, unlikemake_config_str().Example for
list_tests():def list_tests(): + """ + Lists all available tests from CTest. + + Returns: + list: Names of all available tests. + """ proc = run("ctest -N", capture_output=True) out = decode_bytes(proc.stdout) return [line.split(" ")[-1] for line in out.splitlines()[1:-2]]tests/simulator/refined_particle_nbr.py (1)
44-48: Remove the TODO comment as it's already implementedThe code already handles the 3D case correctly right after the TODO comment, so the TODO comment should be removed.
dim2 = refined_particle_nbr * ((cellNbr[0] * 2 + (cellNbr[1] * 2))) if dim == 2: return dim2 - # TODO3D if dim==3? return dim2 * (cellNbr[2] * 2)tools/python3/git.py (1)
19-24: Simplify exception handling.The exception variable 'e' is captured but never used.
def branch_exists(branch): try: run(f"git show-branch {branch}", check=True) - except subprocess.CalledProcessError as e: + except subprocess.CalledProcessError: return False # exit failure means branch does not exist return True🧰 Tools
🪛 Ruff (0.8.2)
22-22: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
tests/simulator/test_initialization.py (3)
392-397: Improve scoping in the domain function.The inner function
domainuses variables from the outer scope, creating unclear dependencies.Consider passing these variables as parameters to make the function more self-contained:
-def domain(patch_data): +def domain(patch_data, dim, nbrGhosts, patch): if dim == 1: return patch_data.dataset[select] return patch_data.dataset[:].reshape( 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)
582-587: Fix unused loop control variable.The variable
piis not used within the loop body.-for pi, patch in enumerate(datahier.level(0).patches): +for _, patch in enumerate(datahier.level(0).patches):🧰 Tools
🪛 Ruff (0.8.2)
582-582: Loop control variable
pinot used within loop bodyRename unused
pito_pi(B007)
586-586: Yoda condition detected
Rewrite as
ppc == H(SIM300)
664-664: Remove unused variable.The variable
outis assigned but never used.-out = "phare_outputs"🧰 Tools
🪛 Ruff (0.8.2)
664-664: Local variable
outis assigned to but never usedRemove assignment to unused variable
out(F841)
pyphare/pyphare/core/box.py (1)
206-207: Use descriptive variable names to avoid ambiguity.The variable name
lis ambiguous and could be mistaken for the number '1' or capital 'I'. Consider using a more descriptive name likelowerorlower_bound.-def select(data, box): - return data[tuple([slice(l, u + 1) for l,u in zip(box.lower, box.upper)])] +def select(data, box): + return data[tuple([slice(lower, upper + 1) for lower, upper in zip(box.lower, box.upper)])]🧰 Tools
🪛 Ruff (0.8.2)
207-207: Ambiguous variable name:
l(E741)
tests/amr/data/field/coarsening/coarsening_utils.py (2)
131-292: Simplify 3D case with helper functions.The 3D case contains extensive repetition in weight calculation logic. Consider creating helper functions to generate weights based on primality patterns and component types.
You could create a function that generates the appropriate shift and weight arrays based on primality, and another function to perform the nested summation. This would make the code more maintainable and less error-prone.
def get_shifts_and_weights(is_primal, direction): """Generate shifts and weights based on primality for a direction.""" if is_primal: return [-1, 0, 1], [0.25, 0.5, 0.25] else: return [0, 1], [0.5, 0.5] def apply_weights(fine_data, fine_indices, shifts_weights_by_dim): """Apply weights using nested loops for any number of dimensions.""" # Implementation that handles the nested loops dynamically # ...🧰 Tools
🪛 Ruff (0.8.2)
181-182: Use a single
ifstatement instead of nestedifstatements(SIM102)
181-182: Use a single if statement instead of nested if statements.Replace
elif not is_primal[0] and is_primal[1] and not is_primal[2]:with a cleaner condition to improve readability.- elif not is_primal[0] and is_primal[1] and not is_primal[2]: - if qty == "By": + elif not is_primal[0] and is_primal[1] and not is_primal[2] and qty == "By":🧰 Tools
🪛 Ruff (0.8.2)
181-182: Use a single
ifstatement instead of nestedifstatements(SIM102)
tests/functional/harris/harris_3d.py (1)
62-64: Rename the ambiguous variablel.
Use more descriptive naming to avoid confusion with digit “1.”-def S(y, y0, l): - return 0.5 * (1.0 + np.tanh((y - y0) / l)) +def S(y, y0, scale_length): + return 0.5 * (1.0 + np.tanh((y - y0) / scale_length))🧰 Tools
🪛 Ruff (0.8.2)
62-62: Ambiguous variable name:
l(E741)
pyphare/pyphare/pharesee/geometry.py (2)
63-74: Check consistency with max ghost cells for 3D ghost boxes.For the 3D ghost boxes, ensure
ghost_box_widthcorrectly reflects the maximum needed cells across all dimensions. An off-by-one error could omit or over-include some boundary cells.
161-167: Rename ambiguous variable.Static analysis flags the variable
las ambiguous. Please rename it to something clearer (e.g.,shift_list) to improve readability.-for k, l in shifts.items(): +for k, shift_list in shifts.items():🧰 Tools
🪛 Ruff (0.8.2)
161-161: Ambiguous variable name:
l(E741)
164-164: Ambiguous variable name:
l(E741)
167-167: Ambiguous variable name:
l(E741)
res/cmake/funcs.cmake (1)
1-193: Overall good structure with flexible MPI and Python testing.The file properly introduces test functions with MPI/no-MPI variants, environment variables, and an ASAN leak detection flag. A couple of considerations for further improvements:
• RevisitASAN_OPTIONS=detect_leaks=0if debugging memory leaks is desired in the future.
• Check whether settingPHARE_MPI_PROCS=2by default is acceptable on all environments or if a dynamic approach is needed.src/amr/data/field/coarsening/magnetic_field_coarsener.hpp (2)
33-36: Clarify assumptions about Yee layout.The comment warns that this coarsener logic assumes certain Yee centering for magnetic fields. Consider explicitly validating or throwing an error if unexpected centerings appear, ensuring no silent miscoarsening occurs.
221-223: Add specific error context for unhandled centerings.The
runtime_errormessages could be more descriptive (e.g., specifying which centering setup was encountered) to ease debugging if users encounter this exception.-throw std::runtime_error("no magnetic field should end up here"); +throw std::runtime_error("Unhandled magnetic field centering combination in coarsening");Also applies to: 331-332
tests/simulator/test_advance.py (3)
286-340: Spelling and partial success approach
The function name and docstring contain a spelling issue: "overlaped" should be "overlapped." Additionally, the function short-circuits upon the first failed assertion, returning a partial success count. Ensure this behavior is intentional, as it can hide subsequent issues.- def base_test_overlaped_fields_are_equal(self, datahier, coarsest_time): + def base_test_overlapped_fields_are_equal(self, datahier, coarsest_time):🧰 Tools
🪛 Ruff (0.8.2)
290-290: Loop control variable
ilvlnot used within loop bodyRename unused
ilvlto_ilvl(B007)
290-290: Rename unused loop variable
The loop variableilvlis not used. Rename it to_ilvlto clearly indicate it's unused.- for ilvl, overlaps in hierarchy_overlaps(datahier, coarsest_time).items(): + for _ilvl, overlaps in hierarchy_overlaps(datahier, coarsest_time).items():🧰 Tools
🪛 Ruff (0.8.2)
290-290: Loop control variable
ilvlnot used within loop bodyRename unused
ilvlto_ilvl(B007)
346-354: Spelling correction
The function name_test_overlaped_fields_are_equalalso has "overlaped" instead of "overlapped."- def _test_overlaped_fields_are_equal(self, datahier, time_step_nbr, time_step): + def _test_overlapped_fields_are_equal(self, datahier, time_step_nbr, time_step):src/amr/data/particles/refine/split_3d.hpp (1)
17-17: Avoid using namespace in header
Including a namespace directive in a header file can inadvertently pollute the global scope. Consider removing this directive or restricting its scope.- using namespace PHARE::core; + // Remove or limit this directive in header scope
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (58)
.github/workflows/cmake_macos.yml(1 hunks)pyphare/pyphare/core/box.py(2 hunks)pyphare/pyphare/core/gridlayout.py(1 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/funcs.cmake(1 hunks)res/cmake/options.cmake(2 hunks)src/amr/data/field/coarsening/coarsen_weighter.hpp(1 hunks)src/amr/data/field/coarsening/magnetic_field_coarsener.hpp(1 hunks)src/amr/data/particles/refine/particles_data_split.hpp(2 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(5 hunks)src/core/data/grid/grid.hpp(1 hunks)src/core/data/grid/gridlayoutdefs.hpp(2 hunks)src/core/data/ndarray/ndarray_vector.hpp(8 hunks)src/core/data/particles/particle.hpp(3 hunks)src/core/utilities/box/box.hpp(1 hunks)src/core/utilities/meta/meta_utilities.hpp(2 hunks)tests/amr/data/field/coarsening/coarsening_utils.py(1 hunks)tests/amr/data/field/coarsening/test_weighter_coarsen.hpp(1 hunks)tests/amr/data/field/refine/CMakeLists.txt(1 hunks)tests/amr/data/field/refine/test_default_field_refinement.cpp(4 hunks)tests/amr/data/field/refine/test_field_refinement_on_hierarchy.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/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/conservation/CMakeLists.txt(1 hunks)tests/functional/conservation/conserv_3d.py(1 hunks)tests/functional/harris/harris_3d.py(1 hunks)tests/simulator/__init__.py(3 hunks)tests/simulator/advance/CMakeLists.txt(1 hunks)tests/simulator/advance/test_fields_advance_1d.py(1 hunks)tests/simulator/advance/test_fields_advance_2d.py(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(16 hunks)tests/simulator/test_initialization.py(13 hunks)tests/simulator/utilities/field_coarsening.py(0 hunks)tests/simulator/utilities/test_refine_field.py(2 hunks)tools/python3/cmake.py(1 hunks)tools/python3/git.py(1 hunks)
💤 Files with no reviewable changes (2)
- tests/simulator/utilities/field_coarsening.py
- tests/simulator/initialize/test_particles_init_1d.py
🚧 Files skipped from review as they are similar to previous changes (34)
- src/amr/data/particles/refine/split.hpp
- src/core/utilities/box/box.hpp
- src/core/data/grid/grid.hpp
- tests/amr/data/field/refine/CMakeLists.txt
- tests/simulator/initialize/test_fields_init_2d.py
- src/amr/data/particles/refine/particles_data_split.hpp
- .github/workflows/cmake_macos.yml
- src/core/data/grid/gridlayoutdefs.hpp
- tests/diagnostic/CMakeLists.txt
- tests/core/data/ndarray/test_main.cpp
- pyphare/pyphare/pharesee/run/utils.py
- tests/simulator/advance/test_fields_advance_2d.py
- tests/diagnostic/job_3d.py.in
- src/amr/data/field/coarsening/coarsen_weighter.hpp
- tests/simulator/advance/CMakeLists.txt
- tests/simulator/initialize/test_particles_init_2d.py
- tests/simulator/advance/test_particles_advance_3d.py
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py
- tests/amr/data/particles/refine/test_split.cpp
- res/cmake/options.cmake
- tests/simulator/utilities/test_refine_field.py
- tests/simulator/advance/test_fields_advance_1d.py
- tests/amr/data/field/refine/test_default_field_refinement.cpp
- tests/simulator/initialize/test_particles_init_3d.py
- tests/simulator/initialize/test_fields_init_3d.py
- res/amr/splitting.yml
- tests/amr/data/field/coarsening/test_weighter_coarsen.hpp
- src/core/data/particles/particle.hpp
- tests/amr/data/field/refine/test_field_refinement_on_hierarchy.cpp
- tests/simulator/advance/test_fields_advance_3d.py
- tests/simulator/initialize/CMakeLists.txt
- src/amr/data/particles/refine/split_1d.hpp
- src/amr/data/particles/refine/split_2d.hpp
- src/amr/data/particles/refine/splitter.hpp
🧰 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.hppsrc/core/data/ndarray/ndarray_vector.hpptests/simulator/per_test.hppsrc/amr/data/field/coarsening/magnetic_field_coarsener.hppsrc/amr/data/particles/refine/split_3d.hpp
🪛 Ruff (0.8.2)
tools/python3/git.py
22-22: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
29-29: Undefined name delete_branch
(F821)
tests/functional/harris/harris_3d.py
62-62: Ambiguous variable name: l
(E741)
tests/amr/data/field/coarsening/coarsening_utils.py
181-182: Use a single if statement instead of nested if statements
(SIM102)
pyphare/pyphare/core/box.py
207-207: Ambiguous variable name: l
(E741)
tests/functional/conservation/conserv_3d.py
188-188: SyntaxError: Expected ',', found name
188-188: SyntaxError: Expected ',', found '='
189-189: SyntaxError: Expected ',', found name
189-189: SyntaxError: Expected ',', found '='
189-190: SyntaxError: Expected ']', found newline
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)
tools/python3/cmake.py
48-48: Undefined name cmd
(F821)
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)
tests/simulator/__init__.py
326-326: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
tests/simulator/test_advance.py
290-290: Loop control variable ilvl not used within loop body
Rename unused ilvl to _ilvl
(B007)
🪛 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 (6)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (53)
tools/python3/cmake.py (2)
23-27: Fix missing parameter in config function.The
use_ccacheparameter is not being passed tomake_config_str(), which could prevent ccache from being enabled.Apply this diff to fix the parameter passing:
- cmd = make_config_str(path, samrai_dir, cxx_flags, use_ninja, extra) + cmd = make_config_str(path, samrai_dir, cxx_flags, use_ninja, use_ccache, extra)
47-48: Fix undefined variable in run_test function.The function uses an undefined variable
cmdinstead of thetestparameter.Apply this diff to fix the variable name:
- run(test_cmd(cmd, verbose=verbose), capture_output=capture_output) + run(test_cmd(test, verbose=verbose), capture_output=capture_output)🧰 Tools
🪛 Ruff (0.8.2)
48-48: Undefined name
cmd(F821)
tests/simulator/refined_particle_nbr.py (4)
33-37: Good addition of the tearDown method!This ensures proper cleanup after tests, especially if an exception occurs during a test. This is good practice for maintaining test isolation and preventing resource leaks.
106-116: Appropriate 3D constants and test implementationThe 3D constants and test method are properly implemented following the same pattern as the 1D and 2D tests. The calculations and rationale for the chosen values are well-documented in the comments.
118-136: Well-structured test for checking deltas and weightsThe
_check_deltas_and_weightsmethod and associatedtest_valuesfunction systematically test the splitter functionality across all supported dimensions, interpolation orders, and refined particle numbers. This provides good coverage for validating the correctness of the 3D implementation.
140-145: Good test organizationThe test suite is now explicitly ordered to ensure deterministic test execution. This is a good practice as it ensures that the
test_valuesmethod runs first to validate the correctness of deltas and weights before the dimension-specific tests.src/core/utilities/meta/meta_utilities.hpp (3)
9-11: Good implementation of conditional compilation control.This preprocessor directive provides a clean way to configure the dimensionality of simulations at compile time. Default value of 3 allows for maximum flexibility while enabling conditional code inclusion.
83-88: Well-structured conditional code inclusion for 2D configurations.The code properly handles comma placement for the tuple syntax and provides a clean separation between dimensionality options.
90-97: Clean implementation of 3D simulator options.The 3D options are properly guarded by the preprocessor condition and include helpful TODOs for future development. The commented out value (27) in line 94 suggests future work or optimization opportunity.
src/core/data/ndarray/ndarray_vector.hpp (5)
6-6: Good addition of necessary include.Adding the algorithm header is appropriate since it's needed for the std::copy function used in the implementation.
123-124: Clean implementation of z-dimension access methods.These methods follow the same pattern as the existing x and y accessors, maintaining consistency in the API design.
348-448: Verify boundary safety in 3D array calculations.The implementation for 3D cell counting is complex with nested calculations. While the approach is valid, ensure all boundary cases are properly handled to prevent potential array access issues or overlaps.
Consider adding range checking to prevent potential out-of-bounds access if
min_ormax_values exceed array dimensions.
384-424: Comprehensive implementation of 3D border filling.The fill3D implementation covers all the faces, edges, and corners of the 3D structure. The code organization with clear comments helps in understanding the complex implementation.
529-595: Well-structured 3D masked view operator implementation.The implementation thoroughly handles all aspects of 3D masked views with clear structure and logic. The use of a lambda for corner handling is a nice touch that reduces code duplication.
tools/python3/git.py (1)
1-17: Well-implemented Git utility functions.These functions provide a clean wrapper around Git commands with proper error handling and output formatting.
tests/simulator/test_initialization.py (3)
254-273: Good performance tracking with timing measurements.Adding timing measurements helps track performance across different dimensions and provides valuable data for optimization efforts.
329-351: Comprehensive 3D testing implementation.The 3D case properly extracts z-components and uses appropriate meshgrid generation for 3D data. The testing approach is consistent with the 1D and 2D implementations.
417-426: Well-calibrated empirical deviation expectations for 3D.The empirical dimension deviations account for the increased uncertainty in 3D simulations, with appropriate scaling from 1D to 3D.
tests/functional/conservation/CMakeLists.txt (1)
13-13: Script name updated to reflect dimensional specificity.The change from
conserv.pytoconserv_2d.pyaligns with the PR's goal of updating to 3D support. This modification appropriately separates the 2D test cases in preparation for adding 3D tests.tests/simulator/__init__.py (2)
6-6: New import for Box supports 3D functionality.The import of
Boxfrompyphare.core.boxis necessary for the implementation of the 3D density function.
98-108: Implementation of 3D density function supports the 3D expansion.The
density_3d_periodicfunction is well-implemented, extending the existing 1D and 2D pattern. It correctly handles the 3D case with exponential decay in all three dimensions plus a background particle density.pyphare/pyphare/core/gridlayout.py (1)
369-381: Well-structured mesh coordinates generator for multi-dimensional simulations.The new
meshCoordsmethod nicely expands the GridLayout functionality by generating appropriate mesh coordinates for 1D, 2D, and 3D simulations. It correctly handles each dimension case and properly structures the output arrays.This implementation is crucial for the 3D update, providing a clean interface for obtaining structured coordinate meshes at the correct positions for field quantities.
tests/simulator/per_test.hpp (1)
111-117: Test infrastructure extended to support 3D simulations.The addition of the
Simulator3dTesttemplate structure and associated type definitions properly extends the testing framework to 3D simulations, following the established pattern from 1D and 2D tests. This is an essential component for validating the 3D functionality being added in this PR.tests/diagnostic/test-diagnostics_3d.cpp (6)
4-4: Fix include directive to use a header file.The include directive is using an implementation file (.ipp) instead of a header file (.hpp). While this might be intentional for template implementations, it's flagged by static analysis.
9-12: LGTM: Clean and concise test implementation.The test case is well-structured and follows the pattern of other test files in the codebase.
14-17: LGTM: Clean and concise test implementation.The test case is well-structured and follows the pattern of other test files in the codebase.
19-22: LGTM: Clean and concise test implementation.The test case is well-structured and follows the pattern of other test files in the codebase.
24-27: LGTM: Clean and concise test implementation.The test case is well-structured and follows the pattern of other test files in the codebase.
🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 24-24: Function parameter 'path' should be passed by const reference.
(passedByValue)
30-35: LGTM: Standard Google Test initialization.The main function correctly initializes Google Test and the PHARE lifecycle.
pyphare/pyphare/core/box.py (3)
65-69: Enhanced Box subtraction method handles collections.This is a good enhancement to support subtracting multiple boxes at once, which simplifies client code.
185-198: Well implemented remove_all function.The implementation handles the case of removing multiple boxes efficiently by processing each box to be removed iteratively and managing the list of remaining boxes.
209-223: Great addition of the DataSelector class.The DataSelector class provides a clean interface for data access and modification using Box instances. The docstring effectively communicates its purpose and usage.
tests/amr/data/field/coarsening/coarsening_utils.py (3)
8-20: Well-documented function with clear purpose.The function documentation clearly explains its purpose and relationship to C++ counterparts, which is valuable for maintainers.
38-56: LGTM: 1D coarsening implementation.The 1D coarsening logic correctly handles both primal and dual cases, with special handling for the "Bx" quantity.
57-130: Consider systematically generating coarsening weights.The 2D case contains several code blocks with repetitive patterns of weighting calculations for different cases. Consider refactoring this into a more systematic approach using functions or data structures.
tests/core/numerics/interpolator/test_main.cpp (3)
724-743: LGTM: Well-structured 3D test fixture setup.The test fixture for 3D interpolation follows the same pattern as the 1D and 2D implementations, which maintains code consistency.
745-769: LGTM: Constructor implementation for 3D particles.The constructor appropriately initializes the test particles with 3D properties.
783-787: LGTM: Test suite registration and instantiation.The test suite is properly registered and instantiated for different 3D interpolator types.
tests/amr/data/particles/refine/input/input_3d_ratio_2.txt (1)
10-10: Fix dimensionality in Main section for 3D simulation.
This is a repeat of a prior comment indicating thatdimis set to 1 in a 3D configuration, which is inconsistent.- dim = 1 + dim = 3tests/functional/harris/harris_3d.py (1)
105-108: Prevent potential assertion errors in temperature calculation.
Similar to a previously raised concern, ifb2(x, y, z) * 0.5exceedsK=1, you risk negative temperature results. Consider adjustingKor adding bounds checks.- K = 1 + K = 1.5 # Provide a safety margin temp = 1.0 / density(x, y, z) * (K - b2(x, y, z) * 0.5) - assert np.all(temp > 0) + if not np.all(temp > 0): + raise ValueError( + "Temperature calculation resulted in negative values. " + "Consider adjusting K or magnetic field components." + )pyphare/pyphare/pharesee/geometry.py (2)
59-60: Consider verifying ghost box coverage for edge cases.The bottom/top ghost boxes use
ghost_box_width = max_ghosts - 1, which might cause an off-by-one for certain patch configurations. Please confirm thatghost_box_widthconsistently covers all intended cells at the domain boundaries.
188-188: Validate the key normalization logic.You're reassigning
shiftsby creating sorted keys with"".join(sorted(k)). Please confirm that this rekeying will not mask or combine different shift sets with identical sorted string representations.🧰 Tools
🪛 Ruff (0.8.2)
188-188: Ambiguous variable name:
l(E741)
src/amr/data/field/coarsening/magnetic_field_coarsener.hpp (2)
82-124: [Duplicate from previous review] Unclear fallback behavior for 1D logic.As previously noted, the code lumps By/Bz under the
dualcentering without additional checks. Ensure that unexpected centerings in 1D are handled or trigger a clear exception.
66-79: Verify the indexing for fine-level data.In
fine_start_index, ensure that multiplyingcoarseIndexbyratio_correctly maps to all relevant fine cells, especially when corner or boundary cells are involved.tests/simulator/test_advance.py (9)
9-9: No issues with new import
30-42: Docstring updates
No concerns here.
98-101: No concerns
415-417: New default parameter
Looks good.
444-445: New default parameter
No issues noted.
458-458: No concerns
464-465: Refined parameters
Implementation is consistent.
797-809: Added tested quantities
No issues.🧰 Tools
🪛 Ruff (0.8.2)
798-798: f-string without any placeholders
Remove extraneous
fprefix(F541)
799-799: f-string without any placeholders
Remove extraneous
fprefix(F541)
816-817: Particle domain checks
No issues found.Also applies to: 823-824
| dls = [0.2,0.1 | ||
| nbrcells = [20, 50] | ||
| nbrdts = [25000, 100000] | ||
|
|
There was a problem hiding this comment.
Fix syntax error caused by missing bracket.
The list is missing a closing bracket, causing parse failures in subsequent lines.
- dls = [0.2,0.1
+ dls = [0.2, 0.1]
nbrcells = [20, 50]
nbrdts = [25000, 100000]📝 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.
| dls = [0.2,0.1 | |
| nbrcells = [20, 50] | |
| nbrdts = [25000, 100000] | |
| dls = [0.2, 0.1] | |
| nbrcells = [20, 50] | |
| nbrdts = [25000, 100000] |
🧰 Tools
🪛 Ruff (0.8.2)
188-188: SyntaxError: Expected ',', found name
188-188: SyntaxError: Expected ',', found '='
189-189: SyntaxError: Expected ',', found name
189-189: SyntaxError: Expected ',', found '='
189-190: SyntaxError: Expected ']', found newline
| paths = sorted( | ||
| paths, | ||
| key=lambda path: float(paths[0].split("/")[-1].strip("vth").split("dx")[1]), | ||
| ) |
There was a problem hiding this comment.
Correct sorting logic to parse each path properly.
Currently, the lambda always references paths[0], resulting in no meaningful sort.
- paths = sorted(
- paths,
- key=lambda path: float(paths[0].split("/")[-1].strip("vth").split("dx")[1]),
- )
+ paths = sorted(
+ paths,
+ key=lambda path: float(path.split("/")[-1].strip("vth").split("dx")[1]),
+ )📝 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.
| paths = sorted( | |
| paths, | |
| key=lambda path: float(paths[0].split("/")[-1].strip("vth").split("dx")[1]), | |
| ) | |
| paths = sorted( | |
| paths, | |
| key=lambda path: float(path.split("/")[-1].strip("vth").split("dx")[1]), | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/functional/harris/harris_3d.py (1)
104-108:⚠️ Potential issuePrevent potential assertion errors in temperature calculation.
The temperature function could raise an assertion error if
b2(x, y, z) * 0.5exceedsK = 1. Consider adding a safety margin toKor implementing bounds checking for the magnetic field pressure.- K = 1 + K = 1.5 # Increased to provide safety margin temp = 1.0 / density(x, y, z) * (K - b2(x, y, z) * 0.5) - assert np.all(temp > 0) + if not np.all(temp > 0): + raise ValueError("Temperature calculation resulted in negative values. " + "Consider adjusting K or magnetic field components.") return temp
🧹 Nitpick comments (7)
tests/functional/harris/harris_3d.py (7)
62-63: Rename ambiguous variablelto improve readability.The parameter name
l(lowercase 'L') is ambiguous and can easily be mistaken for the number '1' in some fonts.-def S(y, y0, l): - return 0.5 * (1.0 + np.tanh((y - y0) / l)) +def S(y, y0, width): + return 0.5 * (1.0 + np.tanh((y - y0) / width))🧰 Tools
🪛 Ruff (0.8.2)
62-62: Ambiguous variable name:
l(E741)
37-52: Consider making simulation parameters configurable.The simulation parameters are currently hardcoded in the config function. Consider making them configurable via function parameters to increase reusability and testability.
-def config(): +def config(time_step=time_step, time_step_nbr=time_step_nbr, dl=dl, cells=cells, + max_nbr_levels=2, resistivity=0.001, hyper_resistivity=0.001, + output_dir=diag_outputs): sim = ph.Simulation( - time_step=time_step, - time_step_nbr=time_step_nbr, - dl=dl, - cells=cells, + time_step=time_step, + time_step_nbr=time_step_nbr, + dl=dl, + cells=cells, refinement="tagging", - max_nbr_levels=2, - hyper_resistivity=0.001, - resistivity=0.001, + max_nbr_levels=max_nbr_levels, + hyper_resistivity=hyper_resistivity, + resistivity=resistivity, diag_options={ "format": "phareh5", - "options": {"dir": diag_outputs, "mode": "overwrite"}, + "options": {"dir": output_dir, "mode": "overwrite"}, }, # strict=True, )
177-179: TODO comments should include more specific action items.The TODO comments about 3D plotting should include more specific information about what needs to be done or potential approaches to solve the problem.
- # TODO3D nico : I removed the plot because the hierarchy plot3d has been removed - # TODO3D we need to find a way to plot the data + # TODO: Implement new 3D visualization approach to replace the removed hierarchy plot3d. + # Options to consider: + # 1. Investigate using matplotlib's 3D plotting capabilities with slices + # 2. Explore integration with specialized 3D visualization tools such as ParaView + # 3. Implement custom slice-based visualization along principal axes
143-172: Consider cleaning up commented-out code in the plot function.The plot function contains a significant amount of commented-out code, which can be confusing. Consider either removing this code, clearly marking it as temporarily disabled, or implementing placeholder functionality.
def plot(diag_dir): run = Run(diag_dir) for time in timestamps: - # run.GetDivB(time).plot( - # filename=plot_file_for_qty("divb", time), - # plot_patches=True, - # vmin=1e-11, - # vmax=2e-10, - # ) - # run.GetRanks(time).plot( - # filename=plot_file_for_qty("Ranks", time), - # plot_patches=True, - # ) run.GetN(time, pop_name="protons").plot( filename=plot_file_for_qty("N", time), plot_patches=True, ) for c in ["x", "y", "z"]: run.GetB(time).plot( filename=plot_file_for_qty(f"b{c}", time), qty=f"B{c}", plot_patches=True, ) - # run.GetJ(time).plot( - # filename=plot_file_for_qty("jz", time), - # qty="Jz", - # plot_patches=True, - # vmin=-2, - # vmax=2, - # )Consider adding a comment at the top of the function to explain the current limitations:
def plot(diag_dir): """ Plot available 3D simulation results. Note: Some visualization capabilities are currently limited in 3D. Only density and magnetic field components are currently plotted. """ run = Run(diag_dir) # Rest of the function...
122-122: Consider making the random seed configurable.The random seed for particle initialization is hardcoded, which ensures reproducibility but limits flexibility. Consider making it a configurable parameter.
- protons = {"charge": 1, "density": density, **vvv, "init": {"seed": 12334}} + # Use a fixed seed for reproducibility, but allow overriding + particle_seed = os.environ.get("PHARE_PARTICLE_SEED", "12334") + protons = {"charge": 1, "density": density, **vvv, "init": {"seed": int(particle_seed)}}
19-20: Consider adding comments explaining the physical meaning of simulation parameters.The grid parameters are defined without context explaining their physical significance. Adding comments would improve maintainability.
-cells = (200, 400, 50) -dl = (0.2, 0.2, 0.2) +# Number of cells in each dimension (x, y, z) +cells = (200, 400, 50) +# Cell size in each dimension (x, y, z) in physical units +dl = (0.2, 0.2, 0.2)
188-188: Consider adding error handling around MPI barrier.The MPI barrier call could potentially fail in case of communication issues. Consider adding error handling.
- cpp.mpi_barrier() + try: + cpp.mpi_barrier() + except Exception as e: + print(f"MPI barrier failed: {e}") + # Consider appropriate cleanup or fallback behavior here
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/functional/harris/harris_3d.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/functional/harris/harris_3d.py
62-62: Ambiguous variable name: l
(E741)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Analyze (python)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-13)
| (np.asarray(actual["offset"]) == np.asarray(exp["offset"])).all() | ||
| ) | ||
| for actual in tested_overlaps[ilvl]: | ||
| self.assertTrue(actual in expected[ilvl]) |
Check notice
Code scanning / CodeQL
Imprecise assert
| overlap_boxes += [actual["box"]] | ||
| self.assertEqual(len(expected), len(tested_overlaps[ilvl])) | ||
| for actual in tested_overlaps[ilvl]: | ||
| self.assertTrue(actual in expected) |
Check notice
Code scanning / CodeQL
Imprecise assert
| (np.asarray(actual["offset"]) == np.asarray(exp["offset"])).all() | ||
| ) | ||
| for actual in tested_overlaps[ilvl]: | ||
| self.assertTrue(actual in expected[ilvl]) |
Check notice
Code scanning / CodeQL
Imprecise assert Note test
| overlap_boxes += [actual["box"]] | ||
| self.assertEqual(len(expected), len(tested_overlaps[ilvl])) | ||
| for actual in tested_overlaps[ilvl]: | ||
| self.assertTrue(actual in expected) |
Check notice
Code scanning / CodeQL
Imprecise assert Note test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pyphare/pyphare/pharesee/geometry.py (1)
87-189: Improved 3D periodicity shifts handling with some optimization opportunitiesThe function has been successfully extended to handle 3D shifts with comprehensive mapping for all possible combinations, including appropriate handling of front/back dimensions. The addition of key uniqueness validation (lines 184-186) is a good defensive programming practice.
However, there are several instances of ambiguous variable names that could be improved.
- front = { - f"{k}front": [(v[0], v[1], shape_z) for v in l] for k, l in shifts.items() - } - back = { - f"{k}back": [([v[0], v[1], -shape_z]) for v in l] for k, l in shifts.items() - } - - shifts = {k: [([v[0], v[1], 0]) for v in l] for k, l in shifts.items()} + front = { + f"{k}front": [(v[0], v[1], shape_z) for v in shift_list] for k, shift_list in shifts.items() + } + back = { + f"{k}back": [([v[0], v[1], -shape_z]) for v in shift_list] for k, shift_list in shifts.items() + } + + shifts = {k: [([v[0], v[1], 0]) for v in shift_list] for k, shift_list in shifts.items()}And similarly on line 188:
- shifts = {"".join(sorted(k)): l for k, l in shifts.items()} + shifts = {"".join(sorted(k)): shift_list for k, shift_list in shifts.items()}🧰 Tools
🪛 Ruff (0.8.2)
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_tests/test_pharesee/test_geometry.py (1)
81-94: Improved test overlap verification with dictionary comprehensionThe test has been enhanced with a more concise dictionary comprehension to collect overlaps for each level, and the comparison logic has been simplified. This update properly tests the changes made to the geometry module.
However, there's an unused loop variable that could be renamed.
- for ilvl, lvl in enumerate(hierarchy.patch_levels): + for ilvl, _lvl in enumerate(hierarchy.patch_levels):🧰 Tools
🪛 Ruff (0.8.2)
89-89: Loop control variable
lvlnot used within loop bodyRename unused
lvlto_lvl(B007)
pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (2)
117-123: Streamlined overlap testing with dictionary comprehensionThe test has been updated to use a dictionary comprehension for more concise collection of overlaps, consistent with changes in other tests. This approach properly validates the updated geometry functionality.
However, there are two minor improvements that could be made:
- for ilvl in expected.keys() + for ilvl in expectedThis removes the unnecessary
.keys()call as dictionaries are directly iterable by keys.🧰 Tools
🪛 Ruff (0.8.2)
122-122: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
124-129: Simplified assert logic with unused variableThe assertion logic has been simplified to check if each actual overlap exists within the expected overlaps using the
inoperator, which enhances readability.However, there's an unused loop variable that could be renamed.
- for ilvl, lvl in enumerate(hierarchy.levels().items()): + for ilvl, _lvl in enumerate(hierarchy.levels().items()):🧰 Tools
🪛 Ruff (0.8.2)
124-124: Loop control variable
lvlnot used within loop bodyRename unused
lvlto_lvl(B007)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pyphare/pyphare/pharesee/geometry.py(7 hunks)pyphare/pyphare_tests/test_pharesee/test_geometry.py(2 hunks)pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (1)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
levels(214-217)
pyphare/pyphare/pharesee/geometry.py (6)
src/core/utilities/box/box.hpp (11)
Box(35-35)Box(37-41)Box(37-37)Box(44-48)Box(198-198)upper(89-89)box(51-51)dim(100-116)dim(118-133)grow(260-265)grow(260-260)pyphare/pyphare/core/box.py (5)
Box(5-72)shape(32-34)grow(114-119)copy(71-72)copy(141-145)pyphare/pyphare/simulator/simulator.py (1)
domain_box(279-281)pyphare/pyphare/core/gridlayout.py (1)
ndim(221-222)pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
update(79-98)pyphare/pyphare/pharesee/hierarchy/patch.py (1)
copy(43-47)
🪛 Ruff (0.8.2)
pyphare/pyphare_tests/test_pharesee/test_geometry.py
89-89: Loop control variable lvl not used within loop body
Rename unused lvl to _lvl
(B007)
pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py
122-122: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
124-124: Loop control variable lvl not used within loop body
Rename unused lvl to _lvl
(B007)
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)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (6)
pyphare/pyphare/pharesee/geometry.py (5)
63-74: Good extension to 3D dimension handlingThe implementation correctly extends the domain border ghost boxes to handle 3D cases, following the same pattern established for 1D and 2D dimensions. The addition of "front" and "back" ghost boxes is consistent with the overall design.
193-226: Well-structured utility for periodic shift calculationThis new function is a well-designed utility that systematically computes all possible periodic shifts for a box within a domain box across all supported dimensions (1D, 2D, and 3D). The implementation follows a consistent pattern for each dimension, which enhances readability and maintainability.
228-267: Improved overlap computation using the new shift utilityThe
compute_overlapsfunction has been rewritten to leverage the newpossible_periodic_shiftsutility, resulting in cleaner and more maintainable code. The implementation correctly handles overlaps across all dimensions and follows good practices for offset handling.
457-492: Robust extension of periodic list handling for 3DThe
get_periodic_listfunction has been properly extended to handle 3D cases by defining appropriate sides for all three dimensions. The implementation correctly handles sorting of side labels before using them as keys for shifts, ensuring consistent lookup regardless of detection order.
596-596: Improved boxm.remove usage with list comprehensionThe code now uses a list comprehension for handling the removal of patch boxes, which is more concise and pythonic.
pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (1)
170-179: Enhanced large patch overlap testing with dictionary comprehensionThe test has been updated to verify overlaps for large patches using a cleaner dictionary comprehension approach. The assertions properly validate that the expected and actual overlaps match, which ensures the geometric calculations correctly handle larger patches.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
pyphare/pyphare/pharesee/geometry.py (1)
270-409: 🛠️ Refactor suggestionConsider merging or differentiating the two overlap functions
There are now two similar functions:
compute_overlapsandcompute_overlaps_. The latter seems to be the original implementation with detailed comments. Consider either:
- Merging them if both are needed and one is meant to replace the other
- Better differentiating their purposes with clear documentation
- Marking one as deprecated if it's being phased out
♻️ Duplicate comments (2)
pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (2)
124-129: 🛠️ Refactor suggestionUse assertIn for better error messages
Replace
assertTrue(a in b)withassertIn(a, b)to get more informative error messages when tests fail.-for ilvl, lvl in enumerate(hierarchy.levels().items()): +for ilvl, _lvl in enumerate(hierarchy.levels().items()): if ilvl not in expected: continue self.assertEqual(len(expected[ilvl]), len(level_overlaps[ilvl])) - for actual in tested_overlaps[ilvl]: - self.assertTrue(actual in expected[ilvl]) + for actual in tested_overlaps[ilvl]: + self.assertIn(actual, expected[ilvl])🧰 Tools
🪛 Ruff (0.8.2)
124-124: Loop control variable
lvlnot used within loop bodyRename unused
lvlto_lvl(B007)
170-179: 🛠️ Refactor suggestionImprove test assertions
Same improvements as above:
-for actual in tested_overlaps[ilvl]: - self.assertTrue(actual in expected) +for actual in tested_overlaps[ilvl]: + self.assertIn(actual, expected)
🧹 Nitpick comments (3)
pyphare/pyphare/pharesee/geometry.py (1)
85-189: Refactor the periodicity shifts function to improve variable namingThe function has been extended to support 3D periodicity shifts, which is great. However, there are several uses of ambiguous variable names
lwhich can be confusing.- front = { - f"{k}front": [(v[0], v[1], shape_z) for v in l] for k, l in shifts.items() - } - back = { - f"{k}back": [([v[0], v[1], -shape_z]) for v in l] for k, l in shifts.items() - } - - shifts = {k: [([v[0], v[1], 0]) for v in l] for k, l in shifts.items()} + front = { + f"{k}front": [(v[0], v[1], shape_z) for v in shift_list] for k, shift_list in shifts.items() + } + back = { + f"{k}back": [([v[0], v[1], -shape_z]) for v in shift_list] for k, shift_list in shifts.items() + } + + shifts = {k: [([v[0], v[1], 0]) for v in shift_list] for k, shift_list in shifts.items()}Also, on line 188:
- shifts = {"".join(sorted(k)): l for k, l in shifts.items()} + shifts = {"".join(sorted(k)): shift_list for k, shift_list in shifts.items()}🧰 Tools
🪛 Ruff (0.8.2)
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_tests/test_pharesee/test_geometry.py (1)
81-95: Improved overlap testing with structured dictionaryThe introduction of the
tested_overlapsdictionary improves code readability and maintainability. The testing logic is now more straightforward.There's an unused loop variable that should be renamed:
-for ilvl, lvl in enumerate(hierarchy.patch_levels): +for ilvl, _lvl in enumerate(hierarchy.patch_levels):or simply:
-for ilvl, lvl in enumerate(hierarchy.patch_levels): +for ilvl in range(len(hierarchy.patch_levels)):🧰 Tools
🪛 Ruff (0.8.2)
89-89: Loop control variable
lvlnot used within loop bodyRename unused
lvlto_lvl(B007)
pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (1)
117-123: Improved structure for overlap testingThe introduction of the
tested_overlapsdictionary improves code organization. There are a couple of minor improvements to make:
- Use
key in dictinstead ofkey in dict.keys()for better performance-for ilvl in expected.keys() +for ilvl in expected🧰 Tools
🪛 Ruff (0.8.2)
122-122: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pyphare/pyphare/pharesee/geometry.py(7 hunks)pyphare/pyphare_tests/test_pharesee/test_geometry.py(2 hunks)pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (1)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
levels(214-217)
pyphare/pyphare/pharesee/geometry.py (3)
pyphare/pyphare/core/box.py (4)
Box(5-72)shape(32-34)copy(71-72)copy(141-145)pyphare/pyphare/simulator/simulator.py (1)
domain_box(279-281)pyphare/pyphare/core/gridlayout.py (1)
ndim(221-222)
🪛 Ruff (0.8.2)
pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py
122-122: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
124-124: Loop control variable lvl not used within loop body
Rename unused lvl to _lvl
(B007)
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_tests/test_pharesee/test_geometry.py
89-89: Loop control variable lvl not used within loop body
Rename unused lvl to _lvl
(B007)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (macos-13)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (4)
pyphare/pyphare/pharesee/geometry.py (4)
63-74: Added 3D support for domain border ghost boxesThe implementation for 3D ghost boxes is complete and follows the same pattern as the 2D implementation. This is a key enhancement for the 3D update.
193-226: New function for calculating possible periodic shiftsThis utility function elegantly handles all possible shifts for 1D, 2D, and 3D cases. It's a good abstraction that simplifies the overlap calculation logic.
228-269: Improved overlap calculation using the new periodic shifts functionThe refactored
compute_overlapsfunction leverages the newpossible_periodic_shiftsfunction to handle all dimensionalities in a more consistent way.
460-478: Added 3D support for domain side definitionThe implementation properly extends the 2D side definitions to include "front" and "back" for the z-dimension in 3D cases.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pyphare/pyphare/pharesee/geometry.py (2)
184-189: Improve readability of dictionary key transformation.The key checking and transformation logic is clever but could be more readable. Also, the variable
lis ambiguous.- assert len(list(shifts.keys())) == len( - set(["".join(sorted(k)) for k in list(shifts.keys())]) - ) - - shifts = {"".join(sorted(k)): l for k, l in shifts.items()} + # Ensure all keys are unique when normalized (sorted characters) + assert len(list(shifts.keys())) == len( + set(["".join(sorted(k)) for k in list(shifts.keys())]) + ) + + # Normalize keys by sorting their characters + shifts = {"".join(sorted(k)): shift_list for k, shift_list in shifts.items()}🧰 Tools
🪛 Ruff (0.8.2)
188-188: Ambiguous variable name:
l(E741)
275-413: Consider removing deprecated function.The
compute_overlaps_function appears to be kept for backward compatibility or as a reference while the newcompute_overlapsfunction is introduced. Once you've verified the new implementation works correctly across all test cases, consider removing this deprecated version to avoid confusion and reduce code duplication.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pyphare/pyphare/pharesee/geometry.py(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pyphare/pyphare/pharesee/geometry.py (2)
pyphare/pyphare/core/box.py (4)
Box(5-72)shape(32-34)copy(71-72)copy(141-145)pyphare/pyphare/core/phare_utilities.py (1)
np_array_ify(62-67)
🪛 Ruff (0.8.2)
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)
🔇 Additional comments (5)
pyphare/pyphare/pharesee/geometry.py (5)
63-74: Extended for 3D support!The domain_border_ghost_boxes function has been properly extended to handle 3D cases, adding "front" and "back" ghost boxes for the z-dimension, following the same pattern as the 2D implementation.
193-226: Well-implemented 3D periodic shift generation!This new function efficiently generates all possible periodic shifts for boxes in 1D, 2D, and 3D configurations. The nested loops approach is clear and properly handles all combinations of shifts.
228-272: Good refactoring of overlap computation.The compute_overlaps function has been well refactored to use the new possible_periodic_shifts function, making the code more concise and easier to understand. The logic handles all dimensions consistently.
474-482: Good extension to 3D for side boxes.The get_periodic_list function has been properly extended to handle 3D cases, adding "front" and "back" sides, consistent with the approach in other parts of the code.
601-601: Using Box.remove_all method correctly.The code now uses the
Box.remove_allmethod to subtract multiple boxes at once, which is more efficient than repeatedly applying the subtraction operator.
| f"{k}front": [(v[0], v[1], shape_z) for v in l] for k, l in shifts.items() | ||
| } | ||
| back = { | ||
| f"{k}back": [([v[0], v[1], -shape_z]) for v in l] for k, l in shifts.items() | ||
| } | ||
|
|
||
| shifts = {k: [([v[0], v[1], 0]) for v in l] for k, l in shifts.items()} | ||
|
|
||
| shifts.update(front) | ||
| shifts.update(back) | ||
| shifts.update( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address ambiguous variable names.
The variables l in the dictionary comprehensions make the code harder to read and maintain.
- front = {
- f"{k}front": [(v[0], v[1], shape_z) for v in l] for k, l in shifts.items()
- }
- back = {
- f"{k}back": [([v[0], v[1], -shape_z]) for v in l] for k, l in shifts.items()
- }
-
- shifts = {k: [([v[0], v[1], 0]) for v in l] for k, l in shifts.items()}
+ front = {
+ f"{k}front": [(v[0], v[1], shape_z) for v in shift_list] for k, shift_list in shifts.items()
+ }
+ back = {
+ f"{k}back": [([v[0], v[1], -shape_z]) for v in shift_list] for k, shift_list in shifts.items()
+ }
+
+ shifts = {k: [([v[0], v[1], 0]) for v in shift_list] for k, shift_list in shifts.items()}📝 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.
| f"{k}front": [(v[0], v[1], shape_z) for v in l] for k, l in shifts.items() | |
| } | |
| back = { | |
| f"{k}back": [([v[0], v[1], -shape_z]) for v in l] for k, l in shifts.items() | |
| } | |
| shifts = {k: [([v[0], v[1], 0]) for v in l] for k, l in shifts.items()} | |
| shifts.update(front) | |
| shifts.update(back) | |
| shifts.update( | |
| front = { | |
| f"{k}front": [(v[0], v[1], shape_z) for v in shift_list] for k, shift_list in shifts.items() | |
| } | |
| back = { | |
| f"{k}back": [([v[0], v[1], -shape_z]) for v in shift_list] for k, shift_list in shifts.items() | |
| } | |
| shifts = {k: [([v[0], v[1], 0]) for v in shift_list] for k, shift_list in shifts.items()} | |
| shifts.update(front) | |
| shifts.update(back) | |
| shifts.update( |
🧰 Tools
🪛 Ruff (0.8.2)
161-161: Ambiguous variable name: l
(E741)
164-164: Ambiguous variable name: l
(E741)
167-167: Ambiguous variable name: l
(E741)
updates PHARE to 3D
final PR description is TBD
puts back the level ghost refinement field advance test : #748