test cleanup / ruff-ing #1039
test cleanup / ruff-ing #1039
Conversation
📝 Walkthrough## Walkthrough
This change refactors test scripts by reorganizing and minimizing imports, especially for packages with heavy binary dependencies like `scipy`, `matplotlib`, and `numpy`. Imports are moved inside functions where possible, redundant imports are removed, and graphical backend setup is standardized. Minor unused variable assignments and code are also cleaned up.
## Changes
| File(s) | Change Summary |
|---------------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------|
| tests/amr/data/field/refine/test_refine_field.py | Removed local override of `refinement_ratio`; now uses imported value directly. |
| tests/core/data/gridlayout/allocSizes.py, .../deriv.py, .../fieldNodeCoordinates.py, .../gridIndexing.py | Cleaned up and reordered import statements; removed unused variables. |
| tests/core/data/gridlayout/test_linear_combinaisons_yee.py | Removed unused imports. |
| tests/diagnostic/__init__.py | Removed unused `numpy` import. |
| tests/functional/alfven_wave/alfven_wave1d.py, .../harris/harris_2d.py, .../td/td1d.py, .../translation/translat1d.py | Standardized disabling GUI via `ph.NO_GUI()`, removed/changed `matplotlib` backend setup and unused imports. |
| tests/functional/ionIonBeam/ion_ion_beam1d.py | Moved heavy imports (`scipy.optimize`, `scipy.signal`) inside functions; reorganized imports; updated MPI rank access. |
| tests/functional/shock/shock.py, .../test_load_balancing.py, .../test_run.py | Deferred `startMPI()` initialization to main block. |
| tests/functional/tdtagged/td1dtagged.py | Removed unused variable in plotting loop. |
| tests/initializer/job.py | Replaced specific class imports with module import and updated usage accordingly. |
| tests/simulator/__init__.py | Removed Caliper profiling integration code and related functions. |
| tests/simulator/data_wrangler.py, .../refined_particle_nbr.py, .../refinement/test_2d_10_core.py, .../test_2d_2_core.py, .../test_advance.py, .../test_diagnostic_timestamps.py, .../test_diagnostics.py, .../test_initialization.py, .../test_restarts.py, .../test_simulation.py, .../test_validation.py | Reorganized import statements; moved instantiation of `cpp` object; removed unused variables and imports. |
| pyphare/pyphare/cpp/__init__.py | Simplified `cpp_lib` function to always import release module; removed override and debug import logic. |
| src/python3/CMakeLists.txt | Removed debug-specific pybind11 module target `cpp_dbg`. |
| tests/diagnostic/job_1d.py.in, tests/diagnostic/job_2d.py.in | Reformatted imports and calls to use `ph` namespace; improved readability without logic changes. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Script
participant ph as pyphare.pharein
participant matplotlib
participant scipy
Script->>ph: import ph
Script->>ph: call ph.NO_GUI() (to disable GUI)
Note over Script,ph: matplotlib imports removed or deferred
Script->>scipy: import inside function (only when needed)Assessment against linked issues
Suggested labels
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
tests/functional/ionIonBeam/ion_ion_beam1d.py (1)
167-167: Remove redundant import.The
find_peaksimport is already present in thegrowth_b_right_handfunction at line 110. Sincemain()callsgrowth_b_right_hand(), this import is redundant.- from scipy.signal import find_peakstests/core/data/gridlayout/allocSizes.py (1)
13-15: Prefer explicit relative import to decouple tests fromPYTHONPATHquirksImporting the sibling module as
from . import gridparamsmakes the script resolvable regardless of the current working directory and avoids accidentally pulling in an unrelated top-levelgridparamsif one exists on the system path.-import gridparams +from . import gridparamstests/diagnostic/__init__.py (1)
1-5:nbr_dump_stepmay be a float — cast before passing tonp.arange.If
sim.final_timeis not an exact multiple ofdump_step,np.arangereceives a float stop which silently truncates the last point. Consider explicit integer rounding for clarity:- nbr_dump_step = sim.final_time / dump_step + nbr_dump_step = int(sim.final_time / dump_step)tests/initializer/job.py (1)
24-26: Generator expression used where a tuple is intended — yields one-shot iterators.
bx, by, bz = (lambda x: 1 for i in range(3))assigns three lambdas but sources them from a generator that is exhausted afterwards. It’s obscure and brittle; prefer an explicit tuple for clarity:-bx, by, bz = (lambda x: 1 for i in range(3)) +bx = by = bz = (lambda x: 1)Same pattern occurs for
ex, ey, ez,vx, vy, vz, andvthx, vthy, vthz.tests/simulator/test_advance.py (3)
1-3: Drop placeholder header commentsThe first two comment lines don’t convey any useful information and just add visual noise.
Consider deleting them for a cleaner file header.-# -# -
29-30: Lazy-loadcpp_lib()to avoid up-front side-effectsInstantiating
cpp = cpp_lib()at module import time can trigger MPI initialisation (or other heavy native setup) before the unit-test framework even decides which tests to run. This makes dry-run discovery slower and complicates mocking.A lightweight pattern:
-cpp = cpp_lib() +_cpp_instance = None + +def cpp(): + """Singleton accessor to cpp_lib(), lazily instantiated.""" + global _cpp_instance + if _cpp_instance is None: + _cpp_instance = cpp_lib() + return _cpp_instanceCall
cpp()where you currently use the globalcpp.
753-755: Useos.path.joinfor platform-safe diagnostic pathsHard-coding forward slashes is fine on POSIX but breaks on Windows. A tiny tweak keeps the tests portable:
-import random +import os, random … -L0_datahier = _getHier("L0_diags") -L0L1_datahier = _getHier("L0L1_diags", refinement_boxes) +L0_datahier = _getHier(os.path.join("L0_diags")) +L0L1_datahier = _getHier(os.path.join("L0L1_diags"), refinement_boxes)tests/simulator/__init__.py (2)
1-3: Remove empty banner commentsThese “# / #” lines serve no functional or documentary purpose after the Caliper clean-up. Deleting them makes the file more concise.
-# -# -
4-4:osimport is now unusedWith the profiling code gone,
osis no longer referenced anywhere in this module and is flagged by Ruff/Flake8. Safe to drop:-import os
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
tests/amr/data/field/refine/test_refine_field.py(0 hunks)tests/core/data/gridlayout/allocSizes.py(1 hunks)tests/core/data/gridlayout/deriv.py(1 hunks)tests/core/data/gridlayout/fieldNodeCoordinates.py(1 hunks)tests/core/data/gridlayout/gridIndexing.py(1 hunks)tests/core/data/gridlayout/test_linear_combinaisons_yee.py(1 hunks)tests/diagnostic/__init__.py(1 hunks)tests/functional/alfven_wave/alfven_wave1d.py(1 hunks)tests/functional/harris/harris_2d.py(2 hunks)tests/functional/ionIonBeam/ion_ion_beam1d.py(4 hunks)tests/functional/shock/shock.py(1 hunks)tests/functional/td/td1d.py(1 hunks)tests/functional/tdtagged/td1dtagged.py(0 hunks)tests/functional/translation/translat1d.py(1 hunks)tests/initializer/job.py(2 hunks)tests/simulator/__init__.py(1 hunks)tests/simulator/data_wrangler.py(1 hunks)tests/simulator/refined_particle_nbr.py(1 hunks)tests/simulator/refinement/test_2d_10_core.py(1 hunks)tests/simulator/refinement/test_2d_2_core.py(1 hunks)tests/simulator/test_advance.py(3 hunks)tests/simulator/test_diagnostic_timestamps.py(1 hunks)tests/simulator/test_diagnostics.py(1 hunks)tests/simulator/test_initialization.py(2 hunks)tests/simulator/test_load_balancing.py(3 hunks)tests/simulator/test_restarts.py(5 hunks)tests/simulator/test_run.py(2 hunks)tests/simulator/test_simulation.py(0 hunks)tests/simulator/test_validation.py(3 hunks)
💤 Files with no reviewable changes (3)
- tests/simulator/test_simulation.py
- tests/functional/tdtagged/td1dtagged.py
- tests/amr/data/field/refine/test_refine_field.py
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
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: 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: 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#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.
tests/functional/harris/harris_2d.py (1)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#910
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:7-7
Timestamp: 2024-10-18T13:23:32.074Z
Learning: In the `pyphare.pharesee.hierarchy` module, importing `PatchHierarchy` and `format_timestamp` from `hierarchy.py` into `hierarchy_utils.py` is acceptable as long as `hierarchy.py` does not import `hierarchy_utils.py`, thereby avoiding a cyclic import.
tests/diagnostic/__init__.py (1)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#910
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:7-7
Timestamp: 2024-10-18T13:23:32.074Z
Learning: In the `pyphare.pharesee.hierarchy` module, importing `PatchHierarchy` and `format_timestamp` from `hierarchy.py` into `hierarchy_utils.py` is acceptable as long as `hierarchy.py` does not import `hierarchy_utils.py`, thereby avoiding a cyclic import.
tests/simulator/test_validation.py (1)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#910
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:7-7
Timestamp: 2024-10-18T13:23:32.074Z
Learning: In the `pyphare.pharesee.hierarchy` module, importing `PatchHierarchy` and `format_timestamp` from `hierarchy.py` into `hierarchy_utils.py` is acceptable as long as `hierarchy.py` does not import `hierarchy_utils.py`, thereby avoiding a cyclic import.
tests/initializer/job.py (4)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#910
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:7-7
Timestamp: 2024-10-18T13:23:32.074Z
Learning: In the `pyphare.pharesee.hierarchy` module, importing `PatchHierarchy` and `format_timestamp` from `hierarchy.py` into `hierarchy_utils.py` is acceptable as long as `hierarchy.py` does not import `hierarchy_utils.py`, thereby avoiding a cyclic import.
Learnt from: rochSmets
PR: PHAREHUB/PHARE#921
File: src/core/data/ions/ions.hpp:138-142
Timestamp: 2025-02-06T10:52:56.461Z
Learning: In PHARE's physics simulations, handling of zero density cells in velocity calculations must be based on physical requirements rather than simple numerical fixes. The solution must account for population-specific behavior in the bulk velocity computation.
Learnt from: rochSmets
PR: PHAREHUB/PHARE#921
File: src/core/data/ions/ions.hpp:138-142
Timestamp: 2025-02-06T10:52:56.461Z
Learning: In PHARE simulations, velocity calculations must account for per-population densities that can be strictly zero in given cells. Simple zero-return solutions for division by zero are inadequate as they don't reflect the correct physics.
Learnt from: rochSmets
PR: PHAREHUB/PHARE#921
File: src/core/data/ions/ions.hpp:138-142
Timestamp: 2025-02-06T10:52:56.461Z
Learning: The bulk velocity calculation in PHARE must maintain physical consistency across both ion and electron calculations. Zero density cases need special handling that preserves the physics, particularly since the calculations are used in level initialization, ion updates, and electron physics.
tests/simulator/data_wrangler.py (1)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#910
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:7-7
Timestamp: 2024-10-18T13:23:32.074Z
Learning: In the `pyphare.pharesee.hierarchy` module, importing `PatchHierarchy` and `format_timestamp` from `hierarchy.py` into `hierarchy_utils.py` is acceptable as long as `hierarchy.py` does not import `hierarchy_utils.py`, thereby avoiding a cyclic import.
tests/simulator/refinement/test_2d_2_core.py (1)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#910
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:7-7
Timestamp: 2024-10-18T13:23:32.074Z
Learning: In the `pyphare.pharesee.hierarchy` module, importing `PatchHierarchy` and `format_timestamp` from `hierarchy.py` into `hierarchy_utils.py` is acceptable as long as `hierarchy.py` does not import `hierarchy_utils.py`, thereby avoiding a cyclic import.
tests/simulator/test_diagnostics.py (1)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#910
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:7-7
Timestamp: 2024-10-18T13:23:32.074Z
Learning: In the `pyphare.pharesee.hierarchy` module, importing `PatchHierarchy` and `format_timestamp` from `hierarchy.py` into `hierarchy_utils.py` is acceptable as long as `hierarchy.py` does not import `hierarchy_utils.py`, thereby avoiding a cyclic import.
tests/simulator/refinement/test_2d_10_core.py (1)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#910
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:7-7
Timestamp: 2024-10-18T13:23:32.074Z
Learning: In the `pyphare.pharesee.hierarchy` module, importing `PatchHierarchy` and `format_timestamp` from `hierarchy.py` into `hierarchy_utils.py` is acceptable as long as `hierarchy.py` does not import `hierarchy_utils.py`, thereby avoiding a cyclic import.
tests/functional/translation/translat1d.py (1)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#910
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:7-7
Timestamp: 2024-10-18T13:23:32.074Z
Learning: In the `pyphare.pharesee.hierarchy` module, importing `PatchHierarchy` and `format_timestamp` from `hierarchy.py` into `hierarchy_utils.py` is acceptable as long as `hierarchy.py` does not import `hierarchy_utils.py`, thereby avoiding a cyclic import.
tests/simulator/test_initialization.py (1)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#910
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:7-7
Timestamp: 2024-10-18T13:23:32.074Z
Learning: In the `pyphare.pharesee.hierarchy` module, importing `PatchHierarchy` and `format_timestamp` from `hierarchy.py` into `hierarchy_utils.py` is acceptable as long as `hierarchy.py` does not import `hierarchy_utils.py`, thereby avoiding a cyclic import.
tests/simulator/test_diagnostic_timestamps.py (1)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#910
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:7-7
Timestamp: 2024-10-18T13:23:32.074Z
Learning: In the `pyphare.pharesee.hierarchy` module, importing `PatchHierarchy` and `format_timestamp` from `hierarchy.py` into `hierarchy_utils.py` is acceptable as long as `hierarchy.py` does not import `hierarchy_utils.py`, thereby avoiding a cyclic import.
tests/simulator/test_restarts.py (1)
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#910
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:7-7
Timestamp: 2024-10-18T13:23:32.074Z
Learning: In the `pyphare.pharesee.hierarchy` module, importing `PatchHierarchy` and `format_timestamp` from `hierarchy.py` into `hierarchy_utils.py` is acceptable as long as `hierarchy.py` does not import `hierarchy_utils.py`, thereby avoiding a cyclic import.
🧬 Code Graph Analysis (8)
tests/functional/shock/shock.py (1)
pyphare/pyphare/simulator/simulator.py (1)
startMPI(39-43)
tests/functional/harris/harris_2d.py (1)
pyphare/pyphare/pharein/__init__.py (1)
NO_GUI(60-64)
tests/simulator/test_run.py (1)
pyphare/pyphare/simulator/simulator.py (1)
startMPI(39-43)
tests/simulator/test_load_balancing.py (1)
pyphare/pyphare/simulator/simulator.py (1)
startMPI(39-43)
tests/functional/alfven_wave/alfven_wave1d.py (1)
pyphare/pyphare/pharein/__init__.py (1)
NO_GUI(60-64)
tests/simulator/refinement/test_2d_10_core.py (2)
pyphare/pyphare/simulator/simulator.py (1)
startMPI(39-43)pyphare/pyphare/pharein/__init__.py (1)
NO_GUI(60-64)
tests/functional/translation/translat1d.py (2)
tests/diagnostic/__init__.py (1)
all_timestamps(8-12)pyphare/pyphare/pharein/__init__.py (1)
NO_GUI(60-64)
tests/simulator/test_initialization.py (1)
pyphare/pyphare/cpp/__init__.py (1)
cpp_lib(12-26)
🪛 Pylint (3.3.7)
tests/diagnostic/__init__.py
[error] 16-16: No name 'pharein' in module 'pyphare'
(E0611)
tests/simulator/test_validation.py
[error] 7-7: No name 'cpp' in module 'pyphare'
(E0611)
[error] 8-8: No name 'core' in module 'pyphare'
(E0611)
[error] 9-9: No name 'simulator' in module 'pyphare'
(E0611)
tests/simulator/data_wrangler.py
[error] 9-9: No name 'cpp' in module 'pyphare'
(E0611)
[error] 10-10: No name 'simulator' in module 'pyphare'
(E0611)
tests/simulator/refinement/test_2d_2_core.py
[error] 14-14: No name 'pharein' in module 'pyphare'
(E0611)
[error] 15-15: No name 'cpp' in module 'pyphare'
(E0611)
[error] 16-16: No name 'core' in module 'pyphare'
(E0611)
[error] 17-17: No name 'simulator' in module 'pyphare'
(E0611)
tests/simulator/test_diagnostics.py
[error] 10-10: No name 'pharein' in module 'pyphare'
(E0611)
[error] 11-11: No name 'cpp' in module 'pyphare'
(E0611)
[error] 12-12: No name 'pharein' in module 'pyphare'
(E0611)
[error] 13-13: No name 'pharesee' in module 'pyphare'
(E0611)
[error] 14-14: No name 'pharesee' in module 'pyphare'
(E0611)
[error] 15-15: No name 'simulator' in module 'pyphare'
(E0611)
tests/simulator/refinement/test_2d_10_core.py
[error] 14-14: No name 'core' in module 'pyphare'
(E0611)
[error] 15-15: No name 'simulator' in module 'pyphare'
(E0611)
tests/simulator/test_initialization.py
[error] 8-8: No name 'cpp' in module 'pyphare'
(E0611)
tests/simulator/refined_particle_nbr.py
[error] 12-12: No name 'cpp' in module 'pyphare'
(E0611)
[error] 13-13: No name 'cpp' in module 'pyphare'
(E0611)
[error] 14-14: No name 'simulator' in module 'pyphare'
(E0611)
tests/simulator/test_diagnostic_timestamps.py
[error] 10-10: No name 'pharein' in module 'pyphare'
(E0611)
[error] 11-11: No name 'cpp' in module 'pyphare'
(E0611)
[error] 12-12: No name 'core' in module 'pyphare'
(E0611)
[error] 13-13: No name 'pharein' in module 'pyphare'
(E0611)
[error] 14-14: No name 'pharesee' in module 'pyphare'
(E0611)
[error] 15-15: No name 'pharesee' in module 'pyphare'
(E0611)
[error] 16-16: No name 'pharesee' in module 'pyphare'
(E0611)
[error] 17-17: No name 'simulator' in module 'pyphare'
(E0611)
tests/simulator/test_advance.py
[error] 8-8: No name 'core' in module 'pyphare'
(E0611)
[error] 10-10: No name 'cpp' in module 'pyphare'
(E0611)
tests/simulator/test_restarts.py
[error] 13-13: No name 'pharein' in module 'pyphare'
(E0611)
[error] 15-15: No name 'cpp' in module 'pyphare'
(E0611)
tests/simulator/__init__.py
[error] 9-9: No name 'pharein' in module 'pyphare'
(E0611)
🪛 Ruff (0.11.9)
tests/simulator/__init__.py
4-4: os imported but unused
(F401)
🪛 Flake8 (7.2.0)
tests/simulator/__init__.py
[error] 4-4: 'os' imported but unused
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
🔇 Additional comments (32)
tests/core/data/gridlayout/test_linear_combinaisons_yee.py (1)
4-8: No danglingnporutilitiesreferences detected in test_linear_combinaisons_yee.py
Verified with:rg -n '\b(np\b|utilities\b)' tests/core/data/gridlayout/test_linear_combinaisons_yee.pyNo matches found—removing the
numpy/utilitiesimports is safe.tests/functional/ionIonBeam/ion_ion_beam1d.py (3)
3-14: Good standardization of imports and test patterns.The reorganization looks clean - moving numpy and matplotlib imports to the top level follows PEP 8 conventions, and the standardization of GUI disabling with
ph.NO_GUI()and MPI rank access throughcpp_lib()aligns well with the test cleanup objectives.
109-110: Approve moving heavy imports inside functions.Moving scipy imports inside functions is a good optimization to avoid import overhead for heavy dependencies unless they're actually needed during execution.
176-176: Approve the MPI rank access standardization.The change from
mpi_rank()tocpp.mpi_rank()properly uses the new cpp instance and aligns with the standardization effort across the test suite.tests/core/data/gridlayout/deriv.py (1)
4-9: Import re-ordering looks good
os,sys, andnumpyare now grouped logically before third-party and local imports, matching Ruff’s style rules. No functional impact observed.tests/core/data/gridlayout/gridIndexing.py (1)
11-16: Clean import grouping LGTMMoving
sys/gridlayoutinto the standard/third-party section removes earlier shadowing and clarifies dependencies. No other issues spotted.tests/core/data/gridlayout/fieldNodeCoordinates.py (1)
5-10: Import cleanup LGTMThe new ordering (
os,sys, third-party, local) adheres to Ruff/PEP-8 and improves readability. All added imports are used later in the file.tests/functional/td/td1d.py (1)
3-8: Import ordering looks good; NO_GUI activated early.Nice consolidation: removing matplotlib imports and calling
ph.NO_GUI()early keeps the test headless-friendly and avoids heavy imports until strictly needed.tests/functional/harris/harris_2d.py (1)
12-15: Headless mode enabled consistently.
ph.NO_GUI()correctly replaces the previous backend tweak and is called before any plotting could occur. 👍tests/functional/translation/translat1d.py (1)
3-10: Good consistency: GUI disabled and heavy matplotlib import removed.This mirrors the pattern across the suite and reduces binary dependencies in test startup time.
tests/simulator/refinement/test_2d_10_core.py (2)
14-14: Import reorganization looks good.The import of
pyphare.core.boxhas been moved to a more logical position in the import sequence. This aligns with the systematic import cleanup across the test suite.
17-17: Excellent standardization of GUI backend handling.Replacing direct matplotlib backend calls with
ph.NO_GUI()centralizes the backend configuration logic. This is functionally equivalent sinceNO_GUI()internally callsmpl.use("Agg")as shown inpyphare/pyphare/pharein/__init__.pylines 60-64, but provides better maintainability.tests/simulator/refinement/test_2d_2_core.py (1)
13-21: Well-organized import cleanup and standardization.The import reorganization improves code structure by:
- Grouping related imports together (
cpp_lib,core.box)- Moving test imports to a logical position
- Standardizing GUI backend handling with
ph.NO_GUI()This aligns perfectly with the systematic cleanup across the test suite.
tests/functional/alfven_wave/alfven_wave1d.py (1)
10-13: Effective import cleanup and GUI standardization.The changes demonstrate good cleanup practices by:
- Retaining only the necessary matplotlib import (
pyplot) that's actually used in themain()function- Adding proper spacing for readability
- Standardizing GUI backend handling with
ph.NO_GUI()This is consistent with the systematic refactoring across the test suite and removes unused imports while preserving functionality.
tests/functional/shock/shock.py (1)
178-178: Good practice: Defer MPI initialization to main execution.Moving
startMPI()into the main execution block prevents MPI initialization on import, improving test isolation and following Python best practices.tests/simulator/test_run.py (1)
249-249: Consistent improvement: MPI initialization deferred to test execution.This change aligns with the broader cleanup effort, ensuring MPI is only initialized when tests are executed directly rather than on import.
tests/simulator/test_initialization.py (1)
8-8: Improved import organization and spacing.The reorganization of
cpp_libimport andcppinstantiation with proper spacing enhances code readability and follows consistent patterns across the test suite.Also applies to: 27-29
tests/simulator/refined_particle_nbr.py (1)
8-8: Consistent import reorganization across test files.The reordering groups standard library imports together and moves project-specific imports and instantiations to a dedicated section, improving code organization.
Also applies to: 12-12, 19-20
tests/simulator/data_wrangler.py (1)
6-8: Completed import reorganization pattern.The final adjustment completes the consistent import organization pattern across test files, grouping standard library imports at the top and moving instantiations after all imports.
Also applies to: 10-10, 14-15
tests/simulator/test_load_balancing.py (3)
22-22: LGTM: Improved spacing after cpp instantiation.The blank line after
cpp = cpp_lib()enhances readability by providing visual separation.
214-217: LGTM: Properly removed unused variable assignment.Removing the
diag_dirvariable assignment is correct since the method expects aRuntimeErrorto be raised and the assignment would never be reached anyway.
285-285: LGTM: Consistent MPI initialization pattern.Moving
startMPI()to theif __name__ == "__main__":block aligns with similar changes across other simulator test files and ensures MPI is only initialized when the script runs directly.tests/simulator/test_diagnostics.py (1)
6-20: LGTM: Excellent import reorganization.The import restructuring follows Python best practices by grouping standard library imports, third-party imports, and project-specific imports. Moving the
cpp = cpp_lib()instantiation to a more logical position after imports improves code organization. This pattern is consistent with similar changes across other test files in the simulator suite.tests/simulator/test_validation.py (3)
7-15: LGTM: Consistent import reorganization.Moving the
cpp_libimport and instantiation follows the same cleanup pattern applied across other simulator test files, improving consistency and code organization.
48-48: LGTM: Removed unused exception variable.Changing
except ValueError as e:toexcept ValueError:correctly removes the unused exception variable, following good Python practices.
389-390: LGTM: Deferred unittest import.Moving the
unittestimport inside theif __name__ == "__main__":block is consistent with the cleanup approach and ensures the import is only loaded when needed.tests/simulator/test_diagnostic_timestamps.py (1)
6-20: LGTM: Consistent import organization pattern.The import reorganization follows the same systematic approach applied across all simulator test files, grouping standard library imports, organizing pyphare-specific imports, and positioning the
cpp = cpp_lib()instantiation logically after imports. This creates a consistent and clean import structure throughout the test suite.tests/simulator/test_restarts.py (3)
1-25: LGTM: Systematic cleanup and import reorganization.The header cleanup, import reorganization, and cpp instantiation positioning follow the same beneficial pattern applied across all simulator test files. These changes improve code organization and consistency without affecting functionality.
387-387: LGTM: Removed unused variable assignments.Removing the unused
modelvariable assignments fromsetup_model()calls is correct since the returned model instances weren't being used. This cleanup aligns with PhilipDeegan's preference for minimal and efficient code.Also applies to: 405-405
424-424: LGTM: Improved exception handling.Changing from a bare
except:toexcept Exception:is a good practice that makes the exception handling more explicit while maintaining the same functional behavior.tests/simulator/test_advance.py (1)
8-10: Minor import redundancyYou import
pyphare.core.boxtwice—once aliased asboxm(line 8) and again asBox(line 11).
That’s perfectly valid, but if the goal is to trim unused imports you might alias one or the other consistently to avoid cognitive overhead.
No action needed if you prefer the current style.tests/simulator/__init__.py (1)
9-9: Caliper removal looks goodConfirming that all Caliper-specific helpers and environment plumbing were removed consistently; this simplifies the test harness without affecting behaviour. Nice cleanup.
nicolasaunai
left a comment
nicolasaunai
left a comment
There was a problem hiding this comment.
add some explainations for the h5 stuff at least
|
todo: move h5py import / explain why h5py should not be imported until later |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/simulator/__init__.py (1)
1-3: Add a meaningful module docstringThe file now starts with two lonely comment markers. Replacing them with a short docstring clarifies purpose and silences doc-lint tools.
-# -# -# +""" +Shared utilities for PHARE simulator integration tests. +"""doc/conventions.md (3)
3-3: Fix heading level to comply with MD001Jumping from
#to###skips a level. Use##instead.-### Reference document for the code base +## Reference document for the code base
31-33: Typo: “depenencies” → “dependencies”-## 2.2 dependencies and imports -Third party depenencies are stated in the file `requirements.txt` in the project root. +## 2.2 Dependencies and imports +Third-party dependencies are stated in the file `requirements.txt` in the project root.
36-36: Remove trailing period in heading (MD026)Headings should not end with punctuation.
-### 2.2.1 Python file import structure. +### 2.2.1 Python file import structure
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
doc/conventions.md(1 hunks)pyphare/pyphare/cpp/__init__.py(2 hunks)pyphare/pyphare/pharesee/hierarchy/fromh5.py(5 hunks)src/python3/CMakeLists.txt(0 hunks)tests/amr/data/field/refine/test_refine_field.py(0 hunks)tests/core/data/gridlayout/allocSizes.py(1 hunks)tests/core/data/gridlayout/deriv.py(1 hunks)tests/core/data/gridlayout/fieldNodeCoordinates.py(1 hunks)tests/core/data/gridlayout/gridIndexing.py(1 hunks)tests/core/data/gridlayout/test_linear_combinaisons_yee.py(1 hunks)tests/diagnostic/__init__.py(3 hunks)tests/diagnostic/job_1d.py.in(1 hunks)tests/diagnostic/job_2d.py.in(1 hunks)tests/functional/alfven_wave/alfven_wave1d.py(1 hunks)tests/functional/harris/harris_2d.py(2 hunks)tests/functional/ionIonBeam/ion_ion_beam1d.py(4 hunks)tests/functional/shock/shock.py(1 hunks)tests/functional/td/td1d.py(1 hunks)tests/functional/tdtagged/td1dtagged.py(0 hunks)tests/functional/translation/translat1d.py(1 hunks)tests/initializer/job.py(2 hunks)tests/simulator/__init__.py(1 hunks)tests/simulator/data_wrangler.py(1 hunks)tests/simulator/refined_particle_nbr.py(1 hunks)tests/simulator/refinement/test_2d_10_core.py(1 hunks)tests/simulator/refinement/test_2d_2_core.py(1 hunks)tests/simulator/test_advance.py(3 hunks)tests/simulator/test_diagnostic_timestamps.py(2 hunks)tests/simulator/test_diagnostics.py(3 hunks)tests/simulator/test_initialization.py(2 hunks)tests/simulator/test_load_balancing.py(7 hunks)tests/simulator/test_restarts.py(5 hunks)tests/simulator/test_run.py(2 hunks)tests/simulator/test_simulation.py(0 hunks)tests/simulator/test_tagging.py.off(2 hunks)tests/simulator/test_validation.py(3 hunks)
💤 Files with no reviewable changes (4)
- tests/simulator/test_simulation.py
- tests/functional/tdtagged/td1dtagged.py
- tests/amr/data/field/refine/test_refine_field.py
- src/python3/CMakeLists.txt
✅ Files skipped from review due to trivial changes (8)
- tests/core/data/gridlayout/test_linear_combinaisons_yee.py
- tests/simulator/test_tagging.py.off
- tests/functional/td/td1d.py
- tests/functional/harris/harris_2d.py
- tests/core/data/gridlayout/allocSizes.py
- tests/simulator/data_wrangler.py
- pyphare/pyphare/pharesee/hierarchy/fromh5.py
- tests/simulator/test_restarts.py
🚧 Files skipped from review as they are similar to previous changes (22)
- tests/functional/shock/shock.py
- tests/core/data/gridlayout/gridIndexing.py
- tests/core/data/gridlayout/deriv.py
- tests/functional/translation/translat1d.py
- tests/simulator/test_run.py
- tests/diagnostic/init.py
- tests/diagnostic/job_1d.py.in
- tests/functional/alfven_wave/alfven_wave1d.py
- tests/initializer/job.py
- tests/simulator/refinement/test_2d_10_core.py
- tests/simulator/test_validation.py
- tests/core/data/gridlayout/fieldNodeCoordinates.py
- tests/simulator/test_diagnostic_timestamps.py
- tests/simulator/refined_particle_nbr.py
- tests/simulator/test_diagnostics.py
- pyphare/pyphare/cpp/init.py
- tests/diagnostic/job_2d.py.in
- tests/simulator/refinement/test_2d_2_core.py
- tests/simulator/test_advance.py
- tests/simulator/test_load_balancing.py
- tests/simulator/test_initialization.py
- tests/functional/ionIonBeam/ion_ion_beam1d.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
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#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.
doc/conventions.md (1)
Learnt from: nicolasaunai
PR: #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.
🪛 markdownlint-cli2 (0.17.2)
doc/conventions.md
3-3: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
36-36: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
🪛 Ruff (0.12.2)
tests/simulator/__init__.py
4-4: os imported but unused
(F401)
⏰ 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). (6)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: Analyze (cpp)
2 participants
closes #1038
ruff check before
after
Deletes cpp_dbg lib as it's redundant