Load balancer #807
Load balancer #807
Conversation
|
Warning Rate limit exceeded@PhilipDeegan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 15 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughWalkthroughThe update introduces significant enhancements to the PyPHARE project, notably improving load balancing strategies, diagnostic capabilities, and simulation tests. Key features include new functions for floating-point precision comparisons, streamlined logging, and support for both C and C++ languages. These modifications enhance the efficiency and robustness of the project, particularly in adaptive mesh refinement (AMR) and simulation management. Changes
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 Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
PhilipDeegan
force-pushed
the
loadBalancer
branch
from
aa39144 to
c42b4cd
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
src/amr/load_balancing/concrete_load_balancer_hybrid_strategy_homogeneous.hpp
Fixed
Show fixed
Hide fixed
src/amr/load_balancing/concrete_load_balancer_hybrid_strategy_homogeneous.hpp
Fixed
Show fixed
Hide fixed
src/amr/load_balancing/concrete_load_balancer_hybrid_strategy_nppc.hpp
Outdated
Show resolved
Hide resolved
src/amr/load_balancing/concrete_load_balancer_hybrid_strategy_homogeneous.hpp
Outdated
Show resolved
Hide resolved
src/amr/load_balancing/concrete_load_balancer_hybrid_strategy_homogeneous.hpp
Outdated
Show resolved
Hide resolved
src/amr/load_balancing/concrete_load_balancer_hybrid_strategy_nppc.hpp
Outdated
Show resolved
Hide resolved
PhilipDeegan
force-pushed
the
loadBalancer
branch
from
c42b4cd to
b8c6324
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
PhilipDeegan
force-pushed
the
loadBalancer
branch
from
680df48 to
c80b8b0
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
PhilipDeegan
force-pushed
the
loadBalancer
branch
from
7def490 to
8fd1512
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
PhilipDeegan
force-pushed
the
loadBalancer
branch
from
8fd1512 to
d301826
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
PhilipDeegan
force-pushed
the
loadBalancer
branch
from
dd0fe18 to
a8dc16a
Compare
PhilipDeegan
force-pushed
the
loadBalancer
branch
2 times, most recently
from
c1cef36 to
c911d4d
Compare
PhilipDeegan
force-pushed
the
loadBalancer
branch
2 times, most recently
from
749286a to
d24d498
Compare
There was a problem hiding this comment.
Actionable comments posted: 28
Out of diff range and nitpick comments (10)
tests/diagnostic/__init__.py (1)
Line range hint
16-16: Remove the unused import ofnumpy.- import pyphare.pharein as ph, numpy as np + import pyphare.pharein as phpyphare/pyphare/simulator/simulator.py (3)
Line range hint
110-110: Avoid using bareexceptstatements. Specify the exception types to catch.- except: + except Exception:
Line range hint
133-133: Avoid using bareexceptstatements. Specify the exception types to catch.- except: + except Exception:
Line range hint
164-164: Useis not Nonefor comparison to follow Python best practices.- if self._auto_dump() and self.post_advance != None: + if self._auto_dump() and self.post_advance is not None:pyphare/pyphare/pharein/maxwellian_fluid_model.py (1)
Line range hint
4-4: Remove the unused importyee_element_is_primalto clean up the code.- from pyphare.core.gridlayout import GridLayout, yee_element_is_primal + from pyphare.core.gridlayout import GridLayouttests/simulator/__init__.py (3)
Line range hint
77-77: Avoid assigning alambdaexpression directly to a variable. Use adefstatement instead for better readability and debugging.- _ = lambda i: 0.1 * np.cos(2 * np.pi * xyz[i] / L[i]) + def _(i): + return 0.1 * np.cos(2 * np.pi * xyz[i] / L[i])
Line range hint
178-178: TheBoxclass is used but not defined or imported. Ensure thatBoxis defined or imported correctly.+ from some_module import BoxAlso applies to: 184-184, 191-191
Line range hint
77-77: Thetime_step_nbrparameter is no longer used and should be removed if it's not required for backward compatibility or other reasons.- "time_step_nbr": 1000,pyphare/pyphare/pharein/__init__.py (1)
Line range hint
32-47: Move all import statements to the top of the file and remove unused imports to follow Python's best practices.- from .uniform_model import UniformModel - from .maxwellian_fluid_model import MaxwellianFluidModel - from .electron_model import ElectronModel - from .diagnostics import ( - FluidDiagnostics, - ElectromagDiagnostics, - ParticleDiagnostics, - MetaDiagnostics, - InfoDiagnostics, - ) - from .simulation import ( - Simulation, - serialize as serialize_sim, - deserialize as deserialize_sim, - ) - from .load_balancer import LoadBalancer + # Move all imports to the top of the file + from .uniform_model import UniformModel + from .maxwellian_fluid_model import MaxwellianFluidModel + from .electron_model import ElectronModel + from .diagnostics import ( + FluidDiagnostics, + ElectromagDiagnostics, + ParticleDiagnostics, + MetaDiagnostics, + InfoDiagnostics, + ) + from .simulation import ( + Simulation, + serialize as serialize_sim, + deserialize as deserialize_sim, + ) + from .load_balancer import LoadBalancerpyphare/pyphare/pharein/simulation.py (1)
630-630: Update documentation to includeloadbalancing.The
wrapperfunction now acceptsloadbalancingas a keyword. Consider updating the documentation to reflect this change and provide examples of how to use this new option.
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (41)
- pyphare/pyphare/pharein/init.py (5 hunks)
- pyphare/pyphare/pharein/load_balancer.py (1 hunks)
- pyphare/pyphare/pharein/maxwellian_fluid_model.py (2 hunks)
- pyphare/pyphare/pharein/simulation.py (7 hunks)
- pyphare/pyphare/pharesee/hierarchy.py (6 hunks)
- pyphare/pyphare/pharesee/particles.py (1 hunks)
- pyphare/pyphare/simulator/simulator.py (5 hunks)
- pyphare/pyphare_tests/test_pharesee/test_geometry_3d.py (1 hunks)
- res/cmake/def.cmake (4 hunks)
- res/cmake/dep/samrai.cmake (1 hunks)
- src/amr/CMakeLists.txt (2 hunks)
- src/amr/data/particles/particles_data.hpp (1 hunks)
- src/amr/level_initializer/hybrid_level_initializer.hpp (2 hunks)
- src/amr/load_balancing/concrete_load_balancer_hybrid_strategy_homogeneous.hpp (1 hunks)
- src/amr/load_balancing/concrete_load_balancer_hybrid_strategy_nppc.hpp (1 hunks)
- src/amr/load_balancing/load_balancer_details.hpp (1 hunks)
- src/amr/load_balancing/load_balancer_estimator.hpp (1 hunks)
- src/amr/load_balancing/load_balancer_estimator_hybrid.hpp (1 hunks)
- src/amr/load_balancing/load_balancer_hybrid_strategy.hpp (1 hunks)
- src/amr/load_balancing/load_balancer_hybrid_strategy_factory.hpp (1 hunks)
- src/amr/load_balancing/load_balancer_manager.hpp (1 hunks)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (11 hunks)
- src/amr/multiphysics_integrator.hpp (13 hunks)
- src/amr/wrappers/integrator.hpp (4 hunks)
- src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp (6 hunks)
- src/core/data/ions/particle_initializers/particle_initializer_factory.hpp (2 hunks)
- src/core/data/ndarray/ndarray_vector.hpp (1 hunks)
- src/core/data/particles/particle_array.hpp (1 hunks)
- src/core/utilities/box/box.hpp (1 hunks)
- src/initializer/data_provider.hpp (1 hunks)
- src/initializer/dictator.cpp (1 hunks)
- src/simulator/simulator.hpp (6 hunks)
- tests/core/data/ions/test_ions.cpp (1 hunks)
- tests/diagnostic/init.py (1 hunks)
- tests/functional/harris/harris_2d_lb.py (1 hunks)
- tests/simulator/CMakeLists.txt (1 hunks)
- tests/simulator/init.py (2 hunks)
- tests/simulator/test_load_balancing.py (1 hunks)
- tests/simulator/test_tagging.py.off (1 hunks)
- tools/python3/init.py (2 hunks)
- tools/python3/caliper.py (1 hunks)
Files not reviewed due to errors (1)
- pyphare/pyphare/pharesee/particles.py (no review received)
Files skipped from review as they are similar to previous changes (24)
- res/cmake/def.cmake
- res/cmake/dep/samrai.cmake
- src/amr/CMakeLists.txt
- src/amr/data/particles/particles_data.hpp
- src/amr/level_initializer/hybrid_level_initializer.hpp
- src/amr/load_balancing/concrete_load_balancer_hybrid_strategy_homogeneous.hpp
- src/amr/load_balancing/concrete_load_balancer_hybrid_strategy_nppc.hpp
- src/amr/load_balancing/load_balancer_details.hpp
- src/amr/load_balancing/load_balancer_estimator.hpp
- src/amr/load_balancing/load_balancer_estimator_hybrid.hpp
- src/amr/load_balancing/load_balancer_hybrid_strategy.hpp
- src/amr/load_balancing/load_balancer_hybrid_strategy_factory.hpp
- src/amr/load_balancing/load_balancer_manager.hpp
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
- src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp
- src/core/data/ions/particle_initializers/particle_initializer_factory.hpp
- src/core/data/ndarray/ndarray_vector.hpp
- src/core/data/particles/particle_array.hpp
- src/core/utilities/box/box.hpp
- src/initializer/data_provider.hpp
- src/initializer/dictator.cpp
- tests/core/data/ions/test_ions.cpp
- tests/simulator/CMakeLists.txt
- tests/simulator/test_tagging.py.off
Additional Context Used
Ruff (46)
pyphare/pyphare/pharein/__init__.py (15)
32-32: Module level import not at top of file
32-32:
.uniform_model.UniformModelimported but unused; consider removing, adding to__all__, or using a redundant alias
33-33: Module level import not at top of file
33-33:
.maxwellian_fluid_model.MaxwellianFluidModelimported but unused; consider removing, adding to__all__, or using a redundant alias
34-34: Module level import not at top of file
34-34:
.electron_model.ElectronModelimported but unused; consider removing, adding to__all__, or using a redundant alias
35-41: Module level import not at top of file
36-36:
.diagnostics.FluidDiagnosticsimported but unused; consider removing, adding to__all__, or using a redundant alias
37-37:
.diagnostics.ElectromagDiagnosticsimported but unused; consider removing, adding to__all__, or using a redundant alias
38-38:
.diagnostics.ParticleDiagnosticsimported but unused; consider removing, adding to__all__, or using a redundant alias
39-39:
.diagnostics.MetaDiagnosticsimported but unused; consider removing, adding to__all__, or using a redundant alias
40-40:
.diagnostics.InfoDiagnosticsimported but unused; consider removing, adding to__all__, or using a redundant alias
42-46: Module level import not at top of file
43-43:
.simulation.Simulationimported but unused; consider removing, adding to__all__, or using a redundant alias
47-47: Module level import not at top of file
pyphare/pyphare/pharein/load_balancer.py (2)
46-46: f-string without any placeholders
49-49: f-string without any placeholders
pyphare/pyphare/pharein/maxwellian_fluid_model.py (1)
4-4:
pyphare.core.gridlayout.yee_element_is_primalimported but unusedpyphare/pyphare/pharein/simulation.py (7)
338-338: Comparison to
Noneshould becond is not None
358-358: Ambiguous variable name:
l
564-564: f-string without any placeholders
572-572: f-string without any placeholders
692-692: Comparison to
Noneshould becond is not None
949-949: Multiple imports on one line
955-955: Multiple imports on one line
pyphare/pyphare/pharesee/hierarchy.py (1)
668-668: Comparison to
Noneshould becond is Nonepyphare/pyphare/pharesee/particles.py (1)
88-88: f-string without any placeholders
pyphare/pyphare/simulator/simulator.py (4)
110-110: Do not use bare
except
133-133: Do not use bare
except
145-145:
pyphare.cpp.cpp_libimported but unused
164-164: Comparison to
Noneshould becond is not Nonepyphare/pyphare_tests/test_pharesee/test_geometry_3d.py (1)
1-1: Undefined name
test_geometry_3dtests/diagnostic/__init__.py (2)
16-16: Multiple imports on one line
16-16:
numpyimported but unused; consider removing, adding to__all__, or using a redundant aliastests/functional/harris/harris_2d_lb.py (1)
74-74: Ambiguous variable name:
ltests/simulator/__init__.py (7)
4-4: Multiple imports on one line
77-77: Do not assign a
lambdaexpression, use adef
178-178: Undefined name
Box
184-184: Undefined name
Box
191-191: Undefined name
Box
217-217: Local variable
rankis assigned to but never used
218-218: Local variable
envis assigned to but never usedtests/simulator/test_load_balancing.py (1)
73-73: Ambiguous variable name:
ltools/python3/__init__.py (1)
72-72: Module level import not at top of file
tools/python3/caliper.py (2)
6-6:
tools.python3.runimported but unused
20-20:
subprocessimported but unused
Learnings (1)
pyphare/pyphare/pharesee/hierarchy.py (1)
User: PhilipDeegan URL: https://github.com/PHAREHUB/PHARE/pull/807 Timestamp: 2024-04-15T11:57:24.673Z Learning: The `__eq__` method in `ParticleData` uses `self.dataset == that.dataset` for dataset comparison, which might not be sufficient for numpy arrays as it returns an array of booleans. The use of `np.array_equal` is suggested for a more robust comparison.
Additional comments not posted (24)
pyphare/pyphare/simulator/simulator.py (1)
147-147: LGTM!tests/simulator/test_load_balancing.py (1)
200-283: LGTM!pyphare/pyphare/pharein/maxwellian_fluid_model.py (2)
75-75: LGTM!
127-127: LGTM!src/amr/wrappers/integrator.hpp (7)
27-28: Includingload_balancer_details.hppis necessary for theLoadBalancerDetailstype used in the class.
47-54: Theadvancemethod correctly includes a call to_should_rebalance_nowfor load balancing.
56-64: The constructor correctly initializes new member variables and sets up the load balancer.
67-73: The new member variables are necessary for the load balancing logic and their initialization is appropriate.
78-90: ThecomputeNonUniformWorkLoadForLevel0method is well-implemented and necessary for load balancing.
94-99: Thecadence_rebalance_checkmethod is straightforward and necessary for load balancing.
156-166: The constructor definition correctly initializes new member variables and sets up the load balancer.pyphare/pyphare/pharein/__init__.py (2)
228-247: The addition of load balancer parameters in thepopulateDictfunction looks good. Ensure that all necessary parameters for the load balancer are included and correctly set.Verification successful
The
LoadBalancerclass has the following parameters:
activemodetolon_initautonext_rebalance_backoff_multipliernext_rebalancemax_next_rebalanceevery_registerThe
populateDictfunction sets the following parameters:
activemodetolon_initautonext_rebalance_backoff_multipliernext_rebalancemax_next_rebalanceeveryAll necessary parameters for the
LoadBalancerare included and correctly set in thepopulateDictfunction.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all load balancer parameters are included and correctly set in the populateDict function. # Test: Search for the load balancer parameters in the populateDict function. Expect: All necessary parameters are included. rg --type py "load_balancer" ./pyphare/pyphare/pharein/__init__.pyLength of output: 173
Script:
#!/bin/bash # Locate the LoadBalancer class definition and display its parameters. # Find the file containing the LoadBalancer class definition. lb_file=$(fd LoadBalancer.py) # Display the LoadBalancer class definition and its parameters. ast-grep --lang python --pattern $'class LoadBalancer($_, $_):\n def __init__($$$):\n $$$' $lb_fileLength of output: 132
Script:
#!/bin/bash # Search for the LoadBalancer class definition across the entire codebase. # Search for the LoadBalancer class definition. ast-grep --lang python --pattern $'class LoadBalancer($_, $_):\n def __init__($$$):\n $$$'Length of output: 99
Script:
#!/bin/bash # Search for the LoadBalancer class definition using rg and display its parameters. # Search for the LoadBalancer class definition. rg --type py "class LoadBalancer" > load_balancer_class_location.txt # Extract the file path from the search result. lb_file=$(awk -F: '{print $1}' load_balancer_class_location.txt | head -n 1) # Display the LoadBalancer class definition and its parameters. if [ -n "$lb_file" ]; then ast-grep --lang python --pattern $'class LoadBalancer($_, $_):\n def __init__($$$):\n $$$' $lb_file else echo "LoadBalancer class definition not found." fiLength of output: 351
Script:
#!/bin/bash # Display the contents of the file containing the LoadBalancer class. # Display the contents of pyphare/pyphare/pharein/load_balancer.py cat pyphare/pyphare/pharein/load_balancer.pyLength of output: 1725
130-132: Theadd_boolfunction is correctly implemented and follows the pattern of otheradd_*functions.src/simulator/simulator.hpp (4)
Line range hint
4-18: The addition of include statements for load balancing headers is necessary and appropriate for the new functionality.
73-75: The update to thedumpmethod ensures that it returns the result ofdMan->dumpifdManis not null. This is a good addition.
266-293: The load balancing setup in thehybrid_initmethod looks comprehensive. Ensure that all necessary configurations for load balancing are included and correctly set.
Line range hint
392-401: The addition of the logging scope in theadvancemethod is beneficial for debugging and performance monitoring.pyphare/pyphare/pharein/simulation.py (5)
130-131: Ensurestart_timeis correctly used in the function.
150-154: The return statement correctly includesstart_timein the calculation offinal_time.
587-594: Thecheck_loadbalancingfunction correctly validates theloadbalancingkeyword.
651-652: The call tocheck_loadbalancingensures theloadbalancingkeyword is validated.
849-849: The addition ofload_balanceras an attribute in theSimulationclass is correct.src/amr/multiphysics_integrator.hpp (1)
628-628: The declaration ofload_balancer_manager_as aunique_ptris appropriate for managing the lifetime of the load balancer manager instance. Ensure that all necessary checks fornullptrare in place before dereferencing this pointer to maintain robustness.pyphare/pyphare/pharesee/hierarchy.py (1)
258-259: LGTM! The__getitem__method is straightforward and correct.
| patch_data_ref = patch_ref.patch_datas[patch_data_key] | ||
| patch_data_cmp = patch_cmp.patch_datas[patch_data_key] | ||
|
|
||
| if patch_data_cmp != patch_data_ref: |
There was a problem hiding this comment.
Use np.array_equal for robust array comparison in numpy arrays within hierarchy_compare.
- if patch_data_cmp != patch_data_ref:
+ if not np.array_equal(patch_data_cmp, patch_data_ref):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 patch_data_cmp != patch_data_ref: | |
| if not np.array_equal(patch_data_cmp, patch_data_ref): |
| plt.xlabel("timestep") | ||
| fig.savefig("timestep_times.png") | ||
|
|
||
| cpp_lib().mpi_barrier() |
There was a problem hiding this comment.
Consider removing the MPI barrier to avoid unnecessary synchronization unless it is explicitly needed for correctness.
- cpp_lib().mpi_barrier()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.
| cpp_lib().mpi_barrier() |
Consider removing the MPI barrier to avoid unnecessary synchronization unless it is explicitly needed for correctness.
- cpp_lib().mpi_barrier()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.
| cpp_lib().mpi_barrier() |
| rho[np.where(np.isclose(y, 10, atol=0.1))] = 0 | ||
| return rho | ||
|
|
||
| def S(y, y0, l): |
There was a problem hiding this comment.
Rename the variable l to length for clarity and to avoid confusion 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): |
| #include "core/logger.hpp" | ||
| #include "core/utilities/algorithm.hpp" | ||
|
|
||
| #include "load_balancing/load_balancer_manager.hpp" | ||
| #include "load_balancing/load_balancer_estimator.hpp" |
There was a problem hiding this comment.
The addition of new includes for logging, load balancer manager, and estimator is appropriate for the new functionality being introduced. However, consider wrapping these includes with conditional compilation or feature flags if load balancing is an optional feature that can be toggled at compile time. This approach can help maintain the modularity and configurability of the codebase.
+#ifdef ENABLE_LOAD_BALANCING
#include "core/logger.hpp"
#include "core/utilities/algorithm.hpp"
#include "load_balancing/load_balancer_manager.hpp"
#include "load_balancing/load_balancer_estimator.hpp"
+#endifCommittable 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.
| #include "core/logger.hpp" | |
| #include "core/utilities/algorithm.hpp" | |
| #include "load_balancing/load_balancer_manager.hpp" | |
| #include "load_balancing/load_balancer_estimator.hpp" | |
| #ifdef ENABLE_LOAD_BALANCING | |
| #include "core/logger.hpp" | |
| #include "core/utilities/algorithm.hpp" | |
| #include "load_balancing/load_balancer_manager.hpp" | |
| #include "load_balancing/load_balancer_estimator.hpp" | |
| #endif |
tools/python3/caliper.py
Outdated
| # https://hatchet.readthedocs.io/en/latest/basic_tutorial.html#introduction | ||
| def hatchet_on_caliper(cali_file="phare_outputs/recorder.0.cali"): | ||
| import hatchet as ht | ||
| import subprocess |
There was a problem hiding this comment.
Remove the unused import of subprocess.
- import subprocessCommittable 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 subprocess |
| + 1.0 / np.cosh((y - Ly * 0.7) / L) ** 2 | ||
| ) | ||
|
|
||
| def S(y, y0, l): |
There was a problem hiding this comment.
Rename the variable l to a more descriptive name to avoid ambiguity and improve readability.
- 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): |
| bool static _is_tagging_refinement(initializer::PHAREDict const& dict) | ||
| { | ||
| return cppdict::get_value(dict, "simulation/AMR/refinement/tagging/method", | ||
| std::string{"none"}) | ||
| == std::string{"auto"}; | ||
| } |
There was a problem hiding this comment.
Use a constant for the string comparison to improve maintainability.
- return cppdict::get_value(dict, "simulation/AMR/refinement/tagging/method", std::string{"none"}) == std::string{"auto"};
+ const std::string AUTO_TAGGING = "auto";
+ return cppdict::get_value(dict, "simulation/AMR/refinement/tagging/method", std::string{"none"}) == AUTO_TAGGING;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.
| bool static _is_tagging_refinement(initializer::PHAREDict const& dict) | |
| { | |
| return cppdict::get_value(dict, "simulation/AMR/refinement/tagging/method", | |
| std::string{"none"}) | |
| == std::string{"auto"}; | |
| } | |
| bool static _is_tagging_refinement(initializer::PHAREDict const& dict) | |
| { | |
| const std::string AUTO_TAGGING = "auto"; | |
| return cppdict::get_value(dict, "simulation/AMR/refinement/tagging/method", | |
| std::string{"none"}) | |
| == AUTO_TAGGING; | |
| } |
| return load; | ||
| } | ||
|
|
||
| bool _should_rebalance_now(); |
There was a problem hiding this comment.
Consider refactoring the _should_rebalance_now method to improve readability and maintainability.
- bool _should_rebalance_now();
+ bool _should_rebalance_now(); // Consider refactoring for simplicityAlso applies to: 216-222
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.
| bool _should_rebalance_now(); | |
| bool _should_rebalance_now(); // Consider refactoring for simplicity |
PhilipDeegan
force-pushed
the
loadBalancer
branch
from
9e669d1 to
0f2686c
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (1)
pyphare/pyphare/pharein/__init__.py (1)
Line range hint
32-32: Move module-level imports to the top of the file to improve code organization and readability.+ from .uniform_model import UniformModel + from .maxwellian_fluid_model import MaxwellianFluidModel + from .electron_model import ElectronModel + from .diagnostics import ( + FluidDiagnostics, + ElectromagDiagnostics, + ParticleDiagnostics, + MetaDiagnostics, + InfoDiagnostics, + ) + from .simulation import ( + Simulation, + serialize as serialize_sim, + deserialize as deserialize_sim, + ) + from .load_balancer import LoadBalancer - # All imports moved to the topAlso applies to: 33-33, 34-34, 35-41, 42-46, 47-47
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- pyphare/pyphare/core/init.py (3 hunks)
- pyphare/pyphare/pharein/init.py (5 hunks)
- pyphare/pyphare/pharein/load_balancer.py (1 hunks)
- src/hdf5/detail/h5/h5_file.hpp (2 hunks)
- tests/simulator/test_load_balancing.py (1 hunks)
- tools/config/config.py (1 hunks)
Files skipped from review due to trivial changes (1)
- src/hdf5/detail/h5/h5_file.hpp
Additional Context Used
Ruff (21)
pyphare/pyphare/pharein/__init__.py (15)
32-32: Module level import not at top of file
32-32:
.uniform_model.UniformModelimported but unused; consider removing, adding to__all__, or using a redundant alias
33-33: Module level import not at top of file
33-33:
.maxwellian_fluid_model.MaxwellianFluidModelimported but unused; consider removing, adding to__all__, or using a redundant alias
34-34: Module level import not at top of file
34-34:
.electron_model.ElectronModelimported but unused; consider removing, adding to__all__, or using a redundant alias
35-41: Module level import not at top of file
36-36:
.diagnostics.FluidDiagnosticsimported but unused; consider removing, adding to__all__, or using a redundant alias
37-37:
.diagnostics.ElectromagDiagnosticsimported but unused; consider removing, adding to__all__, or using a redundant alias
38-38:
.diagnostics.ParticleDiagnosticsimported but unused; consider removing, adding to__all__, or using a redundant alias
39-39:
.diagnostics.MetaDiagnosticsimported but unused; consider removing, adding to__all__, or using a redundant alias
40-40:
.diagnostics.InfoDiagnosticsimported but unused; consider removing, adding to__all__, or using a redundant alias
42-46: Module level import not at top of file
43-43:
.simulation.Simulationimported but unused; consider removing, adding to__all__, or using a redundant alias
47-47: Module level import not at top of file
pyphare/pyphare/pharein/load_balancer.py (3)
36-36: f-string without any placeholders
53-53: f-string without any placeholders
56-56: f-string without any placeholders
tests/simulator/test_load_balancing.py (2)
73-73: Ambiguous variable name:
l
218-218: Local variable
diag_diris assigned to but never usedtools/config/config.py (1)
144-144: Local variable
mpi_typeis assigned to but never used
Additional comments not posted (2)
pyphare/pyphare/pharein/load_balancer.py (2)
56-56: Remove the unnecessary f-string as there are no placeholders used.- f"LoadBalancer is already registered to simulation" + "LoadBalancer is already registered to simulation"Likely invalid or redundant comment.
53-53: Remove the unnecessary f-string as there are no placeholders used.- f"LoadBalancer cannot be registered as no simulation exists" + "LoadBalancer cannot be registered as no simulation exists"Likely invalid or redundant comment.
| return | ||
|
|
||
| with self.assertRaises(RuntimeError): | ||
| diag_dir = self.run_sim( |
There was a problem hiding this comment.
Remove the unused variable diag_dir to clean up the code.
- diag_dir = self.run_sim(
- self.unique_diag_dir_for_test_case(diag_outputs, ndim, interp),
- dict(active=True, mode="nppc", tol=0.01, **lbkwargs),
- )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.
| diag_dir = self.run_sim( |
PhilipDeegan
force-pushed
the
loadBalancer
branch
from
a5b207a to
4f501ca
Compare
PhilipDeegan
force-pushed
the
loadBalancer
branch
from
4f501ca to
3f2f131
Compare
There was a problem hiding this comment.
Actionable comments posted: 29
Outside diff range, codebase verification and nitpick comments (13)
src/amr/load_balancing/load_balancer_estimator_hybrid.hpp (1)
31-32: Remove redundant comment.The comment about needing a destructor is redundant as the default destructor is already defined.
- // the implementation of a virtual class NEEDS a dtorsrc/initializer/dictator.cpp (1)
48-48: Ensure consistent naming conventions.The function binding
add_boolshould follow the same naming convention as other bindings, such asadd_size_t.- m.def("add_bool", add<bool>, "add"); + m.def("add_bool", add<bool>, "add_bool");src/initializer/data_provider.hpp (1)
51-53: The change to boolean keys inPHAREDictis not fully implemented.The search results show multiple instances where
PHAREDictis still being used with string keys. This indicates that the change to boolean keys might not be correct or fully propagated throughout the codebase. Please review and update all interactions withPHAREDictto ensure they handle boolean keys instead of string keys.
- File:
tests/diagnostic/test_diagnostics.hpp- File:
tests/simulator/per_test.hpp- File:
tests/initializer/test_initializer.cpp- File:
tests/core/numerics/ion_updater/test_updater.cpp- File:
tests/core/numerics/ohm/test_main.cpp- File:
src/restarts/restarts.hpp- File:
tests/core/data/ion_population/test_ion_population.cpp- File:
tests/core/data/ion_population/test_ion_population_fixtures.hpp- File:
src/python3/data_wrangler.hpp- File:
src/restarts/restarts_manager.hpp- File:
src/simulator/simulator.hpp- File:
tests/amr/resources_manager/test_resources_manager.cpp- File:
src/simulator/simulator.cpp- File:
tests/amr/models/test_models.cpp- File:
tests/core/data/particle_initializer/test_main.cpp- File:
tests/amr/tagging/test_tagging.cpp- File:
tests/amr/messengers/test_messengers.cpp- File:
tests/core/data/ions/test_ions.cpp- File:
tests/core/data/electrons/test_electrons.cpp- File:
src/initializer/dictator.cpp- File:
src/core/models/hybrid_state.hpp- File:
src/core/numerics/ohm/ohm.hpp- File:
src/core/numerics/ion_updater/ion_updater.hpp- File:
src/core/utilities/timestamps.hpp- File:
src/diagnostic/diagnostics.hpp- File:
src/diagnostic/diagnostic_manager.hpp- File:
src/phare/phare.hpp- File:
src/core/data/vecfield/vecfield_initializer.hpp- File:
src/core/data/ions/ions.hpp- File:
src/core/data/ions/particle_initializers/particle_initializer_factory.hpp- File:
src/diagnostic/detail/h5writer.hpp- File:
src/core/data/ions/ion_population/ion_population.hpp- File:
src/core/data/electrons/electrons.hpp- File:
src/core/data/electromag/electromag.hpp- File:
src/amr/wrappers/integrator.hpp- File:
src/amr/level_initializer/hybrid_level_initializer.hpp- File:
src/amr/wrappers/hierarchy.hpp- File:
src/amr/load_balancing/load_balancer_details.hpp- File:
src/amr/load_balancing/load_balancer_manager.hpp- File:
src/amr/level_initializer/level_initializer_factory.hpp- File:
src/amr/multiphysics_integrator.hpp- File:
src/phare/phare.cpp- File:
src/amr/solvers/solver_ppc.hpp- File:
src/amr/solvers/solver_ppc_model_view.hpp- File:
src/amr/physical_models/hybrid_model.hpp- File:
tools/bench/core/numerics/ion_updater/bench_ion_updater.cppAnalysis chain
Verify the impact of changing the template parameter to
bool.The primary change is the alteration of the first template parameter of the
cppdict::Dictfrominttobool. Ensure that all interactions withPHAREDictare updated to handle boolean keys instead of integer keys.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all interactions with `PHAREDict` to ensure they handle boolean keys instead of integer keys. # Test: Search for the usage of `PHAREDict`. Expect: Only occurances of boolean keys. rg --type cpp -A 5 $'PHAREDict'Length of output: 49839
src/amr/load_balancing/concrete_load_balancer_hybrid_strategy_homogeneous.hpp (1)
45-65: Ensure proper error handling in thecomputemethod.The
computemethod lacks error handling for potential null pointers and invalid dynamic casts. Consider adding checks forpatch_data_lbandload_balancer_valto handle these cases appropriately.
- File:
src/amr/load_balancing/concrete_load_balancer_hybrid_strategy_homogeneous.hpp- Lines: 45-65
Analysis chain
Ensure proper error handling in the
computemethod.The
computemethod performs operations onpatch_data_lbandload_balancer_val. Ensure proper error handling for potential null pointers and invalid dynamic casts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper error handling in the `compute` method. # Test: Search for error handling in the `compute` method. Expect: Proper error handling for null pointers and invalid dynamic casts. rg --type cpp -A 5 $'ConcreteLoadBalancerHybridStrategyHomogeneous<PHARE_T>::compute'Length of output: 806
Script:
#!/bin/bash # Extract the full implementation of the `compute` method to verify error handling. ast-grep --lang cpp --pattern $'void ConcreteLoadBalancerHybridStrategyHomogeneous<PHARE_T>::compute($_, $_) { $$$ }'Length of output: 120
Script:
#!/bin/bash # Extract the full implementation of the `compute` method to verify error handling. rg --type cpp -A 30 'ConcreteLoadBalancerHybridStrategyHomogeneous<PHARE_T>::compute'Length of output: 3319
src/amr/load_balancing/load_balancer_manager.hpp (1)
89-93: Ensure proper error handling in theestimatemethod.The
estimatemethod currently lacks explicit error handling for null pointers. Consider adding logging or exception handling to manage potential null pointer scenarios effectively.
src/amr/load_balancing/load_balancer_manager.hpp(Lines: 89-93)Analysis chain
Ensure proper error handling in the
estimatemethod.The
estimatemethod performs operations on the load balancer estimator. Ensure proper error handling for potential null pointers.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper error handling in the `estimate` method. # Test: Search for error handling in the `estimate` method. Expect: Proper error handling for null pointers. rg --type cpp -A 5 $'LoadBalancerManager<dim>::estimate'Length of output: 602
Script:
#!/bin/bash # Description: Verify if there are additional error handling mechanisms or logging in the surrounding code or related methods. # Search for the class definition and methods to understand the context of error handling. ast-grep --lang cpp --pattern $'class LoadBalancerManager { $$$ }' -A 50 # Search for logging or error handling mechanisms in the file. rg --type cpp 'log|error|exception|throw' src/amr/load_balancing/load_balancer_manager.hppLength of output: 513
Script:
#!/bin/bash # Description: Verify if there are additional error handling mechanisms or logging in the surrounding code or related methods. # Search for the class definition and methods to understand the context of error handling. ast-grep --lang cpp --pattern $'class LoadBalancerManager { $$$ }' -A 50 # Search for logging or error handling mechanisms in the entire load_balancing directory. rg --type cpp 'log|error|exception|throw' src/amr/load_balancing/Length of output: 662
tests/functional/harris/harris_2d_lb.py (1)
74-74: Rename variablelto a more descriptive name.The variable name
lis ambiguous and can be confused with the number1. Consider renaming it tolengthfor better readability.- def S(y, y0, l): + def S(y, y0, length):Tools
Ruff
74-74: Ambiguous variable name:
l(E741)
src/core/data/particles/particle_array.hpp (1)
155-157: Consider adding documentation for new method.Adding documentation for the new method
nbr_particles_inwill improve code readability and maintainability./** * @brief Returns the number of particles in the specified cell. * * @param cell The cell for which to count the number of particles. * @return The number of particles in the specified cell. */ auto nbr_particles_in(cell_t const& cell) const { return cellMap_.size(cell); }src/amr/wrappers/integrator.hpp (2)
36-41: Extract comparison string into a constant.For better readability and maintainability, consider extracting the comparison string "auto" into a constant.
const std::string TAGGING_METHOD_AUTO = "auto"; return cppdict::get_value(dict, "simulation/AMR/refinement/tagging/method", std::string{"none"}) == TAGGING_METHOD_AUTO;
Line range hint
62-167:
Address the TODO comment.The TODO comment regarding
SAMRAI_MPIshould be addressed to ensure clarity.// TODO: Verify if `SAMRAI_MPI::getSAMRAIWorld()` is necessary and remove if not needed.src/core/data/ndarray/ndarray_vector.hpp (2)
216-220: Specify the return type explicitly.Explicitly specifying the return type enhances readability and consistency.
NdArrayView<dim, DataType, c_ordering> make_array_view(DataType* data, std::array<std::uint32_t, dim> const& shape) { return NdArrayView<dim, DataType, c_ordering>{data, shape}; }
222-226: Specify the return type explicitly.Explicitly specifying the return type enhances readability and consistency.
NdArrayView<dim, DataType const, c_ordering> make_array_view(DataType const* const data, std::array<std::uint32_t, dim> shape) { return NdArrayView<dim, DataType const, c_ordering>{data, shape}; }pyphare/pyphare/pharein/simulation.py (1)
839-839: Theload_balancerattribute is initialized but not used elsewhere in the class.The
__init__method initializes theload_balancerattribute toNone, but it is not used anywhere else in the class. This could indicate an incomplete implementation or an oversight. Please review and address this issue.
pyphare/pyphare/pharein/simulation.py:self.load_balancer = Noneon line 839.Analysis chain
Ensure
load_balancerattribute is correctly initialized and used.The
__init__method now includes theload_balancerattribute. Ensure that this attribute is correctly initialized and used throughout the class.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `load_balancer` attribute in the class. # Test: Search for the attribute usage. Expect: Only occurrences of correct usage. rg --type python -A 5 $'self.load_balancer'Length of output: 73
Script:
#!/bin/bash # Description: Verify the usage of `load_balancer` attribute in the class. # Test: Search for the attribute usage. Expect: Only occurrences of correct usage. rg -t py -A 5 'self.load_balancer'Length of output: 441
src/amr/multiphysics_integrator.hpp (1)
Ensure load balancer components are correctly integrated and tested.
The function
initializeLevelDataincludes calls to the load balancer'sallocateandestimatemethods. However, there are no tests verifying theestimatemethod. Please add tests to ensure that the load balancer components are correctly integrated and functioning as expected.
- Add tests for
load_balancer_manager_'sestimatemethod.Analysis chain
Line range hint
303-370:
Ensure load balancer components are correctly integrated and tested.The function now includes calls to the load balancer's
allocateandestimatemethods. Ensure that these components are correctly integrated and thoroughly tested.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of load balancer components in `initializeLevelData`. # Test: Search for the function usage. Expect: Only occurrences of the new integration. rg --type cpp -A 5 $'initializeLevelData'Length of output: 4782
Script:
#!/bin/bash # Description: Verify the testing of load balancer components in `initializeLevelData`. # Test: Search for occurrences of `allocate` and `estimate` in the test files. rg --type cpp 'allocate' tests/ rg --type cpp 'estimate' tests/Length of output: 2825
Script:
#!/bin/bash # Description: Verify the usage of the `estimate` method in the main source files. # Test: Search for occurrences of `estimate` in the main source files. rg --type cpp 'estimate'Length of output: 997
Script:
#!/bin/bash # Description: Verify the testing of `load_balancer_manager_` components. # Test: Search for occurrences of `load_balancer_manager_` in the test files. rg --type cpp 'load_balancer_manager_' tests/Length of output: 45
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (41)
- .github/workflows/cmake_macos.yml (1 hunks)
- pyphare/pyphare/pharein/init.py (5 hunks)
- pyphare/pyphare/pharein/load_balancer.py (1 hunks)
- pyphare/pyphare/pharein/maxwellian_fluid_model.py (2 hunks)
- pyphare/pyphare/pharein/simulation.py (4 hunks)
- pyphare/pyphare/pharesee/hierarchy.py (6 hunks)
- pyphare/pyphare/pharesee/particles.py (1 hunks)
- pyphare/pyphare/simulator/simulator.py (5 hunks)
- res/cmake/def.cmake (4 hunks)
- res/cmake/dep/samrai.cmake (1 hunks)
- src/amr/CMakeLists.txt (1 hunks)
- src/amr/data/particles/particles_data.hpp (1 hunks)
- src/amr/level_initializer/hybrid_level_initializer.hpp (2 hunks)
- src/amr/load_balancing/concrete_load_balancer_hybrid_strategy_homogeneous.hpp (1 hunks)
- src/amr/load_balancing/concrete_load_balancer_hybrid_strategy_nppc.hpp (1 hunks)
- src/amr/load_balancing/load_balancer_details.hpp (1 hunks)
- src/amr/load_balancing/load_balancer_estimator.hpp (1 hunks)
- src/amr/load_balancing/load_balancer_estimator_hybrid.hpp (1 hunks)
- src/amr/load_balancing/load_balancer_hybrid_strategy.hpp (1 hunks)
- src/amr/load_balancing/load_balancer_hybrid_strategy_factory.hpp (1 hunks)
- src/amr/load_balancing/load_balancer_manager.hpp (1 hunks)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (11 hunks)
- src/amr/multiphysics_integrator.hpp (13 hunks)
- src/amr/wrappers/integrator.hpp (4 hunks)
- src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp (6 hunks)
- src/core/data/ions/particle_initializers/particle_initializer_factory.hpp (2 hunks)
- src/core/data/ndarray/ndarray_vector.hpp (1 hunks)
- src/core/data/particles/particle_array.hpp (1 hunks)
- src/core/utilities/box/box.hpp (1 hunks)
- src/hdf5/detail/h5/h5_file.hpp (2 hunks)
- src/initializer/data_provider.hpp (1 hunks)
- src/initializer/dictator.cpp (1 hunks)
- src/simulator/simulator.hpp (6 hunks)
- tests/core/data/ions/test_ions.cpp (1 hunks)
- tests/diagnostic/init.py (1 hunks)
- tests/functional/harris/harris_2d_lb.py (1 hunks)
- tests/simulator/CMakeLists.txt (1 hunks)
- tests/simulator/init.py (2 hunks)
- tests/simulator/test_load_balancing.py (1 hunks)
- tests/simulator/test_tagging.py.off (1 hunks)
- tools/config/config.py (1 hunks)
Files skipped from review due to trivial changes (6)
- .github/workflows/cmake_macos.yml
- res/cmake/dep/samrai.cmake
- src/amr/data/particles/particles_data.hpp
- src/hdf5/detail/h5/h5_file.hpp
- tests/core/data/ions/test_ions.cpp
- tests/simulator/test_tagging.py.off
Additional context used
Path-based instructions (19)
src/amr/load_balancing/load_balancer_hybrid_strategy.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/amr/load_balancing/load_balancer_estimator.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/amr/load_balancing/load_balancer_hybrid_strategy_factory.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/amr/load_balancing/load_balancer_estimator_hybrid.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/amr/load_balancing/load_balancer_details.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/initializer/data_provider.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/amr/load_balancing/concrete_load_balancer_hybrid_strategy_homogeneous.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/amr/load_balancing/load_balancer_manager.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/data/ions/particle_initializers/particle_initializer_factory.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/amr/load_balancing/concrete_load_balancer_hybrid_strategy_nppc.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/utilities/box/box.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/amr/level_initializer/hybrid_level_initializer.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/data/particles/particle_array.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/amr/wrappers/integrator.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/data/ndarray/ndarray_vector.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/simulator/simulator.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/amr/multiphysics_integrator.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Learnings (2)
src/amr/load_balancing/concrete_load_balancer_hybrid_strategy_nppc.hpp (1)
Learnt from: PhilipDeegan URL: https://github.com/PHAREHUB/PHARE/pull/807 Timestamp: 2024-04-15T12:15:22.878Z Learning: The variable `c_ordering` in `ConcreteLoadBalancerHybridStrategyNPPC<PHARE_T>::compute` is used to create an array view `lb_view` for handling load balancing data.pyphare/pyphare/pharesee/hierarchy.py (1)
Learnt from: PhilipDeegan URL: https://github.com/PHAREHUB/PHARE/pull/807 Timestamp: 2024-04-15T11:57:24.673Z Learning: The `__eq__` method in `ParticleData` uses `self.dataset == that.dataset` for dataset comparison, which might not be sufficient for numpy arrays as it returns an array of booleans. The use of `np.array_equal` is suggested for a more robust comparison.
Ruff
pyphare/pyphare/pharein/load_balancer.py
36-36: f-string without any placeholders
Remove extraneous
fprefix(F541)
53-53: f-string without any placeholders
Remove extraneous
fprefix(F541)
56-56: f-string without any placeholders
Remove extraneous
fprefix(F541)
tests/functional/harris/harris_2d_lb.py
74-74: Ambiguous variable name:
l(E741)
tests/simulator/test_load_balancing.py
35-35: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
73-73: Ambiguous variable name:
l(E741)
192-192: Loop control variable
ilvlnot used within loop bodyRename unused
ilvlto_ilvl(B007)
194-194: Loop control variable
pd_keynot used within loop bodyRename unused
pd_keyto_pd_key(B007)
205-205: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
218-218: Local variable
diag_diris assigned to but never usedRemove assignment to unused variable
diag_dir(F841)
tests/simulator/__init__.py
217-217: Local variable
rankis assigned to but never usedRemove assignment to unused variable
rank(F841)
218-218: Local variable
envis assigned to but never usedRemove assignment to unused variable
env(F841)
pyphare/pyphare/pharesee/particles.py
284-284: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
290-290: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
295-295: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
298-298: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
pyphare/pyphare/pharesee/hierarchy.py
1835-1835: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
1848-1848: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
Additional comments not posted (72)
src/amr/load_balancing/load_balancer_hybrid_strategy.hpp (1)
14-25: LGTM! The class design is clean and follows best practices.The use of type aliases improves readability, and the virtual destructor ensures proper cleanup in derived classes. The pure virtual function
computeenforces that derived classes implement this function.src/amr/load_balancing/load_balancer_estimator.hpp (1)
17-34: LGTM! The class design is clean and follows best practices.The constructor properly initializes the
id_member. The virtual destructor ensures proper cleanup in derived classes. The pure virtual functionestimateenforces that derived classes implement this function.src/amr/load_balancing/load_balancer_hybrid_strategy_factory.hpp (1)
14-32: LGTM! The factory pattern is correctly implemented.The use of unique pointers ensures proper memory management. The function checks the strategy name and returns the appropriate strategy or nullptr if the name does not match.
src/amr/load_balancing/load_balancer_estimator_hybrid.hpp (1)
34-37: Ensure proper error handling in theestimatemethod.Consider adding error handling to manage potential issues during the
computemethod call.Are there any potential exceptions that
strat_->computemight throw? If so, should they be handled here?tests/simulator/CMakeLists.txt (1)
27-27: Ensure comprehensive load balancing tests.The addition of
phare_mpi_python3_execfortest_load_balancing.pyis a logical step to ensure that load balancing functionality is tested in a distributed environment. However, it's important to ensure that this test covers a range of scenarios to effectively validate the new load balancing strategy.Consider adding tests with different numbers of MPI ranks to cover various distribution scenarios.
tests/diagnostic/__init__.py (1)
23-27: Ensure the correctness of thetimestampsvariable.The new call to
ph.InfoDiagnosticsuses thetimestampsvariable. Ensure thattimestampsis correctly initialized and contains the expected values.src/amr/load_balancing/concrete_load_balancer_hybrid_strategy_homogeneous.hpp (1)
30-33: Constructor looks good.The constructor initializes the
id_member variable. The initialization is straightforward and looks good.src/amr/load_balancing/load_balancer_manager.hpp (6)
34-34: Destructor looks good.The destructor removes the variable from the variable database. The operation is straightforward and looks good.
36-36: MethodgetIdlooks good.The
getIdmethod returns theid_member variable. The operation is straightforward and looks good.
38-39: MethodaddLoadBalancerEstimatorlooks good.The
addLoadBalancerEstimatormethod adds a load balancer estimator for the specified levels. The operation is straightforward and looks good.
41-45: MethodsetLoadBalancerlooks good.The
setLoadBalancermethod sets the load balancer and assigns the workload patch data index. The operation is straightforward and looks good.
47-84: Methodallocatelooks good.The
allocatemethod allocates patch data for the specified patch and time. The operation is straightforward and looks good.
23-33: Constructor looks good but verify the dictionary key type.The constructor initializes several member variables using the provided dictionary. Ensure that the dictionary key type change from
inttoboolis handled correctly.src/core/data/ions/particle_initializers/particle_initializer_factory.hpp (4)
50-51: LGTM!The introduction of the
densityCutOffvariable with a default value of1e-16enhances flexibility in handling density thresholds during particle initialization.
64-65: LGTM!The initialization of the
magneticFieldarray withnullptrvalues sets up the structure for conditional assignment based on thebasisName.
70-71: LGTM!The assignment of the
magneticFieldarray for the Cartesian basis ensures consistency in passing parameters to the initializer.
81-82: LGTM!The assignment of the
magneticFieldarray for the Magnetic basis ensures that the array is properly utilized in the initializer.src/amr/CMakeLists.txt (1)
68-74: LGTM!The addition of new header files related to load balancing expands the functionality and is correctly referenced in the
SOURCES_INCvariable.src/amr/load_balancing/concrete_load_balancer_hybrid_strategy_nppc.hpp (4)
25-38: LGTM!The class definition and constructor for
ConcreteLoadBalancerHybridStrategyNPPCappear correct and properly initialize theid_member variable.
49-55: LGTM!The
computemethod signature and static constantsc_orderinganddimensionare correctly defined and set up for use within the method.
60-67: LGTM!The loop over patches and the creation of
lb_viewappear correct and ensure that resources are set on the patch for load balancing.
68-88: LGTM!The comments and logic for handling ghost cells and AMR index are clear and correctly build the local index for
lb_view.tests/functional/harris/harris_2d_lb.py (2)
150-151: LGTM!The function
plot_file_for_qtyis clear and concise.
186-208: LGTM!The class
HarrisTestand its methods are well-structured and clear.src/core/utilities/box/box.hpp (1)
162-163: Ensureindex_is valid before dereferencing.The operator overloads assume that
index_is always valid. Consider adding assertions or checks to ensureindex_is valid before dereferencing.+ assert(index_ != nullptr);pyphare/pyphare/simulator/simulator.py (2)
38-43: LGTM!The function
print_rank0is clear and concise.
Line range hint
173-198:
LGTM!The changes to the
runmethod are clear and enhance functionality.src/core/data/ions/particle_initializers/maxwellian_particle_initializer.hpp (2)
181-185: LGTM!The addition of the density cutoff check is a useful enhancement.
45-53: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all constructor calls to
MaxwellianParticleInitializermatch the new signature.tools/config/config.py (1)
247-248: Verify the impact of removing the-qoption.The removal of the
-qoption from thePHARE_MPIRUN_POSTFIXvariable might affect MPI runtime behavior. Ensure that this change does not introduce any issues.src/amr/level_initializer/hybrid_level_initializer.hpp (5)
55-56: LGTM!The introduction of the
isRegriddingL0variable improves readability and control flow.
58-63: LGTM!The regridding logic is clearly separated, enhancing readability.
67-72: LGTM!The root level initialization logic is clearly separated, enhancing readability.
122-137: LGTM!The logic for handling the electric field and current ghost filling is appropriately adjusted.
138-155: LGTM!The logic for handling the electric field and current ghost filling is appropriately adjusted.
tests/simulator/test_load_balancing.py (6)
73-73: Rename ambiguous variablel.The variable
lis ambiguous and can be confused with the number1. Rename it tolengthfor clarity.- def S(y, y0, l): + def S(y, y0, length):Tools
Ruff
73-73: Ambiguous variable name:
l(E741)
218-218: Remove unused variablediag_dir.The variable
diag_diris assigned but never used. Remove the assignment to clean up the code.- diag_dir = self.run_sim( - self.unique_diag_dir_for_test_case(diag_outputs, ndim, interp), - dict(active=True, mode="nppc", tol=0.01, **lbkwargs), - )Tools
Ruff
218-218: Local variable
diag_diris assigned to but never usedRemove assignment to unused variable
diag_dir(F841)
232-245: LGTM!The test logic for checking load balancing appears correct.
246-257: LGTM!The test logic for checking if load balancing has not occurred with default settings appears correct.
259-283: LGTM!The test logic for comparing balanced and unbalanced simulations appears correct.
285-286: LGTM!The function logic for running the unittest main function appears correct.
pyphare/pyphare/pharein/maxwellian_fluid_model.py (1)
75-75: Avoid mutable default arguments.Using mutable data structures as default arguments can lead to unexpected behavior. Replace
init={}withNoneand initialize within the function.- init={}, + init=None, ): if init is None: init = {}Likely invalid or redundant comment.
tests/simulator/__init__.py (4)
217-218: Remove unused variablesrankandenv.The variables
rankandenvare assigned but never used. Remove these assignments to clean up the code.- rank = cpp_lib().mpi_rank() - env = os.environTools
Ruff
217-217: Local variable
rankis assigned to but never usedRemove assignment to unused variable
rank(F841)
218-218: Local variable
envis assigned to but never usedRemove assignment to unused variable
env(F841)
199-200: LGTM!The function logic for generating a file path for JSON output appears correct.
203-204: LGTM!The function logic for generating a file path for CALI recorder output appears correct.
207-211: LGTM!The constant logic for defining different profiling modes appears correct.
res/cmake/def.cmake (4)
268-268: Use${Python_EXECUTABLE}for consistency.Ensure consistency by using
${Python_EXECUTABLE}instead of hardcodedpython3.- COMMAND ${Python_EXECUTABLE} -u ${file} ${CLI_ARGS} + COMMAND ${Python_EXECUTABLE} -u ${file} ${CLI_ARGS}Likely invalid or redundant comment.
194-194: Use${Python_EXECUTABLE}for consistency.Ensure consistency by using
${Python_EXECUTABLE}instead of hardcodedpython3.- add_test(NAME py3_${name} COMMAND ${Python_EXECUTABLE} -u ${file} WORKING_DIRECTORY ${directory}) + add_test(NAME py3_${name} COMMAND ${Python_EXECUTABLE} -u ${file} WORKING_DIRECTORY ${directory})Likely invalid or redundant comment.
292-292: Use${Python_EXECUTABLE}for consistency.Ensure consistency by using
${Python_EXECUTABLE}instead of hardcodedpython3.- add_test(NAME py3_${target} COMMAND ${Python_EXECUTABLE} -u ${file} ${CLI_ARGS} WORKING_DIRECTORY ${directory}) + add_test(NAME py3_${target} COMMAND ${Python_EXECUTABLE} -u ${file} ${CLI_ARGS} WORKING_DIRECTORY ${directory})Likely invalid or redundant comment.
160-160: Ensure the environment variable is set correctly.Verify that the
PATHenvironment variable is correctly set for the test binary.Verification successful
The
PATHenvironment variable is correctly set for the test binary.
- The
PATHis set to use the system'sPATHenvironment variable, which is generally correct for running tests.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the PATH environment variable is correctly set for the test binary. # Test: Check the environment variable setting. Expect: Correct PATH setting. grep -A 1 'set_property(TEST ${binary} PROPERTY ENVIRONMENT "PATH=' res/cmake/def.cmakeLength of output: 269
src/amr/wrappers/integrator.hpp (3)
47-54: LGTM!The
advancefunction is well-structured and correctly uses the_should_rebalance_nowmethod.
56-56: LGTM!The
initializefunction correctly initializes the hierarchy.
78-90: LGTM!The
computeNonUniformWorkLoadForLevel0method is well-structured and correctly computes the workload.pyphare/pyphare/pharein/__init__.py (5)
47-47: LGTM!The import statement for
LoadBalanceris correctly placed.
130-132: LGTM!The
add_boolfunction correctly adds a boolean parameter to the configuration dictionary.
228-247: LGTM!The
as_pathsfunction is well-structured and correctly handles the new load balancing parameters.
Line range hint
229-279:
LGTM!The
populateDictfunction correctly integrates the new load balancing parameters.
90-90: LGTM!The
clearDictfunction correctly clears the simulation dictionary.src/simulator/simulator.hpp (3)
Line range hint
397-406:
LGTM! Ensure logging is consistent.The addition of logging in the
advancefunction helps track the execution flow. Ensure that logging is consistent across all functions.
271-294: Ensure load balancer setup is thoroughly tested.The integration of load balancing components in
hybrid_initis crucial. Ensure that the load balancer setup, including the creation ofLoadBalancerManager,LoadBalancerEstimatorHybrid, andCascadePartitioner, is thoroughly tested, especially in scenarios with complex regridding and refinement patterns.
302-304: Ensure constructor handles all initialization scenarios.The constructor now initializes the load balancer components. Ensure that all initialization scenarios, including those without load balancing, are handled correctly.
pyphare/pyphare/pharein/simulation.py (1)
Line range hint
127-151:
Verify correctness ofrestart_timeintegration.The function now includes
restart_timein the final time calculation. Ensure that the integration ofrestart_timeis correct and does not introduce any issues.Verification successful
Verify correctness of
restart_timeintegration.The
restart_timeintegration in thecheck_timefunction appears correct. The function initializesstart_timefromrestart_optionsand includes it in the final time calculation. The usage ofcheck_timein various parts of the codebase does not indicate any issues with this integration.
- Files reviewed:
tests/simulator/test_load_balancing.pytools/bench/functional/simulation_setup.pypyphare/pyphare/pharein/simulation.pyScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of `restart_time` integration in `check_time`. # Test: Search for the function usage. Expect: Only occurrences of the new integration. rg --type python -A 5 $'check_time'Length of output: 65
Script:
#!/bin/bash # Description: Verify the correctness of `restart_time` integration in `check_time`. # Test: Search for the function usage. Expect: Only occurrences of the new integration. rg --type py -A 5 $'check_time'Length of output: 2803
Script:
#!/bin/bash # Description: Extract relevant code snippets around the `check_time` usage to review the integration of `restart_time`. # Extracting code snippets around `check_time` usage in test_load_balancing.py rg --type py -A 10 -B 10 'check_time' tests/simulator/test_load_balancing.py # Extracting code snippets around `check_time` usage in simulation_setup.py rg --type py -A 10 -B 10 'check_time' tools/bench/functional/simulation_setup.py # Extracting code snippets around `check_time` usage in simulation.py rg --type py -A 10 -B 10 'check_time' pyphare/pyphare/pharein/simulation.pyLength of output: 3474
src/amr/multiphysics_integrator.hpp (2)
384-404: Ensure load balancer components are correctly integrated and tested during restart initialization.The function now includes calls to the load balancer's
allocateandestimatemethods during restart initialization. Ensure that these components are correctly integrated and thoroughly tested during restart scenarios.
Line range hint
510-548:
Ensure load balancer'sestimatemethod is correctly integrated and tested.The function now includes a call to the load balancer's
estimatemethod to ensure proper load distribution. Ensure that this method is correctly integrated and thoroughly tested.src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (6)
85-85: LGTM!The change to make
stratNameinline is a good optimization for linkage and performance.
201-201: LGTM!The introduction of
isRegriddingL0to control root level operations during regridding is a logical enhancement.
218-228: LGTM! But address commented-out code.The conditional execution based on
isRegriddingL0improves efficiency and correctness. However, consider removing or uncommenting the code related tomagSharedNodesRefiners_andelecSharedNodesRefiners_with appropriate logic adjustments.- // magSharedNodesRefiners_.fill(B, levelNumber, initDataTime); - // elecSharedNodesRefiners_.fill(E, levelNumber, initDataTime); + // Consider uncommenting or removing this code block with appropriate logic adjustments.
241-244: LGTM!The conditional call to
lvlGhostPartOldRefiners_.fillensures correct handling of ghost particles, avoiding potential issues with the root level.
341-341: Verify logging level change.The logging level for
fillIonGhostParticlesis changed from 3 to 1. Ensure that this change aligns with the intended logging strategy.
368-368: Verify logging level change and method rename.The method name change to
fillIonPopMomentGhostsis more descriptive. The logging level change from 3 to 1 should be verified to ensure it aligns with the intended logging strategy.pyphare/pyphare/pharesee/hierarchy.py (2)
1809-1813: LGTM!The
EqualityReportclass provides a structured way to report the results of equality checks, enhancing clarity and usability.
1818-1861: LGTM!The
hierarchy_comparefunction is well-implemented, utilizing theEqualityReportclass to provide detailed and clear feedback on comparisons.Tools
Ruff
1835-1835: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
1848-1848: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
| def single_patch_per_level_per_pop_from(hier, only_keep_L0=True): # dragons | ||
| for tidx in hier.times(): | ||
| if only_keep_L0: | ||
| hier.time_hier[tidx] = {0: hier.time_hier[tidx][0]} | ||
|
|
||
| patch_levels = hier.time_hier[tidx] | ||
|
|
||
| for level_idx in patch_levels.keys(): | ||
| patch_level = patch_levels[level_idx] | ||
|
|
||
| patch0 = patch_level.patches[0] | ||
| particles = {} # str:[] | ||
|
|
||
| for key in patch0.patch_datas.keys(): | ||
| if isinstance(patch0[key].dataset, Particles): | ||
| particles[key] = [] | ||
|
|
||
| for patch in patch_level.patches: | ||
| for patch_data_key in patch.patch_datas.keys(): | ||
| particles[key] += [patch[patch_data_key].dataset] | ||
|
|
||
| for key in particles.keys(): | ||
| patch0[key].dataset = aggregate(particles[key]) | ||
|
|
||
| patch_levels[level_idx].patches = [patch0] # just one patch | ||
|
|
||
| return hier |
There was a problem hiding this comment.
Consider adding type hints.
Adding type hints can improve code readability and help with static analysis.
def single_patch_per_level_per_pop_from(hier: Hierarchy, only_keep_L0: bool = True) -> Hierarchy:Tools
Ruff
284-284: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
290-290: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
295-295: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
298-298: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
Ensure proper handling of patch_data_key.
The variable patch_data_key is used without being defined in the loop. Ensure patch_data_key is correctly assigned.
- particles[key] += [patch[patch_data_key].dataset]
+ particles[patch_data_key] += [patch[patch_data_key].dataset]Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def single_patch_per_level_per_pop_from(hier, only_keep_L0=True): # dragons | |
| for tidx in hier.times(): | |
| if only_keep_L0: | |
| hier.time_hier[tidx] = {0: hier.time_hier[tidx][0]} | |
| patch_levels = hier.time_hier[tidx] | |
| for level_idx in patch_levels.keys(): | |
| patch_level = patch_levels[level_idx] | |
| patch0 = patch_level.patches[0] | |
| particles = {} # str:[] | |
| for key in patch0.patch_datas.keys(): | |
| if isinstance(patch0[key].dataset, Particles): | |
| particles[key] = [] | |
| for patch in patch_level.patches: | |
| for patch_data_key in patch.patch_datas.keys(): | |
| particles[key] += [patch[patch_data_key].dataset] | |
| for key in particles.keys(): | |
| patch0[key].dataset = aggregate(particles[key]) | |
| patch_levels[level_idx].patches = [patch0] # just one patch | |
| return hier | |
| def single_patch_per_level_per_pop_from(hier, only_keep_L0=True): # dragons | |
| for tidx in hier.times(): | |
| if only_keep_L0: | |
| hier.time_hier[tidx] = {0: hier.time_hier[tidx][0]} | |
| patch_levels = hier.time_hier[tidx] | |
| for level_idx in patch_levels.keys(): | |
| patch_level = patch_levels[level_idx] | |
| patch0 = patch_level.patches[0] | |
| particles = {} # str:[] | |
| for key in patch0.patch_datas.keys(): | |
| if isinstance(patch0[key].dataset, Particles): | |
| particles[key] = [] | |
| for patch in patch_level.patches: | |
| for patch_data_key in patch.patch_datas.keys(): | |
| particles[patch_data_key] += [patch[patch_data_key].dataset] | |
| for key in particles.keys(): | |
| patch0[key].dataset = aggregate(particles[key]) | |
| patch_levels[level_idx].patches = [patch0] # just one patch | |
| return hier |
Tools
Ruff
284-284: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
290-290: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
295-295: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
298-298: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
Optimize dictionary key checks.
Use key in dict instead of key in dict.keys() for better performance and readability.
- for level_idx in patch_levels.keys():
+ for level_idx in patch_levels:
- for key in patch0.patch_datas.keys():
+ for key in patch0.patch_datas:
- for patch_data_key in patch.patch_datas.keys():
+ for patch_data_key in patch.patch_datas:
- for key in particles.keys():
+ for key in particles:Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def single_patch_per_level_per_pop_from(hier, only_keep_L0=True): # dragons | |
| for tidx in hier.times(): | |
| if only_keep_L0: | |
| hier.time_hier[tidx] = {0: hier.time_hier[tidx][0]} | |
| patch_levels = hier.time_hier[tidx] | |
| for level_idx in patch_levels.keys(): | |
| patch_level = patch_levels[level_idx] | |
| patch0 = patch_level.patches[0] | |
| particles = {} # str:[] | |
| for key in patch0.patch_datas.keys(): | |
| if isinstance(patch0[key].dataset, Particles): | |
| particles[key] = [] | |
| for patch in patch_level.patches: | |
| for patch_data_key in patch.patch_datas.keys(): | |
| particles[key] += [patch[patch_data_key].dataset] | |
| for key in particles.keys(): | |
| patch0[key].dataset = aggregate(particles[key]) | |
| patch_levels[level_idx].patches = [patch0] # just one patch | |
| return hier | |
| def single_patch_per_level_per_pop_from(hier, only_keep_L0=True): # dragons | |
| for tidx in hier.times(): | |
| if only_keep_L0: | |
| hier.time_hier[tidx] = {0: hier.time_hier[tidx][0]} | |
| patch_levels = hier.time_hier[tidx] | |
| for level_idx in patch_levels: | |
| patch_level = patch_levels[level_idx] | |
| patch0 = patch_level.patches[0] | |
| particles = {} # str:[] | |
| for key in patch0.patch_datas: | |
| if isinstance(patch0[key].dataset, Particles): | |
| particles[key] = [] | |
| for patch in patch_level.patches: | |
| for patch_data_key in patch.patch_datas: | |
| particles[key] += [patch[patch_data_key].dataset] | |
| for key in particles: | |
| patch0[key].dataset = aggregate(particles[key]) | |
| patch_levels[level_idx].patches = [patch0] # just one patch | |
| return hier |
Tools
Ruff
284-284: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
290-290: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
295-295: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
298-298: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
| print(list(patch_cmp.patch_datas.keys())) | ||
| return EqualityReport(False, "data keys mismatch") | ||
|
|
||
| for patch_data_key in patch_ref.patch_datas.keys(): |
There was a problem hiding this comment.
Remove unnecessary .keys() call.
Use key in dict instead of key in dict.keys().
- for patch_data_key in patch_ref.patch_datas.keys():
+ for patch_data_key in patch_ref.patch_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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for patch_data_key in patch_ref.patch_datas.keys(): | |
| for patch_data_key in patch_ref.patch_datas: |
Tools
Ruff
1848-1848: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
| def __eq__(self, that): | ||
| return self.name == that.name and self.dataset == that.dataset |
There was a problem hiding this comment.
Use np.array_equal for robust array comparison in numpy arrays.
The current implementation using self.dataset == that.dataset might not behave as expected for numpy arrays since it returns an array of booleans rather than a single boolean value.
- return self.name == that.name and self.dataset == that.dataset
+ return self.name == that.name and np.array_equal(self.dataset, that.dataset)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __eq__(self, that): | |
| return self.name == that.name and self.dataset == that.dataset | |
| def __eq__(self, that): | |
| return self.name == that.name and np.array_equal(self.dataset, that.dataset) |
| if patch_levels_ref.keys() != patch_levels_cmp.keys(): | ||
| return EqualityReport(False, "levels mismatch") | ||
|
|
||
| for level_idx in patch_levels_cmp.keys(): |
There was a problem hiding this comment.
Remove unnecessary .keys() call.
Use key in dict instead of key in dict.keys().
- for level_idx in patch_levels_cmp.keys():
+ for level_idx in patch_levels_cmp:Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for level_idx in patch_levels_cmp.keys(): | |
| for level_idx in patch_levels_cmp: |
Tools
Ruff
1835-1835: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
| #ifndef PHARE_LOAD_BALANCER_ESTIMATOR_HYBRID_HPP | ||
| #define PHARE_LOAD_BALANCER_ESTIMATOR_HYBRID_HPP |
There was a problem hiding this comment.
Use pragma once instead of include guards.
Modern C++ prefers #pragma once over traditional include guards for simplicity and to avoid potential issues with multiple inclusions.
#pragma once| def _parse_rank(patch_id): | ||
| return patch_id.split("#")[0] | ||
|
|
||
| for ilvl, lvl in hier.levels().items(): |
There was a problem hiding this comment.
Rename unused loop variable ilvl.
The loop control variable ilvl is not used within the loop body. Rename it to _ilvl to indicate it's unused.
- for ilvl, lvl in hier.levels().items():
+ for _ilvl, lvl in hier.levels().items():Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for ilvl, lvl in hier.levels().items(): | |
| for _ilvl, lvl in hier.levels().items(): |
Tools
Ruff
192-192: Loop control variable
ilvlnot used within loop bodyRename unused
ilvlto_ilvl(B007)
| timestamps = [x * time_step for x in range(time_step_nbr + 1)] | ||
|
|
||
|
|
||
| def config(diag_dir, loadbalancing={}): |
There was a problem hiding this comment.
Avoid mutable default arguments.
Using mutable data structures as default arguments can lead to unexpected behavior. Replace loadbalancing={} with None and initialize within the function.
- def config(diag_dir, loadbalancing={}):
+ def config(diag_dir, loadbalancing=None):
if loadbalancing is None:
loadbalancing = {}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def config(diag_dir, loadbalancing={}): | |
| def config(diag_dir, loadbalancing=None): | |
| if loadbalancing is None: | |
| loadbalancing = {} |
Tools
Ruff
35-35: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
|
|
||
| for ilvl, lvl in hier.levels().items(): | ||
| for patch in lvl: | ||
| for pd_key, pd in patch.patch_datas.items(): |
There was a problem hiding this comment.
Rename unused loop variable pd_key.
The loop control variable pd_key is not used within the loop body. Rename it to _pd_key to indicate it's unused.
- for pd_key, pd in patch.patch_datas.items():
+ for _pd_key, pd in patch.patch_datas.items():Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for pd_key, pd in patch.patch_datas.items(): | |
| for _pd_key, pd in patch.patch_datas.items(): |
Tools
Ruff
194-194: Loop control variable
pd_keynot used within loop bodyRename unused
pd_keyto_pd_key(B007)
| def tearDown(self): | ||
| ph.global_vars.sim = None | ||
|
|
||
| def run_sim(self, diags_dir, dic={}): |
There was a problem hiding this comment.
Avoid mutable default arguments.
Using mutable data structures as default arguments can lead to unexpected behavior. Replace dic={} with None and initialize within the function.
- def run_sim(self, diags_dir, dic={}):
+ def run_sim(self, diags_dir, dic=None):
if dic is None:
dic = {}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def run_sim(self, diags_dir, dic={}): | |
| def run_sim(self, diags_dir, dic=None): | |
| if dic is None: | |
| dic = {} |
Tools
Ruff
205-205: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
| void setLoadBalancerManager(std::unique_ptr<amr::LoadBalancerManager<dimension>> lbm) | ||
| { | ||
| load_balancer_manager_ = std::move(lbm); | ||
| } |
There was a problem hiding this comment.
Validate input lbm for nullptr before assignment.
The function should validate the input lbm for nullptr before assignment to prevent setting an uninitialized manager.
+ if (!lbm) throw std::invalid_argument("LoadBalancerManager cannot be null");
load_balancer_manager_ = std::move(lbm);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void setLoadBalancerManager(std::unique_ptr<amr::LoadBalancerManager<dimension>> lbm) | |
| { | |
| load_balancer_manager_ = std::move(lbm); | |
| } | |
| void setLoadBalancerManager(std::unique_ptr<amr::LoadBalancerManager<dimension>> lbm) | |
| { | |
| if (!lbm) throw std::invalid_argument("LoadBalancerManager cannot be null"); | |
| load_balancer_manager_ = std::move(lbm); | |
| } |
Summary by CodeRabbit
New Features
Simulatorclass with load balancing capabilities.Bug Fixes
Improvements
Tests
Chores