python module per simulator template permutation#1047
python module per simulator template permutation#1047nicolasaunai merged 6 commits intoPHAREHUB:masterfrom
Conversation
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughIntroduces a SamraiLifeCycle class and migrates SAMRAI usage to this lifecycle. Refactors Python C++ bindings from per-dimension modules to per-simulator-ID pybind modules, centralizes cpp access in a pyphare.cpp namespace, and updates build/test CMake targets and many tests to the new interfaces. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Py as Python test/script
participant Pyphare as pyphare.cpp (module loader)
participant PB as pybind cpp_<sim> module
participant PH as PHARE SamraiLifeCycle
participant SAM as SAMRAI
Py->>Pyphare: import cpp
Pyphare->>PB: load cpp_<sim> (simulator_id)
PB->>PH: SamraiLifeCycle() / make_hierarchy()
PH->>SAM: initialize MPI & SAMRAI manager (rgba(70,130,180,0.5))
Py->>PB: make_simulator(hierarchy)
PB->>PH: access getDatabase/getRestartManager
PH->>SAM: delegate calls (rgba(34,139,34,0.5))
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PhilipDeegan
force-pushed
the
binpersim
branch
15 times, most recently
from
ca57af8 to
3d0cf5b
Compare
PhilipDeegan
force-pushed
the
binpersim
branch
from
3d0cf5b to
bb02c3c
Compare
PhilipDeegan
force-pushed
the
binpersim
branch
5 times, most recently
from
7bb9995 to
be7ebda
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
PhilipDeegan
force-pushed
the
binpersim
branch
from
be7ebda to
717ecf5
Compare
PhilipDeegan
force-pushed
the
binpersim
branch
from
bb841ee to
1533f1d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/utilities/meta/meta_utilities.hpp (1)
173-178: Null pointer dereference in phare.cpp: getSimulator() result lacks null check.The concern is valid. The
makeAtRuntimeoverload at lines 173-178 can return a null pointer when the requestednbRefinedPartvalue is not inphare_exe_default_simulators()(which contains only 2 for dim=1 and 4 for dim=2, versus the broader set inpossibleSimulators()).Critical issue: In
src/phare/phare.cpplines 95-97, the result ofgetSimulator()is dereferenced immediately without null checking:auto simulator = PHARE::getSimulator(hierarchy); std::cout << PHARE::core::to_str(*simulator) << "\n";This will segfault if a user configuration requests an unsupported
nbRefinedPartvalue. In contrast,data_wrangler.hppline 180 correctly handles the null case withif (!simulator_ptr) throw. Thephare.cppcall site must either validate that requested parameters exist in the default set or add explicit null handling and error reporting.src/python3/cpp_simulator.hpp (1)
131-136: Missingextern "C"linkage for ASAN callback function.The
__asan_default_optionsfunction must have C linkage for ASAN to detect and invoke it. Withoutextern "C", C++ name mangling prevents the sanitizer runtime from finding this symbol, rendering the leak detection suppression ineffective.🔧 Suggested fix
// https://stackoverflow.com/a/51061314/795574 // ASAN detects leaks by default, even in system/third party libraries -inline char const* __asan_default_options() +extern "C" inline char const* __asan_default_options() { return "detect_leaks=0"; }
🧹 Nitpick comments (11)
tests/core/data/field/test_field.hpp (1)
104-104: Minor nit: Header guard comment mismatch.The closing comment references
PHARE_TEST_CORE_FIELD_TEST_Hbut the actual guard defined at line 1-2 isPHARE_TEST_CORE_FIELD_TEST_HPP.Suggested fix
-#endif /* PHARE_TEST_CORE_FIELD_TEST_H */ +#endif /* PHARE_TEST_CORE_FIELD_TEST_HPP */src/core/utilities/meta/meta_utilities.hpp (1)
115-133: New helper for reduced template instantiation set looks correct.The function returns a minimal subset of simulator configurations (one particle count per dim/interp combination), which aligns with the PR objective of reducing module permutations. The chosen values (2 for 1D, 4 for 2D, 6 for 3D) are the minimums from
possibleSimulators().Consider whether the informal comment at line 118 ("feel free to change as you wish") provides sufficient guidance. A brief note explaining that this controls which template instantiations are compiled into the executable might help future maintainers understand the distinction from
possibleSimulators().src/diagnostic/CMakeLists.txt (1)
29-29: Consider using modernendif()syntax.The
endif (HighFive)form is valid but deprecated in favor of the argument-lessendif()style in modern CMake (3.0+). This is inconsistent with line 21 which usesendif().♻️ Suggested change for consistency
-endif (HighFive) +endif()src/python3/CMakeLists.txt (1)
5-7: Consider validating the permutations file exists.If
PHARE_PERMUTATIONSpoints to a non-existent file,file(STRINGS ...)will silently produce an empty list, resulting in no modules being built. This could lead to confusing build failures later.🛠️ Proposed fix to add file validation
if(NOT PHARE_PERMUTATIONS) set(PHARE_PERMUTATIONS "${PHARE_PROJECT_DIR}/res/sim/all.txt") endif() + +if(NOT EXISTS ${PHARE_PERMUTATIONS}) + message(FATAL_ERROR "PHARE_PERMUTATIONS file not found: ${PHARE_PERMUTATIONS}") +endif()pyphare/pyphare/cpp/__init__.py (2)
27-28: Consider cachingcpp_etcmodule for consistency.Unlike
cpp_lib()which caches in_libs,cpp_etc_lib()callsimportlib.import_module()on every invocation. While Python'ssys.moduleshandles caching internally, storing the result in a module-level variable would align with the caching pattern used for simulation-specific modules.♻️ Optional: Cache cpp_etc module
+_cpp_etc = None + def cpp_etc_lib(): + global _cpp_etc + if _cpp_etc is None: + _cpp_etc = importlib.import_module("pybindlibs.cpp_etc") + return _cpp_etc - return importlib.import_module("pybindlibs.cpp_etc")
39-56: Replacegetattrwith direct attribute access.Static analysis correctly identifies that using
getattrwith constant string attributes is unnecessary. Direct attribute access is cleaner and more readable.♻️ Proposed refactor
def splitter_type(sim): - return getattr(cpp_lib(sim), "Splitter") + return cpp_lib(sim).Splitter def split_pyarrays_fn(sim): - return getattr(cpp_lib(sim), "split_pyarray_particles") + return cpp_lib(sim).split_pyarray_particles def mpi_rank(): - return getattr(cpp_etc_lib(), "mpi_rank")() + return cpp_etc_lib().mpi_rank() def mpi_size(): - return getattr(cpp_etc_lib(), "mpi_size")() + return cpp_etc_lib().mpi_size() def mpi_barrier(): - return getattr(cpp_etc_lib(), "mpi_barrier")() + return cpp_etc_lib().mpi_barrier()pyphare/pyphare/data/wrangler.py (1)
5-14: Simplifygetattrto direct attribute access and remove unused instance attributes.The three instance attributes (
self.dim,self.interp,self.refined_particle_nbr) are assigned in__init__but are never used in any method of theDataWranglerclass. Additionally, thegetattrcall with a constant string can be replaced with direct attribute access:♻️ Proposed refactor
def __init__(self, simulator): from pyphare import cpp sim = simulator.simulation - self.dim = sim.ndim - self.interp = sim.interp_order - self.refined_particle_nbr = sim.refined_particle_nbr - self.cpp = getattr(cpp.cpp_lib(sim), "DataWrangler")( + self.cpp = cpp.cpp_lib(sim).DataWrangler( simulator.cpp_sim, simulator.cpp_hier )res/cmake/options.cmake (1)
103-108: New options not added toprint_phare_options().Per the file's own guidance on line 2, new options should be added to
print_phare_options(). Consider adding these for discoverability:message("Build SAMRAI shared libraries : " ${SAMRAI_BUILD_SHARED_LIBS}) message("Build PHARE EXE : " ${PHARE_EXE})src/python3/cpp_simulator.cpp (1)
5-7: Compile-time guard is appropriate; consider adding an error message.The guard correctly enforces required macro definitions. However, the
#errordirective would be more informative with a message string:#error "PHARE_SIM_STR and PHARE_SIM_ID must be defined, e.g., PHARE_SIM_STR=\"1, 1, 1\""The static analysis hint is a false positive—the
#erroris intentional compile-time enforcement.res/cmake/dep/samrai.cmake (1)
46-46: Consider modern CMake style forendif().Modern CMake style prefers plain
endif()without repeating the condition. This is a minor stylistic suggestion.Suggested change
-endif(DEFINED SAMRAI_ROOT) +endif()src/python3/cpp_simulator.hpp (1)
1-7: Minor header guard comment inconsistency.Line 140's closing comment says
PHARE_PYTHON_CPP_SIMULATOR_Hbut the actual guard isPHARE_PYTHON_CPP_SIMULATOR_HPP. This doesn't affect functionality but could cause confusion.✏️ Suggested fix
-#endif /*PHARE_PYTHON_CPP_SIMULATOR_H*/ +#endif /*PHARE_PYTHON_CPP_SIMULATOR_HPP*/
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (75)
.github/workflows/cmake_macos.ymlCMakeLists.txtpyphare/pyphare/cpp/__init__.pypyphare/pyphare/cpp/validate.pypyphare/pyphare/data/wrangler.pypyphare/pyphare/pharesee/hierarchy/fromh5.pypyphare/pyphare/pharesee/particles.pypyphare/pyphare/simulator/simulator.pyres/cmake/bench.cmakeres/cmake/dep/highfive.cmakeres/cmake/dep/samrai.cmakeres/cmake/options.cmakeres/sim/all.txtsrc/amr/CMakeLists.txtsrc/amr/data/particles/particles_data.hppsrc/amr/load_balancing/load_balancer_manager.hppsrc/amr/multiphysics_integrator.hppsrc/amr/resources_manager/resources_manager.hppsrc/amr/samrai.cppsrc/amr/samrai.hppsrc/amr/wrappers/hierarchy.hppsrc/amr/wrappers/integrator.hppsrc/core/data/grid/gridlayout.hppsrc/core/utilities/meta/meta_utilities.hppsrc/diagnostic/CMakeLists.txtsrc/phare/CMakeLists.txtsrc/phare/phare.cppsrc/phare/phare.hppsrc/python3/CMakeLists.txtsrc/python3/cpp_etc.cppsrc/python3/cpp_simulator.cppsrc/python3/cpp_simulator.hppsrc/python3/patch_level.hppsrc/simulator/simulator.hpptests/amr/data/field/coarsening/CMakeLists.txttests/amr/data/field/copy_pack/CMakeLists.txttests/amr/data/field/geometry/CMakeLists.txttests/amr/data/field/refine/CMakeLists.txttests/amr/data/field/time_interpolate/CMakeLists.txttests/amr/data/field/variable/CMakeLists.txttests/amr/data/particles/copy/CMakeLists.txttests/amr/data/particles/copy_overlap/CMakeLists.txttests/amr/data/particles/stream_pack/CMakeLists.txttests/amr/messengers/CMakeLists.txttests/amr/messengers/test_messengers.cpptests/amr/multiphysics_integrator/CMakeLists.txttests/amr/tagging/CMakeLists.txttests/amr/tagging/test_tagging.cpptests/core/data/field/test_field.hpptests/functional/alfven_wave/alfven_wave1d.pytests/functional/harris/harris_2d.pytests/functional/ionIonBeam/ion_ion_beam1d.pytests/functional/shock/shock.pytests/functional/tdtagged/td1dtagged.pytests/simulator/__init__.pytests/simulator/data_wrangler.pytests/simulator/initialize/test_particles_init_1d.pytests/simulator/per_test.hpptests/simulator/refined_particle_nbr.pytests/simulator/refinement/test_2d_10_core.pytests/simulator/refinement/test_2d_2_core.pytests/simulator/test_advance.pytests/simulator/test_diagnostic_timestamps.pytests/simulator/test_diagnostics.pytests/simulator/test_initialization.pytests/simulator/test_load_balancing.pytests/simulator/test_restarts.pytests/simulator/test_run.pytests/simulator/test_tagging.py.offtests/simulator/test_validation.pytools/bench/amr/data/particles/copy_data.cpptools/bench/hi5/write_particles.cpptools/bench/real/CMakeLists.txttools/bench/real/bench_harris.pytools/bench/real/sim/sim_2_1_4.cpp
💤 Files with no reviewable changes (9)
- .github/workflows/cmake_macos.yml
- src/phare/phare.hpp
- tests/simulator/test_validation.py
- res/cmake/bench.cmake
- tools/bench/real/CMakeLists.txt
- src/core/data/grid/gridlayout.hpp
- tests/simulator/data_wrangler.py
- tools/bench/real/bench_harris.py
- tools/bench/real/sim/sim_2_1_4.cpp
✅ Files skipped from review due to trivial changes (2)
- tests/simulator/test_initialization.py
- src/python3/patch_level.hpp
🚧 Files skipped from review as they are similar to previous changes (23)
- tests/amr/messengers/test_messengers.cpp
- tests/simulator/test_run.py
- res/cmake/dep/highfive.cmake
- src/amr/multiphysics_integrator.hpp
- src/phare/phare.cpp
- pyphare/pyphare/pharesee/hierarchy/fromh5.py
- tests/simulator/test_restarts.py
- res/sim/all.txt
- tests/amr/tagging/CMakeLists.txt
- tests/simulator/per_test.hpp
- tests/amr/tagging/test_tagging.cpp
- tests/simulator/test_diagnostics.py
- src/amr/data/particles/particles_data.hpp
- tests/functional/alfven_wave/alfven_wave1d.py
- tests/amr/data/field/copy_pack/CMakeLists.txt
- src/amr/samrai.hpp
- src/amr/load_balancing/load_balancer_manager.hpp
- tests/simulator/refinement/test_2d_10_core.py
- tests/simulator/test_advance.py
- src/amr/wrappers/integrator.hpp
- tests/functional/harris/harris_2d.py
- tests/simulator/test_tagging.py.off
- tests/functional/ionIonBeam/ion_ion_beam1d.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.hpp
⚙️ CodeRabbit configuration file
Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Files:
src/amr/resources_manager/resources_manager.hpptests/core/data/field/test_field.hppsrc/simulator/simulator.hppsrc/amr/wrappers/hierarchy.hppsrc/python3/cpp_simulator.hppsrc/core/utilities/meta/meta_utilities.hpp
🧠 Learnings (10)
📓 Common learnings
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1047
File: pyphare/pyphare/simulator/simulator.py:35-47
Timestamp: 2026-01-05T10:21:13.509Z
Learning: In pyphare/pyphare/simulator/simulator.py, SamraiLifeCycle is designed as a singleton that lives for the entire program lifetime because MPI can only be started once per process. The instance in life_cycles["samrai"] is intentionally not removed after reset(), and the static SamraiLifeCycle::reset() method reinitializes SAMRAI state to support multiple sequential simulations without MPI re-initialization.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-07-26T22:04:34.160Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-10-09T08:32:15.667Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1068
File: src/amr/data/field/coarsening/electric_field_coarsener.hpp:1-2
Timestamp: 2025-09-17T13:35:11.533Z
Learning: PhilipDeegan prefers header guard names that include the full directory path structure, following the pattern PHARE_[PATH_WITH_UNDERSCORES]_HPP. For example, a file at src/amr/data/field/coarsening/electric_field_coarsener.hpp should use PHARE_AMR_DATA_FIELD_COARSENING_ELECTRIC_FIELD_COARSENER_HPP as the header guard.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-10-09T08:32:15.667Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-09-05T17:02:58.784Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
📚 Learning: 2025-07-09T17:18:05.771Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/amr/CMakeLists.txt:103-105
Timestamp: 2025-07-09T17:18:05.771Z
Learning: In the PHARE project, `HighFive` is a CMake option defined in `res/cmake/options.cmake` as `option(HighFive "Build with highfive usage" ON)`, not a target that would be created by find_package(). The condition `if (HighFive)` correctly checks this option.
Applied to files:
src/diagnostic/CMakeLists.txtsrc/phare/CMakeLists.txtres/cmake/dep/samrai.cmakesrc/amr/CMakeLists.txtres/cmake/options.cmakeCMakeLists.txt
📚 Learning: 2025-02-07T14:35:14.630Z
Learnt from: nicolasaunai
Repo: PHAREHUB/PHARE PR: 591
File: res/cmake/options.cmake:73-75
Timestamp: 2025-02-07T14:35:14.630Z
Learning: In CMake configuration for PHARE, `lowResourceTests` and `highResourceTests` options must be mutually exclusive and cannot be enabled simultaneously.
Applied to files:
src/diagnostic/CMakeLists.txtsrc/phare/CMakeLists.txttests/amr/multiphysics_integrator/CMakeLists.txtres/cmake/options.cmaketests/amr/data/field/refine/CMakeLists.txttests/amr/data/field/variable/CMakeLists.txttests/amr/data/field/coarsening/CMakeLists.txttests/amr/data/particles/copy/CMakeLists.txttests/amr/messengers/CMakeLists.txt
📚 Learning: 2025-09-01T09:41:40.618Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1056
File: tests/core/numerics/interpolator/test_main.cpp:314-318
Timestamp: 2025-09-01T09:41:40.618Z
Learning: The PHARE project uses C++20, which makes the typename keyword optional for dependent types in obvious contexts. The project prefers minimal code without unnecessary keywords, so typename suggestions for dependent types should be avoided when the code compiles successfully in C++20.
Applied to files:
src/phare/CMakeLists.txtsrc/amr/resources_manager/resources_manager.hpptools/bench/hi5/write_particles.cpp
📚 Learning: 2025-11-10T09:37:57.021Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1084
File: src/diagnostic/diagnostic_model_view.hpp:219-221
Timestamp: 2025-11-10T09:37:57.021Z
Learning: In PHARE, temporary fields like tmpField_, tmpVec_, and tmpTensor_ in src/diagnostic/diagnostic_model_view.hpp use a name-based resource allocation pattern. Fields with names "PHARE_sumField", "PHARE_sumVec", and "PHARE_sumTensor" are allocated globally elsewhere in the resources manager (e.g., in src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp), and components reference the same storage through identically-named field objects. These do not require local allocation via rm.enumerate().
Applied to files:
src/amr/resources_manager/resources_manager.hpptools/bench/hi5/write_particles.cpp
📚 Learning: 2025-09-17T13:35:11.533Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1068
File: src/amr/data/field/coarsening/electric_field_coarsener.hpp:1-2
Timestamp: 2025-09-17T13:35:11.533Z
Learning: PhilipDeegan prefers header guard names that include the full directory path structure, following the pattern PHARE_[PATH_WITH_UNDERSCORES]_HPP. For example, a file at src/amr/data/field/coarsening/electric_field_coarsener.hpp should use PHARE_AMR_DATA_FIELD_COARSENING_ELECTRIC_FIELD_COARSENER_HPP as the header guard.
Applied to files:
tests/core/data/field/test_field.hppsrc/amr/wrappers/hierarchy.hpptools/bench/hi5/write_particles.cpp
📚 Learning: 2024-10-16T09:54:01.455Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 898
File: src/amr/data/particles/refine/particles_data_split.hpp:181-182
Timestamp: 2024-10-16T09:54:01.455Z
Learning: The codebase uses C++17 features such as class template argument deduction, allowing template arguments to be omitted in declarations like `std::array`.
Applied to files:
tests/core/data/field/test_field.hpp
📚 Learning: 2026-01-05T10:21:13.509Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1047
File: pyphare/pyphare/simulator/simulator.py:35-47
Timestamp: 2026-01-05T10:21:13.509Z
Learning: In pyphare/pyphare/simulator/simulator.py, SamraiLifeCycle is designed as a singleton that lives for the entire program lifetime because MPI can only be started once per process. The instance in life_cycles["samrai"] is intentionally not removed after reset(), and the static SamraiLifeCycle::reset() method reinitializes SAMRAI state to support multiple sequential simulations without MPI re-initialization.
Applied to files:
tools/bench/amr/data/particles/copy_data.cppsrc/amr/wrappers/hierarchy.hppsrc/python3/cpp_simulator.hpppyphare/pyphare/simulator/simulator.pysrc/amr/samrai.cpp
📚 Learning: 2024-10-18T13:23:32.074Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 910
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:7-7
Timestamp: 2024-10-18T13:23:32.074Z
Learning: In the `pyphare.pharesee.hierarchy` module, importing `PatchHierarchy` and `format_timestamp` from `hierarchy.py` into `hierarchy_utils.py` is acceptable as long as `hierarchy.py` does not import `hierarchy_utils.py`, thereby avoiding a cyclic import.
Applied to files:
pyphare/pyphare/cpp/__init__.py
📚 Learning: 2024-09-09T13:57:02.285Z
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:566-568
Timestamp: 2024-09-09T13:57:02.285Z
Learning: Avoid commenting on draft PRs for PhilipDeegan unless explicitly requested.
Applied to files:
src/python3/cpp_simulator.hpp
🧬 Code graph analysis (15)
pyphare/pyphare/cpp/validate.py (1)
pyphare/pyphare/cpp/__init__.py (2)
mpi_rank(47-48)mpi_barrier(55-56)
src/amr/resources_manager/resources_manager.hpp (1)
src/amr/samrai.cpp (4)
getDatabase(43-46)getDatabase(43-43)getPatchDataRestartManager(48-51)getPatchDataRestartManager(48-48)
tests/simulator/test_load_balancing.py (1)
pyphare/pyphare/pharein/__init__.py (1)
NO_GUI(60-64)
tests/functional/shock/shock.py (2)
pyphare/pyphare/simulator/simulator.py (1)
startMPI(43-45)pyphare/pyphare/pharein/__init__.py (1)
NO_GUI(60-64)
tests/simulator/__init__.py (1)
pyphare/pyphare/cpp/__init__.py (3)
mpi_size(51-52)mpi_barrier(55-56)mpi_rank(47-48)
pyphare/pyphare/pharesee/particles.py (1)
pyphare/pyphare/cpp/__init__.py (1)
split_pyarrays_fn(43-44)
src/python3/cpp_simulator.cpp (1)
src/python3/cpp_simulator.hpp (2)
declare_macro_sim(105-125)declare_macro_sim(105-105)
src/amr/wrappers/hierarchy.hpp (1)
src/amr/samrai.cpp (4)
getRestartManager(54-57)getRestartManager(54-54)getPatchDataRestartManager(48-51)getPatchDataRestartManager(48-48)
pyphare/pyphare/cpp/__init__.py (3)
pyphare/pyphare/core/gridlayout.py (1)
ndim(221-222)pyphare/pyphare/simulator/simulator.py (1)
interp_order(277-279)src/simulator/simulator.hpp (1)
interp_order(106-106)
tests/simulator/refined_particle_nbr.py (3)
pyphare/pyphare/simulator/simulator.py (1)
Simulator(66-299)tests/simulator/__init__.py (2)
NoOverwriteDict(22-31)populate_simulation(147-162)pyphare/pyphare/cpp/__init__.py (1)
splitter_type(39-40)
src/python3/cpp_simulator.hpp (1)
src/simulator/simulator.hpp (3)
name(133-133)makeSimulator(521-524)makeSimulator(521-521)
pyphare/pyphare/simulator/simulator.py (2)
pyphare/pyphare/cpp/__init__.py (4)
cpp_lib(18-24)cpp_etc_lib(27-28)mpi_rank(47-48)mpi_barrier(55-56)pyphare/pyphare/simulator/monitoring.py (3)
timing_setup(60-65)setup_monitoring(40-49)monitoring_shutdown(52-57)
src/core/utilities/meta/meta_utilities.hpp (1)
src/core/utilities/types.hpp (2)
apply(97-100)apply(97-97)
src/amr/samrai.cpp (3)
src/amr/samrai.hpp (2)
SamraiLifeCycle(36-36)SamraiLifeCycle(38-38)src/simulator/simulator.hpp (2)
initialize(391-431)initialize(391-391)src/amr/wrappers/hierarchy.hpp (1)
getRestartManager(97-97)
src/python3/cpp_etc.cpp (3)
src/core/data/particles/particle_array.hpp (2)
dim(21-65)dim(251-288)src/amr/samrai.cpp (2)
reset(33-40)reset(33-33)src/amr/wrappers/hierarchy.hpp (4)
Hierarchy(136-141)Hierarchy(231-244)make(215-227)make(215-215)
🪛 Cppcheck (2.19.0)
src/python3/cpp_simulator.cpp
[error] 6-6: #error
(preprocessorErrorDirective)
src/amr/samrai.cpp
[error] 18-18: There is an unknown macro here somewhere. Configuration is required. If ENABLE_WARNING is a macro then please configure it.
(unknownMacro)
[error] 21-21: syntax error
(syntaxError)
src/python3/cpp_etc.cpp
[error] 18-18: There is an unknown macro here somewhere. Configuration is required. If ENABLE_WARNING is a macro then please configure it.
(unknownMacro)
[error] 14-14: syntax error
(syntaxError)
🪛 Ruff (0.14.10)
pyphare/pyphare/cpp/__init__.py
40-40: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
44-44: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
48-48: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
52-52: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
56-56: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
pyphare/pyphare/simulator/simulator.py
125-125: Consider moving this statement to an else block
(TRY300)
126-126: Do not catch blind exception: Exception
(BLE001)
pyphare/pyphare/data/wrangler.py
12-12: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
🔇 Additional comments (69)
tests/simulator/initialize/test_particles_init_1d.py (1)
10-13: LGTM! Removal of unusedcpp_libimport and instantiation.The
cppvariable was not used anywhere in this test file. Removing the dead import and instantiation aligns with the broader refactoring to a module-level cpp interface.tests/core/data/field/test_field.hpp (1)
7-10: Include reorganization looks good.The gtest include with the explanatory comment clarifying its usage for
EXPECT_FLOAT_EQis helpful for future readers.tools/bench/hi5/write_particles.cpp (1)
6-7: Include change correctly centralizes SAMRAI lifecycle management.The switch from
phare/phare.hpptoamr/samrai.hppis correct. ThePHARE::SamraiLifeCycleclass is properly defined inamr/samrai.hppwith the expected constructor signature, and the usage on line 78 aligns correctly. The IWYU pragma is appropriate.tools/bench/amr/data/particles/copy_data.cpp (2)
87-93: LGTM!The
SamraiLifeCycleRAII wrapper correctly initializes SAMRAI/MPI before benchmark execution and ensures proper cleanup. Based on learnings, this singleton pattern is required because MPI can only be started once per process.
2-2: Include update is correct and properly exposes SamraiLifeCycle.The change from
"phare/phare.hpp"to"amr/samrai.hpp"correctly centralizes SAMRAI lifecycle management. Thesrc/amr/samrai.hppheader properly definesSamraiLifeCyclein thePHARE::namespace, matching the usage on line 89. TheIWYU pragma: keepis appropriate as the include is necessary for this class.src/core/utilities/meta/meta_utilities.hpp (1)
141-148: Usage change is consistent with the new reduced simulator set.Switching from
possibleSimulators()tophare_exe_default_simulators()limits runtime instantiation to the reduced configuration set. This is intentional per the PR objectives.CMakeLists.txt (1)
23-23: LGTM - Git is now a hard requirement.This will cause configuration to fail on systems without
gitinstalled. Ensure this is documented if not already, as it's a breaking change for minimal build environments.tests/amr/messengers/CMakeLists.txt (1)
30-31: LGTM - Formatting only.tests/amr/multiphysics_integrator/CMakeLists.txt (1)
25-26: LGTM - Formatting only.tests/amr/data/particles/stream_pack/CMakeLists.txt (1)
22-25: LGTM - Addedphare_initializerdependency.Consistent with the broader PR pattern of adding this dependency across test targets.
tests/amr/data/field/time_interpolate/CMakeLists.txt (1)
16-19: LGTM - Addedphare_initializerdependency.Consistent with the broader PR pattern of adding this dependency across test targets.
tests/simulator/test_diagnostic_timestamps.py (1)
5-10: LGTM! Import cleanup looks correct.The removal of unused
cpp_libimports and formatting adjustments align with the PR-wide migration to the module-levelcppnamespace. The test correctly usesstartMPI()from the simulator module.tests/amr/data/field/coarsening/CMakeLists.txt (1)
21-24: LGTM! Consistent with PR-wide linkage updates.Adding
phare_initializerto the test target aligns with the broader initialization refactoring across the test suite.tests/amr/data/particles/copy/CMakeLists.txt (1)
22-25: LGTM!Consistent linkage update matching the pattern applied across other test targets.
pyphare/pyphare/pharesee/particles.py (1)
160-165: LGTM! Cleaner sim-centric API.The refactored call to
split_pyarrays_fn(sim)simplifies the interface by passing the simulation object directly, which internally resolves to the appropriate C++ binding viacpp_lib(sim). This aligns with the PR-wide transition to sim-centric access patterns.src/diagnostic/CMakeLists.txt (1)
24-25: LGTM! Unconditional phare_amr linkage is appropriate.Adding
phare_amras an unconditional INTERFACE dependency ensures the diagnostic library has access to AMR functionality regardless of HighFive configuration.tests/amr/data/field/refine/CMakeLists.txt (1)
26-30: LGTM!The addition of
phare_initializerto the link libraries is consistent with the broader PR changes that unify initializer dependencies across test targets. The multi-line formatting improves readability.tests/simulator/test_load_balancing.py (1)
6-19: LGTM!Good changes:
- Adding the missing
numpyimport fixes a latent bug sincenpwas already used throughout the file- Migration to module-level
cppimport aligns with the PR's unified binding approachph.NO_GUI()correctly enables headless operation for CI environmentstests/functional/tdtagged/td1dtagged.py (1)
15-15: LGTM!The migration from
cpp_lib()instance to the module-levelcppimport is consistent with the PR's unified Python-C++ binding approach.tests/amr/data/field/variable/CMakeLists.txt (1)
16-19: LGTM!Adding
phare_initializerto the link libraries is consistent with the PR's broader effort to unify initializer dependencies across test targets.tests/amr/data/field/geometry/CMakeLists.txt (1)
19-22: LGTM!The addition of
phare_initializerfollows the same pattern applied across other test targets in this PR, maintaining consistency in the build configuration.tests/simulator/__init__.py (3)
166-167: LGTM!The local import of
Boxfrompyphare.core.boxis appropriate for this function and avoids a top-level import dependency.
251-253: LGTM!The migration from
cpp_lib()tocpp.mpi_size()correctly aligns with the new module-levelcppinterface introduced in this PR.
256-266: LGTM!The
clean_up_diags_dirsmethod correctly usescpp.mpi_barrier()andcpp.mpi_rank()from the new unifiedcppinterface. The barrier synchronization before and after cleanup ensures safe parallel file operations.src/simulator/simulator.hpp (2)
86-93: LGTM!The constructor/destructor declarations are clean. The destructor correctly guards the
coutbufrestoration with a nullptr check to avoid undefined behavior when logging wasn't redirected.
520-524: LGTM!The
makeSimulatorfactory template provides a clean API for creating simulator instances. It correctly retrieves the globalPHAREDictand constructs the simulator with the provided hierarchy.src/python3/CMakeLists.txt (1)
9-27: LGTM - Clean loop-based module generation.The loop correctly generates per-simulation pybind11 modules. The naming convention (
cpp_<dim>_<interp>_<nbRefinedPart>) and compile definitions (PHARE_SIM_ID,PHARE_SIM_STR) provide clear module identification at both build and runtime.src/amr/CMakeLists.txt (3)
76-77: LGTM!Adding
samrai.hppto the public headers enables the new SAMRAI lifecycle management functionality.
83-87: SHARED library change aligns with PR requirements.The change to
SHAREDlibrary is necessary per the PR description: static SAMRAI libraries cause segfaults. The comment clearly documents the rationale.
92-110: LGTM - Complete SAMRAI linkage and conditional HDF5 support.The additional SAMRAI components (
SAMRAI_hier,SAMRAI_math,SAMRAI_mesh,SAMRAI_pdat,SAMRAI_solv) complete the required SAMRAI dependencies. The conditional HDF5 linkage whenHighFiveis enabled correctly handles SAMRAI's internal HDF5 usage. Based on learnings, theif (HighFive)check correctly references the CMake option defined inres/cmake/options.cmake.tests/amr/data/particles/copy_overlap/CMakeLists.txt (1)
23-26: Explicit dependency is redundant but acceptable.Since
phare_amrpublicly linksphare_initializer(confirmed insrc/amr/CMakeLists.txtline 94), explicitly addingphare_initializerhere is transitively available and technically redundant. However, being explicit serves as defensive coding against potential future linkage changes, making this approach acceptable. This pattern is consistent with similar test files (tests/amr/data/particles/stream_pack/andtests/amr/data/particles/copy/follow the same pattern).tests/simulator/refined_particle_nbr.py (3)
12-16: LGTM! Import updates align with the new sim-centric cpp namespace.The imports correctly reflect the refactored API:
from pyphare import cppfor the centralized namespace andNoOverwriteDict/populate_simulationfromtests.simulator.
41-51: LGTM! Method correctly updated to use sim-centric API.The signature change to accept
simand the use ofcpp.splitter_type(sim)properly integrates with the new per-simulation module loading pattern.
63-67: LGTM! Test flow correctly sequences simulation creation.The flow properly creates the simulation object first via
populate_simulation(), uses it for delta/weight checks, then instantiates theSimulator. This ensures the sim object is available for the cpp module lookup.pyphare/pyphare/cpp/__init__.py (1)
11-24: LGTM! Per-simulation module caching implementation looks correct.The
_libscache withsimulator_id()key generation and lazyimportlib.import_module()loading is a clean approach for the per-simulation module pattern. The global cache ensures modules are only loaded once per unique simulation configuration.pyphare/pyphare/cpp/validate.py (1)
109-113: LGTM! MPI calls correctly use centralized cpp namespace.The refactored calls to
cpp.mpi_rank()andcpp.mpi_barrier()align with the PR's goal of centralizing cpp access through the module-level namespace.src/python3/cpp_etc.cpp (3)
25-54: LGTM! Template binding utilities are well-structured.The
declarePatchDataanddeclareDimtemplates provide clean, reusable pybind11 bindings. TheContiguousParticlesbinding correctly exposesiCell,delta,weight,charge,vas readwrite and thesize()method.
83-92: LGTM! MPI and lifecycle bindings are correctly exposed.The MPI wrappers cleanly delegate to
core::mpifunctions. TheSamraiLifeCyclebinding withreset()method aligns with the singleton pattern for MPI lifetime management. TheAMRHierarchyandmake_hierarchyfactory provide the necessary Python interface for hierarchy creation.Based on learnings, the
SamraiLifeCyclesingleton design is intentional since MPI can only be started once per process.
131-139: LGTM! Dimension bindings correctly registered.All dimension variants (1D, 2D, 3D) are properly registered for both
ContiguousParticlesandPatchDataVectorDouble. This ensures the Python bindings are available regardless of the simulation dimension.The static analysis errors from Cppcheck (lines 14, 18) are false positives caused by conditional compilation macros (
_PHARE_WITH_HIGHFIVE) that the tool cannot parse correctly.src/phare/CMakeLists.txt (1)
5-17: LGTM! The conditional guard correctly gates the executable build.The
if(PHARE_EXE)guard properly makes the executable build optional, aligning with the newPHARE_EXEoption (default OFF) inoptions.cmake.Minor style note: Modern CMake prefers plain
endif()overendif(PHARE_EXE), though both are valid.tests/simulator/refinement/test_2d_2_core.py (1)
14-14: LGTM! Import updated to use the new unifiedcppmodule interface.The change from
cpp_lib()instantiation to directcpp.mpi_rank()calls aligns with the PR-wide refactor centralizing C++ bindings behind a module-level namespace.src/python3/cpp_simulator.cpp (1)
19-22: LGTM! Clean simplification to macro-based binding.The module initialization is now streamlined to a single
declare_macro_sim(m)call, which handles simulator declaration,make_simulatorregistration, anddeclare_etcper the header implementation.src/amr/resources_manager/resources_manager.hpp (3)
9-9: LGTM! New include for SamraiLifeCycle abstraction.The
amr/samrai.hppinclude supports the centralized SAMRAI lifecycle management introduced in this PR.
105-106: LGTM! Database access routed through SamraiLifeCycle.Using
SamraiLifeCycle::getDatabase()centralizes SAMRAI database access, supporting the singleton pattern that manages MPI/SAMRAI initialization ordering. Based on learnings, this design ensures MPI is only started once per process.
299-304: LGTM! PatchDataRestartManager access centralized.Routing through
SamraiLifeCycle::getPatchDataRestartManager()maintains consistency with the lifecycle abstraction pattern used elsewhere.tests/functional/shock/shock.py (3)
4-15: LGTM!The import restructuring and addition of
ph.NO_GUI()for headless operation aligns well with the PR-wide migration to the centralizedcppmodule. The placement ofNO_GUI()at module level ensures matplotlib is configured before any plotting imports are used.
126-126: Migration tocpp.mpi_rank()is consistent.The rank-gated operations correctly use the new
cpp.mpi_rank()API instead of the previouscpp_lib().mpi_rank()pattern, consistent with the centralized cpp namespace approach.Also applies to: 159-159
178-179: Entry point correctly initializes MPI before main.The
startMPI()call beforemain()ensures the SAMRAI lifecycle is properly initialized before any simulation operations.res/cmake/dep/samrai.cmake (3)
7-8: Clean conditional SAMRAI discovery.The root-driven flow is well-structured: external SAMRAI installations via
SAMRAI_ROOT, otherwise fetch and build as a subproject.
27-29: Shared library option aligns with PR constraints.The PR description notes static SAMRAI is not supported (segfaults). This conditional properly exposes
BUILD_SHARED_LIBSonly whenSAMRAI_BUILD_SHARED_LIBSis set.
39-43: Temporary linkage workaround documented.The comment referencing llnl/SAMRAI#294 provides good context for future cleanup. Since this PR remains closed and unmerged, the workaround is still necessary.
src/amr/samrai.cpp (4)
7-23: Well-structured SAMRAI lifecycle initialization.The constructor correctly sequences: MPI init → SAMRAIManager initialize/startup → logger configuration → optional scope timing. The commented debug line is a useful note for future debugging.
25-31: Correct shutdown ordering in destructor.The destructor properly reverses the initialization: scope timer reset → SAMRAI shutdown → SAMRAI finalize → MPI finalize. This ensures resources are released in the correct order.
33-40: Reset method supports sequential simulations.Based on learnings, this design allows multiple sequential simulations without MPI re-initialization. The sequence (reset timers → stop dict handler → restart SAMRAI → clear restart items) is appropriate for reinitializing SAMRAI state.
43-57: Static accessor wrappers centralize SAMRAI singleton access.These methods provide a single point of access for SAMRAI singletons, improving maintainability and making it easier to manage the lifecycle across the codebase.
src/amr/wrappers/hierarchy.hpp (4)
47-48: Private member storage enables proper cleanup.Storing
sim_dictas a private member allows the destructor to access restart IDs for proper unregistration, ensuring symmetry between construction and destruction.Also applies to: 104-105
69-83: Destructor properly mirrors constructor registration.The destructor correctly unregisters the same patch data IDs that were registered in the constructor, preventing resource leaks when HierarchyRestarter goes out of scope.
97-101: Lifecycle-based access replaces direct SAMRAI calls.Using
SamraiLifeCycle::getRestartManager()andgetPatchDataRestartManager()centralizes SAMRAI singleton access, aligning with the broader refactoring.
215-227: Improved error handling in Hierarchy::make().Throwing
std::runtime_erroron failure is better than returning a potentially null pointer. The error message includes relevant parameters for debugging.pyphare/pyphare/simulator/simulator.py (6)
15-15: Centralized cpp import aligns with PR refactoring.The module-level
from pyphare import cppimport centralizes access to the C++ bindings, replacing scatteredcpp_lib()calls throughout the file.
34-40: Simplified make_cpp_simulator signature.The new signature
(cpp_lib, hier)is cleaner than the previous dimension/interp/refinement parameters. The assertion on line 39 ensures the required factory method exists.
43-45: SamraiLifeCycle singleton management.Based on learnings, this correctly ensures the lifecycle instance lives for the entire program since MPI can only be started once per process.
107-131: Setup method properly wires cpp components.The setup flow correctly:
- Initializes MPI via
startMPI()- Validates configuration via
cpp.validate- Creates simulation-specific
cpp_liband hierarchy- Constructs the simulator via
make_cpp_simulatorThe
traceback.format_exc()usage at line 130 is correct for capturing the full exception traceback.Regarding static analysis hints: The bare
Exceptioncatch (BLE001) is intentional here to ensure all errors are logged before re-raising. Thereturnplacement (TRY300) is a matter of style preference.
248-259: Reset correctly uses static reset method.Line 258 calls
type(life_cycles["samrai"]).reset()which invokes the staticSamraiLifeCycle::reset()method. This reinitializes SAMRAI state without affecting the singleton instance, supporting multiple sequential simulations as documented in learnings.
202-202: Monitoring uses stored cpp_lib reference.Using
self.cpp_libinstead of callingcpp_lib()repeatedly is more efficient and ensures consistency throughout the simulation run.Also applies to: 224-224
src/python3/cpp_simulator.hpp (4)
9-26: LGTM!Include organization is clean with appropriate IWYU pragma annotations to prevent accidental removal of necessary headers.
34-49: LGTM!Clean template implementation using forwarding reference for the PyClass parameter. The method bindings are well-structured with appropriate named arguments.
51-102: LGTM!The unified binding surface for DataWrangler, PatchLevel, and Splitter is well-structured. Using
Sim::dimension,Sim::interp_order, andSim::nbRefinedPartfor the Splitter template parameters ensures consistency with the simulator configuration.
105-125: LGTM!The unified entry point cleanly exposes the Simulator type with runtime-queryable properties (
dims,interp_order,refined_particle_nbr) and delegates todeclare_etcfor associated types. This aligns well with the sim-centric binding refactor.
PhilipDeegan
force-pushed
the
binpersim
branch
2 times, most recently
from
c7b284b to
1c8de66
Compare
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/python3/patch_level.hpp (1)
4-11: Duplicate include detected.
"patch_data.hpp"on line 4 and"python3/patch_data.hpp"on line 11 resolve to the same header file since this file resides insrc/python3/. One of these includes should be removed.Proposed fix
`#include` "patch_data.hpp" `#include` "phare_solver.hpp" `#include` <string> `#include` <cstring> `#include` <cstddef> -#include "python3/patch_data.hpp" -
🤖 Fix all issues with AI agents
In `@res/__tc/clang_samraisub/0_config.sh`:
- Around line 1-6: Add a shebang at the top of the script (e.g., #!/bin/bash or
#!/bin/sh) and move the existing set -ex invocation to immediately after the
shebang so error tracing and command echoing are enabled before any commands run
(currently pwd appears before set -ex); keep the remaining commands (pwd, ulimit
-a, clang -v, ldd --version, ld -v) in their original order.
In `@res/__tc/clang_samraisub/1_build.sh`:
- Around line 1-3: The script lacks an explicit shebang which can cause
ambiguous shell selection; add a Bash shebang (e.g. #!/usr/bin/env bash) as the
first line before the existing "set -ex" so the script always runs under Bash
and preserves the expected behavior of "set -ex" and the N_CORES check ([ -z
"$N_CORES" ] && ...). Ensure the shebang precedes any other text in the file.
In `@res/__tc/clang_samraisub/2_test.sh`:
- Around line 1-18: Add a portable shebang line (e.g. #!/usr/bin/env bash) as
the very first line of the script before the existing "set -ex" so the
interpreter is explicit and consistent across environments; keep the rest of the
file (environment exports and the subshell block starting with "(" and the "cd
build" / "ctest -j$N_CORES --output-on-failure" commands) unchanged.
In `@res/__tc/clang/0_config.sh`:
- Around line 1-2: Add a shebang as the first line of the script so it always
runs under the intended shell (e.g., use /usr/bin/env bash); ensure the shebang
precedes the existing commands like the initial "pwd" and the "set -ex" line so
the script is executed by bash.
In `@res/__tc/clang/1_build.sh`:
- Line 1: Add a bash shebang at the top of the script so the bash-specific
setting "set -ex" runs under a consistent shell; update the file to start with a
line like "#!/usr/bin/env bash" (or "#!/bin/bash") before the existing "set -ex"
so the script is executed with bash regardless of how it's invoked.
In `@res/__tc/clang/2_test.sh`:
- Line 1: The test script res/__tc/clang/2_test.sh is missing a shebang so its
shell behavior can vary; add a proper shebang as the very first line (for
example using /usr/bin/env bash) so that the script runs with a consistent shell
when executing the commands like set -ex, and ensure the shebang precedes any
other content in the file.
In `@res/__tc/clang/3_test_mpi.sh`:
- Around line 1-2: The script is missing a shebang and defines GCC_ASAN_PRELOAD
but never uses it; add a proper shebang (e.g., #!/usr/bin/env bash) at the top
of the file to ensure portability, and either remove the unused GCC_ASAN_PRELOAD
assignment or replace the reference to CLANG_ASAN_PRELOAD with GCC_ASAN_PRELOAD
where appropriate so the intended preload variable is actually used (search for
GCC_ASAN_PRELOAD and CLANG_ASAN_PRELOAD in the script to decide which variable
should be kept/used).
In `@res/__tc/gcc_samraisub/0_config.sh`:
- Around line 1-2: The script currently starts with commands "pwd" and "set -ex"
but lacks a shebang, triggering shellcheck SC2148; add a proper shebang line
(for example "#!/usr/bin/env bash") as the first line of the file so the script
declares its target shell and enables shell-specific linting for the existing
"set -ex" usage.
In `@res/__tc/gcc_samraisub/2_test.sh`:
- Around line 1-2: Add an explicit shebang at the top of the script to ensure
portable execution when run directly: modify the file containing the line "set
-ex" (script 2_test.sh) to insert the appropriate shell shebang (e.g.,
#!/usr/bin/env bash) as the very first line so the interpreter is defined before
the existing set -ex invocation.
In `@res/__tc/gcc/0_config.sh`:
- Around line 1-2: Add a shebang line at the top of the script so the shell
interpreter is explicit; insert e.g. "#!/usr/bin/env bash" as the first line
above the existing commands (the block containing "pwd" and "set -ex") so the
script runs with the intended shell interpreter.
In `@res/__tc/gcc/1_build.sh`:
- Around line 1-2: The script currently only contains "set -ex" which leaves the
interpreter ambiguous; add a shebang as the first line (for example
"#!/usr/bin/env bash") above the existing set -ex so the shell interpreter is
explicit and the script can be executed directly.
In `@res/__tc/gcc/2_test.sh`:
- Around line 1-3: The script lacks a shebang, causing inconsistent shell
selection; add a shebang as the very first line (for example "#!/usr/bin/env
bash") so the script runs with a known shell, leaving the existing "set -ex" and
the N_CORES check ([ -z "$N_CORES" ] && echo ... && exit 1) unchanged and
functioning as before.
In `@res/__tc/gcc/3_test_mpi.sh`:
- Line 1: Add a shebang as the first line to explicitly set the shell
interpreter (the script currently starts with the line "set -ex"); prepend
something like "#!/usr/bin/env bash" above "set -ex" so the script always runs
under bash and avoids relying on the invoking shell.
In `@res/__tc/nightly/0_config.sh`:
- Around line 1-6: Add a shebang (e.g., #!/usr/bin/env bash) at the top of the
script and move the set -ex line to immediately after the shebang so
tracing/debug flags apply to all commands; ensure the rest of the commands (pwd,
ulimit -a, gcc -v, ldd --version, ld -v) remain in their original order
following set -ex so they run with -x and -e enabled.
In `@res/__tc/nightly/1_build.sh`:
- Around line 1-2: Add an explicit shebang as the very first line of the script
so the interpreter is unambiguous; place a bash shebang (for example, using
/usr/bin/env bash) immediately above the existing set -ex line in the script to
ensure it runs with the intended shell.
In `@res/__tc/nightly/2_test.sh`:
- Line 1: The script currently starts with "set -ex" but lacks a shebang; add a
shebang as the first line (for example "#!/usr/bin/env bash" or the same
interpreter used in 3_test_mpi.sh) so the shell interpreter is explicit, keeping
the existing "set -ex" line immediately after the shebang; ensure the shebang is
the very first line of the 2_test.sh file.
- Line 6: The MODULEPATH export line contains a duplicate entry
'/etc/scl/modulefiles'; update the export of MODULEPATH (the "export
MODULEPATH=...") to remove the repeated '/etc/scl/modulefiles' so each path
appears only once (i.e., ensure the colon-separated list in MODULEPATH is
de-duplicated), or replace the literal assignment with a small dedup step that
builds MODULEPATH uniquely before exporting.
In `@res/__tc/nightly/3_test_mpi.sh`:
- Line 6: The MODULEPATH environment variable export contains a duplicate entry
"/etc/scl/modulefiles"; update the export statement that sets MODULEPATH so each
path appears only once (remove the redundant "/etc/scl/modulefiles") to avoid
duplicated paths when MODULEPATH is used.
- Line 1: Add an explicit bash shebang as the very first line of the script (use
env to locate bash) so the system knows which shell to run, placing it before
the existing "set -ex" line; ensure the shebang is the file's first line so the
interpreter is selected when the script is executed directly.
In `@src/python3/data_wrangler.hpp`:
- Around line 76-87: The DataWrangler constructors currently dereference the
passed shared_ptr and store a reference (simulator_) which can dangle and lacks
null checks; update the class to hold a std::shared_ptr<ISimulator> (or
std::shared_ptr<Simulator> as appropriate) as a member (e.g., simulator_ptr_)
and in both constructors validate the incoming shared_ptr (throw or return/error
on null) before assigning simulator_ptr_ = simulator (or =
cast_simulator(simulator)); change uses of simulator_ to use
simulator_ptr_.get() or dereference when needed so ownership is retained and
null is guarded against.
♻️ Duplicate comments (2)
tests/core/data/field/test_field.hpp (1)
7-10: Add missing standard headers for used symbols.The file uses
std::isnan,std::ostringstream,std::runtime_error,std::string, andstd::vectorwithout direct includes; relying on transitive includes is fragile and can break builds.♻️ Proposed fix
`#include` "gtest/gtest.h" // EXPECT_FLOAT_EQ `#include` <cassert> +#include <cmath> +#include <sstream> +#include <stdexcept> +#include <string> +#include <vector>src/python3/cpp_simulator.hpp (1)
118-121: Duplicate: makeSimulator should be called without a template parameter.
This was already flagged in prior review comments for this file.
🧹 Nitpick comments (7)
res/__tc/gcc/3_test_mpi.sh (1)
21-22: Quote variables to prevent word splitting issues.While
N_CORESis validated to be non-empty, quoting the variable is a shell best practice to prevent potential issues with whitespace or special characters.Proposed fix
- ninja -j$N_CORES -v - ctest -j$N_CORES --output-on-failure + ninja -j"$N_CORES" -v + ctest -j"$N_CORES" --output-on-failurepyphare/pyphare/data/wrangler.py (1)
6-14: Replacegetattrwith direct attribute access.Using
getattrwith a constant attribute name ("DataWrangler") provides no benefit over direct attribute access and reduces readability.Proposed fix
- self.cpp = getattr(cpp.cpp_lib(sim), "DataWrangler")( - simulator.cpp_sim, simulator.cpp_hier - ) + self.cpp = cpp.cpp_lib(sim).DataWrangler( + simulator.cpp_sim, simulator.cpp_hier + )src/amr/samrai.hpp (2)
5-30: Prefer explicit Logger include + non-null StreamAppender handle.
StreamAppenderinherits fromSAMRAI::tbox::Logger::Appender, but the header doesn’t include the Logger definition and accepts a nullable pointer. Consider a direct include and a reference to prevent null deref; also addoverridefor safer polymorphism.♻️ Proposed change (header)
+#include <SAMRAI/tbox/Logger.h> ... -class StreamAppender : public SAMRAI::tbox::Logger::Appender +class StreamAppender : public SAMRAI::tbox::Logger::Appender { public: - StreamAppender(std::ostream* stream) { d_stream = stream; } - void logMessage(std::string const& message, std::string const& filename, int const line) + explicit StreamAppender(std::ostream& stream) : d_stream(stream) {} + void logMessage(std::string const& message, std::string const& filename, int const line) override { - (*d_stream) << "At :" << filename << " line :" << line << " message: " << message - << std::endl; + d_stream << "At :" << filename << " line :" << line << " message: " << message + << std::endl; } private: - std::ostream* d_stream; + std::ostream& d_stream; };If you adopt this, update construction to pass
std::coutby reference.
33-46: Prevent accidental copies ofSamraiLifeCycle.Copying this type would risk multiple shutdown/finalize calls on SAMRAI/MPI. Consider deleting copy/move to enforce singleton-like semantics.
Based on learnings, SamraiLifeCycle is intended to live for the program lifetime.♻️ Proposed change
class SamraiLifeCycle // { public: SamraiLifeCycle(int argc = 0, char** argv = nullptr); ~SamraiLifeCycle(); + + SamraiLifeCycle(SamraiLifeCycle const&) = delete; + SamraiLifeCycle& operator=(SamraiLifeCycle const&) = delete; + SamraiLifeCycle(SamraiLifeCycle&&) = delete; + SamraiLifeCycle& operator=(SamraiLifeCycle&&) = delete;res/__tc/clang/3_test_mpi.sh (1)
21-23: Drop the unused GCC_ASAN_PRELOAD (or wire it in).
Currently only CLANG_ASAN_PRELOAD is used.♻️ Proposed cleanup
-GCC_ASAN_PRELOAD=$(gcc -print-file-name=libasan.so) CLANG_ASAN_PRELOAD=$(clang -print-file-name=libclang_rt.asan.so)res/__tc/clang/2_test.sh (1)
21-22:GCC_ASAN_PRELOADis unused.The variable is defined but only
CLANG_ASAN_PRELOADis used in the test execution. Consider removing it or adding a comment explaining why it's kept (e.g., for reference or future GCC testing).Proposed fix (remove unused variable)
export ASAN_SYMBOLIZER_PATH=/usr/bin/llvm-symbolizer export ASAN_OPTIONS=detect_leaks=0 -GCC_ASAN_PRELOAD=$(gcc -print-file-name=libasan.so) CLANG_ASAN_PRELOAD=$(clang -print-file-name=libclang_rt.asan.so)pyphare/pyphare/cpp/__init__.py (1)
39-56: Replacegetattrwith direct attribute access.Using
getattrwith constant attribute names provides no safety benefit over direct attribute access and reduces code clarity.Proposed fix
def splitter_type(sim): - return getattr(cpp_lib(sim), "Splitter") + return cpp_lib(sim).Splitter def split_pyarrays_fn(sim): - return getattr(cpp_lib(sim), "split_pyarray_particles") + return cpp_lib(sim).split_pyarray_particles def mpi_rank(): - return getattr(cpp_etc_lib(), "mpi_rank")() + return cpp_etc_lib().mpi_rank() def mpi_size(): - return getattr(cpp_etc_lib(), "mpi_size")() + return cpp_etc_lib().mpi_size() def mpi_barrier(): - return getattr(cpp_etc_lib(), "mpi_barrier")() + return cpp_etc_lib().mpi_barrier()
PhilipDeegan
force-pushed
the
binpersim
branch
from
45d0232 to
6356a75
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@ISSUES.TXT`:
- Around line 33-34: Update the typo in the documentation line that currently
reads "shared libaries," to "shared libraries,"; locate the sentence referencing
the CMake flag `-DSAMRAI_ROOT=/path` and replace the misspelled token "libaries"
with the correct spelling "libraries" so the sentence reads "By default, PHARE
will build SAMRAI in place with shared libraries,".
In `@res/cmake/dep/samrai.cmake`:
- Around line 27-29: The option call for BUILD_SHARED_LIBS doesn't override a
cached value, so replace the option/conditional behavior in the
SAMRAI_BUILD_SHARED_LIBS branch by forcing the cache variable: set
BUILD_SHARED_LIBS to ON in the CMake cache (e.g. use set(BUILD_SHARED_LIBS ON
CACHE BOOL "Make shared libs" FORCE)) when SAMRAI_BUILD_SHARED_LIBS is true so
the shared-libs flag actually takes effect; update the logic around
SAMRAI_BUILD_SHARED_LIBS to use this cache-set approach instead of option().
In `@src/amr/wrappers/hierarchy.hpp`:
- Around line 47-63: The constructor HierarchyRestarter registers patch data IDs
via pdrm->registerPatchDataForRestart for each entry in
sim_dict["simulation"]["restarts"]["restart_ids"] before calling
SamraiLifeCycle::getRestartManager()->openRestartFile; if openRestartFile throws
those registrations will leak. Modify the constructor to perform the
registrations inside a try block (or use a simple RAII/scope-guard object) and
if openRestartFile throws catch the exception, unregister the same IDs via
pdrm->unregisterPatchDataForRestart (or the appropriate unregister API) and
rethrow; also add a short inline comment near restart_ids explaining what those
IDs represent and why they must be registered prior to opening the restart file.
♻️ Duplicate comments (1)
src/amr/wrappers/hierarchy.hpp (1)
65-75: Add a comment explaining why patch data IDs are unregistered.Per the previous review feedback, the purpose of unregistering these IDs in the destructor is unclear to readers. A brief comment explaining that this cleanup is necessary to prevent stale registrations when the hierarchy is destroyed (especially relevant for supporting multiple sequential simulations via
SamraiLifeCycle::reset()) would improve maintainability.
PhilipDeegan
force-pushed
the
binpersim
branch
from
7914e1b to
26d4666
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@res/cmake/dep/samrai.cmake`:
- Around line 7-38: When SAMRAI_ROOT is provided the code must validate that the
discovered SAMRAI installation is built as shared libs; after the existing
find_package(SAMRAI ...) call add a guard that inspects the SAMRAI build-type
variable (e.g. SAMRAI_BUILD_SHARED_LIBS or the equivalent exported variable set
by SAMRAI's config) and if it indicates static libraries, abort with a clear
message (message(FATAL_ERROR ...)) explaining static SAMRAI is unsupported
(reference ISSUES.TXT) and instructing the user to provide a shared build or
unset SAMRAI_ROOT; place this check in the same branch that handles SAMRAI_ROOT
so the add_subdirectory/static-enforcement logic in the else branch is
symmetric.
In `@src/python3/patch_level.hpp`:
- Around line 11-12: Remove the duplicate include of the same header in
src/python3/patch_level.hpp: keep a single include of patch_data.hpp and delete
the extra line that includes "python3/patch_data.hpp" so only one `#include` for
patch_data.hpp remains (locate the duplicated includes near the top of the file
where "patch_data.hpp" and "python3/patch_data.hpp" are present).
In `@tests/functional/shock/shock.py`:
- Around line 4-15: Move the ph.NO_GUI() call (and the import of ph) so it
occurs before importing matplotlib.pyplot: import pyphare.pharein as ph and call
ph.NO_GUI() prior to the line "import matplotlib.pyplot as plt" so matplotlib's
backend is set to "Agg" before pyplot locks the backend; update any uses of
ph.NO_GUI(), the ph import, and the "import matplotlib.pyplot as plt" ordering
accordingly.
♻️ Duplicate comments (1)
src/python3/cpp_simulator.hpp (1)
118-121: Fix invalid template call to makeSimulator.
makeSimulatoris not templated (seesrc/simulator/simulator.hpp), somakeSimulator<Sim>(hier)will not compile. Use the non‑templated call.🐛 Proposed fix
- m.def(name.c_str(), [](std::shared_ptr<PHARE::amr::Hierarchy> const& hier) { - return makeSimulator<Sim>(hier); - }); + m.def(name.c_str(), [](std::shared_ptr<PHARE::amr::Hierarchy> const& hier) { + return makeSimulator(hier); + });
🧹 Nitpick comments (4)
src/core/utilities/meta/meta_utilities.hpp (1)
116-132: Consider adding a brief doc comment to clarify purpose.The function provides sensible defaults and follows the existing pattern. A short documentation comment explaining the distinction from
possibleSimulators()(which provides all valid combinations vs. these defaults) would help future maintainers understand when to use each.📝 Suggested documentation
+ /** \brief Returns the default simulator configurations used by PHARE executables. + * + * Unlike possibleSimulators() which lists all valid combinations, + * this provides a single default particle count per dim/interp pair. + */ constexpr decltype(auto) phare_exe_default_simulators() {src/amr/CMakeLists.txt (1)
86-88: Clarify the “static SAMRAI” comment for a SHARED target.The code now builds a shared library, so the comment is slightly misleading.
♻️ Suggested comment update
-# single source of entry for static SAMRAI things +# single source of entry for SAMRAI lifecycle integrationsrc/amr/samrai.hpp (1)
19-30: Prefer a non-null stream handle and addoverride.Storing a raw pointer allows a null dereference in
logMessage. A reference (or not_null) plusoverridemakes intent explicit.♻️ Suggested refactor
-class StreamAppender : public SAMRAI::tbox::Logger::Appender +class StreamAppender : public SAMRAI::tbox::Logger::Appender { public: - StreamAppender(std::ostream* stream) { d_stream = stream; } - void logMessage(std::string const& message, std::string const& filename, int const line) + explicit StreamAppender(std::ostream& stream) : d_stream(stream) {} + void logMessage(std::string const& message, std::string const& filename, int const line) override { - (*d_stream) << "At :" << filename << " line :" << line << " message: " << message + d_stream << "At :" << filename << " line :" << line << " message: " << message << std::endl; } private: - std::ostream* d_stream; + std::ostream& d_stream; };Remember to adjust the call site in
src/amr/samrai.cppaccordingly.src/python3/CMakeLists.txt (1)
5-9: Consider validating the permutations file exists.If
PHARE_PERMUTATIONSpoints to a non-existent file,file(STRINGS ...)will silently produce an empty list, resulting in nocpp_*modules being built. This could lead to confusing runtime errors.♻️ Suggested validation
if(NOT PHARE_PERMUTATIONS) set(PHARE_PERMUTATIONS "${PHARE_PROJECT_DIR}/res/sim/all.txt") endif() + +if(NOT EXISTS ${PHARE_PERMUTATIONS}) + message(FATAL_ERROR "PHARE_PERMUTATIONS file not found: ${PHARE_PERMUTATIONS}") +endif() file(STRINGS ${PHARE_PERMUTATIONS} THIS_FILE)
lib per sim permutation
lib per sim permutation
2 participants
for #978
MacOS does not work anymore, so removed from actions
static samrai libraries are not supported
there is some investigations here into how to support that