particle count in run #843
Conversation
WalkthroughWalkthroughThe changes primarily involve updates to the PyPHARE library's testing and hierarchy management functionalities. Key modifications include updating import statements, adding new functions for handling HDF5 files and hierarchical data, and refining existing functions to accept additional parameters. New test cases have been introduced to ensure the robustness of the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant TestSuite
participant Hierarchy
participant HDF5
User ->> TestSuite: Run tests
TestSuite ->> Hierarchy: Call hierarchy_from(times)
Hierarchy ->> HDF5: Read data
HDF5 -->> Hierarchy: Return data
Hierarchy -->> TestSuite: Return hierarchical structure
TestSuite -->> User: Display test results
sequenceDiagram
participant User
participant PhareSee
User ->> PhareSee: GetParticleCount(time, **kwargs)
PhareSee ->> Hierarchy: Retrieve particle data
Hierarchy -->> PhareSee: Return particle count
PhareSee -->> User: Display particle count
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
pyphare/pyphare/pharesee/run.py (2)
Line range hint
371-371: Remove unnecessary f-string syntax.- logger.debug(f"No file for function {fn.__name__}") + logger.debug("No file for function " + fn.__name__)Also applies to: 373-373
Line range hint
371-371: Remove unnecessary f-string syntax.- logger.debug(f"No file for function {fn.__name__}") + logger.debug("No file for function " + fn.__name__)Also applies to: 373-373
pyphare/pyphare/pharesee/hierarchy.py (1)
Line range hint
668-668: Comparison toNoneshould useisinstead of==.- if qty == None: + if qty is None:
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- pyphare/pyphare/pharesee/hierarchy.py (5 hunks)
- pyphare/pyphare/pharesee/run.py (1 hunks)
Additional Context Used
Ruff (3)
pyphare/pyphare/pharesee/hierarchy.py (1)
668-668: Comparison to
Noneshould becond is Nonepyphare/pyphare/pharesee/run.py (2)
371-371: f-string without any placeholders
373-373: f-string without any placeholders
Additional comments not posted (1)
pyphare/pyphare/pharesee/hierarchy.py (1)
Line range hint
1429-1558: Thehierarchy_fromh5function has been significantly modified to handle different scenarios such as creating a hierarchy from all times, a single time, loading data into an existing hierarchy, and more. This complex function now handles various edge cases and parameters more robustly. It's crucial to ensure that the function's behavior aligns with the expected use cases and that it handles errors gracefully.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
There was a problem hiding this comment.
Actionable comments posted: 14
Outside diff range and nitpick comments (4)
pyphare/pyphare/pharesee/hierarchy/fromh5.py (1)
173-177: Unused local variables inhierarchy_fromh5.The variables
basename,root_cell_width,interp, anddomain_boxare assigned but never used. Consider removing them if they are not needed.tests/simulator/test_initialization.py (3)
Line range hint
5-24: Reorganize module-level imports to the top of the file.+ from pyphare.cpp import cpp_lib + cpp = cpp_lib() + import unittest + import numpy as np + from ddt import ddt + from pyphare.core.box import nDBox + from pyphare.core.phare_utilities import assert_fp_any_all_close + from pyphare.pharein import ElectronModel, MaxwellianFluidModel + from pyphare.pharein.diagnostics import ( + ElectromagDiagnostics, + FluidDiagnostics, + ParticleDiagnostics, + ) + from pyphare.pharein.simulation import Simulation + from pyphare.pharesee.geometry import level_ghost_boxes + from pyphare.pharesee.hierarchy.hierarchy_utils import merge_particles + from pyphare.pharesee.hierarchy import hierarchy_from + from pyphare.pharesee.particles import aggregate as aggregate_particles + from pyphare.simulator.simulator import Simulator + from tests.simulator import SimulatorTestOrganize all module-level imports at the top of the file to improve readability and maintain the standard Python coding style.
Line range hint
33-33: Replace lambda expressions with function definitions.- _ = lambda i: -((xyz[i] - hL[i]) ** 2) + def compute_square_difference(i, xyz, hL): + return -((xyz[i] - hL[i]) ** 2)Using
deffor function definitions instead oflambdaimproves readability and allows for more complex functionalities.Also applies to: 95-95, 100-100, 105-105, 110-110, 115-115
Line range hint
703-703: Remove unused local variableout.- out = "phare_outputs"The variable
outis assigned but never used, which can lead to confusion and clutter in the codebase.
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (12)
- pyphare/pyphare/pharesee/hierarchy/init.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/fromh5.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/fromsim.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/patch.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/patchdata.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/patchlevel.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy_.py (1 hunks)
- pyphare/pyphare/pharesee/run.py (2 hunks)
- res/cmake/dep/pybind.cmake (1 hunks)
- tests/simulator/test_initialization.py (1 hunks)
Additional Context Used
Ruff (62)
pyphare/pyphare/pharesee/hierarchy/__init__.py (1)
1-1:
.hierarchy.PatchHierarchyimported but unused; consider removing, adding to__all__, or using a redundant aliaspyphare/pyphare/pharesee/hierarchy/fromh5.py (4)
173-173: Local variable
basenameis assigned to but never used
174-174: Local variable
root_cell_widthis assigned to but never used
175-175: Local variable
interpis assigned to but never used
177-177: Local variable
domain_boxis assigned to but never usedpyphare/pyphare/pharesee/hierarchy/fromsim.py (9)
36-36: Undefined name
FieldData
36-36: Undefined name
field_qties
38-38: Undefined name
Patch
69-69: Undefined name
Particles
77-77: Undefined name
ParticleData
80-80: Undefined name
Patch
95-95: Undefined name
Particles
116-116: Undefined name
PatchLevel
118-118: Undefined name
PatchHierarchypyphare/pyphare/pharesee/hierarchy/hierarchy.py (2)
3-3:
.patchdata.PatchDataimported but unused
105-105: Comparison to
Noneshould becond is Nonepyphare/pyphare/pharesee/hierarchy/patch.py (1)
1-1:
.patchdata.PatchDataimported but unusedpyphare/pyphare/pharesee/hierarchy/patchdata.py (13)
20-20: Undefined name
deep_copy
81-81: Undefined name
Box
98-98: Undefined name
np
117-117: Undefined name
np
119-119: Undefined name
np
145-145: Undefined name
boxm
147-147: Undefined name
np
148-148: Undefined name
np
164-164: Undefined name
np
165-165: Undefined name
np
191-191: Undefined name
np
193-193: Undefined name
np
199-199: Undefined name
boxmpyphare/pyphare/pharesee/hierarchy_.py (9)
1-1:
osimported but unused
4-4:
matplotlib.pyplotimported but unused
6-6:
..core.boximported but unused
7-7:
..core.box.Boximported but unused
8-8:
..core.gridlayout.GridLayoutimported but unused
9-9:
..core.phare_utilities.deep_copyimported but unused
9-9:
..core.phare_utilities.refinement_ratioimported but unused
10-10:
.particles.Particlesimported but unused
11-11:
..core.phare_utilities.listifyimported but unusedpyphare/pyphare/pharesee/run.py (2)
370-370: f-string without any placeholders
372-372: f-string without any placeholders
tests/simulator/test_initialization.py (21)
5-5: Module level import not at top of file
7-7: Module level import not at top of file
8-8: Module level import not at top of file
9-9: Module level import not at top of file
10-10: Module level import not at top of file
11-11: Module level import not at top of file
12-16: Module level import not at top of file
17-17: Module level import not at top of file
18-18: Module level import not at top of file
19-19: Module level import not at top of file
20-20: Module level import not at top of file
21-21: Module level import not at top of file
22-22: Module level import not at top of file
24-24: Module level import not at top of file
33-33: Do not assign a
lambdaexpression, use adef
95-95: Do not assign a
lambdaexpression, use adef
100-100: Do not assign a
lambdaexpression, use adef
105-105: Do not assign a
lambdaexpression, use adef
110-110: Do not assign a
lambdaexpression, use adef
115-115: Do not assign a
lambdaexpression, use adef
703-703: Local variable
outis assigned to but never used
GitHub Check Runs (1)
CodeQL failure (27)
pyphare/pyphare/pharesee/hierarchy/fromh5.py: [notice] 173-173: Unused local variable
Variable basename is not used.
pyphare/pyphare/pharesee/hierarchy/fromh5.py: [notice] 174-174: Unused local variable
Variable root_cell_width is not used.
pyphare/pyphare/pharesee/hierarchy/fromh5.py: [notice] 175-175: Unused local variable
Variable interp is not used.
pyphare/pyphare/pharesee/hierarchy/fromh5.py: [notice] 177-177: Unused local variable
Variable domain_box is not used.
pyphare/pyphare/pharesee/hierarchy/fromh5.py: [notice] 180-180: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 492-492: Cyclic import
Import of module pyphare.pharesee.plotting begins an import cycle.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 493-493: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 3-3: Unused import
Import of 'PatchData' is not used.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 480-480: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [warning] 88-88: Comparison of identical values
Comparison of identical values; use cmath.isnan() if testing for not-a-number.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 105-105: Testing equality to None
Testing for None should use the 'is' operator.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [failure] 131-131: Potentially uninitialized local variable
Local variable 'slice_dim' may be used before it is initialized.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [failure] 132-132: Potentially uninitialized local variable
Local variable 'cst_dim' may be used before it is initialized.
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py: [notice] 1-1: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy begins an import cycle.
pyphare/pyphare/pharesee/hierarchy/patch.py: [notice] 1-1: Unused import
Import of 'PatchData' is not used.
pyphare/pyphare/pharesee/hierarchy/patch.py: [notice] 49-49: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
pyphare/pyphare/pharesee/hierarchy/patchdata.py: [warning] 114-114: Overwriting attribute in super-class or sub-class
Assignment overwrites attribute layout, which was previously defined in superclass PatchData.
pyphare/pyphare/pharesee/hierarchy_.py: [notice] 1-1: Unused import
Import of 'os' is not used.
pyphare/pyphare/pharesee/hierarchy_.py: [notice] 4-4: Unused import
Import of 'plt' is not used.
pyphare/pyphare/pharesee/hierarchy_.py: [notice] 6-6: Unused import
Import of 'boxm' is not used.
pyphare/pyphare/pharesee/hierarchy_.py: [notice] 7-7: Unused import
Import of 'Box' is not used.
pyphare/pyphare/pharesee/hierarchy_.py: [notice] 8-8: Unused import
Import of 'GridLayout' is not used.
pyphare/pyphare/pharesee/hierarchy_.py: [notice] 9-9: Unused import
Import of 'deep_copy' is not used.
Import of 'refinement_ratio' is not used.
pyphare/pyphare/pharesee/hierarchy_.py: [notice] 10-10: Unused import
Import of 'Particles' is not used.
pyphare/pyphare/pharesee/hierarchy_.py: [notice] 11-11: Unused import
Import of 'listify' is not used.
pyphare/pyphare/pharesee/run.py: [notice] 4-4: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
pyphare/pyphare/pharesee/run.py: [notice] 5-5: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
Additional comments not posted (15)
pyphare/pyphare/pharesee/hierarchy_.py (1)
14-15: The functionare_adjacentis correctly implemented to check adjacency within a specified tolerance.pyphare/pyphare/pharesee/hierarchy/patchlevel.py (3)
4-6: Initialization ofPatchLevelis correctly implemented.
8-9: The iterator method forPatchLevelis correctly implemented.
11-15: The methodlevel_rangecorrectly calculates the range ofxvalues across patches.res/cmake/dep/pybind.cmake (2)
Line range hint
1-32: Theget_pybindfunction is well-implemented to handle the setup of pybind11 based on the environment and compiler.
34-34: The conditional logic to handle pybind11 packages is correctly implemented.pyphare/pyphare/pharesee/hierarchy/__init__.py (1)
4-30: The functionhierarchy_fromis correctly implemented to handle different scenarios based on input parameters.pyphare/pyphare/pharesee/hierarchy/patch.py (4)
9-32: The constructor of thePatchclass is well-implemented, handling different initialization scenarios based on the presence oflayout.
34-38: The__str__and__repr__methods of thePatchclass are correctly implemented to return a string representation of the object.
40-47: Thecopyand__copy__methods of thePatchclass are correctly implemented to return a deep copy of the object, excludingpatchdatas.datasets.
49-66: The__call__method of thePatchclass is correctly implemented to handle slicing of arrays based on specified coordinates.pyphare/pyphare/pharesee/hierarchy/fromh5.py (2)
71-124: Functionadd_to_patchdataimplementation looks correct.
41-45: Functionmake_layoutimplementation looks correct.pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (2)
16-50: Merge particles logic seems correct but lacks documentation.The function
merge_particlescorrectly merges particle data across different patches and levels. However, it would be beneficial to add more detailed comments explaining the logic, especially how ghost particles are handled and merged. This would improve maintainability and understandability of the code.
75-93: Use oflistifyfunction without validation.The function
compute_hier_fromuseslistifyon the inputhierarchieswithout checking if the input is already a list or validating its contents. This could lead to unexpected behavior ifhierarchiesis not in the expected format. Verify thatlistifyhandles various input formats appropriately, or add input validation before its use.
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range and nitpick comments (10)
tests/simulator/test_diagnostics.py (1)
Line range hint
8-8: Reorganize imports and replace lambda expressions with functions.- from pyphare.pharesee.hierarchy.fromh5 import h5_filename_from, h5_time_grp_key - from pyphare.pharesee.hierarchy import hierarchy_from - from pyphare.simulator.simulator import Simulator + from pyphare.pharesee.hierarchy.fromh5 import h5_filename_from, h5_time_grp_key + from pyphare.pharesee.hierarchy import hierarchy_from + from pyphare.simulator.simulator import Simulator - def by(*xyz): - from pyphare.pharein.global_vars import sim + def by(*xyz, sim): L = sim.simulation_domain() _ = lambda i: 0.1 * np.sin(2 * np.pi * xyz[i] / L[i]) return np.asarray([_(i) for i in range(len(xyz))]).prod(axis=0)Also applies to: 9-9, 11-11, 12-12, 13-13, 14-14, 15-15, 16-16, 17-17, 18-18, 20-20, 31-31, 38-38, 51-51, 58-58
tests/amr/data/field/coarsening/test_coarsen_field.py (1)
Line range hint
9-9: Remove unused import.- import pyphare.core.box as boxmpyphare/pyphare_tests/test_pharesee/__init__.py (1)
Line range hint
4-4: Remove unused imports and variables.- from pyphare.core.box import nDBox - local variable `interp_order` is assigned to but never used - local variable `domain_size` is assigned to but never used - local variable `cell_width` is assigned to but never usedAlso applies to: 257-257, 258-258, 259-259
pyphare/pyphare_tests/test_pharesee/test_geometry.py (3)
Line range hint
13-13: Unused import detected.- from pyphare.core.gridlayout import GridLayout, yee_element_is_primal + from pyphare.core.gridlayout import yee_element_is_primal
Line range hint
120-120: Local variableparticlesis assigned but never used.- particles = "particles"
Line range hint
296-296: Comparison toNoneshould useis.- self.assertTrue(ref_box * cmp_box == None) + self.assertTrue(ref_box * cmp_box is None)pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (3)
Line range hint
10-10: Unused import detected.- from pyphare.pharesee.geometry.toFieldBox import toFieldBox
Line range hint
14-14: Unused imports detected.- from pyphare.core.gridlayout import GridLayout, yee_element_is_primal
Line range hint
168-168: Local variablelvlis assigned but never used.- lvl = hierarchy.level(ilvl)pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
194-195: Commented-out code detected.Consider removing or enabling the commented-out code for clarity and maintainability.
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (10)
- pyphare/pyphare/pharesee/geometry.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/fromsim.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/patchdata.py (1 hunks)
- pyphare/pyphare_tests/test_pharesee/init.py (1 hunks)
- pyphare/pyphare_tests/test_pharesee/test_geometry.py (1 hunks)
- pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (1 hunks)
- tests/amr/data/field/coarsening/test_coarsen_field.py (1 hunks)
- tests/amr/data/field/refine/test_refine_field.py (1 hunks)
- tests/simulator/test_diagnostics.py (1 hunks)
Additional Context Used
Ruff (42)
pyphare/pyphare/pharesee/hierarchy/fromsim.py (9)
36-36: Undefined name
FieldData
36-36: Undefined name
field_qties
38-38: Undefined name
Patch
69-69: Undefined name
Particles
77-77: Undefined name
ParticleData
80-80: Undefined name
Patch
95-95: Undefined name
Particles
116-116: Undefined name
PatchLevel
118-118: Undefined name
PatchHierarchypyphare/pyphare/pharesee/hierarchy/hierarchy.py (2)
3-3:
.patchdata.PatchDataimported but unused
106-106: Comparison to
Noneshould becond is Nonepyphare/pyphare/pharesee/hierarchy/patchdata.py (1)
87-87: Undefined name
Boxpyphare/pyphare_tests/test_pharesee/__init__.py (4)
4-4:
pyphare.core.box.nDBoximported but unused; consider removing, adding to__all__, or using a redundant alias
257-257: Local variable
interp_orderis assigned to but never used
258-258: Local variable
domain_sizeis assigned to but never used
259-259: Local variable
cell_widthis assigned to but never usedpyphare/pyphare_tests/test_pharesee/test_geometry.py (4)
4-4:
pyphare.core.boximported but unused
13-13:
pyphare.core.gridlayout.GridLayoutimported but unused
120-120: Local variable
particlesis assigned to but never used
296-296: Comparison to
Noneshould becond is Nonepyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (5)
4-4:
pyphare.core.boximported but unused
10-10:
pyphare.pharesee.geometry.toFieldBoximported but unused
14-14:
pyphare.core.gridlayout.GridLayoutimported but unused
14-14:
pyphare.core.gridlayout.yee_element_is_primalimported but unused
168-168: Local variable
lvlis assigned to but never usedtests/amr/data/field/coarsening/test_coarsen_field.py (1)
9-9:
pyphare.core.boximported but unusedtests/amr/data/field/refine/test_refine_field.py (1)
9-9: Redefinition of unused
refinement_ratiofrom line 5tests/simulator/test_diagnostics.py (15)
8-8: Module level import not at top of file
9-9: Module level import not at top of file
11-11: Module level import not at top of file
12-12: Module level import not at top of file
13-13: Module level import not at top of file
14-14: Module level import not at top of file
15-15: Module level import not at top of file
16-16: Module level import not at top of file
17-17: Module level import not at top of file
18-18: Module level import not at top of file
20-20: Module level import not at top of file
31-31: Do not assign a
lambdaexpression, use adef
38-38: Do not assign a
lambdaexpression, use adef
51-51: Do not assign a
lambdaexpression, use adef
58-58: Do not assign a
lambdaexpression, use adef
GitHub Check Runs (1)
CodeQL failure (28)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 493-493: Cyclic import
Import of module pyphare.pharesee.plotting begins an import cycle.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 494-494: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 3-3: Unused import
Import of 'PatchData' is not used.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 481-481: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [warning] 89-89: Comparison of identical values
Comparison of identical values; use cmath.isnan() if testing for not-a-number.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 106-106: Testing equality to None
Testing for None should use the 'is' operator.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [failure] 132-132: Potentially uninitialized local variable
Local variable 'slice_dim' may be used before it is initialized.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [failure] 133-133: Potentially uninitialized local variable
Local variable 'cst_dim' may be used before it is initialized.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 194-195: Commented-out code
This comment appears to contain commented-out code.
pyphare/pyphare/pharesee/hierarchy/patchdata.py: [warning] 120-120: Overwriting attribute in super-class or sub-class
Assignment overwrites attribute layout, which was previously defined in superclass PatchData.
Additional comments not posted (22)
pyphare/pyphare/pharesee/hierarchy/patchdata.py (2)
7-27: No issues found inPatchDataclass.
179-213: No issues found inParticleDataclass.tests/amr/data/field/refine/test_refine_field.py (4)
Line range hint
29-34: The functionfine_layout_fromcorrectly computes the fine grid layout. Well implemented.
Line range hint
36-50: The functioncropToFieldDatais well implemented for handling different dimensions. Good use of conditional logic based on the dimensionality of the data.
Line range hint
52-90: The functionrefine_electrichandles the refinement of electric fields with appropriate conditions for different dimensions. The logic is clear and well-commented.
6-6: The import ofFieldDatais added but not used in the visible code. Ensure it's utilized or remove it if unnecessary.Verification successful
The import of
FieldDatais indeed utilized in the filetest_refine_field.py.
FieldDatais used in the functioncropToFieldData.- It is also used in assertions and comments within the file.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of `FieldData` in the file `test_refine_field.py`. # Test: Search for the usage of `FieldData`. Expect: At least one occurrence. rg --type python 'FieldData' tests/amr/data/field/refine/test_refine_field.pyLength of output: 108
Script:
#!/bin/bash # Description: Verify usage of `FieldData` in the file `test_refine_field.py`. # Test: Search for the usage of `FieldData`. Expect: At least one occurrence. rg --type py 'FieldData' tests/amr/data/field/refine/test_refine_field.pyLength of output: 716
pyphare/pyphare/pharesee/geometry.py (12)
Line range hint
12-22: The functiontoFieldBoxcorrectly adjusts the box dimensions based on the centering of the patch data. Well implemented.
Line range hint
24-29: The functionshift_patchis correctly implemented to shift a patch and its associated data. Good handling of both the patch and its data.
Line range hint
31-50: The functiondomain_border_ghost_boxeseffectively calculates ghost boxes along the domain borders for different dimensions. The logic is clear and well-structured.
Line range hint
52-60: The functiontouch_domain_bordercorrectly checks if a box touches a specified domain border. The implementation is straightforward and effective.
Line range hint
62-116: The functionperiodicity_shiftshandles the calculation of periodicity shifts for different dimensions effectively. The logic is comprehensive and well-commented.
Line range hint
118-216: The functioncompute_overlapseffectively computes overlaps between patches in a domain. The implementation is robust, handling various cases and conditions with detailed logic.
Line range hint
218-225: The functionhierarchy_overlapscorrectly calculates overlaps for all levels in a hierarchy. The implementation is concise and effective.
Line range hint
227-297: The functionget_periodic_listeffectively sorts patches based on their position relative to the domain borders. The logic is comprehensive and well-commented.
Line range hint
299-338: The functionghost_area_boxeseffectively calculates ghost cell boxes for all levels. The implementation is robust, handling various cases and conditions with detailed logic.
Line range hint
340-404: The functionlevel_ghost_boxeseffectively calculates level ghost cell boxes for all levels. The implementation is robust, handling various cases and conditions with detailed logic.
6-6: The import ofis_root_lvlis added but not used in the visible code. Ensure it's utilized or remove it if unnecessary.Verification successful
The import of
is_root_lvlis indeed used in the filegeometry.py.
is_root_lvlis used in the following line:
if is_root_lvl(lvl): # level ghost do not make sense for periodic root levelScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of `is_root_lvl` in the file `geometry.py`. # Test: Search for the usage of `is_root_lvl`. Expect: At least one occurrence. rg --type python 'is_root_lvl' pyphare/pyphare/pharesee/geometry.pyLength of output: 98
Script:
#!/bin/bash # Description: Verify usage of `is_root_lvl` in the file `geometry.py`. # Test: Search for the usage of `is_root_lvl`. Expect: At least one occurrence. rg 'is_root_lvl' pyphare/pyphare/pharesee/geometry.pyLength of output: 190
5-5: The import ofFieldDatais added but not used in the visible code. Ensure it's utilized or remove it if unnecessary.Verification successful
The import of
FieldDatais indeed used in the filegeometry.py.
- Line 5:
from .hierarchy.patchdata import FieldData- Line 7:
assert isinstance(patch_data, FieldData)Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of `FieldData` in the file `geometry.py`. # Test: Search for the usage of `FieldData`. Expect: At least one occurrence. rg --type python 'FieldData' pyphare/pyphare/pharesee/geometry.pyLength of output: 96
Script:
#!/bin/bash # Description: Verify usage of `FieldData` in the file `geometry.py`. # Test: Search for the usage of `FieldData`. Expect: At least one occurrence. rg --type py 'FieldData' pyphare/pyphare/pharesee/geometry.pyLength of output: 149
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (4)
3-3: Unused import detected.
106-106: Comparison toNoneshould useis.
481-481: Mixed implicit and explicit returns.
132-133: Uninitialized local variables used.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (6)
tests/simulator/test_initialization.py (3)
Line range hint
5-24: Consider grouping all module-level imports at the top of the file for better readability and adherence to PEP 8 guidelines.+ from pyphare.cpp import cpp_lib + cpp = cpp_lib() + import unittest + import numpy as np + from ddt import ddt + from pyphare.core.box import nDBox + from pyphare.core.phare_utilities import assert_fp_any_all_close + from pyphare.pharein import ElectronModel, MaxwellianFluidModel + from pyphare.pharein.diagnostics import ElectromagDiagnostics, FluidDiagnostics, ParticleDiagnostics + from pyphare.pharein.simulation import Simulation + from pyphare.pharesee.geometry import level_ghost_boxes + from pyphare.pharesee.hierarchy.hierarchy_utils import merge_particles + from pyphare.pharesee.hierarchy import hierarchy_from + from pyphare.pharesee.particles import aggregate as aggregate_particles + from pyphare.simulator.simulator import Simulator + from tests.simulator import SimulatorTest
Line range hint
33-115: Replace lambda expressions with def for better readability and potential reuse of functions.- _ = lambda i: -((xyz[i] - hL[i]) ** 2) + def compute_value(i, xyz, hL): + return -((xyz[i] - hL[i]) ** 2)Apply this change to all similar lambda expressions in the file.
Line range hint
703-703: The local variableoutis assigned but never used. Consider removing it if it's not needed.- out = "phare_outputs"tests/simulator/test_advance.py (3)
Line range hint
5-25: Consider grouping all module-level imports at the top of the file for better readability and adherence to PEP 8 guidelines.+ from pyphare.cpp import cpp_lib + cpp = cpp_lib() + import unittest + import numpy as np + import pyphare.core.box as boxm + from ddt import ddt + from pyphare.core.box import Box + from pyphare.core.phare_utilities import assert_fp_any_all_close, np_array_ify + from pyphare.pharein import ElectronModel, MaxwellianFluidModel + from pyphare.pharein.diagnostics import ElectromagDiagnostics, FluidDiagnostics, ParticleDiagnostics + from pyphare.pharein.simulation import Simulation + from pyphare.pharesee.geometry import hierarchy_overlaps, level_ghost_boxes + from pyphare.pharesee.hierarchy import hierarchy_from + from pyphare.pharesee.hierarchy.hierarchy_utils import merge_particles + from pyphare.simulator.simulator import Simulator + from tests.diagnostic import all_timestamps + from tests.simulator import SimulatorTest, diff_boxes
Line range hint
34-113: Replace lambda expressions with def for better readability and potential reuse of functions.- _ = lambda i: -((xyz[i] - hL[i]) ** 2) + def compute_value(i, xyz, hL): + return -((xyz[i] - hL[i]) ** 2)Apply this change to all similar lambda expressions in the file.
Line range hint
731-732: Remove unused f-string placeholders and local variable assignments.- f-string without any placeholders - `pyphare.pharein` imported but unused - Local variable `out` is assigned to but never usedAlso applies to: 760-765
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- pyphare/pyphare/pharesee/hierarchy/patchdata.py (1 hunks)
- tests/simulator/test_advance.py (1 hunks)
- tests/simulator/test_initialization.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- pyphare/pyphare/pharesee/hierarchy/patchdata.py
Additional Context Used
Ruff (46)
tests/simulator/test_advance.py (25)
5-5: Module level import not at top of file
7-7: Module level import not at top of file
8-8: Module level import not at top of file
9-9: Module level import not at top of file
10-10: Module level import not at top of file
11-11: Module level import not at top of file
12-12: Module level import not at top of file
13-17: Module level import not at top of file
18-18: Module level import not at top of file
19-19: Module level import not at top of file
20-20: Module level import not at top of file
21-21: Module level import not at top of file
22-22: Module level import not at top of file
24-24: Module level import not at top of file
25-25: Module level import not at top of file
34-34: Do not assign a
lambdaexpression, use adef
93-93: Do not assign a
lambdaexpression, use adef
98-98: Do not assign a
lambdaexpression, use adef
103-103: Do not assign a
lambdaexpression, use adef
108-108: Do not assign a
lambdaexpression, use adef
113-113: Do not assign a
lambdaexpression, use adef
731-731: f-string without any placeholders
732-732: f-string without any placeholders
760-760:
pyphare.phareinimported but unused
765-765: Local variable
outis assigned to but never usedtests/simulator/test_initialization.py (21)
5-5: Module level import not at top of file
7-7: Module level import not at top of file
8-8: Module level import not at top of file
9-9: Module level import not at top of file
10-10: Module level import not at top of file
11-11: Module level import not at top of file
12-16: Module level import not at top of file
17-17: Module level import not at top of file
18-18: Module level import not at top of file
19-19: Module level import not at top of file
20-20: Module level import not at top of file
21-21: Module level import not at top of file
22-22: Module level import not at top of file
24-24: Module level import not at top of file
33-33: Do not assign a
lambdaexpression, use adef
95-95: Do not assign a
lambdaexpression, use adef
100-100: Do not assign a
lambdaexpression, use adef
105-105: Do not assign a
lambdaexpression, use adef
110-110: Do not assign a
lambdaexpression, use adef
115-115: Do not assign a
lambdaexpression, use adef
703-703: Local variable
outis assigned to but never used
Additional comments not posted (2)
tests/simulator/test_initialization.py (1)
19-20: Reorganized imports to reflect the new location ofmerge_particles.tests/simulator/test_advance.py (1)
20-21: Reorganized imports to reflect the new location ofmerge_particles.
There was a problem hiding this comment.
Actionable comments posted: 10
Outside diff range and nitpick comments (1)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
495-495: Module 'copy' is imported with both 'import' and 'import from'.Consider using a consistent import style for the 'copy' module to improve code readability and maintainability.
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- pyphare/pyphare/pharesee/hierarchy/init.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/fromsim.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1 hunks)
- pyphare/pyphare/pharesee/plotting.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- pyphare/pyphare/pharesee/hierarchy/fromsim.py
Additional Context Used
Ruff (13)
pyphare/pyphare/pharesee/hierarchy/__init__.py (5)
1-1:
.hierarchy.PatchHierarchyimported but unused; consider removing, adding to__all__, or using a redundant alias
2-2:
.fromh5.hierarchy_fromh5imported but unused; consider removing, adding to__all__, or using a redundant alias
3-3:
.fromsim.hierarchy_from_simimported but unused; consider removing, adding to__all__, or using a redundant alias
9-9: Redefinition of unused
hierarchy_fromh5from line 2
10-10: Redefinition of unused
hierarchy_from_simfrom line 3pyphare/pyphare/pharesee/hierarchy/hierarchy.py (4)
3-3:
.patchdata.PatchDataimported but unused
105-105: Comparison to
Noneshould becond is None
589-589: Undefined name
refinement_ratio
648-648: Undefined name
refinement_ratiopyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1)
7-7:
...core.boximported but unusedpyphare/pyphare/pharesee/plotting.py (3)
250-250:
pyphare.core.gridlayoutimported but unused
257-257: Local variable
domainis assigned to but never used
329-329: Local variable
cbis assigned to but never used
GitHub Check Runs (1)
CodeQL neutral (22)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 3-3: Unused import
Import of 'PatchData' is not used.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 485-485: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [warning] 88-88: Comparison of identical values
Comparison of identical values; use cmath.isnan() if testing for not-a-number.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 105-105: Testing equality to None
Testing for None should use the 'is' operator.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [failure] 131-131: Potentially uninitialized local variable
Local variable 'slice_dim' may be used before it is initialized.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [failure] 132-132: Potentially uninitialized local variable
Local variable 'cst_dim' may be used before it is initialized.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 497-497: Cyclic import
Import of module pyphare.pharesee.plotting begins an import cycle.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 495-495: Module is imported with 'import' and 'import from'
Module 'copy' is imported with both 'import' and 'import from'.
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py: [notice] 1-1: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy begins an import cycle.
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py: [notice] 7-7: Unused import
Import of 'boxm' is not used.
pyphare/pyphare/pharesee/plotting.py: [notice] 247-247: Cyclic import
Import of module pyphare.pharesee.hierarchy.fromh5 begins an import cycle.
Additional comments not posted (2)
pyphare/pyphare/pharesee/hierarchy/__init__.py (1)
6-32: The functionhierarchy_fromis well-structured with clear error handling for conflicting or missing parameters.pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1)
36-402: The utility functions inhierarchy_utils.pyare well-designed and cater to various aspects of hierarchy and patch data manipulation effectively.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- pyphare/pyphare/pharesee/hierarchy/fromsim.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- pyphare/pyphare/pharesee/hierarchy/fromsim.py
Additional Context Used
Ruff (1)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
105-105: Comparison to
Noneshould becond is None
Additional comments not posted (1)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
105-105: Useisfor comparison toNone.- if qty == None: + if qty is None:Likely invalid or redundant comment.
nicolasaunai
force-pushed
the
h5pcount
branch
2 times, most recently
from
8687bbc
to
1b8e1ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 22
Outside diff range and nitpick comments (14)
tests/simulator/test_diagnostics.py (1)
Line range hint
31-58: Replace lambda expressions with function definitions.def density(xyz): return 1.0 def by(xyz): from pyphare.pharein.global_vars import sim L = sim.simulation_domain() return np.asarray([0.1 * np.sin(2 * np.pi * xyz[i] / L[i]) for i in range(len(xyz))]).prod(axis=0) def bz(xyz): from pyphare.pharein.global_vars import sim L = sim.simulation_domain() return np.asarray([0.1 * np.sin(2 * np.pi * xyz[i] / L[i]) for i in range(len(xyz))]).prod(axis=0) def vx(xyz): return 0.0 def vy(xyz): from pyphare.pharein.global_vars import sim L = sim.simulation_domain() return np.asarray([0.1 * np.cos(2 * np.pi * xyz[i] / L[i]) for i in range(len(xyz))]).prod(axis=0) def vz(xyz): from pyphare.pharein.global_vars import sim L = sim.simulation_domain() return np.asarray([0.1 * np.cos(2 * np.pi * xyz[i] / L[i]) for i in range(len(xyz))]).prod(axis=0) def vthx(xyz): return 0.01 def vthy(xyz): return 0.01 def vthz(xyz): return 0.01Replacing lambda expressions with named function definitions improves readability and maintainability of the code.
tests/amr/data/field/coarsening/test_coarsen_field.py (1)
Line range hint
1-1: Remove unused import.- import pyphare.core.box as boxmThe
boxmimport is unused in this file and should be removed to clean up the code.pyphare/pyphare_tests/test_pharesee/__init__.py (2)
Line range hint
4-4: Remove unused import.- from pyphare.core.box import nDBoxThe
nDBoximport is unused in this file and should be removed to clean up the code.
Line range hint
257-259: Remove unused local variables.- interp_order = kwargs["interp_order"] - domain_size = kwargs["domain_size"] - cell_width = kwargs["cell_width"]These variables are assigned but never used within the function. Removing them can help to clean up the code and avoid confusion.
pyphare/pyphare/pharesee/plotting.py (2)
Line range hint
257-257: Remove unused local variable to clean up the code.- domain = r.GetDomainSize()
Line range hint
329-329: Remove unused local variable to clean up the code.- cb = plt.colorbar(im, ax=ax, cax=cax)pyphare/pyphare_tests/test_pharesee/test_geometry.py (2)
Line range hint
119-119: Remove unused local variable to clean up the code.- particles = "particles"
Line range hint
295-295: Useis Nonefor None comparison to follow Python best practices.- if time != None: + if time is not None:pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (2)
Line range hint
12-12: Remove unused imports.- from pyphare.core.gridlayout import GridLayout, yee_element_is_primalAlso applies to: 12-12
Line range hint
166-166: Remove unused local variablelvl.- lvl = hierarchy.level(ilvl)tests/simulator/test_advance.py (4)
Line range hint
5-25: Reorganize imports to follow PEP 8 guidelines.- from pyphare.cpp import cpp_lib - cpp = cpp_lib() - import unittest - import numpy as np - import pyphare.core.box as boxm - from ddt import ddt - from pyphare.core.box import Box - from pyphare.core.phare_utilities import assert_fp_any_all_close, np_array_ify - from pyphare.pharein import ElectronModel, MaxwellianFluidModel - from pyphare.pharein.diagnostics import ( - ElectromagDiagnostics, - FluidDiagnostics, - ParticleDiagnostics, - ) - from pyphare.pharein.simulation import Simulation - from pyphare.pharesee.geometry import hierarchy_overlaps, level_ghost_boxes - from pyphare.pharesee.hierarchy import hierarchy_from - from pyphare.pharesee.hierarchy.hierarchy_utils import merge_particles - from pyphare.simulator.simulator import Simulator - from tests.diagnostic import all_timestamps - from tests.simulator import SimulatorTest, diff_boxes + import unittest + from ddt import ddt + import numpy as np + from pyphare.cpp import cpp_lib + from pyphare.core.box import Box + import pyphare.core.box as boxm + from pyphare.core.phare_utilities import assert_fp_any_all_close, np_array_ify + from pyphare.pharein import ElectronModel, MaxwellianFluidModel + from pyphare.pharein.diagnostics import ElectromagDiagnostics, FluidDiagnostics, ParticleDiagnostics + from pyphare.pharein.simulation import Simulation + from pyphare.pharesee.geometry import hierarchy_overlaps, level_ghost_boxes + from pyphare.pharesee.hierarchy import hierarchy_from + from pyphare.pharesee.hierarchy.hierarchy_utils import merge_particles + from pyphare.simulator.simulator import Simulator + from tests.diagnostic import all_timestamps + from tests.simulator import SimulatorTest, diff_boxes + cpp = cpp_lib()
Line range hint
34-34: Replace lambda expressions with function definitions for clarity and maintainability.- _ = lambda i: -((xyz[i] - hL[i]) ** 2) + def calculate_negative_square_difference(i, xyz, hL): + return -((xyz[i] - hL[i]) ** 2)Apply similar changes for all lambda expressions in the file.
Also applies to: 93-93, 98-98, 103-103, 108-108, 113-113
Line range hint
731-732: Remove unnecessary f-strings or add placeholders if needed.- print("test_field_coarsening_via_subcycles for dim/interp : {}/{}".format(ndim, interp_order)) + print(f"test_field_coarsening_via_subcycles for dim/interp : {ndim}/{interp_order}")
Line range hint
760-765: Remove unused imports and variables.- import pyphare.pharein as ph - out = "domain_particles"
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (19)
- pyphare/pyphare/pharesee/geometry.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/init.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/fromh5.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/fromsim.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/patch.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/patchdata.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/patchlevel.py (1 hunks)
- pyphare/pyphare/pharesee/plotting.py (1 hunks)
- pyphare/pyphare/pharesee/run.py (2 hunks)
- pyphare/pyphare_tests/test_pharesee/init.py (1 hunks)
- pyphare/pyphare_tests/test_pharesee/test_geometry.py (1 hunks)
- pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (1 hunks)
- tests/amr/data/field/coarsening/test_coarsen_field.py (1 hunks)
- tests/amr/data/field/refine/test_refine_field.py (1 hunks)
- tests/simulator/test_advance.py (1 hunks)
- tests/simulator/test_diagnostics.py (1 hunks)
- tests/simulator/test_initialization.py (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- pyphare/pyphare/pharesee/geometry.py
- pyphare/pyphare/pharesee/hierarchy/fromsim.py
- pyphare/pyphare/pharesee/hierarchy/patchlevel.py
Additional Context Used
Ruff (89)
pyphare/pyphare/pharesee/hierarchy/__init__.py (5)
1-1:
.hierarchy.PatchHierarchyimported but unused; consider removing, adding to__all__, or using a redundant alias
2-2:
.fromh5.hierarchy_fromh5imported but unused; consider removing, adding to__all__, or using a redundant alias
3-3:
.fromsim.hierarchy_from_simimported but unused; consider removing, adding to__all__, or using a redundant alias
9-9: Redefinition of unused
hierarchy_fromh5from line 2
10-10: Redefinition of unused
hierarchy_from_simfrom line 3pyphare/pyphare/pharesee/hierarchy/fromh5.py (4)
150-150: Local variable
basenameis assigned to but never used
151-151: Local variable
root_cell_widthis assigned to but never used
152-152: Local variable
interpis assigned to but never used
154-154: Local variable
domain_boxis assigned to but never usedpyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
105-105: Comparison to
Noneshould becond is Nonepyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1)
7-7:
...core.boximported but unusedpyphare/pyphare/pharesee/hierarchy/patch.py (1)
1-1:
.patchdata.PatchDataimported but unusedpyphare/pyphare/pharesee/plotting.py (3)
250-250:
pyphare.core.gridlayoutimported but unused
257-257: Local variable
domainis assigned to but never used
329-329: Local variable
cbis assigned to but never usedpyphare/pyphare/pharesee/run.py (2)
370-370: f-string without any placeholders
372-372: f-string without any placeholders
pyphare/pyphare_tests/test_pharesee/__init__.py (4)
4-4:
pyphare.core.box.nDBoximported but unused; consider removing, adding to__all__, or using a redundant alias
257-257: Local variable
interp_orderis assigned to but never used
258-258: Local variable
domain_sizeis assigned to but never used
259-259: Local variable
cell_widthis assigned to but never usedpyphare/pyphare_tests/test_pharesee/test_geometry.py (2)
119-119: Local variable
particlesis assigned to but never used
295-295: Comparison to
Noneshould becond is Nonepyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (3)
12-12:
pyphare.core.gridlayout.GridLayoutimported but unused
12-12:
pyphare.core.gridlayout.yee_element_is_primalimported but unused
166-166: Local variable
lvlis assigned to but never usedtests/amr/data/field/coarsening/test_coarsen_field.py (1)
9-9:
pyphare.core.boximported but unusedtests/amr/data/field/refine/test_refine_field.py (1)
9-9: Redefinition of unused
refinement_ratiofrom line 5tests/simulator/test_advance.py (25)
5-5: Module level import not at top of file
7-7: Module level import not at top of file
8-8: Module level import not at top of file
9-9: Module level import not at top of file
10-10: Module level import not at top of file
11-11: Module level import not at top of file
12-12: Module level import not at top of file
13-17: Module level import not at top of file
18-18: Module level import not at top of file
19-19: Module level import not at top of file
20-20: Module level import not at top of file
21-21: Module level import not at top of file
22-22: Module level import not at top of file
24-24: Module level import not at top of file
25-25: Module level import not at top of file
34-34: Do not assign a
lambdaexpression, use adef
93-93: Do not assign a
lambdaexpression, use adef
98-98: Do not assign a
lambdaexpression, use adef
103-103: Do not assign a
lambdaexpression, use adef
108-108: Do not assign a
lambdaexpression, use adef
113-113: Do not assign a
lambdaexpression, use adef
731-731: f-string without any placeholders
732-732: f-string without any placeholders
760-760:
pyphare.phareinimported but unused
765-765: Local variable
outis assigned to but never usedtests/simulator/test_diagnostics.py (15)
8-8: Module level import not at top of file
9-9: Module level import not at top of file
11-11: Module level import not at top of file
12-12: Module level import not at top of file
13-13: Module level import not at top of file
14-14: Module level import not at top of file
15-15: Module level import not at top of file
16-16: Module level import not at top of file
17-17: Module level import not at top of file
18-18: Module level import not at top of file
20-20: Module level import not at top of file
31-31: Do not assign a
lambdaexpression, use adef
38-38: Do not assign a
lambdaexpression, use adef
51-51: Do not assign a
lambdaexpression, use adef
58-58: Do not assign a
lambdaexpression, use adeftests/simulator/test_initialization.py (21)
5-5: Module level import not at top of file
7-7: Module level import not at top of file
8-8: Module level import not at top of file
9-9: Module level import not at top of file
10-10: Module level import not at top of file
11-11: Module level import not at top of file
12-16: Module level import not at top of file
17-17: Module level import not at top of file
18-18: Module level import not at top of file
19-19: Module level import not at top of file
20-20: Module level import not at top of file
21-21: Module level import not at top of file
22-22: Module level import not at top of file
24-24: Module level import not at top of file
33-33: Do not assign a
lambdaexpression, use adef
95-95: Do not assign a
lambdaexpression, use adef
100-100: Do not assign a
lambdaexpression, use adef
105-105: Do not assign a
lambdaexpression, use adef
110-110: Do not assign a
lambdaexpression, use adef
115-115: Do not assign a
lambdaexpression, use adef
703-703: Local variable
outis assigned to but never used
GitHub Check Runs (1)
CodeQL failure (22)
pyphare/pyphare/pharesee/hierarchy/fromh5.py: [notice] 150-150: Unused local variable
Variable basename is not used.
pyphare/pyphare/pharesee/hierarchy/fromh5.py: [notice] 151-151: Unused local variable
Variable root_cell_width is not used.
pyphare/pyphare/pharesee/hierarchy/fromh5.py: [notice] 152-152: Unused local variable
Variable interp is not used.
pyphare/pyphare/pharesee/hierarchy/fromh5.py: [notice] 154-154: Unused local variable
Variable domain_box is not used.
pyphare/pyphare/pharesee/hierarchy/fromh5.py: [notice] 157-157: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
pyphare/pyphare/pharesee/hierarchy/fromh5.py: [notice] 8-8: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy begins an import cycle.
pyphare/pyphare/pharesee/hierarchy/fromh5.py: [notice] 12-12: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 485-485: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [warning] 88-88: Comparison of identical values
Comparison of identical values; use cmath.isnan() if testing for not-a-number.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 105-105: Testing equality to None
Testing for None should use the 'is' operator.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [failure] 131-131: Potentially uninitialized local variable
Local variable 'slice_dim' may be used before it is initialized.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [failure] 132-132: Potentially uninitialized local variable
Local variable 'cst_dim' may be used before it is initialized.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 497-497: Cyclic import
Import of module pyphare.pharesee.plotting begins an import cycle.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 495-495: Module is imported with 'import' and 'import from'
Module 'copy' is imported with both 'import' and 'import from'.
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py: [notice] 1-1: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy begins an import cycle.
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py: [notice] 7-7: Unused import
Import of 'boxm' is not used.
pyphare/pyphare/pharesee/hierarchy/patch.py: [notice] 1-1: Unused import
Import of 'PatchData' is not used.
pyphare/pyphare/pharesee/hierarchy/patch.py: [notice] 49-49: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
pyphare/pyphare/pharesee/hierarchy/patchdata.py: [warning] 121-121: Overwriting attribute in super-class or sub-class
Assignment overwrites attribute layout, which was previously defined in superclass PatchData.
pyphare/pyphare/pharesee/plotting.py: [notice] 247-247: Cyclic import
Import of module pyphare.pharesee.hierarchy.fromh5 begins an import cycle.
pyphare/pyphare/pharesee/run.py: [notice] 4-4: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
pyphare/pyphare/pharesee/run.py: [notice] 5-5: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
Additional comments not posted (5)
tests/amr/data/field/refine/test_refine_field.py (1)
6-6: The import ofFieldDatais correctly updated to reflect the new module locations.pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
497-497: Cyclic import detected.
Consider refactoring the import structure to avoid cyclic dependencies, which can lead to maintenance issues and unexpected behavior.tests/simulator/test_initialization.py (3)
19-20: The imports formerge_particlesandhierarchy_fromhave been added. Ensure that these functions are used appropriately in the file and that their introduction does not introduce any side effects or dependencies that are not handled.
19-20: The usage ofmerge_particlesandhierarchy_fromwithin thegetHierarchyfunction is appropriate and aligns with the intended functionality of handling different quantities in the simulation hierarchy.
722-726: The logic to check the number of patch ghost data on level 1 usingnbrPatchGhostPatchDatasOnL1is correctly implemented. The use ofhierarchy_fromto retrieve the hierarchy data based on diagnostic outputs is appropriate.
[APROVED]
There was a problem hiding this comment.
Actionable comments posted: 14
Outside diff range and nitpick comments (17)
tests/amr/data/field/coarsening/test_coarsen_field.py (1)
Line range hint
9-9: Remove unused import to clean up the code.- import pyphare.core.box as boxmpyphare/pyphare_tests/test_pharesee/__init__.py (2)
Line range hint
4-4: Remove unused import to clean up the code.- from pyphare.core.box.nDBox import nDBox
Line range hint
257-259: Remove unused local variables to clean up the code.- interp_order = kwargs["interp_order"] - domain_size = kwargs["domain_size"] - cell_width = kwargs["cell_width"]These variables are assigned but never used, which could lead to confusion and clutter in the code.
pyphare/pyphare/pharesee/plotting.py (2)
Line range hint
258-258: Local variabledomainis assigned to but never used.Consider removing the
domainvariable if it's not being used, to clean up the code.
Line range hint
330-330: Local variablecbis assigned to but never used.The variable
cbis assigned but never used. If it's not needed, consider removing it to clean up the code.pyphare/pyphare_tests/test_pharesee/test_geometry.py (2)
Line range hint
119-119: Local variableparticlesis assigned to but never used.The variable
particlesis assigned but never used. If it's not needed, consider removing it to clean up the code.
Line range hint
295-295: Comparison toNoneshould becond is None.Use
is Nonefor comparisons toNoneto ensure that the comparison is done using identity rather than equality.pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (2)
Line range hint
12-12: Remove unused imports to clean up the code.- from pyphare.core.gridlayout import GridLayout, yee_element_is_primal
Line range hint
166-166: Remove unused local variable to clean up the code.- lvl = hierarchy.level(ilvl)pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
495-495: Module 'copy' is imported with both 'import' and 'import from'.Consider using a consistent import style for the 'copy' module to improve code readability and maintainability.
tests/simulator/test_initialization.py (3)
Line range hint
5-24: Reorganize module-level imports to the top of the file.+ from pyphare.cpp import cpp_lib + cpp = cpp_lib() + import unittest + import numpy as np + from ddt import ddt + from pyphare.core.box import nDBox + from pyphare.core.phare_utilities import assert_fp_any_all_close + from pyphare.pharein import ElectronModel, MaxwellianFluidModel + from pyphare.pharein.diagnostics import ( + ElectromagDiagnostics, + FluidDiagnostics, + ParticleDiagnostics, + ) + from pyphare.pharein.simulation import Simulation + from pyphare.pharesee.geometry import level_ghost_boxes + from pyphare.pharesee.hierarchy.hierarchy_utils import merge_particles + from pyphare.pharesee.hierarchy import hierarchy_from + from pyphare.pharesee.particles import aggregate as aggregate_particles + from pyphare.simulator.simulator import Simulator + from tests.simulator import SimulatorTest
Line range hint
33-33: Replace lambda expressions with function definitions for clarity and maintainability.- _ = lambda i: -((xyz[i] - hL[i]) ** 2) + def compute_square_difference(i, xyz, hL): + return -((xyz[i] - hL[i]) ** 2)Apply similar changes to other lambda expressions in the file.
Also applies to: 95-95, 100-100, 105-105, 110-110, 115-115
Line range hint
703-703: Remove unused local variableout.- out = "phare_outputs"tests/simulator/test_advance.py (4)
Line range hint
5-25: Reorder imports to the top of the file.+ from pyphare.cpp import cpp_lib + cpp = cpp_lib() + import unittest + import numpy as np + import pyphare.core.box as boxm + from ddt import ddt + from pyphare.core.box import Box + from pyphare.core.phare_utilities import assert_fp_any_all_close, np_array_ify + from pyphare.pharein import ElectronModel, MaxwellianFluidModel + from pyphare.pharein.diagnostics import ( + ElectromagDiagnostics, + FluidDiagnostics, + ParticleDiagnostics, + ) + from pyphare.pharein.simulation import Simulation + from pyphare.pharesee.geometry import hierarchy_overlaps, level_ghost_boxes + from pyphare.pharesee.hierarchy import hierarchy_from + from pyphare.pharesee.hierarchy.hierarchy_utils import merge_particles + from pyphare.simulator.simulator import Simulator + from tests.diagnostic import all_timestamps + from tests.simulator import SimulatorTest, diff_boxesMove all imports to the top of the file to comply with Python best practices and improve code readability.
Line range hint
34-34: Replace lambda expressions with function definitions.- _ = lambda i: -((xyz[i] - hL[i]) ** 2) + def square_difference(i, xyz, hL): + return -((xyz[i] - hL[i]) ** 2)Convert lambda expressions to function definitions for better readability and potential reuse.
Also applies to: 93-93, 98-98, 103-103, 108-108, 113-113
Line range hint
765-765: Remove unused variableout.- out = "domain_particles"The variable
outis assigned but never used, which could lead to confusion and clutter in the codebase.
Line range hint
731-732: Replace f-strings with regular strings.- print(f"FAIL level ghost subcycle_coarsening qty {qty}") + print("FAIL level ghost subcycle_coarsening qty")The f-strings are used without placeholders, which is unnecessary. Replace them with regular strings for clarity.
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (19)
- pyphare/pyphare/pharesee/geometry.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/init.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/fromh5.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/fromsim.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/patch.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/patchdata.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/patchlevel.py (1 hunks)
- pyphare/pyphare/pharesee/plotting.py (1 hunks)
- pyphare/pyphare/pharesee/run.py (2 hunks)
- pyphare/pyphare_tests/test_pharesee/init.py (1 hunks)
- pyphare/pyphare_tests/test_pharesee/test_geometry.py (1 hunks)
- pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (1 hunks)
- tests/amr/data/field/coarsening/test_coarsen_field.py (1 hunks)
- tests/amr/data/field/refine/test_refine_field.py (1 hunks)
- tests/simulator/test_advance.py (1 hunks)
- tests/simulator/test_diagnostics.py (1 hunks)
- tests/simulator/test_initialization.py (2 hunks)
Files not reviewed due to errors (1)
- pyphare/pyphare/pharesee/run.py (no review received)
Files skipped from review as they are similar to previous changes (3)
- pyphare/pyphare/pharesee/geometry.py
- pyphare/pyphare/pharesee/hierarchy/fromsim.py
- pyphare/pyphare/pharesee/hierarchy/patchlevel.py
Additional Context Used
Ruff (88)
pyphare/pyphare/pharesee/hierarchy/__init__.py (5)
1-1:
.hierarchy.PatchHierarchyimported but unused; consider removing, adding to__all__, or using a redundant alias
2-2:
.fromh5.hierarchy_fromh5imported but unused; consider removing, adding to__all__, or using a redundant alias
3-3:
.fromsim.hierarchy_from_simimported but unused; consider removing, adding to__all__, or using a redundant alias
9-9: Redefinition of unused
hierarchy_fromh5from line 2
10-10: Redefinition of unused
hierarchy_from_simfrom line 3pyphare/pyphare/pharesee/hierarchy/fromh5.py (4)
150-150: Local variable
basenameis assigned to but never used
151-151: Local variable
root_cell_widthis assigned to but never used
152-152: Local variable
interpis assigned to but never used
154-154: Local variable
domain_boxis assigned to but never usedpyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1)
7-7:
...core.boximported but unusedpyphare/pyphare/pharesee/hierarchy/patch.py (1)
1-1:
.patchdata.PatchDataimported but unusedpyphare/pyphare/pharesee/plotting.py (3)
251-251:
pyphare.core.gridlayoutimported but unused
258-258: Local variable
domainis assigned to but never used
330-330: Local variable
cbis assigned to but never usedpyphare/pyphare/pharesee/run.py (2)
370-370: f-string without any placeholders
372-372: f-string without any placeholders
pyphare/pyphare_tests/test_pharesee/__init__.py (4)
4-4:
pyphare.core.box.nDBoximported but unused; consider removing, adding to__all__, or using a redundant alias
257-257: Local variable
interp_orderis assigned to but never used
258-258: Local variable
domain_sizeis assigned to but never used
259-259: Local variable
cell_widthis assigned to but never usedpyphare/pyphare_tests/test_pharesee/test_geometry.py (2)
119-119: Local variable
particlesis assigned to but never used
295-295: Comparison to
Noneshould becond is Nonepyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (3)
12-12:
pyphare.core.gridlayout.GridLayoutimported but unused
12-12:
pyphare.core.gridlayout.yee_element_is_primalimported but unused
166-166: Local variable
lvlis assigned to but never usedtests/amr/data/field/coarsening/test_coarsen_field.py (1)
9-9:
pyphare.core.boximported but unusedtests/amr/data/field/refine/test_refine_field.py (1)
9-9: Redefinition of unused
refinement_ratiofrom line 5tests/simulator/test_advance.py (25)
5-5: Module level import not at top of file
7-7: Module level import not at top of file
8-8: Module level import not at top of file
9-9: Module level import not at top of file
10-10: Module level import not at top of file
11-11: Module level import not at top of file
12-12: Module level import not at top of file
13-17: Module level import not at top of file
18-18: Module level import not at top of file
19-19: Module level import not at top of file
20-20: Module level import not at top of file
21-21: Module level import not at top of file
22-22: Module level import not at top of file
24-24: Module level import not at top of file
25-25: Module level import not at top of file
34-34: Do not assign a
lambdaexpression, use adef
93-93: Do not assign a
lambdaexpression, use adef
98-98: Do not assign a
lambdaexpression, use adef
103-103: Do not assign a
lambdaexpression, use adef
108-108: Do not assign a
lambdaexpression, use adef
113-113: Do not assign a
lambdaexpression, use adef
731-731: f-string without any placeholders
732-732: f-string without any placeholders
760-760:
pyphare.phareinimported but unused
765-765: Local variable
outis assigned to but never usedtests/simulator/test_diagnostics.py (15)
8-8: Module level import not at top of file
9-9: Module level import not at top of file
11-11: Module level import not at top of file
12-12: Module level import not at top of file
13-13: Module level import not at top of file
14-14: Module level import not at top of file
15-15: Module level import not at top of file
16-16: Module level import not at top of file
17-17: Module level import not at top of file
18-18: Module level import not at top of file
20-20: Module level import not at top of file
31-31: Do not assign a
lambdaexpression, use adef
38-38: Do not assign a
lambdaexpression, use adef
51-51: Do not assign a
lambdaexpression, use adef
58-58: Do not assign a
lambdaexpression, use adeftests/simulator/test_initialization.py (21)
5-5: Module level import not at top of file
7-7: Module level import not at top of file
8-8: Module level import not at top of file
9-9: Module level import not at top of file
10-10: Module level import not at top of file
11-11: Module level import not at top of file
12-16: Module level import not at top of file
17-17: Module level import not at top of file
18-18: Module level import not at top of file
19-19: Module level import not at top of file
20-20: Module level import not at top of file
21-21: Module level import not at top of file
22-22: Module level import not at top of file
24-24: Module level import not at top of file
33-33: Do not assign a
lambdaexpression, use adef
95-95: Do not assign a
lambdaexpression, use adef
100-100: Do not assign a
lambdaexpression, use adef
105-105: Do not assign a
lambdaexpression, use adef
110-110: Do not assign a
lambdaexpression, use adef
115-115: Do not assign a
lambdaexpression, use adef
703-703: Local variable
outis assigned to but never used
GitHub Check Runs (1)
CodeQL failure (21)
pyphare/pyphare/pharesee/hierarchy/fromh5.py: [notice] 150-150: Unused local variable
Variable basename is not used.
pyphare/pyphare/pharesee/hierarchy/fromh5.py: [notice] 151-151: Unused local variable
Variable root_cell_width is not used.
pyphare/pyphare/pharesee/hierarchy/fromh5.py: [notice] 152-152: Unused local variable
Variable interp is not used.
pyphare/pyphare/pharesee/hierarchy/fromh5.py: [notice] 154-154: Unused local variable
Variable domain_box is not used.
pyphare/pyphare/pharesee/hierarchy/fromh5.py: [notice] 157-157: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
pyphare/pyphare/pharesee/hierarchy/fromh5.py: [notice] 8-8: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy begins an import cycle.
pyphare/pyphare/pharesee/hierarchy/fromh5.py: [notice] 12-12: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 485-485: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [warning] 88-88: Comparison of identical values
Comparison of identical values; use cmath.isnan() if testing for not-a-number.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [failure] 131-131: Potentially uninitialized local variable
Local variable 'slice_dim' may be used before it is initialized.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [failure] 132-132: Potentially uninitialized local variable
Local variable 'cst_dim' may be used before it is initialized.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 497-497: Cyclic import
Import of module pyphare.pharesee.plotting begins an import cycle.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py: [notice] 495-495: Module is imported with 'import' and 'import from'
Module 'copy' is imported with both 'import' and 'import from'.
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py: [notice] 1-1: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy begins an import cycle.
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py: [notice] 7-7: Unused import
Import of 'boxm' is not used.
pyphare/pyphare/pharesee/hierarchy/patch.py: [notice] 1-1: Unused import
Import of 'PatchData' is not used.
pyphare/pyphare/pharesee/hierarchy/patch.py: [notice] 49-49: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
pyphare/pyphare/pharesee/hierarchy/patchdata.py: [warning] 121-121: Overwriting attribute in super-class or sub-class
Assignment overwrites attribute layout, which was previously defined in superclass PatchData.
pyphare/pyphare/pharesee/plotting.py: [notice] 248-248: Cyclic import
Import of module pyphare.pharesee.hierarchy.fromh5 begins an import cycle.
pyphare/pyphare/pharesee/run.py: [notice] 4-4: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
pyphare/pyphare/pharesee/run.py: [notice] 5-5: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
Additional comments not posted (17)
pyphare/pyphare/pharesee/hierarchy/patch.py (1)
49-66: Ensure explicit returns in all branches of the__call__method.elif idim == 1: - return pd.dataset[nbrGhosts:-nbrGhosts, idx + nbrGhosts] + return pd.dataset[nbrGhosts:-nbrGhosts, idx + nbrGhosts] +return None # Explicitly return None if no conditions are metThis change ensures that the method explicitly returns
Nonewhen no conditions are met, avoiding implicit returns which can lead to bugs.Likely invalid or redundant comment.
pyphare/pyphare/pharesee/hierarchy/patchdata.py (3)
30-106: The implementation ofFieldDatalooks good.
180-216: The implementation ofParticleDatalooks good.
121-121: Avoid overwriting attributes defined in the superclass.- self.layout = layoutThe
layoutattribute is already set in the superclass constructor and should not be overwritten in the subclass unless intentionally modifying its behavior.Likely invalid or redundant comment.
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (10)
40-74: LGTM! The optimizations to the merging process look effective.
76-96: LGTM! The addition of error handling for no patches found is a good improvement.
131-137: LGTM! The optimization of dictionary operations innew_patchdatas_fromis well implemented.
155-157: LGTM! Theis_root_lvlfunction is correctly implemented.
204-236: LGTM! Theoverlap_mask_1dfunction is correctly implemented and handles edge cases effectively.
239-282: LGTM! Theoverlap_mask_2dfunction is correctly implemented and handles edge cases effectively.
286-307: LGTM! Theflat_finest_fieldfunction is correctly implemented and handles different dimensions effectively.
309-346: LGTM! Theflat_finest_field_1dfunction is correctly implemented and handles edge cases effectively.
349-402: LGTM! Theflat_finest_field_2dfunction is correctly implemented and handles edge cases effectively.
180-201: LGTM! TheisFieldQtyfunction is correctly implemented.tests/amr/data/field/refine/test_refine_field.py (2)
6-6: Update import to reflect new submodule locations.
9-9: This redefinition ofrefinement_ratioshadows the imported value frompyphare.core.phare_utilities. This could lead to confusion or errors if the imported value is expected to be used elsewhere in the module or if the module's behavior depends on the imported value.tests/simulator/test_advance.py (1)
20-21: Ensure correct usage of the newly imported functions.The imports
hierarchy_fromandmerge_particlesare correctly placed and used within the context of the file. This aligns with the PR's objective to enhance hierarchical data management.
| @@ -0,0 +1,66 @@ | |||
| from .patchdata import PatchData | |||
There was a problem hiding this comment.
Remove unused import to clean up the code.
- from .patchdata import PatchDataCommittable 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.
| from .patchdata import PatchData |
| from pyphare.pharesee.hierarchy.fromh5 import h5_filename_from, h5_time_grp_key | ||
| from pyphare.pharesee.hierarchy import hierarchy_from | ||
| from pyphare.simulator.simulator import Simulator |
There was a problem hiding this comment.
Move module-level imports to the top of the file to adhere to PEP 8 standards.
+ from pyphare.pharesee.hierarchy.fromh5 import h5_filename_from, h5_time_grp_key
+ from pyphare.pharesee.hierarchy import hierarchy_from
+ from pyphare.simulator.simulator import Simulator
- from pyphare.pharesee.hierarchy.fromh5 import h5_filename_from, h5_time_grp_key
- from pyphare.pharesee.hierarchy import hierarchy_from
- from pyphare.simulator.simulator import SimulatorReorganize the imports to adhere to PEP 8 standards, which recommend that all module-level imports be at the top of the file.
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.
| from pyphare.pharesee.hierarchy.fromh5 import h5_filename_from, h5_time_grp_key | |
| from pyphare.pharesee.hierarchy import hierarchy_from | |
| from pyphare.simulator.simulator import Simulator | |
| from pyphare.pharesee.hierarchy.fromh5 import h5_filename_from, h5_time_grp_key | |
| from pyphare.pharesee.hierarchy import hierarchy_from | |
| from pyphare.simulator.simulator import Simulator |
| @@ -0,0 +1,402 @@ | |||
| from .hierarchy import PatchHierarchy | |||
Check notice
Code scanning / CodeQL
Cyclic import Note
| flat_finest_field, | ||
| hierarchy_from, | ||
| ) | ||
| from .hierarchy.hierarchy_utils import compute_hier_from |
Check notice
Code scanning / CodeQL
Cyclic import Note
| @@ -0,0 +1,66 @@ | |||
| from .patchdata import PatchData | |||
Check notice
Code scanning / CodeQL
Unused import Note
|
|
||
| return fig, ax | ||
|
|
||
| def plot(self, **kwargs): |
Check notice
Code scanning / CodeQL
Explicit returns mixed with implicit (fall through) returns Note
| """ | ||
| plot phase space of a particle hierarchy | ||
| """ | ||
| import copy |
Check notice
Code scanning / CodeQL
Module is imported with 'import' and 'import from' Note
| from ...core.box import Box | ||
| from ...core.phare_utilities import refinement_ratio | ||
| from ...core.gridlayout import GridLayout | ||
| from .hierarchy_utils import field_qties |
Check notice
Code scanning / CodeQL
Cyclic import Note
There was a problem hiding this comment.
Actionable comments posted: 10
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- pyphare/pyphare/pharesee/hierarchy/fromh5.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1 hunks)
Additional context used
GitHub Check: CodeQL
pyphare/pyphare/pharesee/hierarchy/fromh5.py
[notice] 8-8: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy begins an import cycle.
[notice] 12-12: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
[notice] 146-146: Module is imported more than once
This import of module os is redundant, as it was previously imported on line 1.
[notice] 243-243: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.pyphare/pyphare/pharesee/hierarchy/hierarchy.py
[notice] 519-519: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
[warning] 107-107: Comparison of identical values
Comparison of identical values; use cmath.isnan() if testing for not-a-number.
[failure] 150-150: Potentially uninitialized local variable
Local variable 'slice_dim' may be used before it is initialized.
[failure] 151-151: Potentially uninitialized local variable
Local variable 'cst_dim' may be used before it is initialized.
[notice] 531-531: Cyclic import
Import of module pyphare.pharesee.plotting begins an import cycle.
[notice] 529-529: Module is imported with 'import' and 'import from'
Module 'copy' is imported with both 'import' and 'import from'.
[warning] 548-548: Use of the return value of a procedure
The result of amr_grid is used even though it is always None.
| if finest: | ||
| final = finest_part_data(self) | ||
| if axis[0] == "x": | ||
| xbins = amr_grid(self, time) |
There was a problem hiding this comment.
Correct the use of the return value of a procedure.
The result of amr_grid is used even though it is always None. Ensure that the function returns a value if it is intended to be used.
Tools
GitHub Check: CodeQL
[warning] 548-548: Use of the return value of a procedure
The result of amr_grid is used even though it is always None.
There was a problem hiding this comment.
Actionable comments posted: 8
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- pyphare/pyphare/pharesee/hierarchy/fromh5.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1 hunks)
Additional context used
GitHub Check: CodeQL
pyphare/pyphare/pharesee/hierarchy/fromh5.py
[notice] 8-8: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy begins an import cycle.
[notice] 12-12: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
[notice] 146-146: Module is imported more than once
This import of module os is redundant, as it was previously imported on line 1.
[notice] 243-243: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.pyphare/pyphare/pharesee/hierarchy/hierarchy.py
[notice] 521-521: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
[warning] 109-109: Comparison of identical values
Comparison of identical values; use cmath.isnan() if testing for not-a-number.
[failure] 152-152: Potentially uninitialized local variable
Local variable 'slice_dim' may be used before it is initialized.
[failure] 153-153: Potentially uninitialized local variable
Local variable 'cst_dim' may be used before it is initialized.
[notice] 533-533: Cyclic import
Import of module pyphare.pharesee.plotting begins an import cycle.
[notice] 531-531: Module is imported with 'import' and 'import from'
Module 'copy' is imported with both 'import' and 'import from'.
Additional comments not posted (28)
pyphare/pyphare/pharesee/hierarchy/fromh5.py (1)
49-102: The functionadd_to_patchdatais well-structured and handles different data types effectively.pyphare/pyphare/pharesee/hierarchy/hierarchy.py (27)
15-23: Constructor parameters and initialization look good.
31-35: Good use of assertions to ensuretimesandselection_boxlengths match.
168-176: Methodfinest_levelcorrectly handles default time.
178-192: MethodslevelsandlevelNbrscorrectly handle default time.
194-202: Methodadd_timecorrectly updatestime_hierandselection_box.
204-216: Methodis_homogeneoususes a clear and efficient approach to check data consistency across patches.
218-225: Method_quantitiesefficiently retrieves patch data names.
227-230: Methodquantitiescorrectly raises an error when the hierarchy is not homogeneous.
232-243: Methodsglobal_minandglobal_maxare well-implemented with clear logic.
260-265: Methodrefined_domain_boxcorrectly calculates the refined domain box for a given level.
267-270: Methodformat_timestamphandles both string and numeric timestamps correctly.
272-275: Methodlevel_domain_boxcorrectly returns the domain box for level 0 and callsrefined_domain_boxfor other levels.
277-293: Method__str__provides a detailed string representation of the hierarchy.
295-300: Methodshas_timeandhas_filecorrectly check for the existence of times and files.
301-302: Methodtimescorrectly sorts and returns the times.
304-318: Methodplot_patchescorrectly plots patches and handles thesaveparameter.
319-322: Methodbox_to_Rectanglecorrectly converts a box to aRectangle.
324-347: Methodplot_2d_patchescorrectly handles collections of boxes and plots them.
367-420: Methodplot1dcorrectly handles plotting of 1D data and manages various plotting parameters.
421-517: Methodplot2dcorrectly handles plotting of 2D data and manages various plotting parameters.
519-526: Methodplotcorrectly delegates toplot1dorplot2dbased on the dimensionality.
527-578: Methoddist_plotcorrectly handles phase space plotting and manages various parameters.
581-640: Functionfinest_part_datacorrectly retrieves particle data from the finest patches.
643-710: Functionamr_gridcorrectly calculates a non-uniform contiguous primal grid for the given hierarchy.
81-97: Ensure proper error handling when deserializing simulation data.Verification successful
The
simproperty method inpyphare/pyphare/pharesee/hierarchy/hierarchy.pyincludes a try-except block to handle exceptions during the deserialization process. This ensures that any errors raised by thedeserializefunction are properly caught and handled.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py(lines 81-97): Proper error handling is implemented in thesimproperty method.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the deserialization process handles all possible errors. # Test: Search for the `deserialize` function usage. Expect: Proper error handling. rg --type python $'deserialize\('Length of output: 65
Script:
#!/bin/bash # Description: Verify that the deserialization process handles all possible errors. # Test: Search for the `deserialize` function usage. Expect: Proper error handling. rg 'deserialize\('Length of output: 248
Script:
#!/bin/bash # Description: Extract the `deserialize` function definition and its usage in the test file. # Extract the `deserialize` function definition. rg --context 10 --max-count 1 'def deserialize' pyphare/pyphare/pharein/simulation.py # Extract the usage of `deserialize` in the test file. rg --context 10 'deserialize\(' tests/simulator/test_diagnostics.pyLength of output: 1286
37-37: Ensure thattimesandpatch_levelshave the same length.
53-54: Ensure thatdata_filesis always passed as an object with afilenameattribute.
| Patch( | ||
| patch_datas, | ||
| h5_patch_grp.name.split("/")[-1], | ||
| layout=layout, | ||
| attrs={k: v for k, v in h5_patch_grp.attrs.items()}, | ||
| ) | ||
| ) | ||
|
|
||
| patch_levels[ilvl] = PatchLevel(ilvl, patches[ilvl]) | ||
|
|
||
| diag_hier = PatchHierarchy( | ||
| patch_levels, domain_box, refinement_ratio, t, data_file | ||
| ) | ||
|
|
||
| return diag_hier | ||
|
|
||
| if load_one_time(time, hier): | ||
| if not silent: | ||
| print("loading data at time {} into existing hierarchy".format(time)) | ||
| h5_time_grp = data_file[h5_time_grp_key][time] | ||
| t = time | ||
|
|
||
| if t in hier.time_hier: | ||
| if not silent: | ||
| print("time already exist, adding data...") | ||
|
|
||
| # time already exists in the hierarchy | ||
| # all we need to do is adding the data | ||
| # as patchDatas in the appropriate patches | ||
| # and levels, if data compatible with hierarchy | ||
|
|
||
| patch_levels = hier.time_hier[t] | ||
|
|
||
| for plvl_key in h5_time_grp.keys(): | ||
| ilvl = int(plvl_key[2:]) | ||
| lvl_cell_width = root_cell_width / refinement_ratio**ilvl | ||
|
|
||
| for ipatch, pkey in enumerate(h5_time_grp[plvl_key].keys()): | ||
| h5_patch_grp = h5_time_grp[plvl_key][pkey] | ||
|
|
||
| if patch_has_datasets(h5_patch_grp): | ||
| hier_patch = patch_levels[ilvl].patches[ipatch] | ||
| origin = h5_time_grp[plvl_key][pkey].attrs["origin"] | ||
| upper = h5_time_grp[plvl_key][pkey].attrs["upper"] | ||
| lower = h5_time_grp[plvl_key][pkey].attrs["lower"] | ||
| file_patch_box = Box(lower, upper) | ||
|
|
||
| assert file_patch_box == hier_patch.box | ||
| assert (abs(origin - hier_patch.origin) < 1e-6).all() | ||
| assert (abs(lvl_cell_width - hier_patch.dl) < 1e-6).all() | ||
|
|
||
| layout = make_layout(h5_patch_grp, lvl_cell_width, interp) | ||
| add_to_patchdata( | ||
| hier_patch.patch_datas, h5_patch_grp, basename, layout | ||
| ) | ||
|
|
||
| return hier | ||
|
|
||
| if not silent: | ||
| print("adding data to new time") | ||
| # time does not exist in the hierarchy | ||
| # we have to create a brand new set of patchLevels | ||
| # containing patches, and load data in their patchdatas | ||
|
|
||
| patch_levels = {} | ||
|
|
||
| for plvl_key in h5_time_grp.keys(): | ||
| ilvl = int(plvl_key[2:]) | ||
|
|
||
| lvl_cell_width = root_cell_width / refinement_ratio**ilvl | ||
| lvl_patches = [] | ||
|
|
||
| for ipatch, pkey in enumerate(h5_time_grp[plvl_key].keys()): | ||
| h5_patch_grp = h5_time_grp[plvl_key][pkey] | ||
|
|
||
| layout = make_layout(h5_patch_grp, lvl_cell_width, interp) | ||
| patch_datas = {} | ||
| if patch_has_datasets(h5_patch_grp): | ||
| add_to_patchdata(patch_datas, h5_patch_grp, basename, layout) | ||
| lvl_patches.append( | ||
| Patch( | ||
| patch_datas, | ||
| h5_patch_grp.name.split("/")[-1], | ||
| layout=layout, | ||
| attrs={k: v for k, v in h5_patch_grp.attrs.items()}, | ||
| ) | ||
| ) | ||
|
|
||
| patch_levels[ilvl] = PatchLevel(ilvl, lvl_patches) | ||
|
|
||
| hier.time_hier[t] = patch_levels | ||
| return hier | ||
|
|
||
| if load_all_times(time, hier): | ||
| if not silent: | ||
| print("loading all times in existing hier") | ||
| for time in data_file[h5_time_grp_key].keys(): | ||
| hier = hierarchy_fromh5(h5_filename, time, hier, silent=silent) | ||
|
|
||
| return hier |
There was a problem hiding this comment.
Ensure consistent use of return statements in hierarchy_fromh5.
+ else:
+ return None # Explicitly return None for clarityCommittable 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.
| def hierarchy_fromh5(h5_filename, time, hier, silent=True): | |
| data_file = h5py.File(h5_filename, "r") | |
| basename = os.path.basename(h5_filename) | |
| root_cell_width = np.asarray(data_file.attrs["cell_width"]) | |
| interp = data_file.attrs["interpOrder"] | |
| domain_box = Box( | |
| [0] * len(data_file.attrs["domain_box"]), data_file.attrs["domain_box"] | |
| ) | |
| if create_from_all_times(time, hier): | |
| # first create from first time | |
| # then add all other times | |
| if not silent: | |
| print("creating hierarchy from all times in file") | |
| times = list(data_file[h5_time_grp_key].keys()) | |
| hier = hierarchy_fromh5(h5_filename, time=times[0], hier=hier, silent=silent) | |
| if len(times) > 1: | |
| for t in times[1:]: | |
| hierarchy_fromh5(h5_filename, t, hier, silent=silent) | |
| return hier | |
| if create_from_one_time(time, hier): | |
| if not silent: | |
| print("creating hierarchy from time {}".format(time)) | |
| t = time | |
| h5_time_grp = data_file[h5_time_grp_key][time] | |
| patch_levels = {} | |
| for plvl_key in h5_time_grp.keys(): | |
| h5_patch_lvl_grp = h5_time_grp[plvl_key] | |
| ilvl = int(plvl_key[2:]) | |
| lvl_cell_width = root_cell_width / refinement_ratio**ilvl | |
| patches = {} | |
| if ilvl not in patches: | |
| patches[ilvl] = [] | |
| for pkey in h5_patch_lvl_grp.keys(): | |
| h5_patch_grp = h5_patch_lvl_grp[pkey] | |
| patch_datas = {} | |
| layout = make_layout(h5_patch_grp, lvl_cell_width, interp) | |
| if patch_has_datasets(h5_patch_grp): | |
| add_to_patchdata(patch_datas, h5_patch_grp, basename, layout) | |
| patches[ilvl].append( | |
| Patch( | |
| patch_datas, | |
| h5_patch_grp.name.split("/")[-1], | |
| layout=layout, | |
| attrs={k: v for k, v in h5_patch_grp.attrs.items()}, | |
| ) | |
| ) | |
| patch_levels[ilvl] = PatchLevel(ilvl, patches[ilvl]) | |
| diag_hier = PatchHierarchy( | |
| patch_levels, domain_box, refinement_ratio, t, data_file | |
| ) | |
| return diag_hier | |
| if load_one_time(time, hier): | |
| if not silent: | |
| print("loading data at time {} into existing hierarchy".format(time)) | |
| h5_time_grp = data_file[h5_time_grp_key][time] | |
| t = time | |
| if t in hier.time_hier: | |
| if not silent: | |
| print("time already exist, adding data...") | |
| # time already exists in the hierarchy | |
| # all we need to do is adding the data | |
| # as patchDatas in the appropriate patches | |
| # and levels, if data compatible with hierarchy | |
| patch_levels = hier.time_hier[t] | |
| for plvl_key in h5_time_grp.keys(): | |
| ilvl = int(plvl_key[2:]) | |
| lvl_cell_width = root_cell_width / refinement_ratio**ilvl | |
| for ipatch, pkey in enumerate(h5_time_grp[plvl_key].keys()): | |
| h5_patch_grp = h5_time_grp[plvl_key][pkey] | |
| if patch_has_datasets(h5_patch_grp): | |
| hier_patch = patch_levels[ilvl].patches[ipatch] | |
| origin = h5_time_grp[plvl_key][pkey].attrs["origin"] | |
| upper = h5_time_grp[plvl_key][pkey].attrs["upper"] | |
| lower = h5_time_grp[plvl_key][pkey].attrs["lower"] | |
| file_patch_box = Box(lower, upper) | |
| assert file_patch_box == hier_patch.box | |
| assert (abs(origin - hier_patch.origin) < 1e-6).all() | |
| assert (abs(lvl_cell_width - hier_patch.dl) < 1e-6).all() | |
| layout = make_layout(h5_patch_grp, lvl_cell_width, interp) | |
| add_to_patchdata( | |
| hier_patch.patch_datas, h5_patch_grp, basename, layout | |
| ) | |
| return hier | |
| if not silent: | |
| print("adding data to new time") | |
| # time does not exist in the hierarchy | |
| # we have to create a brand new set of patchLevels | |
| # containing patches, and load data in their patchdatas | |
| patch_levels = {} | |
| for plvl_key in h5_time_grp.keys(): | |
| ilvl = int(plvl_key[2:]) | |
| lvl_cell_width = root_cell_width / refinement_ratio**ilvl | |
| lvl_patches = [] | |
| for ipatch, pkey in enumerate(h5_time_grp[plvl_key].keys()): | |
| h5_patch_grp = h5_time_grp[plvl_key][pkey] | |
| layout = make_layout(h5_patch_grp, lvl_cell_width, interp) | |
| patch_datas = {} | |
| if patch_has_datasets(h5_patch_grp): | |
| add_to_patchdata(patch_datas, h5_patch_grp, basename, layout) | |
| lvl_patches.append( | |
| Patch( | |
| patch_datas, | |
| h5_patch_grp.name.split("/")[-1], | |
| layout=layout, | |
| attrs={k: v for k, v in h5_patch_grp.attrs.items()}, | |
| ) | |
| ) | |
| patch_levels[ilvl] = PatchLevel(ilvl, lvl_patches) | |
| hier.time_hier[t] = patch_levels | |
| return hier | |
| if load_all_times(time, hier): | |
| if not silent: | |
| print("loading all times in existing hier") | |
| for time in data_file[h5_time_grp_key].keys(): | |
| hier = hierarchy_fromh5(h5_filename, time, hier, silent=silent) | |
| return hier | |
| def hierarchy_fromh5(h5_filename, time, hier, silent=True): | |
| data_file = h5py.File(h5_filename, "r") | |
| basename = os.path.basename(h5_filename) | |
| root_cell_width = np.asarray(data_file.attrs["cell_width"]) | |
| interp = data_file.attrs["interpOrder"] | |
| domain_box = Box( | |
| [0] * len(data_file.attrs["domain_box"]), data_file.attrs["domain_box"] | |
| ) | |
| if create_from_all_times(time, hier): | |
| # first create from first time | |
| # then add all other times | |
| if not silent: | |
| print("creating hierarchy from all times in file") | |
| times = list(data_file[h5_time_grp_key].keys()) | |
| hier = hierarchy_fromh5(h5_filename, time=times[0], hier=hier, silent=silent) | |
| if len(times) > 1: | |
| for t in times[1:]: | |
| hierarchy_fromh5(h5_filename, t, hier, silent=silent) | |
| return hier | |
| if create_from_one_time(time, hier): | |
| if not silent: | |
| print("creating hierarchy from time {}".format(time)) | |
| t = time | |
| h5_time_grp = data_file[h5_time_grp_key][time] | |
| patch_levels = {} | |
| for plvl_key in h5_time_grp.keys(): | |
| h5_patch_lvl_grp = h5_time_grp[plvl_key] | |
| ilvl = int(plvl_key[2:]) | |
| lvl_cell_width = root_cell_width / refinement_ratio**ilvl | |
| patches = {} | |
| if ilvl not in patches: | |
| patches[ilvl] = [] | |
| for pkey in h5_patch_lvl_grp.keys(): | |
| h5_patch_grp = h5_patch_lvl_grp[pkey] | |
| patch_datas = {} | |
| layout = make_layout(h5_patch_grp, lvl_cell_width, interp) | |
| if patch_has_datasets(h5_patch_grp): | |
| add_to_patchdata(patch_datas, h5_patch_grp, basename, layout) | |
| patches[ilvl].append( | |
| Patch( | |
| patch_datas, | |
| h5_patch_grp.name.split("/")[-1], | |
| layout=layout, | |
| attrs={k: v for k, v in h5_patch_grp.attrs.items()}, | |
| ) | |
| ) | |
| patch_levels[ilvl] = PatchLevel(ilvl, patches[ilvl]) | |
| diag_hier = PatchHierarchy( | |
| patch_levels, domain_box, refinement_ratio, t, data_file | |
| ) | |
| return diag_hier | |
| if load_one_time(time, hier): | |
| if not silent: | |
| print("loading data at time {} into existing hierarchy".format(time)) | |
| h5_time_grp = data_file[h5_time_grp_key][time] | |
| t = time | |
| if t in hier.time_hier: | |
| if not silent: | |
| print("time already exist, adding data...") | |
| # time already exists in the hierarchy | |
| # all we need to do is adding the data | |
| # as patchDatas in the appropriate patches | |
| # and levels, if data compatible with hierarchy | |
| patch_levels = hier.time_hier[t] | |
| for plvl_key in h5_time_grp.keys(): | |
| ilvl = int(plvl_key[2:]) | |
| lvl_cell_width = root_cell_width / refinement_ratio**ilvl | |
| for ipatch, pkey in enumerate(h5_time_grp[plvl_key].keys()): | |
| h5_patch_grp = h5_time_grp[plvl_key][pkey] | |
| if patch_has_datasets(h5_patch_grp): | |
| hier_patch = patch_levels[ilvl].patches[ipatch] | |
| origin = h5_time_grp[plvl_key][pkey].attrs["origin"] | |
| upper = h5_time_grp[plvl_key][pkey].attrs["upper"] | |
| lower = h5_time_grp[plvl_key][pkey].attrs["lower"] | |
| file_patch_box = Box(lower, upper) | |
| assert file_patch_box == hier_patch.box | |
| assert (abs(origin - hier_patch.origin) < 1e-6).all() | |
| assert (abs(lvl_cell_width - hier_patch.dl) < 1e-6).all() | |
| layout = make_layout(h5_patch_grp, lvl_cell_width, interp) | |
| add_to_patchdata( | |
| hier_patch.patch_datas, h5_patch_grp, basename, layout | |
| ) | |
| return hier | |
| if not silent: | |
| print("adding data to new time") | |
| # time does not exist in the hierarchy | |
| # we have to create a brand new set of patchLevels | |
| # containing patches, and load data in their patchdatas | |
| patch_levels = {} | |
| for plvl_key in h5_time_grp.keys(): | |
| ilvl = int(plvl_key[2:]) | |
| lvl_cell_width = root_cell_width / refinement_ratio**ilvl | |
| lvl_patches = [] | |
| for ipatch, pkey in enumerate(h5_time_grp[plvl_key].keys()): | |
| h5_patch_grp = h5_time_grp[plvl_key][pkey] | |
| layout = make_layout(h5_patch_grp, lvl_cell_width, interp) | |
| patch_datas = {} | |
| if patch_has_datasets(h5_patch_grp): | |
| add_to_patchdata(patch_datas, h5_patch_grp, basename, layout) | |
| lvl_patches.append( | |
| Patch( | |
| patch_datas, | |
| h5_patch_grp.name.split("/")[-1], | |
| layout=layout, | |
| attrs={k: v for k, v in h5_patch_grp.attrs.items()}, | |
| ) | |
| ) | |
| patch_levels[ilvl] = PatchLevel(ilvl, lvl_patches) | |
| hier.time_hier[t] = patch_levels | |
| return hier | |
| if load_all_times(time, hier): | |
| if not silent: | |
| print("loading all times in existing hier") | |
| for time in data_file[h5_time_grp_key].keys(): | |
| hier = hierarchy_fromh5(h5_filename, time, hier, silent=silent) | |
| return hier | |
| else: | |
| return None # Explicitly return None for clarity |
Tools
GitHub Check: CodeQL
[notice] 243-243: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (1)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
15-23: Ensure consistent use of default arguments in function signatures.It's a good practice to use immutable default arguments for functions. Here,
data_files=Noneis correctly used to avoid potential bugs associated with mutable default arguments.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1 hunks)
- pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (5 hunks)
- tools/python3/phloping.py (1 hunks)
Additional context used
Ruff
pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py
12-12:
pyphare.core.gridlayout.GridLayoutimported but unused
12-12:
pyphare.core.gridlayout.yee_element_is_primalimported but unused
166-166: Local variable
lvlis assigned to but never used
GitHub Check: CodeQL
pyphare/pyphare/pharesee/hierarchy/hierarchy.py
[notice] 524-524: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
[warning] 112-112: Comparison of identical values
Comparison of identical values; use cmath.isnan() if testing for not-a-number.
[failure] 155-155: Potentially uninitialized local variable
Local variable 'slice_dim' may be used before it is initialized.
[failure] 156-156: Potentially uninitialized local variable
Local variable 'cst_dim' may be used before it is initialized.
[notice] 536-536: Cyclic import
Import of module pyphare.pharesee.plotting begins an import cycle.
[notice] 534-534: Module is imported with 'import' and 'import from'
Module 'copy' is imported with both 'import' and 'import from'.
Additional comments not posted (7)
tools/python3/phloping.py (1)
45-45: Ensure the new logic for particle count handling is optimized for performance.pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (2)
118-118: Ensure the new overlap checking logic integrates correctly with existing tests.
381-381: Verify the correctness of the new ghost box checking logic intest_level_ghost_boxes.pyphare/pyphare/pharesee/hierarchy/hierarchy.py (4)
25-26: Convert single values to lists or tuples if necessary.The use of
listifyto ensuretimesis always a list or tuple is a good practice, as it prevents type-related errors later in the code.
28-29: Ensurepatch_levelsis always a list or tuple.Similar to the previous comment, ensuring
patch_levelsis always a list or tuple is crucial for consistent handling throughout the class.
31-35: Validateselection_boxand ensure it matches the length oftimes.The validation and conversion of
selection_boxto a list or tuple, and the assertion that its length matchestimes, are important for data integrity and error prevention.
37-37: Check the length oftimesagainstpatch_levels.This assertion ensures that the time dimension aligns with the number of patch levels, which is crucial for maintaining data consistency across the structure.
| """ | ||
| import copy | ||
|
|
||
| from ..plotting import dist_plot as dp |
There was a problem hiding this comment.
Address cyclic imports to maintain clean architecture.
Consider refactoring the import structure to avoid cyclic dependencies, which can lead to maintenance issues and unexpected behavior.
Tools
GitHub Check: CodeQL
[notice] 536-536: Cyclic import
Import of module pyphare.pharesee.plotting begins an import cycle.
| """ | ||
| plot phase space of a particle hierarchy | ||
| """ | ||
| import copy |
There was a problem hiding this comment.
Avoid mixed import styles for clarity and consistency.
- import copy
- from copy import deepcopy
+ from copy import copy, deepcopyCommittable 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.
| import copy | |
| from copy import copy, deepcopy |
Tools
GitHub Check: CodeQL
[notice] 534-534: Module is imported with 'import' and 'import from'
Module 'copy' is imported with both 'import' and 'import from'.
tools/python3/phloping.py
Outdated
| # pcount_hier = self.run.GetParticleCount() | ||
| # TODO |
There was a problem hiding this comment.
Tip
Codebase Verification
The search results indicate that the hierarchy_from function is used extensively across various test files and the phloping.py file. However, there is no explicit error handling (e.g., try blocks) surrounding its usage in the provided context.
- File:
tools/python3/phloping.py- Lines: 44-45
- Code:
# pcount_hier = self.run.GetParticleCount() # TODO pcount_hier = hierarchy_from(h5_filename=self.run.path + "/particle_count.h5")
The absence of error handling around the hierarchy_from function call could lead to unhandled exceptions if the file does not exist or is corrupted. It is recommended to add appropriate error handling to manage potential issues gracefully.
Suggested Action:
Add a try-except block around the hierarchy_from function call to handle possible exceptions.
Example:
try:
pcount_hier = hierarchy_from(h5_filename=self.run.path + "/particle_count.h5")
except Exception as e:
# Handle the exception (e.g., log the error, raise a custom exception, etc.)
print(f"Error loading particle count hierarchy: {e}")
pcount_hier = None # or handle it as appropriateLocations to Address:
tools/python3/phloping.pyat lines 44-45
Analysis chain
Switched from GetParticleCount to hierarchy_from for particle count retrieval.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `hierarchy_from` is correctly integrated and handles errors as expected.
# Test: Search for error handling related to `hierarchy_from`.
rg --type python $'hierarchy_from' | grep 'try'
Length of output: 75
Script:
#!/bin/bash
# Description: Verify that `hierarchy_from` is correctly integrated and handles errors as expected.
# Test: Search for error handling related to `hierarchy_from`.
rg --type py 'hierarchy_from' | grep 'try'
Length of output: 40
Script:
#!/bin/bash
# Description: Verify the integration of `hierarchy_from` by examining its usage and surrounding code.
# Test: Search for `hierarchy_from` usage and display surrounding lines for context.
rg --type py 'hierarchy_from' -A 5 -B 5
Length of output: 26577
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1 hunks)
Additional context used
Ruff
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py
7-7:
...core.boximported but unused
GitHub Check: CodeQL
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py
[notice] 1-1: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy begins an import cycle.
[notice] 7-7: Unused import
Import of 'boxm' is not used.
Additional comments not posted (3)
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (3)
50-84: Consider optimizing the merging of particle data as previously suggested.
86-106: Ensure error handling for cases where no patches are found, as previously suggested.
144-150: Optimize dictionary operations innew_patchdatas_fromas previously suggested.
| from .patch import Patch | ||
| from ...core.phare_utilities import listify | ||
| from ...core.phare_utilities import refinement_ratio | ||
| from ...core import box as boxm |
There was a problem hiding this comment.
Remove unused import to clean up the code.
- from ...core import box as boxmCommittable 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.
| from ...core import box as boxm |
Tools
Ruff
7-7:
...core.boximported but unused
GitHub Check: CodeQL
[notice] 7-7: Unused import
Import of 'boxm' is not used.
| def are_compatible_hierarchies(hierarchies): | ||
| ref = hierarchies[0] | ||
| same_box = True | ||
| same_selection = True | ||
| same_files = True | ||
| same_times = True | ||
| for hier in hierarchies[1:]: | ||
| same_box = same_box and (hier.domain_box == ref.domain_box) | ||
| same_selection = same_selection and (hier.selection_box == ref.selection_box) | ||
| same_files = same_files and (hier.data_files.keys() == ref.data_files.keys()) | ||
| same_times = same_times and (hier.time_hier.keys() == ref.time_hier.keys()) | ||
| return True |
There was a problem hiding this comment.
Ensure that are_compatible_hierarchies function returns the correct boolean value.
- return True
+ return same_box and same_selection and same_files and same_timesCommittable 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.
| def are_compatible_hierarchies(hierarchies): | |
| ref = hierarchies[0] | |
| same_box = True | |
| same_selection = True | |
| same_files = True | |
| same_times = True | |
| for hier in hierarchies[1:]: | |
| same_box = same_box and (hier.domain_box == ref.domain_box) | |
| same_selection = same_selection and (hier.selection_box == ref.selection_box) | |
| same_files = same_files and (hier.data_files.keys() == ref.data_files.keys()) | |
| same_times = same_times and (hier.time_hier.keys() == ref.time_hier.keys()) | |
| return True | |
| def are_compatible_hierarchies(hierarchies): | |
| ref = hierarchies[0] | |
| same_box = True | |
| same_selection = True | |
| same_files = True | |
| same_times = True | |
| for hier in hierarchies[1:]: | |
| same_box = same_box and (hier.domain_box == ref.domain_box) | |
| same_selection = same_selection and (hier.selection_box == ref.selection_box) | |
| same_files = same_files and (hier.data_files.keys() == ref.data_files.keys()) | |
| same_times = same_times and (hier.time_hier.keys() == ref.time_hier.keys()) | |
| return same_box and same_selection and same_files and same_times |
There was a problem hiding this comment.
Actionable comments posted: 10
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- pyphare/pyphare/pharesee/hierarchy/fromh5.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1 hunks)
Files not reviewed due to errors (1)
- pyphare/pyphare/pharesee/hierarchy/fromh5.py (no review received)
Additional context used
GitHub Check: CodeQL
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py
[notice] 1-1: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy begins an import cycle.pyphare/pyphare/pharesee/hierarchy/fromh5.py
[notice] 8-8: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy begins an import cycle.
[notice] 12-12: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
[notice] 146-146: Module is imported more than once
This import of module os is redundant, as it was previously imported on line 1.
[notice] 286-286: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.pyphare/pyphare/pharesee/hierarchy/hierarchy.py
[notice] 525-525: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
[warning] 112-112: Comparison of identical values
Comparison of identical values; use cmath.isnan() if testing for not-a-number.
[failure] 155-155: Potentially uninitialized local variable
Local variable 'slice_dim' may be used before it is initialized.
[failure] 156-156: Potentially uninitialized local variable
Local variable 'cst_dim' may be used before it is initialized.
[notice] 537-537: Cyclic import
Import of module pyphare.pharesee.plotting begins an import cycle.
[notice] 535-535: Module is imported with 'import' and 'import from'
Module 'copy' is imported with both 'import' and 'import from'.
Additional comments not posted (2)
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1)
35-46: Ensure thatare_compatible_hierarchiesfunction returns the correct boolean value.- return True + return same_box and same_selection and same_files and same_timesLikely invalid or redundant comment.
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
15-59: Constructor ofPatchHierarchyhas been updated to handleselection_boxand ensuretimesandpatch_levelsare lists.
| def merge_particles(hierarchy): | ||
| for time, patch_levels in hierarchy.time_hier.items(): | ||
| for ilvl, plvl in patch_levels.items(): | ||
| for ip, patch in enumerate(plvl.patches): | ||
| pdatas = patch.patch_datas | ||
| domain_pdata = [ | ||
| (pdname, pd) for pdname, pd in pdatas.items() if "domain" in pdname | ||
| ][0] | ||
|
|
||
| pghost_pdatas = [ | ||
| (pdname, pd) | ||
| for pdname, pd in pdatas.items() | ||
| if "patchGhost" in pdname | ||
| ] | ||
| lghost_pdatas = [ | ||
| (pdname, pd) | ||
| for pdname, pd in pdatas.items() | ||
| if "levelGhost" in pdname | ||
| ] | ||
|
|
||
| pghost_pdata = pghost_pdatas[0] if pghost_pdatas else None | ||
| lghost_pdata = lghost_pdatas[0] if lghost_pdatas else None | ||
|
|
||
| if pghost_pdata is not None: | ||
| domain_pdata[1].dataset.add(pghost_pdata[1].dataset) | ||
| del pdatas[pghost_pdata[0]] | ||
|
|
||
| if lghost_pdata is not None: | ||
| domain_pdata[1].dataset.add(lghost_pdata[1].dataset) | ||
| del pdatas[lghost_pdata[0]] | ||
|
|
||
| popname = domain_pdata[0].split("_")[0] | ||
| pdatas[popname + "_particles"] = pdatas[domain_pdata[0]] | ||
| del pdatas[domain_pdata[0]] | ||
|
|
There was a problem hiding this comment.
Consider optimizing the merging of particle data.
The merging of particle data in merge_particles can be optimized by reducing the number of dictionary operations and improving the handling of ghost data. Consider refactoring to streamline the merging process and reduce computational overhead.
| @@ -0,0 +1,414 @@ | |||
| from .hierarchy import PatchHierarchy | |||
There was a problem hiding this comment.
Address cyclic import to maintain clean architecture.
Consider refactoring the import structure to avoid cyclic dependencies, which can lead to initialization problems and make the codebase harder to maintain.
Tools
GitHub Check: CodeQL
[notice] 1-1: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy begins an import cycle.
| def new_patchdatas_from(compute, patchdatas, layout, id, **kwargs): | ||
| new_patch_datas = {} | ||
| datas = compute(patchdatas, id=id, **kwargs) | ||
| for data in datas: | ||
| pd = FieldData(layout, data["name"], data["data"], centering=data["centering"]) | ||
| new_patch_datas[data["name"]] = pd | ||
| return new_patch_datas |
There was a problem hiding this comment.
Optimize dictionary operations in new_patchdatas_from.
- new_patch_datas = {}
- datas = compute(patchdatas, id=id, **kwargs)
- for data in datas:
- pd = FieldData(layout, data["name"], data["data"], centering=data["centering"])
- new_patch_datas[data["name"]] = pd
- return new_patch_datas
+ datas = compute(patchdatas, id=id, **kwargs)
+ return {data["name"]: FieldData(layout, data["name"], data["data"], centering=data["centering"]) for data in datas}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.
| def new_patchdatas_from(compute, patchdatas, layout, id, **kwargs): | |
| new_patch_datas = {} | |
| datas = compute(patchdatas, id=id, **kwargs) | |
| for data in datas: | |
| pd = FieldData(layout, data["name"], data["data"], centering=data["centering"]) | |
| new_patch_datas[data["name"]] = pd | |
| return new_patch_datas | |
| def new_patchdatas_from(compute, patchdatas, layout, id, **kwargs): | |
| datas = compute(patchdatas, id=id, **kwargs) | |
| return {data["name"]: FieldData(layout, data["name"], data["data"], centering=data["centering"]) for data in datas} |
| def getPatch(hier, point): | ||
| """ | ||
| convenience function mainly for debug. returns a dict | ||
| {ilevel:patch} for patches in which the given point is | ||
| """ | ||
| patches = {} | ||
| counts = {ilvl: 0 for ilvl in hier.levels().keys()} | ||
| for ilvl, lvl in hier.levels().items(): | ||
| for p in lvl.patches: | ||
| px, py = point | ||
| dx, dy = p.layout.dl | ||
| ix = int(px / dx) | ||
| iy = int(py / dy) | ||
| if (ix, iy) in p.box: | ||
| patches[ilvl] = p | ||
| counts[ilvl] += 1 | ||
| for k, v in counts.items(): | ||
| if v > 1: | ||
| print("error : ", k, v) | ||
| raise RuntimeError("more than one patch found for point") | ||
| return patches |
There was a problem hiding this comment.
Add error handling for the case where no patches are found.
+ if not patches:
+ raise RuntimeError("No patches found for the given point")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.
| def getPatch(hier, point): | |
| """ | |
| convenience function mainly for debug. returns a dict | |
| {ilevel:patch} for patches in which the given point is | |
| """ | |
| patches = {} | |
| counts = {ilvl: 0 for ilvl in hier.levels().keys()} | |
| for ilvl, lvl in hier.levels().items(): | |
| for p in lvl.patches: | |
| px, py = point | |
| dx, dy = p.layout.dl | |
| ix = int(px / dx) | |
| iy = int(py / dy) | |
| if (ix, iy) in p.box: | |
| patches[ilvl] = p | |
| counts[ilvl] += 1 | |
| for k, v in counts.items(): | |
| if v > 1: | |
| print("error : ", k, v) | |
| raise RuntimeError("more than one patch found for point") | |
| return patches | |
| def getPatch(hier, point): | |
| """ | |
| convenience function mainly for debug. returns a dict | |
| {ilevel:patch} for patches in which the given point is | |
| """ | |
| patches = {} | |
| counts = {ilvl: 0 for ilvl in hier.levels().keys()} | |
| for ilvl, lvl in hier.levels().items(): | |
| for p in lvl.patches: | |
| px, py = point | |
| dx, dy = p.layout.dl | |
| ix = int(px / dx) | |
| iy = int(py / dy) | |
| if (ix, iy) in p.box: | |
| patches[ilvl] = p | |
| counts[ilvl] += 1 | |
| if not patches: | |
| raise RuntimeError("No patches found for the given point") | |
| for k, v in counts.items(): | |
| if v > 1: | |
| print("error : ", k, v) | |
| raise RuntimeError("more than one patch found for point") | |
| return patches |
| def overlap_mask_1d(x, dl, level, qty): | ||
| """ | ||
| return the mask for x where x is overlaped by the qty patch datas | ||
| on the given level, assuming that this level is finer than the one of x | ||
|
|
||
| :param x: 1d array containing the [x] position | ||
| :param dl: list containing the grid steps where x is defined | ||
| :param level: a given level associated to a hierarchy | ||
| :param qty: ['Bx', 'By', 'Bz', 'Ex', 'Ey', 'Ez', 'Fx', 'Fy', 'Fz', 'Vx', 'Vy', 'Vz', 'rho'] | ||
| """ | ||
|
|
||
| is_overlaped = np.ones(x.shape[0], dtype=bool) * False | ||
|
|
||
| for patch in level.patches: | ||
| pdata = patch.patch_datas[qty] | ||
| ghosts_nbr = pdata.ghosts_nbr | ||
|
|
||
| fine_x = pdata.x[ghosts_nbr[0] - 1 : -ghosts_nbr[0] + 1] | ||
|
|
||
| fine_dl = pdata.dl | ||
| local_dl = dl | ||
|
|
||
| if fine_dl[0] < local_dl[0]: | ||
| xmin, xmax = fine_x.min(), fine_x.max() | ||
|
|
||
| overlaped_idx = np.where((x > xmin) & (x < xmax))[0] | ||
|
|
||
| is_overlaped[overlaped_idx] = True | ||
|
|
||
| else: | ||
| raise ValueError("level needs to have finer grid resolution than that of x") | ||
|
|
There was a problem hiding this comment.
Refactor overlap_mask_1d to handle edge cases and improve readability.
- is_overlaped = np.ones(x.shape[0], dtype=bool) * False
+ is_overlaped = np.zeros(x.shape[0], dtype=bool)
- if fine_dl[0] < local_dl[0]:
+ if fine_dl[0] >= local_dl[0]:
+ raise ValueError("level needs to have finer grid resolution than that of x")
- else:
- raise ValueError("level needs to have finer grid resolution than that of x")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.
| def overlap_mask_1d(x, dl, level, qty): | |
| """ | |
| return the mask for x where x is overlaped by the qty patch datas | |
| on the given level, assuming that this level is finer than the one of x | |
| :param x: 1d array containing the [x] position | |
| :param dl: list containing the grid steps where x is defined | |
| :param level: a given level associated to a hierarchy | |
| :param qty: ['Bx', 'By', 'Bz', 'Ex', 'Ey', 'Ez', 'Fx', 'Fy', 'Fz', 'Vx', 'Vy', 'Vz', 'rho'] | |
| """ | |
| is_overlaped = np.ones(x.shape[0], dtype=bool) * False | |
| for patch in level.patches: | |
| pdata = patch.patch_datas[qty] | |
| ghosts_nbr = pdata.ghosts_nbr | |
| fine_x = pdata.x[ghosts_nbr[0] - 1 : -ghosts_nbr[0] + 1] | |
| fine_dl = pdata.dl | |
| local_dl = dl | |
| if fine_dl[0] < local_dl[0]: | |
| xmin, xmax = fine_x.min(), fine_x.max() | |
| overlaped_idx = np.where((x > xmin) & (x < xmax))[0] | |
| is_overlaped[overlaped_idx] = True | |
| else: | |
| raise ValueError("level needs to have finer grid resolution than that of x") | |
| def overlap_mask_1d(x, dl, level, qty): | |
| """ | |
| return the mask for x where x is overlaped by the qty patch datas | |
| on the given level, assuming that this level is finer than the one of x | |
| :param x: 1d array containing the [x] position | |
| :param dl: list containing the grid steps where x is defined | |
| :param level: a given level associated to a hierarchy | |
| :param qty: ['Bx', 'By', 'Bz', 'Ex', 'Ey', 'Ez', 'Fx', 'Fy', 'Fz', 'Vx', 'Vy', 'Vz', 'rho'] | |
| """ | |
| is_overlaped = np.zeros(x.shape[0], dtype=bool) | |
| for patch in level.patches: | |
| pdata = patch.patch_datas[qty] | |
| ghosts_nbr = pdata.ghosts_nbr | |
| fine_x = pdata.x[ghosts_nbr[0] - 1 : -ghosts_nbr[0] + 1] | |
| fine_dl = pdata.dl | |
| local_dl = dl | |
| if fine_dl[0] >= local_dl[0]: | |
| raise ValueError("level needs to have finer grid resolution than that of x") | |
| xmin, xmax = fine_x.min(), fine_x.max() | |
| overlaped_idx = np.where((x > xmin) & (x < xmax))[0] | |
| is_overlaped[overlaped_idx] = True |
| def overlap_mask_2d(x, y, dl, level, qty): | ||
| """ | ||
| return the mask for x & y where ix & y are overlaped by the qty patch datas | ||
| on the given level, assuming that this level is finer than the one of x & y | ||
| important note : this mask is flatten | ||
|
|
||
| :param x: 1d array containing the [x] position | ||
| :param y: 1d array containing the [y] position | ||
| :param dl: list containing the grid steps where x and y are defined | ||
| :param level: a given level associated to a hierarchy | ||
| :param qty: ['Bx', 'By', 'Bz', 'Ex', 'Ey', 'Ez', 'Fx', 'Fy', 'Fz', 'Vx', 'Vy', 'Vz', 'rho'] | ||
| """ | ||
|
|
||
| is_overlaped = np.ones([x.shape[0] * y.shape[0]], dtype=bool) * False | ||
|
|
||
| for patch in level.patches: | ||
| pdata = patch.patch_datas[qty] | ||
| ghosts_nbr = pdata.ghosts_nbr | ||
|
|
||
| fine_x = pdata.x[ghosts_nbr[0] - 1 : -ghosts_nbr[0] + 1] | ||
| fine_y = pdata.y[ghosts_nbr[1] - 1 : -ghosts_nbr[1] + 1] | ||
|
|
||
| fine_dl = pdata.dl | ||
| local_dl = dl | ||
|
|
||
| if (fine_dl[0] < local_dl[0]) and (fine_dl[1] < local_dl[1]): | ||
| xmin, xmax = fine_x.min(), fine_x.max() | ||
| ymin, ymax = fine_y.min(), fine_y.max() | ||
|
|
||
| xv, yv = np.meshgrid(x, y, indexing="ij") | ||
| xf = xv.flatten() | ||
| yf = yv.flatten() | ||
|
|
||
| overlaped_idx = np.where( | ||
| (xf > xmin) & (xf < xmax) & (yf > ymin) & (yf < ymax) | ||
| )[0] | ||
|
|
||
| is_overlaped[overlaped_idx] = True | ||
|
|
||
| else: | ||
| raise ValueError( | ||
| "level needs to have finer grid resolution than that of x or y" | ||
| ) |
There was a problem hiding this comment.
Refactor overlap_mask_2d to handle edge cases and improve readability.
- is_overlaped = np.ones([x.shape[0] * y.shape[0]], dtype=bool) * False
+ is_overlaped = np.zeros([x.shape[0] * y.shape[0]], dtype=bool)
- if (fine_dl[0] < local_dl[0]) and (fine_dl[1] < local_dl[1]):
+ if (fine_dl[0] >= local_dl[0]) or (fine_dl[1] >= local_dl[1]):
+ raise ValueError("level needs to have finer grid resolution than that of x or y")
- else:
- raise ValueError("level needs to have finer grid resolution than that of x or y")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.
| def overlap_mask_2d(x, y, dl, level, qty): | |
| """ | |
| return the mask for x & y where ix & y are overlaped by the qty patch datas | |
| on the given level, assuming that this level is finer than the one of x & y | |
| important note : this mask is flatten | |
| :param x: 1d array containing the [x] position | |
| :param y: 1d array containing the [y] position | |
| :param dl: list containing the grid steps where x and y are defined | |
| :param level: a given level associated to a hierarchy | |
| :param qty: ['Bx', 'By', 'Bz', 'Ex', 'Ey', 'Ez', 'Fx', 'Fy', 'Fz', 'Vx', 'Vy', 'Vz', 'rho'] | |
| """ | |
| is_overlaped = np.ones([x.shape[0] * y.shape[0]], dtype=bool) * False | |
| for patch in level.patches: | |
| pdata = patch.patch_datas[qty] | |
| ghosts_nbr = pdata.ghosts_nbr | |
| fine_x = pdata.x[ghosts_nbr[0] - 1 : -ghosts_nbr[0] + 1] | |
| fine_y = pdata.y[ghosts_nbr[1] - 1 : -ghosts_nbr[1] + 1] | |
| fine_dl = pdata.dl | |
| local_dl = dl | |
| if (fine_dl[0] < local_dl[0]) and (fine_dl[1] < local_dl[1]): | |
| xmin, xmax = fine_x.min(), fine_x.max() | |
| ymin, ymax = fine_y.min(), fine_y.max() | |
| xv, yv = np.meshgrid(x, y, indexing="ij") | |
| xf = xv.flatten() | |
| yf = yv.flatten() | |
| overlaped_idx = np.where( | |
| (xf > xmin) & (xf < xmax) & (yf > ymin) & (yf < ymax) | |
| )[0] | |
| is_overlaped[overlaped_idx] = True | |
| else: | |
| raise ValueError( | |
| "level needs to have finer grid resolution than that of x or y" | |
| ) | |
| def overlap_mask_2d(x, y, dl, level, qty): | |
| """ | |
| return the mask for x & y where ix & y are overlaped by the qty patch datas | |
| on the given level, assuming that this level is finer than the one of x & y | |
| important note : this mask is flatten | |
| :param x: 1d array containing the [x] position | |
| :param y: 1d array containing the [y] position | |
| :param dl: list containing the grid steps where x and y are defined | |
| :param level: a given level associated to a hierarchy | |
| :param qty: ['Bx', 'By', 'Bz', 'Ex', 'Ey', 'Ez', 'Fx', 'Fy', 'Fz', 'Vx', 'Vy', 'Vz', 'rho'] | |
| """ | |
| is_overlaped = np.zeros([x.shape[0] * y.shape[0]], dtype=bool) | |
| for patch in level.patches: | |
| pdata = patch.patch_datas[qty] | |
| ghosts_nbr = pdata.ghosts_nbr | |
| fine_x = pdata.x[ghosts_nbr[0] - 1 : -ghosts_nbr[0] + 1] | |
| fine_y = pdata.y[ghosts_nbr[1] - 1 : -ghosts_nbr[1] + 1] | |
| fine_dl = pdata.dl | |
| local_dl = dl | |
| if (fine_dl[0] >= local_dl[0]) or (fine_dl[1] >= local_dl[1]): | |
| raise ValueError( | |
| "level needs to have finer grid resolution than that of x or y" | |
| ) | |
| xmin, xmax = fine_x.min(), fine_x.max() | |
| ymin, ymax = fine_y.min(), fine_y.max() | |
| xv, yv = np.meshgrid(x, y, indexing="ij") | |
| xf = xv.flatten() | |
| yf = yv.flatten() | |
| overlaped_idx = np.where( | |
| (xf > xmin) & (xf < xmax) & (yf > ymin) & (yf < ymax) | |
| )[0] | |
| is_overlaped[overlaped_idx] = True |
There was a problem hiding this comment.
Actionable comments posted: 10
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- pyphare/pyphare/pharesee/hierarchy/fromh5.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1 hunks)
Additional context used
GitHub Check: CodeQL
pyphare/pyphare/pharesee/hierarchy/fromh5.py
[notice] 8-8: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy begins an import cycle.
[notice] 12-12: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
[notice] 146-146: Module is imported more than once
This import of module os is redundant, as it was previously imported on line 1.
[notice] 286-286: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.pyphare/pyphare/pharesee/hierarchy/hierarchy.py
[notice] 534-534: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
[warning] 121-121: Comparison of identical values
Comparison of identical values; use cmath.isnan() if testing for not-a-number.
[failure] 164-164: Potentially uninitialized local variable
Local variable 'slice_dim' may be used before it is initialized.
[failure] 165-165: Potentially uninitialized local variable
Local variable 'cst_dim' may be used before it is initialized.
[notice] 546-546: Cyclic import
Import of module pyphare.pharesee.plotting begins an import cycle.
[notice] 544-544: Module is imported with 'import' and 'import from'
Module 'copy' is imported with both 'import' and 'import from'.
| return add_data_from_h5(hier, h5_filename, time) | ||
|
|
||
|
|
||
| def hierarchy_fromh5(h5_filename, time, hier, silent=True): |
There was a problem hiding this comment.
Ensure consistent use of return statements.
The function hierarchy_fromh5 mixes implicit and explicit returns. This can lead to unexpected behavior where the function might return None implicitly. Consider adding explicit return statements or restructuring the function to avoid this issue.
Tools
GitHub Check: CodeQL
[notice] 286-286: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
| def __init__( | ||
| self, | ||
| patch_levels, | ||
| domain_box, | ||
| refinement_ratio=2, | ||
| time=0.0, | ||
| data_files=None, | ||
| **kwargs, | ||
| ): |
There was a problem hiding this comment.
Ensure consistent initialization of times and patch_levels.
- times = time
- if not isinstance(times, (tuple, list)):
- times = listify(times)
- if not isinstance(patch_levels, (tuple, list)):
- patch_levels = listify(patch_levels)
+ times = listify(time) if not isinstance(time, (tuple, list)) else time
+ patch_levels = listify(patch_levels) if not isinstance(patch_levels, (tuple, list)) else patch_levelsThis change simplifies the handling of the time and patch_levels parameters by ensuring they are always lists or tuples, reducing redundancy and improving readability.
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.
| def __init__( | |
| self, | |
| patch_levels, | |
| domain_box, | |
| refinement_ratio=2, | |
| time=0.0, | |
| data_files=None, | |
| **kwargs, | |
| ): | |
| def __init__( | |
| self, | |
| patch_levels, | |
| domain_box, | |
| refinement_ratio=2, | |
| time=0.0, | |
| data_files=None, | |
| **kwargs, | |
| ): | |
| times = listify(time) if not isinstance(time, (tuple, list)) else time | |
| patch_levels = listify(patch_levels) if not isinstance(patch_levels, (tuple, list)) else patch_levels |
There was a problem hiding this comment.
Actionable comments posted: 6
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- pyphare/pyphare/pharesee/hierarchy/init.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/fromh5.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1 hunks)
- pyphare/pyphare/pharesee/run.py (7 hunks)
Additional context used
Ruff
pyphare/pyphare/pharesee/hierarchy/__init__.py
1-1:
.hierarchy.PatchHierarchyimported but unused; consider removing, adding to__all__, or using a redundant alias (F401)
2-2:
.fromh5.hierarchy_fromh5imported but unused; consider removing, adding to__all__, or using a redundant alias (F401)
3-3:
.fromsim.hierarchy_from_simimported but unused; consider removing, adding to__all__, or using a redundant alias (F401)
9-9: Redefinition of unused
hierarchy_fromh5from line 2 (F811)
10-10: Redefinition of unused
hierarchy_from_simfrom line 3 (F811)pyphare/pyphare/pharesee/hierarchy/fromh5.py
316-316: Undefined name
create_from_one_time(F821)pyphare/pyphare/pharesee/run.py
378-378: f-string without any placeholders (F541)
380-380: f-string without any placeholders (F541)
466-466: Undefined name
kwargs(F821)
Additional comments not posted (3)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (3)
24-28: Ensure consistent data types fortimesandpatch_levels.
The constructor ofPatchHierarchyuseslistifyto ensure that bothtimesandpatch_levelsare lists. This is a good practice to avoid type errors later in the code. However, it would be more robust to enforce this at the API level, ensuring that these parameters are always passed as lists or tuples, thus avoiding the need for type checks inside the constructor.- times = time - if not isinstance(times, (tuple, list)): - times = listify(times) - if not isinstance(patch_levels, (tuple, list)): - patch_levels = listify(patch_levels) + assert isinstance(times, (tuple, list)), "Expected 'times' to be a tuple or list" + assert isinstance(patch_levels, (tuple, list)), "Expected 'patch_levels' to be a tuple or list"
30-38: Validateselection_boxparameter in the constructor.
The current implementation dynamically checks and convertsselection_boxto a list if it's not already one. This is done inside the constructor, which can be cleaned up by ensuringselection_boxis always a list before it's passed to the constructor, simplifying the code and improving performance.- if self.selection_box is not None: - if not isinstance(self.selection_box, (tuple, list)): - self.selection_box = listify(self.selection_box) - self.selection_box = { - self.format_timestamp(t): box - for t, box in zip(times, self.selection_box) - } - assert len(times) == len(self.selection_box) + self.selection_box = listify(kwargs.get("selection_box", [])) + if len(times) != len(self.selection_box): + raise ValueError("Length of 'times' and 'selection_box' must be equal.")This modification ensures that
selection_boxis always a list and adds a more descriptive error message if the lengths oftimesandselection_boxdo not match.
126-193: Complex method__call__. Consider breaking down into smaller, more manageable methods.
The__call__method is quite complex and handles multiple responsibilities, which can make it difficult to maintain and understand. Consider refactoring it into smaller, more focused methods.- def cuts(c, coord): - return c > coord.min() and c < coord.max() - class Extractor: - def __init__(self): - self.exclusions = [] - def extract(self, coord, data): - mask = coord == coord - for exclusion in self.exclusions: - idx = np.where( - (coord > exclusion[0] - 1e-6) & (coord < exclusion[1] + 1e-6) - )[0] - mask[idx] = False - self.exclusions += [(coord.min(), coord.max())] - return coord[mask], data[mask] - def domain_coords(patch, qty): - pd = patch.patch_datas[qty] - nbrGhosts = pd.ghosts_nbr[0] - return pd.x[nbrGhosts:-nbrGhosts], pd.y[nbrGhosts:-nbrGhosts] + self.handle_call(qty, **kwargs)And then define a new method
handle_callto handle the initialization.
| if simulator is not None and h5_filename is not None: | ||
| raise ValueError("cannot pass both a simulator and a h5 file") | ||
|
|
||
| if h5_filename is not None: | ||
| return hierarchy_fromh5(h5_filename, times, hier, **kwargs) | ||
|
|
||
| if simulator is not None and qty is not None: | ||
| return hierarchy_from_sim(simulator, qty, pop=pop) | ||
|
|
||
| raise ValueError("can't make hierarchy") |
There was a problem hiding this comment.
Improve error handling for input validation.
if simulator is not None and h5_filename is not None:
raise ValueError("Cannot pass both a simulator and an h5 file. Please provide only one.")
elif h5_filename is None and simulator is None:
raise ValueError("Either a simulator or an h5 file must be provided.")| from .hierarchy import PatchHierarchy | ||
| from .fromh5 import hierarchy_fromh5 | ||
| from .fromsim import hierarchy_from_sim |
There was a problem hiding this comment.
Remove unused imports to clean up the code.
- from .hierarchy import PatchHierarchy
- from .fromh5 import hierarchy_fromh5
- from .fromsim import hierarchy_from_simCommittable 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.
| from .hierarchy import PatchHierarchy | |
| from .fromh5 import hierarchy_fromh5 | |
| from .fromsim import hierarchy_from_sim |
Tools
Ruff
1-1:
.hierarchy.PatchHierarchyimported but unused; consider removing, adding to__all__, or using a redundant alias (F401)
2-2:
.fromh5.hierarchy_fromh5imported but unused; consider removing, adding to__all__, or using a redundant alias (F401)
3-3:
.fromsim.hierarchy_from_simimported but unused; consider removing, adding to__all__, or using a redundant alias (F401)
| from .fromh5 import hierarchy_fromh5 | ||
| from .fromsim import hierarchy_from_sim |
There was a problem hiding this comment.
Avoid redefining imports inside functions.
- from .fromh5 import hierarchy_fromh5
- from .fromsim import hierarchy_from_simCommittable 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.
| from .fromh5 import hierarchy_fromh5 | |
| from .fromsim import hierarchy_from_sim |
Tools
Ruff
9-9: Redefinition of unused
hierarchy_fromh5from line 2 (F811)
10-10: Redefinition of unused
hierarchy_from_simfrom line 3 (F811)
pyphare/pyphare/pharesee/run.py
Outdated
| @@ -452,7 +463,7 @@ | |||
| if time is None: | |||
| time = float(list(data_file[h5_time_grp_key].keys())[0]) | |||
|
|
|||
| hier = self._get_hierarchy(time, h5_filename) | |||
| hier = self._get_hierarchy(time, h5_filename, **kwargs) | |||
There was a problem hiding this comment.
Undefined variable kwargs in method call.
- hier = self._get_hierarchy(time, h5_filename, **kwargs)
+ hier = self._get_hierarchy(time, h5_filename, **kwargs) # Ensure kwargs is defined or passed correctlyCommittable suggestion was skipped due to low confidence.
Tools
Ruff
466-466: Undefined name
kwargs(F821)
| hierarchy_from, | ||
| ) | ||
| from .hierarchy.hierarchy_utils import compute_hier_from | ||
| from .hierarchy.hierarchy_utils import flat_finest_field |
Check notice
Code scanning / CodeQL
Cyclic import Note
| creates a dictionary of PatchLevels from a given time in a h5 file | ||
| {ilvl: PatchLevel} | ||
| """ | ||
| import os |
Check notice
Code scanning / CodeQL
Module is imported more than once Note
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- pyphare/pyphare/pharesee/hierarchy/fromh5.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1 hunks)
- tests/functional/tdtagged/td1dtagged.py (1 hunks)
Additional context used
Ruff
tests/functional/tdtagged/td1dtagged.py
23-23: Ambiguous variable name:
l(E741)
177-177: Ambiguous variable name:
l(E741)
207-207: Local variable
dxis assigned to but never used (F841)
252-252: Module level import not at top of file (E402)
253-253: Module level import not at top of file (E402)
pyphare/pyphare/pharesee/hierarchy/fromh5.py
318-318: Undefined name
create_from_one_time(F821)
Additional comments not posted (5)
tests/functional/tdtagged/td1dtagged.py (3)
266-266: The modification to accept a list of times inget_timealigns well with the PR objectives.
266-266: The changes inpost_advancefunction correctly utilize the updatedget_timefunction to manage particle diagnostics.
266-266: Themainfunction effectively orchestrates the simulation runs using the updated functionalities.pyphare/pyphare/pharesee/hierarchy/fromh5.py (2)
275-295: Thehierarchy_fromh5function handles various scenarios for hierarchy management effectively. However, ensure thecreate_from_one_timefunction is defined or imported as it is used here.
244-273: Thenew_from_h5function effectively creates a new hierarchy from HDF5 data, aligning with the PR objectives.
| def __call__(self, qty=None, **kwargs): | ||
| # take slice/slab of 1/2d array from 2/3d array | ||
| def cuts(c, coord): | ||
| return c > coord.min() and c < coord.max() | ||
|
|
||
| class Extractor: | ||
| def __init__(self): | ||
| self.exclusions = [] | ||
|
|
||
| def extract(self, coord, data): | ||
| mask = coord == coord | ||
| for exclusion in self.exclusions: | ||
| idx = np.where( | ||
| (coord > exclusion[0] - 1e-6) & (coord < exclusion[1] + 1e-6) | ||
| )[0] | ||
| mask[idx] = False | ||
|
|
||
| self.exclusions += [(coord.min(), coord.max())] | ||
| return coord[mask], data[mask] | ||
|
|
||
| def domain_coords(patch, qty): | ||
| pd = patch.patch_datas[qty] | ||
| nbrGhosts = pd.ghosts_nbr[0] | ||
| return pd.x[nbrGhosts:-nbrGhosts], pd.y[nbrGhosts:-nbrGhosts] | ||
|
|
||
| if len(kwargs) < 1 or len(kwargs) > 3: | ||
| raise ValueError("Error - must provide coordinates") | ||
| if qty is None: | ||
| if len(self.quantities()) == 1: | ||
| qty = self.quantities()[0] | ||
| else: | ||
| raise ValueError( | ||
| "The PatchHierarchy has several quantities but none is specified" | ||
| ) | ||
|
|
||
| if len(kwargs) == 1: # 1D cut | ||
| if "x" in kwargs: | ||
| c = kwargs["x"] | ||
| slice_dim = 1 | ||
| cst_dim = 0 | ||
| else: | ||
| c = kwargs["y"] | ||
| slice_dim = 0 | ||
| cst_dim = 1 | ||
|
|
||
| extractor = Extractor() | ||
| datas = [] | ||
| coords = [] | ||
| ilvls = list(self.levels().keys())[::-1] | ||
|
|
||
| for ilvl in ilvls: | ||
| lvl = self.patch_levels[ilvl] | ||
| for patch in lvl.patches: | ||
| slice_coord = domain_coords(patch, qty)[slice_dim] | ||
| cst_coord = domain_coords(patch, qty)[cst_dim] | ||
|
|
||
| if cuts(c, cst_coord): | ||
| data = patch(qty, **kwargs) | ||
| coord_keep, data_keep = extractor.extract(slice_coord, data) | ||
| datas += [data_keep] | ||
| coords += [coord_keep] | ||
|
|
||
| cut = np.concatenate(datas) | ||
| coords = np.concatenate(coords) | ||
| ic = np.argsort(coords) | ||
| coords = coords[ic] | ||
| cut = cut[ic] | ||
| return coords, cut |
There was a problem hiding this comment.
Complex method __call__. Consider breaking down into smaller, more manageable methods.
- def cuts(c, coord):
- return c > coord.min() and c < coord.max()
- class Extractor:
- def __init__(self):
- self.exclusions = []
- def extract(self, coord, data):
- mask = coord == coord
- for exclusion in self.exclusions:
- idx = np.where(
- (coord > exclusion[0] - 1e-6) & (coord < exclusion[1] + 1e-6)
- )[0]
- mask[idx] = False
- self.exclusions += [(coord.min(), coord.max())]
- return coord[mask], data[mask]
- def domain_coords(patch, qty):
- pd = patch.patch_datas[qty]
- nbrGhosts = pd.ghosts_nbr[0]
- return pd.x[nbrGhosts:-nbrGhosts], pd.y[nbrGhosts:-nbrGhosts]
+ self.handle_call(qty, **kwargs)This change simplifies the __call__ method by extracting complex logic into a separate method, improving readability and maintainability.
Committable suggestion was skipped due to low confidence.
| hierarchy_fromh5(h5_filename, t, hier, silent=silent) | ||
| return hier | ||
|
|
||
| if create_from_one_time(time, hier): |
There was a problem hiding this comment.
Undefined function create_from_one_time detected. Please define or import this function to avoid runtime errors.
- if create_from_one_time(time, hier):
+ # TODO: Define or import `create_from_one_time` functionCommittable suggestion was skipped due to low confidence.
Tools
Ruff
318-318: Undefined name
create_from_one_time(F821)
| @@ -0,0 +1,291 @@ | |||
| import os | |||
Check notice
Code scanning / CodeQL
Unused import Note
There was a problem hiding this comment.
Actionable comments posted: 8
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- pyphare/pyphare/pharesee/hierarchy/fromh5.py (1 hunks)
- pyphare/pyphare/pharesee/run.py (6 hunks)
Additional context used
Ruff
pyphare/pyphare/pharesee/hierarchy/fromh5.py
1-1:
osimported but unused (F401)
146-146: Redefinition of unused
osfrom line 1 (F811)pyphare/pyphare/pharesee/run.py
378-378: f-string without any placeholders (F541)
380-380: f-string without any placeholders (F541)
GitHub Check: CodeQL
pyphare/pyphare/pharesee/hierarchy/fromh5.py
[notice] 8-8: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy begins an import cycle.
[notice] 12-12: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
[notice] 146-146: Module is imported more than once
This import of module os is redundant, as it was previously imported on line 1.
[notice] 276-276: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
[notice] 1-1: Unused import
Import of 'os' is not used.pyphare/pyphare/pharesee/run.py
[notice] 4-4: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
[notice] 5-5: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
| from .patchlevel import PatchLevel | ||
| from .patchdata import FieldData, ParticleData | ||
| from ..particles import Particles | ||
| from .hierarchy import PatchHierarchy |
There was a problem hiding this comment.
Address cyclic imports to maintain clean architecture.
Consider refactoring the import structure to avoid cyclic dependencies, which can lead to initialization problems and make the codebase harder to maintain.
Tools
GitHub Check: CodeQL
[notice] 8-8: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy begins an import cycle.
| from ...core.box import Box | ||
| from ...core.phare_utilities import refinement_ratio | ||
| from ...core.gridlayout import GridLayout | ||
| from .hierarchy_utils import field_qties |
There was a problem hiding this comment.
Address cyclic imports to maintain clean architecture.
Consider refactoring the import structure to avoid cyclic dependencies, which can lead to initialization problems and make the codebase harder to maintain.
Tools
GitHub Check: CodeQL
[notice] 12-12: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
| flat_finest_field, | ||
| hierarchy_from, | ||
| ) | ||
| from .hierarchy.hierarchy_utils import compute_hier_from |
There was a problem hiding this comment.
Address cyclic imports to maintain clean architecture.
Consider refactoring the import structure to avoid cyclic dependencies, which can lead to initialization problems and make the codebase harder to maintain.
Tools
GitHub Check: CodeQL
[notice] 4-4: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
| hierarchy_from, | ||
| ) | ||
| from .hierarchy.hierarchy_utils import compute_hier_from | ||
| from .hierarchy.hierarchy_utils import flat_finest_field |
There was a problem hiding this comment.
Address cyclic imports to maintain clean architecture.
Consider refactoring the import structure to avoid cyclic dependencies, which can lead to initialization problems and make the codebase harder to maintain.
Tools
GitHub Check: CodeQL
[notice] 5-5: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
There was a problem hiding this comment.
Actionable comments posted: 12
Outside diff range and nitpick comments (3)
pyphare/pyphare/pharesee/hierarchy/fromh5.py (2)
8-8: Address cyclic imports to maintain clean architecture.Consider refactoring the import structure to avoid cyclic dependencies, which can lead to initialization problems and make the codebase harder to maintain.
Tools
GitHub Check: CodeQL
[notice] 8-8: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy begins an import cycle.
12-12: Address cyclic imports to maintain clean architecture.Consider refactoring the import structure to avoid cyclic dependencies, which can lead to initialization problems and make the codebase harder to maintain.
Tools
GitHub Check: CodeQL
[notice] 12-12: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.pyphare/pyphare/pharesee/run.py (1)
4-5: Address cyclic imports to maintain clean architecture.Consider refactoring the import structure to avoid cyclic dependencies, which can lead to initialization problems and make the codebase harder to maintain.
Tools
GitHub Check: CodeQL
[notice] 4-4: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
[notice] 5-5: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- pyphare/pyphare/pharesee/hierarchy/fromh5.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1 hunks)
- pyphare/pyphare/pharesee/run.py (6 hunks)
Additional context used
Ruff
pyphare/pyphare/pharesee/hierarchy/fromh5.py
1-1:
osimported but unused (F401)
149-149: Redefinition of unused
osfrom line 1 (F811)pyphare/pyphare/pharesee/run.py
378-378: f-string without any placeholders (F541)
380-380: f-string without any placeholders (F541)
GitHub Check: CodeQL
pyphare/pyphare/pharesee/hierarchy/fromh5.py
[notice] 8-8: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy begins an import cycle.
[notice] 12-12: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
[notice] 149-149: Module is imported more than once
This import of module os is redundant, as it was previously imported on line 1.
[notice] 1-1: Unused import
Import of 'os' is not used.pyphare/pyphare/pharesee/run.py
[notice] 4-4: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
[notice] 5-5: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.pyphare/pyphare/pharesee/hierarchy/hierarchy.py
[notice] 552-552: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
[warning] 138-138: Comparison of identical values
Comparison of identical values; use cmath.isnan() if testing for not-a-number.
[failure] 181-181: Potentially uninitialized local variable
Local variable 'slice_dim' may be used before it is initialized.
[failure] 182-182: Potentially uninitialized local variable
Local variable 'cst_dim' may be used before it is initialized.
[notice] 564-564: Cyclic import
Import of module pyphare.pharesee.plotting begins an import cycle.
[notice] 562-562: Module is imported with 'import' and 'import from'
Module 'copy' is imported with both 'import' and 'import from'.
Additional comments not posted (1)
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
27-28: Ensure consistent data types forpatch_levels.- if not isinstance(patch_levels, (tuple, list)): - patch_levels = listify(patch_levels) + patch_levels = listify(patch_levels) if not isinstance(patch_levels, (tuple, list)) else patch_levelsThis change ensures that
patch_levelsis always treated as a list or tuple, which is crucial for the operations that iterate over this variable.Likely invalid or redundant comment.
| return hier | ||
|
|
||
|
|
||
| def hierarchy_fromh5(h5_filename, time=None, hier=None, silent=True, **kwargs): |
There was a problem hiding this comment.
Ensure consistent use of return statements.
The function hierarchy_fromh5 mixes implicit and explicit returns. This can lead to unexpected behavior where the function might return None implicitly. Consider adding explicit return statements or restructuring the function to avoid this issue.
There was a problem hiding this comment.
Actionable comments posted: 10
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- pyphare/pyphare/pharesee/hierarchy/fromh5.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1 hunks)
- pyphare/pyphare/pharesee/run.py (6 hunks)
Additional context used
Ruff
pyphare/pyphare/pharesee/hierarchy/fromh5.py
1-1:
osimported but unused (F401)
149-149: Redefinition of unused
osfrom line 1 (F811)pyphare/pyphare/pharesee/run.py
378-378: f-string without any placeholders (F541)
380-380: f-string without any placeholders (F541)
GitHub Check: CodeQL
pyphare/pyphare/pharesee/hierarchy/fromh5.py
[notice] 8-8: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy begins an import cycle.
[notice] 12-12: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
[notice] 149-149: Module is imported more than once
This import of module os is redundant, as it was previously imported on line 1.
[notice] 1-1: Unused import
Import of 'os' is not used.pyphare/pyphare/pharesee/run.py
[notice] 4-4: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
[notice] 5-5: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.pyphare/pyphare/pharesee/hierarchy/hierarchy.py
[notice] 552-552: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
[warning] 138-138: Comparison of identical values
Comparison of identical values; use cmath.isnan() if testing for not-a-number.
[failure] 181-181: Potentially uninitialized local variable
Local variable 'slice_dim' may be used before it is initialized.
[failure] 182-182: Potentially uninitialized local variable
Local variable 'cst_dim' may be used before it is initialized.
[notice] 564-564: Cyclic import
Import of module pyphare.pharesee.plotting begins an import cycle.
[notice] 562-562: Module is imported with 'import' and 'import from'
Module 'copy' is imported with both 'import' and 'import from'.
Additional comments not posted (22)
pyphare/pyphare/pharesee/hierarchy/fromh5.py (14)
26-27: This function correctly identifies population fluid files based on naming conventions.
41-42: This function efficiently checks if a filename matches known particle file patterns.
49-102: This function robustly adds data from an HDF5 group to patch data, handling both particle and field data types and includes appropriate error handling.
105-107: This function correctly checks for the presence of datasets in an HDF5 group.
114-123: This function correctly retrieves and sorts time values from an HDF5 file, with options to return them as floats or strings, and ensures the file is closed properly.
128-129: This function correctly checks if bothtimeandhierareNone.
132-133: This function correctly checks iftimesis notNoneandhierisNone.
136-137: This function correctly checks iftimeisNoneandhieris notNone.
140-141: This function correctly checks if bothtimeandhierare notNone.
144-201: This function effectively creates a dictionary ofPatchLevelobjects from an HDF5 file, handling selection boxes and intersections correctly.Tools
Ruff
149-149: Redefinition of unused
osfrom line 1 (F811)GitHub Check: CodeQL
[notice] 149-149: Module is imported more than once
This import of module os is redundant, as it was previously imported on line 1.
204-218: This function correctly adds time data to aPatchHierarchyfrom an HDF5 file, handling selection boxes and using keyword arguments effectively.
221-244: This function correctly adds newPatchDataobjects to an existingPatchHierarchyfor a specified time, including appropriate error handling for non-existent times.
247-277: This function effectively creates a newPatchHierarchyfrom an HDF5 file for specified times, handling optional selection boxes and managing domain boxes correctly.
280-307: This function effectively creates or updates aPatchHierarchyfrom an HDF5 file, handling multiple scenarios for time and hierarchy management.pyphare/pyphare/pharesee/run.py (8)
288-297: This method correctly retrieves a hierarchy based on provided times and filename, handling different scenarios for single and multiple hierarchies effectively.
Line range hint
305-314: This method correctly retrieves data from a hierarchy, offering options to merge data and handle interpolation settings effectively.
331-332: This method correctly retrieves tags from a hierarchy based on a specified time and interpolation settings.
335-336: This method correctly retrieves magnetic field data from a hierarchy based on a specified time and interpolation settings.
339-340: This method correctly retrieves electric field data from a hierarchy based on a specified time and interpolation settings.
343-344: This method correctly retrieves mass density data from a hierarchy based on a specified time and interpolation settings.
347-348: This method correctly retrieves ion density data from a hierarchy based on a specified time and interpolation settings.
351-352: This method correctly retrieves population-specific density data from a hierarchy based on a specified time and interpolation settings.
| if not isinstance(times, (tuple, list)): | ||
| times = listify(times) |
There was a problem hiding this comment.
Ensure consistent data types for times.
- if not isinstance(times, (tuple, list)):
- times = listify(times)
+ assert isinstance(times, (tuple, list)), "Expected 'times' to be a tuple or list"This change ensures that times are always treated as lists or tuples, which is crucial for the operations that iterate over these variables.
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.
| if not isinstance(times, (tuple, list)): | |
| times = listify(times) | |
| assert isinstance(times, (tuple, list)), "Expected 'times' to be a tuple or list" |
| if not isinstance(patch_levels, (tuple, list)): | ||
| patch_levels = listify(patch_levels) |
There was a problem hiding this comment.
Ensure consistent data types for patch_levels.
- if not isinstance(patch_levels, (tuple, list)):
- patch_levels = listify(patch_levels)
+ assert isinstance(patch_levels, (tuple, list)), "Expected 'patch_levels' to be a tuple or list"This change ensures that patch_levels are always treated as lists or tuples, which is crucial for the operations that iterate over these variables.
Committable suggestion was skipped due to low confidence.
| def make_layout(h5_patch_grp, cell_width, interp_order): | ||
| origin = h5_patch_grp.attrs["origin"] | ||
| upper = h5_patch_grp.attrs["upper"] | ||
| lower = h5_patch_grp.attrs["lower"] | ||
| return GridLayout(Box(lower, upper), origin, cell_width, interp_order=interp_order) |
There was a problem hiding this comment.
Consider adding error handling for missing attributes in h5_patch_grp.
Would you like me to help draft the error handling code for this function?
| def h5_filename_from(diagInfo): | ||
| # diagInfo.quantity starts with a / , hence [1:] | ||
| return (diagInfo.quantity + ".h5").replace("/", "_")[1:] | ||
|
|
There was a problem hiding this comment.
Validate the format of diagInfo.quantity to ensure it starts with a '/' as expected.
+ if not diagInfo.quantity.startswith('/'):
+ raise ValueError("Expected 'diagInfo.quantity' to start with '/'")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.
| def h5_filename_from(diagInfo): | |
| # diagInfo.quantity starts with a / , hence [1:] | |
| return (diagInfo.quantity + ".h5").replace("/", "_")[1:] | |
| def h5_filename_from(diagInfo): | |
| if not diagInfo.quantity.startswith('/'): | |
| raise ValueError("Expected 'diagInfo.quantity' to start with '/'") | |
| # diagInfo.quantity starts with a / , hence [1:] | |
| return (diagInfo.quantity + ".h5").replace("/", "_")[1:] |
| def pop_name(basename): | ||
| return basename.strip(".h5").split("_")[2] |
There was a problem hiding this comment.
Validate the format of basename to ensure it contains enough parts to extract the population name.
+ if len(basename.strip(".h5").split("_")) < 3:
+ raise ValueError("Basename format is incorrect")Committable suggestion was skipped due to low confidence.
| def particle_dataset_name(basename): | ||
| """ | ||
| return "alpha_domain" from ions_pop_alpha_domain.h5 | ||
| """ | ||
| popname = basename.strip(".h5").split("_")[-2] | ||
| part_type = basename.strip(".h5").split("_")[-1] | ||
| dataset_name = popname + "_" + part_type | ||
|
|
||
| return dataset_name |
There was a problem hiding this comment.
Validate the format of basename to prevent index errors.
+ if len(basename.strip(".h5").split("_")) < 3:
+ raise ValueError("Basename format is incorrect")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.
| def particle_dataset_name(basename): | |
| """ | |
| return "alpha_domain" from ions_pop_alpha_domain.h5 | |
| """ | |
| popname = basename.strip(".h5").split("_")[-2] | |
| part_type = basename.strip(".h5").split("_")[-1] | |
| dataset_name = popname + "_" + part_type | |
| return dataset_name | |
| def particle_dataset_name(basename): | |
| """ | |
| return "alpha_domain" from ions_pop_alpha_domain.h5 | |
| """ | |
| if len(basename.strip(".h5").split("_")) < 3: | |
| raise ValueError("Basename format is incorrect") | |
| popname = basename.strip(".h5").split("_")[-2] | |
| part_type = basename.strip(".h5").split("_")[-1] | |
| dataset_name = popname + "_" + part_type | |
| return dataset_name |
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (6)
tests/simulator/refinement/test_2d_2_core.py (2)
Line range hint
155-155: Module level imports should be at the top of the file to improve readability and maintainability.+from pyphare.cpp import cpp_lib +from tests.simulator.test_advance import AdvanceTestBaseMove these imports to the top of the file.
Also applies to: 157-157
Line range hint
172-172: The variableL0_datahieris assigned but never used. Consider removing it if it's not needed.- L0_datahier = get_hier(L0_diags)tests/simulator/test_advance.py (4)
Line range hint
5-25: Module level imports should be at the top of the file. This helps in understanding the dependencies of the module at a glance and is a common Python best practice.+from pyphare.cpp import cpp_lib +import unittest +import numpy as np +import pyphare.core.box as boxm +from ddt import ddt +from pyphare.core.box import Box +from pyphare.core.phare_utilities import assert_fp_any_all_close, np_array_ify +from pyphare.pharein import ElectronModel, MaxwellianFluidModel +from pyphare.pharein.diagnostics import ( + ElectromagDiagnostics, + FluidDiagnostics, + ParticleDiagnostics, +) +from pyphare.pharein.simulation import Simulation +from pyphare.pharesee.geometry import hierarchy_overlaps, level_ghost_boxes +from pyphare.pharesee.hierarchy.hierarchy_utils import merge_particles +from pyphare.simulator.simulator import Simulator +from tests.diagnostic import all_timestamps +from tests.simulator import SimulatorTest, diff_boxesMove these imports to the top of the file.
Tools
Ruff
18-18: Module level import not at top of file (E402)
19-19: Module level import not at top of file (E402)
20-20: Module level import not at top of file (E402)
21-21: Module level import not at top of file (E402)
22-22: Module level import not at top of file (E402)
24-24: Module level import not at top of file (E402)
Line range hint
34-34: Replace lambda expressions with function definitions for better readability and potential reuse.- _ = lambda i: -((xyz[i] - hL[i]) ** 2) + def calculate_negative_square_difference(index, values, half_lengths): + return -((values[index] - half_lengths[index]) ** 2)Use this function instead of the lambda expression.
Also applies to: 93-93, 98-98, 103-103, 108-108, 113-113
Tools
Ruff
18-18: Module level import not at top of file (E402)
19-19: Module level import not at top of file (E402)
20-20: Module level import not at top of file (E402)
21-21: Module level import not at top of file (E402)
22-22: Module level import not at top of file (E402)
24-24: Module level import not at top of file (E402)
Line range hint
732-732: The f-strings here do not contain any placeholders, which makes them unnecessary. Consider using regular strings instead.- print(f"test_field_coarsening_via_subcycles for dim/interp : {}/{}") + print("test_field_coarsening_via_subcycles for dim/interp : {}/{}")Also applies to: 733-733
Tools
Ruff
18-18: Module level import not at top of file (E402)
19-19: Module level import not at top of file (E402)
20-20: Module level import not at top of file (E402)
21-21: Module level import not at top of file (E402)
22-22: Module level import not at top of file (E402)
24-24: Module level import not at top of file (E402)
Line range hint
761-761: The importpyphare.phareinis unused, and the local variableoutis assigned but never used. Consider removing them if they are not needed.- import pyphare.pharein as ph - out = "domain_particles"Also applies to: 766-766
Tools
Ruff
18-18: Module level import not at top of file (E402)
19-19: Module level import not at top of file (E402)
20-20: Module level import not at top of file (E402)
21-21: Module level import not at top of file (E402)
22-22: Module level import not at top of file (E402)
24-24: Module level import not at top of file (E402)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- tests/simulator/refinement/test_2d_2_core.py (2 hunks)
- tests/simulator/test_advance.py (3 hunks)
Additional context used
Ruff
tests/simulator/refinement/test_2d_2_core.py
155-155: Module level import not at top of file (E402)
157-157: Module level import not at top of file (E402)
172-172: Local variable
L0_datahieris assigned to but never used (F841)tests/simulator/test_advance.py
5-5: Module level import not at top of file (E402)
7-7: Module level import not at top of file (E402)
8-8: Module level import not at top of file (E402)
9-9: Module level import not at top of file (E402)
10-10: Module level import not at top of file (E402)
11-11: Module level import not at top of file (E402)
12-12: Module level import not at top of file (E402)
13-17: Module level import not at top of file (E402)
18-18: Module level import not at top of file (E402)
19-19: Module level import not at top of file (E402)
20-20: Module level import not at top of file (E402)
21-21: Module level import not at top of file (E402)
22-22: Module level import not at top of file (E402)
24-24: Module level import not at top of file (E402)
25-25: Module level import not at top of file (E402)
34-34: Do not assign a
lambdaexpression, use adef(E731)
93-93: Do not assign a
lambdaexpression, use adef(E731)
98-98: Do not assign a
lambdaexpression, use adef(E731)
103-103: Do not assign a
lambdaexpression, use adef(E731)
108-108: Do not assign a
lambdaexpression, use adef(E731)
113-113: Do not assign a
lambdaexpression, use adef(E731)
732-732: f-string without any placeholders (F541)
733-733: f-string without any placeholders (F541)
761-761:
pyphare.phareinimported but unused (F401)
766-766: Local variable
outis assigned to but never used (F841)
Additional comments not posted (1)
tests/simulator/refinement/test_2d_2_core.py (1)
191-191: Ensure proper type casting forrandom.randintto avoid potential type mismatches.Verification successful
The instances of
random.randintin the following files are already properly type casted usingint(1e10):
tests/simulator/test_advance.pytests/simulator/refinement/test_2d_2_core.pytests/simulator/refinement/test_2d_10_core.pyThe instance in
pyphare/pyphare/pharesee/particles.pyusesnp.random.randint, which is a different function and does not require the same type casting.No further action is needed regarding type casting for
random.randint.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the random integer generation is properly type casted. # Test: Search for the usage of `random.randint`. Expect: Proper type casting. rg --type python $'random.randint'Length of output: 64
Script:
#!/bin/bash # Description: Verify that the random integer generation is properly type casted. # Test: Search for the usage of `random.randint`. Expect: Proper type casting. rg --type py 'random.randint'Length of output: 362
| datahier = hierarchy_from(h5_filename=path + "/EM_E.h5", times=time, hier=datahier) | ||
| datahier = hierarchy_from(h5_filename=path + "/EM_B.h5", times=time, hier=datahier) |
There was a problem hiding this comment.
The function hierarchy_from is called twice with the same parameters except for the filename. Consider refactoring to avoid code duplication.
- datahier = hierarchy_from(h5_filename=path + "/EM_E.h5", times=time, hier=datahier)
- datahier = hierarchy_from(h5_filename=path + "/EM_B.h5", times=time, hier=datahier)
+ for filename in ["/EM_E.h5", "/EM_B.h5"]:
+ datahier = hierarchy_from(h5_filename=path + filename, times=time, hier=datahier)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.
| datahier = hierarchy_from(h5_filename=path + "/EM_E.h5", times=time, hier=datahier) | |
| datahier = hierarchy_from(h5_filename=path + "/EM_B.h5", times=time, hier=datahier) | |
| for filename in ["/EM_E.h5", "/EM_B.h5"]: | |
| datahier = hierarchy_from(h5_filename=path + filename, times=time, hier=datahier) |
| from pyphare.pharesee.hierarchy import hierarchy_from | ||
| from pyphare.pharesee.hierarchy.hierarchy_utils import merge_particles |
There was a problem hiding this comment.
The import of hierarchy_from is no longer needed due to the refactoring in hierarchy.py. Ensure that this import is removed if it's not used elsewhere in the file.
- from pyphare.pharesee.hierarchy import hierarchy_fromCommittable 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.
| from pyphare.pharesee.hierarchy import hierarchy_from | |
| from pyphare.pharesee.hierarchy.hierarchy_utils import merge_particles | |
| from pyphare.pharesee.hierarchy.hierarchy_utils import merge_particles |
Tools
Ruff
20-20: Module level import not at top of file (E402)
21-21: Module level import not at top of file (E402)
| r = Run(diag_outputs) | ||
| time = 0.0 | ||
| B = r.GetB(time) | ||
| self.assertTrue(B.nbytes() > 0) |
Check notice
Code scanning / CodeQL
Imprecise assert Note
| r = Run(diag_outputs) | ||
| time = 0.0 | ||
| B = r.GetB(time) | ||
| self.assertTrue(B.nbrPatches() > 0) |
Check notice
Code scanning / CodeQL
Imprecise assert Note
| fig.savefig(f"{self.id()}.png") | ||
|
|
||
| self.assertTrue(isinstance(Bpartial, PatchHierarchy)) | ||
| self.assertTrue(Bpartial.nbrPatches() < B.nbrPatches()) |
Check notice
Code scanning / CodeQL
Imprecise assert Note
| from .patchlevel import PatchLevel | ||
| from .patchdata import FieldData, ParticleData | ||
| from ..particles import Particles | ||
| from .hierarchy import PatchHierarchy |
Check notice
Code scanning / CodeQL
Cyclic import Note
| @@ -0,0 +1,233 @@ | |||
| import unittest | |||
| from ddt import ddt, data, unpack | |||
Check notice
Code scanning / CodeQL
Unused import Note
| @classmethod | ||
| def setUpClass(cls): | ||
| from pyphare.pharein import global_vars | ||
| import pyphare.pharein as ph |
Check notice
Code scanning / CodeQL
Module is imported with 'import' and 'import from' Note
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range and nitpick comments (3)
pyphare/pyphare/pharesee/hierarchy/fromh5.py (1)
8-8: Address cyclic imports to maintain a clean architecture. Consider refactoring the import structure to avoid cyclic dependencies, which can lead to initialization problems and make the codebase harder to maintain.Also applies to: 17-17
Tools
GitHub Check: CodeQL
[notice] 8-8: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy begins an import cycle.pyphare/pyphare/pharesee/run.py (2)
Line range hint
432-448: Optimize file operations and add error handling inGetMass.Consider using a context manager for file operations to ensure files are properly closed. Also, add error handling to manage exceptions that may occur during file operations.
- h5_file = h5py.File(file, "r") + with h5py.File(file, "r") as h5_file:Tools
Ruff
391-391: f-string without any placeholders (F541)
393-393: f-string without any placeholders (F541)
Line range hint
491-509: Improve error handling and logging inGetAllAvailableQties.Enhance the error handling by logging the specific function and file that caused the FileNotFoundError. This will help in debugging and maintaining the code.
- logger.debug(f"No file for function {fn.__name__}") + logger.error(f"FileNotFoundError for {fn.__name__} with args {args} and kwargs {kwargs}")
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- pyphare/pyphare/pharesee/hierarchy/fromh5.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1 hunks)
- pyphare/pyphare/pharesee/run.py (7 hunks)
- pyphare/pyphare_tests/test_pharesee/CMakeLists.txt (1 hunks)
- pyphare/pyphare_tests/test_pharesee/test_hierarchy.py (1 hunks)
Additional context used
Ruff
pyphare/pyphare_tests/test_pharesee/test_hierarchy.py
2-2:
ddt.dataimported but unused (F401)
2-2:
ddt.unpackimported but unused (F401)
53-53: Ambiguous variable name:
l(E741)pyphare/pyphare/pharesee/hierarchy/fromh5.py
1-1:
osimported but unused (F401)
154-154: Redefinition of unused
osfrom line 1 (F811)pyphare/pyphare/pharesee/run.py
391-391: f-string without any placeholders (F541)
393-393: f-string without any placeholders (F541)
GitHub Check: CodeQL
pyphare/pyphare_tests/test_pharesee/test_hierarchy.py
[notice] 186-186: Imprecise assert
assertTrue(a > b) cannot provide an informative message. Using assertGreater(a, b) instead will give more informative messages.
[notice] 192-192: Imprecise assert
assertTrue(a > b) cannot provide an informative message. Using assertGreater(a, b) instead will give more informative messages.
[notice] 229-229: Imprecise assert
assertTrue(a < b) cannot provide an informative message. Using assertLess(a, b) instead will give more informative messages.
[notice] 2-2: Unused import
Import of 'data' is not used.
Import of 'unpack' is not used.
[notice] 25-25: Module is imported with 'import' and 'import from'
Module 'pyphare.pharein' is imported with both 'import' and 'import from'.pyphare/pyphare/pharesee/hierarchy/fromh5.py
[notice] 17-17: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
[notice] 154-154: Module is imported more than once
This import of module os is redundant, as it was previously imported on line 1.
[notice] 1-1: Unused import
Import of 'os' is not used.
[notice] 8-8: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy begins an import cycle.pyphare/pyphare/pharesee/run.py
[notice] 4-4: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
[notice] 5-5: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.pyphare/pyphare/pharesee/hierarchy/hierarchy.py
[notice] 552-552: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
[warning] 138-138: Comparison of identical values
Comparison of identical values; use cmath.isnan() if testing for not-a-number.
[failure] 181-181: Potentially uninitialized local variable
Local variable 'slice_dim' may be used before it is initialized.
[failure] 182-182: Potentially uninitialized local variable
Local variable 'cst_dim' may be used before it is initialized.
[notice] 564-564: Cyclic import
Import of module pyphare.pharesee.plotting begins an import cycle.
[notice] 562-562: Module is imported with 'import' and 'import from'
Module 'copy' is imported with both 'import' and 'import from'.
Additional comments not posted (8)
pyphare/pyphare_tests/test_pharesee/CMakeLists.txt (1)
8-8: The addition of the MPI-based test configuration looks correct and aligns with the existing test setup.pyphare/pyphare/pharesee/run.py (1)
294-310: LGTM! The changes to support multiple times and selection boxes align with the PR objectives.pyphare/pyphare/pharesee/hierarchy/hierarchy.py (6)
24-25: Ensure consistent data types fortimesandpatch_levels.Also applies to: 27-28
30-38: Validateselection_boxparameter in the constructor.
69-89: Complex logic in methodupdate. Consider refactoring into a separate method for clarity.
128-195: Complex method__call__. Consider breaking down into smaller, more manageable methods.Tools
GitHub Check: CodeQL
[warning] 138-138: Comparison of identical values
Comparison of identical values; use cmath.isnan() if testing for not-a-number.
[failure] 181-181: Potentially uninitialized local variable
Local variable 'slice_dim' may be used before it is initialized.
[failure] 182-182: Potentially uninitialized local variable
Local variable 'cst_dim' may be used before it is initialized.
552-552: Ensure consistent use of return statements.Tools
GitHub Check: CodeQL
[notice] 552-552: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
562-564: Address cyclic imports to maintain clean architecture.Tools
GitHub Check: CodeQL
[notice] 564-564: Cyclic import
Import of module pyphare.pharesee.plotting begins an import cycle.
[notice] 562-562: Module is imported with 'import' and 'import from'
Module 'copy' is imported with both 'import' and 'import from'.
| + 1.0 / np.cosh((y - L * 0.7) / 0.5) ** 2 | ||
| ) | ||
|
|
||
| def S(y, y0, l): |
There was a problem hiding this comment.
Consider renaming the variable l to length to avoid ambiguity with the number 1.
- def S(y, y0, l):
+ def S(y, y0, length):Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def S(y, y0, l): | |
| def S(y, y0, length): |
Tools
Ruff
53-53: Ambiguous variable name:
l(E741)
| fig.savefig(f"{self.id()}.png") | ||
|
|
||
| self.assertTrue(isinstance(Bpartial, PatchHierarchy)) | ||
| self.assertTrue(Bpartial.nbrPatches() < B.nbrPatches()) |
There was a problem hiding this comment.
Replace assertTrue with assertLess for a more informative assertion message.
- self.assertTrue(Bpartial.nbrPatches() < B.nbrPatches())
+ self.assertLess(Bpartial.nbrPatches(), B.nbrPatches())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.
| self.assertTrue(Bpartial.nbrPatches() < B.nbrPatches()) | |
| self.assertLess(Bpartial.nbrPatches(), B.nbrPatches()) |
Tools
GitHub Check: CodeQL
[notice] 229-229: Imprecise assert
assertTrue(a < b) cannot provide an informative message. Using assertLess(a, b) instead will give more informative messages.
| r = Run(diag_outputs) | ||
| time = 0.0 | ||
| B = r.GetB(time) | ||
| self.assertTrue(B.nbrPatches() > 0) |
There was a problem hiding this comment.
Replace assertTrue with assertGreater for a more informative assertion message.
- self.assertTrue(B.nbrPatches() > 0)
+ self.assertGreater(B.nbrPatches(), 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.
| self.assertTrue(B.nbrPatches() > 0) | |
| self.assertGreater(B.nbrPatches(), 0) |
Tools
GitHub Check: CodeQL
[notice] 192-192: Imprecise assert
assertTrue(a > b) cannot provide an informative message. Using assertGreater(a, b) instead will give more informative messages.
| creates a dictionary of PatchLevels from a given time in a h5 file | ||
| {ilvl: PatchLevel} | ||
| """ | ||
| import os |
There was a problem hiding this comment.
Remove the redundant import of os as it is already imported at the beginning of the file.
- import osCommittable 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.
| import os |
Tools
Ruff
154-154: Redefinition of unused
osfrom line 1 (F811)
GitHub Check: CodeQL
[notice] 154-154: Module is imported more than once
This import of module os is redundant, as it was previously imported on line 1.
| from ...core.phare_utilities import ( | ||
| refinement_ratio, | ||
| none_iterable, | ||
| all_iterables, | ||
| listify, | ||
| ) |
Check notice
Code scanning / CodeQL
Unused import Note
There was a problem hiding this comment.
Actionable comments posted: 15
Outside diff range and nitpick comments (2)
tests/simulator/refinement/test_2d_10_core.py (1)
Line range hint
132-132: Move module-level imports to the top of the file.+ from pyphare.cpp import cpp_lib + from tests.simulator.test_advance import AdvanceTestBase - from pyphare.cpp import cpp_lib - from tests.simulator.test_advance import AdvanceTestBaseThis change addresses the E402 error flagged by static analysis and adheres to PEP 8 guidelines.
Also applies to: 134-134
pyphare/pyphare/pharesee/hierarchy/fromh5.py (1)
17-17: Address cyclic imports to maintain clean architecture.Consider refactoring the import structure to avoid cyclic dependencies, which can lead to initialization problems and make the codebase harder to maintain.
Also applies to: 8-8
Tools
GitHub Check: CodeQL
[notice] 17-17: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- pyphare/pyphare/pharesee/hierarchy/fromh5.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1 hunks)
- pyphare/pyphare/pharesee/run.py (7 hunks)
- pyphare/pyphare_tests/test_pharesee/CMakeLists.txt (1 hunks)
- pyphare/pyphare_tests/test_pharesee/test_hierarchy.py (1 hunks)
- tests/simulator/refinement/test_2d_10_core.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- pyphare/pyphare_tests/test_pharesee/CMakeLists.txt
Additional context used
Ruff
tests/simulator/refinement/test_2d_10_core.py
132-132: Module level import not at top of file (E402)
134-134: Module level import not at top of file (E402)
pyphare/pyphare_tests/test_pharesee/test_hierarchy.py
2-2:
ddt.dataimported but unused (F401)
2-2:
ddt.unpackimported but unused (F401)
53-53: Ambiguous variable name:
l(E741)pyphare/pyphare/pharesee/hierarchy/fromh5.py
1-1:
osimported but unused (F401)
14-14:
...core.phare_utilities.listifyimported but unused (F401)
154-154: Redefinition of unused
osfrom line 1 (F811)pyphare/pyphare/pharesee/run.py
389-389: f-string without any placeholders (F541)
391-391: f-string without any placeholders (F541)
GitHub Check: CodeQL
pyphare/pyphare_tests/test_pharesee/test_hierarchy.py
[notice] 186-186: Imprecise assert
assertTrue(a > b) cannot provide an informative message. Using assertGreater(a, b) instead will give more informative messages.
[notice] 192-192: Imprecise assert
assertTrue(a > b) cannot provide an informative message. Using assertGreater(a, b) instead will give more informative messages.
[notice] 229-229: Imprecise assert
assertTrue(a < b) cannot provide an informative message. Using assertLess(a, b) instead will give more informative messages.
[notice] 2-2: Unused import
Import of 'data' is not used.
Import of 'unpack' is not used.
[notice] 25-25: Module is imported with 'import' and 'import from'
Module 'pyphare.pharein' is imported with both 'import' and 'import from'.pyphare/pyphare/pharesee/hierarchy/fromh5.py
[notice] 17-17: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
[notice] 154-154: Module is imported more than once
This import of module os is redundant, as it was previously imported on line 1.
[notice] 1-1: Unused import
Import of 'os' is not used.
[notice] 8-8: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy begins an import cycle.
[notice] 10-15: Unused import
Import of 'listify' is not used.pyphare/pyphare/pharesee/run.py
[notice] 4-4: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
[notice] 5-5: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.pyphare/pyphare/pharesee/hierarchy/hierarchy.py
[notice] 553-553: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
[warning] 138-138: Comparison of identical values
Comparison of identical values; use cmath.isnan() if testing for not-a-number.
[failure] 181-181: Potentially uninitialized local variable
Local variable 'slice_dim' may be used before it is initialized.
[failure] 182-182: Potentially uninitialized local variable
Local variable 'cst_dim' may be used before it is initialized.
[notice] 565-565: Cyclic import
Import of module pyphare.pharesee.plotting begins an import cycle.
[notice] 563-563: Module is imported with 'import' and 'import from'
Module 'copy' is imported with both 'import' and 'import from'.
Additional comments not posted (2)
tests/simulator/refinement/test_2d_10_core.py (1)
123-124: Update thehierarchy_fromfunction calls to match the new signature.The function now correctly uses the
timesparameter instead oftime, aligning with the changes described in the PR summary.pyphare/pyphare_tests/test_pharesee/test_hierarchy.py (1)
53-53: Consider renaming the variableltolengthto avoid ambiguity with the number1.- def S(y, y0, l): + def S(y, y0, length):This change addresses the E741 error flagged by static analysis and improves code readability.
Likely invalid or redundant comment.
Tools
Ruff
53-53: Ambiguous variable name:
l(E741)
| def GetParticleCount(self, time, **kwargs): | ||
| c = self._get_hierarchy(time, "particle_count.h5", **kwargs) | ||
| return c |
There was a problem hiding this comment.
Add documentation for the GetParticleCount method.
It's good practice to include docstrings for public methods to explain their purpose, parameters, and return values. Would you like me to help draft a docstring for this method?
| from .hierarchy.hierarchy_utils import compute_hier_from | ||
| from .hierarchy.hierarchy_utils import flat_finest_field |
There was a problem hiding this comment.
Address cyclic imports to maintain clean architecture.
Consider refactoring the import structure to avoid cyclic dependencies, which can lead to initialization problems and make the codebase harder to maintain.
Tools
GitHub Check: CodeQL
[notice] 4-4: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
[notice] 5-5: Cyclic import
Import of module pyphare.pharesee.hierarchy.hierarchy_utils begins an import cycle.
| massDensity = self.GetMassDensity(time) | ||
| Vi = self._get_hierarchy(time, f"ions_bulkVelocity.h5") | ||
| def GetPi(self, time, merged=False, interp="nearest", **kwargs): | ||
| M = self._get_hierarchy(time, f"ions_momentum_tensor.h5", **kwargs) |
There was a problem hiding this comment.
Remove unnecessary f-string.
- return f"ions_pop_{pop_name}_domain.h5"
+ return "ions_pop_" + pop_name + "_domain.h5"Committable suggestion was skipped due to low confidence.
Tools
Ruff
389-389: f-string without any placeholders (F541)
| def GetPi(self, time, merged=False, interp="nearest", **kwargs): | ||
| M = self._get_hierarchy(time, f"ions_momentum_tensor.h5", **kwargs) | ||
| massDensity = self.GetMassDensity(time, **kwargs) | ||
| Vi = self._get_hierarchy(time, f"ions_bulkVelocity.h5", **kwargs) |
There was a problem hiding this comment.
Remove unnecessary f-string.
- return f"ions_bulkVelocity.h5"
+ return "ions_bulkVelocity.h5"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.
| Vi = self._get_hierarchy(time, f"ions_bulkVelocity.h5", **kwargs) | |
| Vi = self._get_hierarchy(time, "ions_bulkVelocity.h5", **kwargs) |
Tools
Ruff
391-391: f-string without any placeholders (F541)
|
|
||
| return fig, ax | ||
|
|
||
| def plot(self, **kwargs): |
There was a problem hiding this comment.
Ensure consistent use of return statements.
The method plot mixes implicit and explicit returns. This can lead to unexpected behavior where the method might return None implicitly. Consider adding explicit return statements or restructuring the method to avoid this issue.
Tools
GitHub Check: CodeQL
[notice] 553-553: Explicit returns mixed with implicit (fall through) returns
Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
| @@ -0,0 +1,233 @@ | |||
| import unittest | |||
| from ddt import ddt, data, unpack | |||
There was a problem hiding this comment.
Remove unused imports to clean up the code.
- from ddt import ddt, data, unpack
+ from ddt import ddtThis change addresses the F401 error flagged by static analysis and removes unused imports, improving code cleanliness.
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.
| from ddt import ddt, data, unpack | |
| from ddt import ddt |
Tools
Ruff
2-2:
ddt.dataimported but unused (F401)
2-2:
ddt.unpackimported but unused (F401)
GitHub Check: CodeQL
[notice] 2-2: Unused import
Import of 'data' is not used.
Import of 'unpack' is not used.
| if not isinstance(times, (tuple, list)): | ||
| times = listify(times) | ||
|
|
||
| if not isinstance(patch_levels, (tuple, list)): | ||
| patch_levels = listify(patch_levels) |
There was a problem hiding this comment.
Ensure consistent data types for times and patch_levels.
- if not isinstance(times, (tuple, list)):
- times = listify(times)
- if not isinstance(patch_levels, (tuple, list)):
- patch_levels = listify(patch_levels)
+ assert isinstance(times, (tuple, list)), "Expected 'times' to be a tuple or list"
+ assert isinstance(patch_levels, (tuple, list)), "Expected 'patch_levels' to be a tuple or list"This change ensures that times and patch_levels are always treated as lists or tuples, which is crucial for the operations that iterate over these variables.
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.
| if not isinstance(times, (tuple, list)): | |
| times = listify(times) | |
| if not isinstance(patch_levels, (tuple, list)): | |
| patch_levels = listify(patch_levels) | |
| assert isinstance(times, (tuple, list)), "Expected 'times' to be a tuple or list" | |
| assert isinstance(patch_levels, (tuple, list)), "Expected 'patch_levels' to be a tuple or list" |
| import copy | ||
|
|
||
| from ..plotting import dist_plot as dp |
There was a problem hiding this comment.
Address cyclic imports to maintain clean architecture.
- from ..plotting import dist_plot as dp
+ # Consider restructuring imports or using lazy loading to resolve cyclic dependenciesCommittable suggestion was skipped due to low confidence.
Tools
GitHub Check: CodeQL
[notice] 565-565: Cyclic import
Import of module pyphare.pharesee.plotting begins an import cycle.
[notice] 563-563: Module is imported with 'import' and 'import from'
Module 'copy' is imported with both 'import' and 'import from'.
| refinement_ratio, | ||
| none_iterable, | ||
| all_iterables, | ||
| listify, |
There was a problem hiding this comment.
Remove the unused import of listify.
- from ...core.phare_utilities import (
- refinement_ratio,
- none_iterable,
- all_iterables,
- listify,
- )
+ from ...core.phare_utilities import (
+ refinement_ratio,
+ none_iterable,
+ all_iterables,
+ )This change addresses the unused import issue flagged by static analysis and removes an unused import, improving code cleanliness.
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.
| listify, | |
| from ...core.phare_utilities import ( | |
| refinement_ratio, | |
| none_iterable, | |
| all_iterables, | |
| ) |
Tools
Ruff
14-14:
...core.phare_utilities.listifyimported but unused (F401)
| @@ -0,0 +1,316 @@ | |||
| import os | |||
There was a problem hiding this comment.
Remove redundant and unused imports of os.
- import osThis change addresses the F401 and F811 errors flagged by static analysis and removes redundant imports, improving code cleanliness.
Also applies to: 154-154
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.
| import os |
Tools
Ruff
1-1:
osimported but unused (F401)
GitHub Check: CodeQL
[notice] 1-1: Unused import
Import of 'os' is not used.
run particle count
refactoring hierarchy.py in separate files
partial Patch Hierarchies with selection box
refactoring of hierarchy_fromh5 into separate functions
run.Getters can be given a list of times
hierarchy.patch_levels not used anymore
PatchHierarchy unit tests
Also some refractory to split hierarchy.py in different files.
Summary by CodeRabbit
New Features
PatchHierarchyclass for managing hierarchical patch data.Bug Fixes
Tests
PatchHierarchyclass.test-pharesee-hierarchywith MPI support.