overlaps during tests #1031
overlaps during tests #1031
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
tests/simulator/refinement/test_2d_overlaps.py (7)
3-3: Remove unused import.The
osmodule is imported but never used in the code.-import os
10-10: Remove unused import.The
Runclass frompyphare.pharesee.runis imported but never used.-from pyphare.pharesee.run import Run
58-58: Use a more descriptive variable name.The variable name
lis ambiguous. Consider using a more descriptive name likelength_scaleorsigma_y.- def S(y, y0, l): - return 0.5 * (1.0 + np.tanh((y - y0) / l)) + def S(y, y0, length_scale): + return 0.5 * (1.0 + np.tanh((y - y0) / length_scale))You'll also need to update the function call on line 91:
- return v1 + (v2 - v1) * (S(y, Ly * 0.3, L) - S(y, Ly * 0.7, L)) + return v1 + (v2 - v1) * (S(y, Ly * 0.3, L) - S(y, Ly * 0.7, L))Note: The
Lvariable is already well-named, so only the parameter name needs updating.
71-74: Remove unused variables or integrate them into the return statement.The variables
dBy1anddBy2are computed but never used. Either remove them or integrate them into the return statement as the comment suggests.Option 1: Remove unused variables
- dBy1 = 2 * dB * x0 * np.exp(-(x0**2 + y1**2) / (sigma) ** 2) - dBy2 = -2 * dB * x0 * np.exp(-(x0**2 + y2**2) / (sigma) ** 2) - - return 0.0 # dBy1 + dBy2 + return 0.0Option 2: Use the computed perturbations
dBy1 = 2 * dB * x0 * np.exp(-(x0**2 + y1**2) / (sigma) ** 2) dBy2 = -2 * dB * x0 * np.exp(-(x0**2 + y2**2) / (sigma) ** 2) - return 0.0 # dBy1 + dBy2 + return dBy1 + dBy2
86-91: Remove unused variables or integrate them into the return statement.Similar to the
byfunction,dBx1anddBx2are computed but never used. Either remove them or integrate them into the return statement.Option 1: Remove unused variables
- dBx1 = -2 * dB * y1 * np.exp(-(x0**2 + y1**2) / (sigma) ** 2) - dBx2 = 2 * dB * y2 * np.exp(-(x0**2 + y2**2) / (sigma) ** 2) - v1 = -1 v2 = 1.0 - return v1 + (v2 - v1) * (S(y, Ly * 0.3, L) - S(y, Ly * 0.7, L)) # + dBx1 + dBx2 + return v1 + (v2 - v1) * (S(y, Ly * 0.3, L) - S(y, Ly * 0.7, L))Option 2: Use the computed perturbations
dBx1 = -2 * dB * y1 * np.exp(-(x0**2 + y1**2) / (sigma) ** 2) dBx2 = 2 * dB * y2 * np.exp(-(x0**2 + y2**2) / (sigma) ** 2) v1 = -1 v2 = 1.0 - return v1 + (v2 - v1) * (S(y, Ly * 0.3, L) - S(y, Ly * 0.7, L)) # + dBx1 + dBx2 + return v1 + (v2 - v1) * (S(y, Ly * 0.3, L) - S(y, Ly * 0.7, L)) + dBx1 + dBx2
167-167: Modernize super() call.Consider using Python 3 style super() without arguments for cleaner code.
- super(HarrisTest, self).__init__(*args, **kwargs) + super().__init__(*args, **kwargs)
173-173: Modernize super() call.Consider using Python 3 style super() without arguments for cleaner code.
- super(HarrisTest, self).tearDown() + super().tearDown()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/simulator/refinement/CMakeLists.txt(1 hunks)tests/simulator/refinement/test_2d_overlaps.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
tests/simulator/refinement/test_2d_overlaps.py
3-3: os imported but unused
Remove unused import: os
(F401)
10-10: pyphare.pharesee.run.Run imported but unused
Remove unused import: pyphare.pharesee.run.Run
(F401)
58-58: Ambiguous variable name: l
(E741)
71-71: Local variable dBy1 is assigned to but never used
Remove assignment to unused variable dBy1
(F841)
72-72: Local variable dBy2 is assigned to but never used
Remove assignment to unused variable dBy2
(F841)
86-86: Local variable dBx1 is assigned to but never used
Remove assignment to unused variable dBx1
(F841)
87-87: Local variable dBx2 is assigned to but never used
Remove assignment to unused variable dBx2
(F841)
🪛 Flake8 (7.2.0)
tests/simulator/refinement/test_2d_overlaps.py
[error] 3-3: 'os' imported but unused
(F401)
[error] 10-10: 'pyphare.pharesee.run.Run' imported but unused
(F401)
[error] 58-58: ambiguous variable name 'l'
(E741)
[error] 71-71: local variable 'dBy1' is assigned to but never used
(F841)
[error] 72-72: local variable 'dBy2' is assigned to but never used
(F841)
[error] 86-86: local variable 'dBx1' is assigned to but never used
(F841)
[error] 87-87: local variable 'dBx2' is assigned to but never used
(F841)
🪛 Pylint (3.3.7)
tests/simulator/refinement/test_2d_overlaps.py
[error] 8-8: No name 'pharein' in module 'pyphare'
(E0611)
[error] 9-9: No name 'cpp' in module 'pyphare'
(E0611)
[error] 10-10: No name 'pharesee' in module 'pyphare'
(E0611)
[error] 11-11: No name 'simulator' in module 'pyphare'
(E0611)
[refactor] 30-30: Too many local variables (17/15)
(R0914)
[error] 149-149: No name 'pharesee' in module 'pyphare'
(E0611)
[refactor] 167-167: Consider using Python 3 style super() without arguments
(R1725)
[refactor] 173-173: Consider using Python 3 style super() without arguments
(R1725)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
🔇 Additional comments (3)
tests/simulator/refinement/CMakeLists.txt (1)
18-18: LGTM! Clean integration of the new overlap test.The addition of the non-MPI test execution is well-placed within the HighFive conditional block, allowing it to run unconditionally while the existing MPI tests remain gated by the testMPI condition. This approach ensures the overlap verification test runs regardless of MPI configuration.
tests/simulator/refinement/test_2d_overlaps.py (2)
183-187: Well-implemented overlap verification logic.The post-advance callback correctly implements the overlap verification by:
- Running only on MPI rank 0 to avoid duplicate checks
- Loading hierarchical data for the current time step
- Calling the base test method to verify overlapped fields are equal
This approach ensures consistent verification of refinement overlaps after each simulation step.
190-192: Proper MPI initialization and test execution.The main execution block correctly initializes MPI before running the test and properly tears down afterwards. This ensures the test runs in the expected MPI environment.
PhilipDeegan
force-pushed
the
L1_overlaps
branch
from
ab1045d to
4037a66
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
tests/simulator/refinement/test_2d_overlaps.py (6)
26-26: Fix typo in diagnostic directory path.There's a typo in the directory path that should be corrected.
-diag_dir = "phare_outputs/test/refinenment/2d/overlaps" +diag_dir = "phare_outputs/test/refinement/2d/overlaps"
58-59: Use a more descriptive variable name.The variable name
lis ambiguous and should be more descriptive.-def S(y, y0, l): - return 0.5 * (1.0 + np.tanh((y - y0) / l)) +def S(y, y0, length_scale): + return 0.5 * (1.0 + np.tanh((y - y0) / length_scale))
163-163: Use modern Python 3 style super() calls.The super() calls should use the modern Python 3 syntax without arguments.
- super(HarrisTest, self).__init__(*args, **kwargs) + super().__init__(*args, **kwargs) - super(HarrisTest, self).tearDown() + super().tearDown()Also applies to: 169-169
175-177: Consider improving error handling and test structure.The test method creates a simulator but doesn't store it as an instance variable, which could make cleanup inconsistent. Also, the method returns
selfwhich is unusual for a test method.def test_run(self): - Simulator(config(), post_advance=self.post_advance).run().reset() - return self + self.simulator = Simulator(config(), post_advance=self.post_advance) + self.simulator.run()This ensures proper cleanup in the tearDown method and follows more conventional test patterns.
179-183: Add error handling for the post-advance callback.The post-advance callback could benefit from error handling to make test failures more informative.
def post_advance(self, new_time): if cpp.mpi_rank() == 0: - L0L1_datahier = get_hier(diag_dir) - test.base_test_overlaped_fields_are_equal(L0L1_datahier, new_time) + try: + L0L1_datahier = get_hier(diag_dir) + test.base_test_overlaped_fields_are_equal(L0L1_datahier, new_time) + except Exception as e: + print(f"Error in overlap verification at time {new_time}: {e}") + raise
185-187: Improve main execution block structure.The main execution could be more robust with proper error handling.
if __name__ == "__main__": startMPI() - HarrisTest().test_run().tearDown() + test_instance = HarrisTest() + try: + test_instance.test_run() + finally: + test_instance.tearDown()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/simulator/refinement/CMakeLists.txt(1 hunks)tests/simulator/refinement/test_2d_overlaps.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/simulator/refinement/CMakeLists.txt
🧰 Additional context used
🪛 Ruff (0.11.9)
tests/simulator/refinement/test_2d_overlaps.py
3-3: os imported but unused
Remove unused import: os
(F401)
10-10: pyphare.pharesee.run.Run imported but unused
Remove unused import: pyphare.pharesee.run.Run
(F401)
58-58: Ambiguous variable name: l
(E741)
71-71: Local variable dBy1 is assigned to but never used
Remove assignment to unused variable dBy1
(F841)
72-72: Local variable dBy2 is assigned to but never used
Remove assignment to unused variable dBy2
(F841)
86-86: Local variable dBx1 is assigned to but never used
Remove assignment to unused variable dBx1
(F841)
87-87: Local variable dBx2 is assigned to but never used
Remove assignment to unused variable dBx2
(F841)
🪛 Flake8 (7.2.0)
tests/simulator/refinement/test_2d_overlaps.py
[error] 3-3: 'os' imported but unused
(F401)
[error] 10-10: 'pyphare.pharesee.run.Run' imported but unused
(F401)
[error] 58-58: ambiguous variable name 'l'
(E741)
[error] 71-71: local variable 'dBy1' is assigned to but never used
(F841)
[error] 72-72: local variable 'dBy2' is assigned to but never used
(F841)
[error] 86-86: local variable 'dBx1' is assigned to but never used
(F841)
[error] 87-87: local variable 'dBx2' is assigned to but never used
(F841)
🪛 Pylint (3.3.7)
tests/simulator/refinement/test_2d_overlaps.py
[error] 8-8: No name 'pharein' in module 'pyphare'
(E0611)
[error] 9-9: No name 'cpp' in module 'pyphare'
(E0611)
[error] 10-10: No name 'pharesee' in module 'pyphare'
(E0611)
[error] 11-11: No name 'simulator' in module 'pyphare'
(E0611)
[refactor] 30-30: Too many local variables (17/15)
(R0914)
[error] 149-149: No name 'pharesee' in module 'pyphare'
(E0611)
[refactor] 163-163: Consider using Python 3 style super() without arguments
(R1725)
[refactor] 169-169: Consider using Python 3 style super() without arguments
(R1725)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
PhilipDeegan
force-pushed
the
L1_overlaps
branch
from
4037a66 to
1998aec
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
tests/simulator/refinement/test_2d_overlaps.py (3)
1-11: Clean up unused imports.
30-143: Refactor configuration function to improve maintainability.
61-92: Remove unused variables or implement commented functionality.
🧹 Nitpick comments (3)
tests/simulator/refinement/test_2d_overlaps.py (2)
26-26: Fix typo in directory path.The directory name contains a typo: "refinenment" should be "refinement".
-diag_dir = "phare_outputs/test/refinenment/2d/overlaps" +diag_dir = "phare_outputs/test/refinement/2d/overlaps"
58-58: Rename ambiguous variable name.The variable name
lis ambiguous and should be more descriptive.-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))src/core/debug.hpp (1)
36-119: Consider thread safety for global debugging configuration.The global static
debuggingmap could potentially cause issues in multi-threaded environments if modified during runtime.Consider making this configuration
constif it's not meant to be modified during runtime, or protect access with appropriate synchronization if runtime modification is needed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/amr/data/particles/particles_data.hpp(0 hunks)src/amr/multiphysics_integrator.hpp(3 hunks)src/amr/resources_manager/resources_manager.hpp(1 hunks)src/amr/solvers/solver_ppc.hpp(11 hunks)src/amr/utilities/box/amr_box.hpp(2 hunks)src/core/CMakeLists.txt(1 hunks)src/core/data/grid/grid.hpp(2 hunks)src/core/data/grid/gridlayout.hpp(1 hunks)src/core/debug.cpp(1 hunks)src/core/debug.hpp(1 hunks)src/core/logger.hpp(1 hunks)src/core/utilities/box/box.hpp(4 hunks)src/core/utilities/point/point.hpp(3 hunks)src/simulator/simulator.hpp(3 hunks)tests/simulator/refinement/CMakeLists.txt(1 hunks)tests/simulator/refinement/test_2d_overlaps.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/amr/data/particles/particles_data.hpp
✅ Files skipped from review due to trivial changes (4)
- src/core/CMakeLists.txt
- src/amr/resources_manager/resources_manager.hpp
- src/simulator/simulator.hpp
- src/amr/multiphysics_integrator.hpp
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/simulator/refinement/CMakeLists.txt
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/core/logger.hppsrc/core/data/grid/grid.hppsrc/amr/solvers/solver_ppc.hppsrc/core/utilities/point/point.hppsrc/amr/utilities/box/amr_box.hppsrc/core/data/grid/gridlayout.hppsrc/core/utilities/box/box.hppsrc/core/debug.hpp
🪛 Ruff (0.11.9)
tests/simulator/refinement/test_2d_overlaps.py
3-3: os imported but unused
Remove unused import: os
(F401)
10-10: pyphare.pharesee.run.Run imported but unused
Remove unused import: pyphare.pharesee.run.Run
(F401)
58-58: Ambiguous variable name: l
(E741)
71-71: Local variable dBy1 is assigned to but never used
Remove assignment to unused variable dBy1
(F841)
72-72: Local variable dBy2 is assigned to but never used
Remove assignment to unused variable dBy2
(F841)
86-86: Local variable dBx1 is assigned to but never used
Remove assignment to unused variable dBx1
(F841)
87-87: Local variable dBx2 is assigned to but never used
Remove assignment to unused variable dBx2
(F841)
🪛 Flake8 (7.2.0)
tests/simulator/refinement/test_2d_overlaps.py
[error] 3-3: 'os' imported but unused
(F401)
[error] 10-10: 'pyphare.pharesee.run.Run' imported but unused
(F401)
[error] 58-58: ambiguous variable name 'l'
(E741)
[error] 71-71: local variable 'dBy1' is assigned to but never used
(F841)
[error] 72-72: local variable 'dBy2' is assigned to but never used
(F841)
[error] 86-86: local variable 'dBx1' is assigned to but never used
(F841)
[error] 87-87: local variable 'dBx2' is assigned to but never used
(F841)
🪛 Pylint (3.3.7)
tests/simulator/refinement/test_2d_overlaps.py
[error] 8-8: No name 'pharein' in module 'pyphare'
(E0611)
[error] 9-9: No name 'cpp' in module 'pyphare'
(E0611)
[error] 10-10: No name 'pharesee' in module 'pyphare'
(E0611)
[error] 11-11: No name 'simulator' in module 'pyphare'
(E0611)
[refactor] 30-30: Too many local variables (17/15)
(R0914)
[error] 149-149: No name 'pharesee' in module 'pyphare'
(E0611)
[refactor] 163-163: Consider using Python 3 style super() without arguments
(R1725)
[refactor] 169-169: Consider using Python 3 style super() without arguments
(R1725)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-14)
🔇 Additional comments (15)
src/core/logger.hpp (1)
15-15: LGTM! Enhanced logging control flexibility.The addition of
PHARE_FORCE_LOG_LINEprovides a third independent way to enable logging lines, complementing the existing debug mode andPHARE_FORCE_DEBUG_DOconditions. This aligns well with the new debugging framework being introduced.src/core/debug.cpp (1)
8-29: Consider thread safety for the debug scope stack.The singleton implementation and debug scope stack management are correct for single-threaded use. However, if this code is used in a multi-threaded context (e.g., parallel AMR computations), the shared
stack_ptrcould lead to race conditions.Consider adding thread-local storage or mutex protection if multi-threaded access is expected:
Debuggerino& Debuggerino::INSTANCE() { - static Debuggerino i; + thread_local static Debuggerino i; return i; }src/core/data/grid/gridlayout.hpp (2)
1162-1176: LGTM! Well-structured AMR ghost box computation.The
AMRGhostBoxFormethod correctly:
- Computes ghost node counts based on field centering
- Grows the AMR box by the appropriate ghost width
- Adjusts the upper bound for primal-centered directions
The implementation aligns well with the AMR framework requirements.
1152-1158: ```shell
#!/bin/bashSearch for definitions or declarations of
_BoxForacross the repository with contextrg -n -C 3 'template.*_BoxFor' .
rg -n -C 3 '_BoxFor' .</details> <details> <summary>src/amr/solvers/solver_ppc.hpp (2)</summary> `21-21`: **Excellent debug instrumentation coverage!** The debug macros are strategically placed at all critical solver phases: - Method entry/exit tracking with `PHARE_DEBUG_SCOPE` - Overlap verification after each computational step - Field inspection for validation This comprehensive instrumentation will greatly aid in debugging refinement overlap issues. Also applies to: 260-267, 297-297, 325-326, 335-335, 364-364, 375-375, 406-406, 460-460, 486-488 --- `436-436`: **Good cleanup of unused variables.** Removing the unused particle count variables simplifies the debug logging function while retaining the essential ghost particle validation logic. </details> <details> <summary>src/amr/utilities/box/amr_box.hpp (2)</summary> `97-108`: **LGTM!** The particle-in-box check is implemented correctly with proper bounds checking for all dimensions. --- `111-123`: **LGTM!** Clean implementation of SAMRAI to PHARE point conversions using template metaprogramming. </details> <details> <summary>src/core/utilities/box/box.hpp (4)</summary> `28-28`: **LGTM!** Adding the static `dimension` member improves template metaprogramming capabilities. --- `75-87`: **LGTM!** The refactored `grow` methods provide a more flexible API supporting both uniform and per-dimension growth. --- `202-227`: **LGTM!** The new `isIn` overloads provide better type safety and a cleaner API for spatial containment checks. --- `274-281`: **LGTM!** The `shift` function provides a clean utility for box translation. </details> <details> <summary>src/core/utilities/point/point.hpp (3)</summary> `133-159`: **LGTM!** The compound assignment operators provide flexible element-wise operations supporting both scalar and array-like operands. --- `194-208`: **LGTM!** The multiplication operators follow the established pattern and provide consistent element-wise operations. --- `225-240`: **Handle all type sizes in conversion methods.** The `as_unsigned()` and `as_signed()` methods only handle 4-byte types and lack return statements for other sizes, which will cause compilation errors. ```diff auto as_unsigned() const { PHARE_DEBUG_DO({ for (auto iDim = 0u; iDim < dim; ++iDim) assert(r[iDim] >= 0); }) if constexpr (sizeof(Type) == 4) return as<std::uint32_t>(); - // else no return cause not yet handled + else if constexpr (sizeof(Type) == 8) + return as<std::uint64_t>(); + else + static_assert(sizeof(Type) == 4 || sizeof(Type) == 8, + "Only 4-byte and 8-byte types are supported"); } auto as_signed() const { if constexpr (sizeof(Type) == 4) return as<std::int32_t>(); - // else no return cause not yet handled + else if constexpr (sizeof(Type) == 8) + return as<std::int64_t>(); + else + static_assert(sizeof(Type) == 4 || sizeof(Type) == 8, + "Only 4-byte and 8-byte types are supported"); }⛔ Skipped due to learnings
Learnt from: PhilipDeegan PR: PHAREHUB/PHARE#998 File: src/core/utilities/point/point.hpp:195-210 Timestamp: 2025-04-17T14:41:53.643Z Learning: In `src/core/utilities/point/point.hpp`, the `as_signed()` and `as_unsigned()` methods are intentionally designed to cause compilation errors for unsupported types (only 4-byte types are supported). This forces developers to implement proper conversions themselves when extending the functionality to new types. The comment "else no return cause not yet handled" indicates this is a deliberate design choice.
PhilipDeegan
force-pushed
the
L1_overlaps
branch
from
fe05876 to
349fc9c
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
PhilipDeegan
force-pushed
the
L1_overlaps
branch
from
349fc9c to
9e5e7ed
Compare
📝 WalkthroughWalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant TestScript as test_2d_overlaps.py
participant Simulator as ph.Simulation
participant Diagnostics as Output/Diagnostics
participant MPI as MPI Environment
TestScript->>MPI: Initialize MPI
TestScript->>Simulator: Create simulation config (domain, plasma, models)
TestScript->>Simulator: Initialize simulation
loop For each advance step
TestScript->>Simulator: Advance simulation
Simulator->>Diagnostics: Output simulation data
TestScript->>Diagnostics: Load hierarchical data at new_time
TestScript->>TestScript: Check overlap consistency
end
TestScript->>Simulator: Reset simulation
TestScript->>MPI: Finalize MPI
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/simulator/refinement/test_2d_overlaps.py (3)
56-56: Use a more descriptive variable name.The variable
lis ambiguous. Consider using a more descriptive name likewidthortransition_width.- 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))
28-142: Consider refactoring the config function to reduce complexity.The
config()function has 17 local variables and contains many nested function definitions. This makes it hard to read and maintain. Consider extracting the mathematical functions to module level or organizing them into logical groups.You could extract the physics functions outside the config function:
def create_density_profile(L, sim): def density(x, y): Ly = sim.simulation_domain()[1] return ( 0.4 + 1.0 / np.cosh((y - Ly * 0.3) / L) ** 2 + 1.0 / np.cosh((y - Ly * 0.7) / L) ** 2 ) return density def create_magnetic_field_functions(L, sim): # Group magnetic field functions together # ... existing functions return bx, by, bz def config(): L = 0.5 sim = ph.Simulation(...) density = create_density_profile(L, sim) bx, by, bz = create_magnetic_field_functions(L, sim) # ... rest of config
161-161: Use modern Python 3 style super() calls.Consider using the more concise
super()syntax without arguments.- super(HarrisTest, self).__init__(*args, **kwargs) + super().__init__(*args, **kwargs)- super(HarrisTest, self).tearDown() + super().tearDown()Also applies to: 167-167
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)tests/simulator/refinement/CMakeLists.txt(1 hunks)tests/simulator/refinement/test_2d_overlaps.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-07-26T22:04:34.160Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-10-09T08:32:15.667Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-09-05T17:02:58.784Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-10-09T08:32:15.667Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#888
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:566-568
Timestamp: 2024-09-09T13:57:02.285Z
Learning: Avoid commenting on draft PRs for PhilipDeegan unless explicitly requested.
tests/simulator/refinement/CMakeLists.txt (1)
Learnt from: nicolasaunai
PR: PHAREHUB/PHARE#591
File: res/cmake/options.cmake:73-75
Timestamp: 2025-02-07T14:35:14.630Z
Learning: In CMake configuration for PHARE, `lowResourceTests` and `highResourceTests` options must be mutually exclusive and cannot be enabled simultaneously.
🪛 Ruff (0.11.9)
tests/simulator/refinement/test_2d_overlaps.py
56-56: Ambiguous variable name: l
(E741)
69-69: Local variable dBy1 is assigned to but never used
Remove assignment to unused variable dBy1
(F841)
70-70: Local variable dBy2 is assigned to but never used
Remove assignment to unused variable dBy2
(F841)
84-84: Local variable dBx1 is assigned to but never used
Remove assignment to unused variable dBx1
(F841)
85-85: Local variable dBx2 is assigned to but never used
Remove assignment to unused variable dBx2
(F841)
🪛 Flake8 (7.2.0)
tests/simulator/refinement/test_2d_overlaps.py
[error] 56-56: ambiguous variable name 'l'
(E741)
[error] 69-69: local variable 'dBy1' is assigned to but never used
(F841)
[error] 70-70: local variable 'dBy2' is assigned to but never used
(F841)
[error] 84-84: local variable 'dBx1' is assigned to but never used
(F841)
[error] 85-85: local variable 'dBx2' is assigned to but never used
(F841)
🪛 Pylint (3.3.7)
tests/simulator/refinement/test_2d_overlaps.py
[error] 7-7: No name 'pharein' in module 'pyphare'
(E0611)
[error] 8-8: No name 'cpp' in module 'pyphare'
(E0611)
[error] 9-9: No name 'simulator' in module 'pyphare'
(E0611)
[refactor] 28-28: Too many local variables (17/15)
(R0914)
[error] 147-147: No name 'pharesee' in module 'pyphare'
(E0611)
[refactor] 161-161: Consider using Python 3 style super() without arguments
(R1725)
[refactor] 167-167: Consider using Python 3 style super() without arguments
(R1725)
⏰ 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 (5)
.gitignore (1)
25-26: LGTM! Standard ignore entries added.The additions of
.cacheand.gdbinitto the ignore list are appropriate and follow standard practices for development environments.tests/simulator/refinement/CMakeLists.txt (1)
18-18: LGTM! Test integration correctly configured.The new test execution is properly conditioned on HighFive and placed outside the MPI conditional, allowing it to run independently of MPI-specific tests.
tests/simulator/refinement/test_2d_overlaps.py (3)
1-26: LGTM! Clean setup and imports.The imports and global configuration are well-structured and appropriate for a simulation test.
180-183: LGTM! Core test logic is well-implemented.The post-advance callback correctly validates overlapped fields for consistency, which is the main purpose of this test.
186-189: LGTM! Clean test execution pattern.The main execution block follows a clean pattern with proper MPI initialization and teardown.
| dBy1 = 2 * dB * x0 * np.exp(-(x0**2 + y1**2) / (sigma) ** 2) | ||
| dBy2 = -2 * dB * x0 * np.exp(-(x0**2 + y2**2) / (sigma) ** 2) | ||
|
|
||
| return 0.0 # dBy1 + dBy2 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused variables or implement the intended functionality.
The variables dBy1 and dBy2 are calculated but never used, and the function returns 0.0. If these perturbations are intended for future use, consider removing them for now or documenting the intent.
def by(x, y):
- Lx = sim.simulation_domain()[0]
- Ly = sim.simulation_domain()[1]
- sigma = 1.0
- dB = 0.1
-
- x0 = x - 0.5 * Lx
- y1 = y - 0.3 * Ly
- y2 = y - 0.7 * Ly
-
- dBy1 = 2 * dB * x0 * np.exp(-(x0**2 + y1**2) / (sigma) ** 2)
- dBy2 = -2 * dB * x0 * np.exp(-(x0**2 + y2**2) / (sigma) ** 2)
-
- return 0.0 # dBy1 + dBy2
+ return 0.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.
| dBy1 = 2 * dB * x0 * np.exp(-(x0**2 + y1**2) / (sigma) ** 2) | |
| dBy2 = -2 * dB * x0 * np.exp(-(x0**2 + y2**2) / (sigma) ** 2) | |
| return 0.0 # dBy1 + dBy2 | |
| def by(x, y): | |
| return 0.0 |
🧰 Tools
🪛 Ruff (0.11.9)
69-69: Local variable dBy1 is assigned to but never used
Remove assignment to unused variable dBy1
(F841)
70-70: Local variable dBy2 is assigned to but never used
Remove assignment to unused variable dBy2
(F841)
🪛 Flake8 (7.2.0)
[error] 69-69: local variable 'dBy1' is assigned to but never used
(F841)
[error] 70-70: local variable 'dBy2' is assigned to but never used
(F841)
🤖 Prompt for AI Agents
In tests/simulator/refinement/test_2d_overlaps.py around lines 69 to 72, the
variables dBy1 and dBy2 are computed but not used, and the function returns 0.0.
To fix this, either remove the calculations of dBy1 and dBy2 if they are not
needed, or update the return statement to return the sum of dBy1 and dBy2 if
that is the intended functionality. Additionally, add a comment explaining the
purpose if you keep the variables for future use.
| dBx1 = -2 * dB * y1 * np.exp(-(x0**2 + y1**2) / (sigma) ** 2) | ||
| dBx2 = 2 * dB * y2 * np.exp(-(x0**2 + y2**2) / (sigma) ** 2) | ||
|
|
||
| v1 = -1 | ||
| v2 = 1.0 | ||
| return v1 + (v2 - v1) * (S(y, Ly * 0.3, L) - S(y, Ly * 0.7, L)) # + dBx1 + dBx2 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused variables or implement the intended functionality.
Similar to the by function, dBx1 and dBx2 are calculated but commented out in the return statement. Consider removing the unused calculations for cleaner code.
def bx(x, y):
Lx = sim.simulation_domain()[0]
Ly = sim.simulation_domain()[1]
- sigma = 1.0
- dB = 0.1
-
- x0 = x - 0.5 * Lx
- y1 = y - 0.3 * Ly
- y2 = y - 0.7 * Ly
-
- dBx1 = -2 * dB * y1 * np.exp(-(x0**2 + y1**2) / (sigma) ** 2)
- dBx2 = 2 * dB * y2 * np.exp(-(x0**2 + y2**2) / (sigma) ** 2)
v1 = -1
v2 = 1.0
- return v1 + (v2 - v1) * (S(y, Ly * 0.3, L) - S(y, Ly * 0.7, L)) # + dBx1 + dBx2
+ return v1 + (v2 - v1) * (S(y, Ly * 0.3, L) - S(y, Ly * 0.7, L))📝 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.
| dBx1 = -2 * dB * y1 * np.exp(-(x0**2 + y1**2) / (sigma) ** 2) | |
| dBx2 = 2 * dB * y2 * np.exp(-(x0**2 + y2**2) / (sigma) ** 2) | |
| v1 = -1 | |
| v2 = 1.0 | |
| return v1 + (v2 - v1) * (S(y, Ly * 0.3, L) - S(y, Ly * 0.7, L)) # + dBx1 + dBx2 | |
| def bx(x, y): | |
| Lx = sim.simulation_domain()[0] | |
| Ly = sim.simulation_domain()[1] | |
| v1 = -1 | |
| v2 = 1.0 | |
| return v1 + (v2 - v1) * (S(y, Ly * 0.3, L) - S(y, Ly * 0.7, L)) |
🧰 Tools
🪛 Ruff (0.11.9)
84-84: Local variable dBx1 is assigned to but never used
Remove assignment to unused variable dBx1
(F841)
85-85: Local variable dBx2 is assigned to but never used
Remove assignment to unused variable dBx2
(F841)
🪛 Flake8 (7.2.0)
[error] 84-84: local variable 'dBx1' is assigned to but never used
(F841)
[error] 85-85: local variable 'dBx2' is assigned to but never used
(F841)
🤖 Prompt for AI Agents
In tests/simulator/refinement/test_2d_overlaps.py around lines 84 to 89, the
variables dBx1 and dBx2 are calculated but not used because their addition in
the return statement is commented out. To clean up the code, either remove the
calculations of dBx1 and dBx2 entirely if they are not needed, or uncomment and
properly integrate them into the return expression if their effect is intended
to be included.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/simulator/refinement/test_2d_overlaps.py (2)
69-72: Remove unused variables or implement the intended functionality.The variables
dBy1anddBy2are calculated but never used, and the function returns 0.0.
84-89: Remove unused variables or implement the intended functionality.Similar to the
byfunction,dBx1anddBx2are calculated but commented out in the return statement.
🧹 Nitpick comments (1)
tests/simulator/refinement/test_2d_overlaps.py (1)
56-57: Rename ambiguous parameter name.The parameter
lcan be confused with the number1. Consider using a more descriptive name.-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))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)tests/simulator/refinement/CMakeLists.txt(1 hunks)tests/simulator/refinement/test_2d_overlaps.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- tests/simulator/refinement/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#888
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:566-568
Timestamp: 2024-09-09T13:57:02.285Z
Learning: Avoid commenting on draft PRs for PhilipDeegan unless explicitly requested.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-09-05T17:02:58.784Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-10-09T08:32:15.667Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-07-26T22:04:34.160Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-10-09T08:32:15.667Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
🪛 Ruff (0.11.9)
tests/simulator/refinement/test_2d_overlaps.py
56-56: Ambiguous variable name: l
(E741)
69-69: Local variable dBy1 is assigned to but never used
Remove assignment to unused variable dBy1
(F841)
70-70: Local variable dBy2 is assigned to but never used
Remove assignment to unused variable dBy2
(F841)
84-84: Local variable dBx1 is assigned to but never used
Remove assignment to unused variable dBx1
(F841)
85-85: Local variable dBx2 is assigned to but never used
Remove assignment to unused variable dBx2
(F841)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
| time_step = 0.005 | ||
| final_time = 1 | ||
| timestamps = np.arange(0, final_time + time_step, time_step) | ||
| diag_dir = "phare_outputs/test/refinenment/2d/overlaps" |
There was a problem hiding this comment.
Fix typo in directory path.
The directory name contains a typo: "refinenment" should be "refinement".
-diag_dir = "phare_outputs/test/refinenment/2d/overlaps"
+diag_dir = "phare_outputs/test/refinement/2d/overlaps"📝 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.
| diag_dir = "phare_outputs/test/refinenment/2d/overlaps" | |
| diag_dir = "phare_outputs/test/refinement/2d/overlaps" |
🤖 Prompt for AI Agents
In tests/simulator/refinement/test_2d_overlaps.py at line 24, fix the typo in
the directory path by changing "refinenment" to "refinement" in the diag_dir
string assignment.
|
could make sense to improve the plots for debugging issues like that. |
this is here to demonstrate an issue on master