Simulator template struct #1056
Conversation
📝 WalkthroughWalkthroughThis change refactors the codebase to unify simulator template parameters into a single Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PythonBindings
participant SimulatorFactory
participant Simulator
User->>PythonBindings: Request simulator instantiation (with dim/interp/nbRefinedPart)
PythonBindings->>SimulatorFactory: Build SimOpts struct
SimulatorFactory->>Simulator: Instantiate Simulator<SimOpts>
Simulator-->>SimulatorFactory: Simulator instance
SimulatorFactory-->>PythonBindings: Return Simulator instance
PythonBindings-->>User: Simulator ready for use
Estimated code review effort5 (>120 minutes) Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (32)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (29)
🧰 Additional context used📓 Path-based instructions (1)**/*.hpp⚙️ CodeRabbit Configuration File
Files:
🧠 Learnings (1)📓 Common learnings🔇 Additional comments (6)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
closes #1004 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/cmake_ubuntu.yml (1)
101-102: Redundant compiler re-exportThe job has already set
CC/CXXearlier; re-exporting them in every step is unnecessary noise and risks diverging from the matrix value if one line is updated in the future.-export CC=gcc CXX=g++ -[ "${{ matrix.cc }}" = "clang" ] && export CC=clang CXX=clang++ +# CC/CXX already set at job level – drop duplication
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
.github/workflows/cmake_ubuntu.yml(1 hunks)CMakeLists.txt(1 hunks)src/amr/data/particles/refine/split.hpp(1 hunks)src/amr/data/particles/refine/splitter.hpp(0 hunks)src/core/utilities/meta/meta_utilities.hpp(4 hunks)src/phare_amr.hpp(1 hunks)src/phare_core.hpp(1 hunks)src/phare_simulator_options.hpp(1 hunks)src/phare_solver.hpp(1 hunks)src/python3/cpp_simulator.hpp(6 hunks)src/python3/data_wrangler.hpp(5 hunks)src/python3/patch_level.hpp(2 hunks)src/simulator/phare_types.hpp(1 hunks)src/simulator/simulator.hpp(13 hunks)tests/amr/messengers/test_messengers.cpp(2 hunks)tests/amr/tagging/test_tagging.cpp(3 hunks)tests/core/data/ion_population/test_ion_population_fixtures.hpp(5 hunks)tests/core/data/particles/test_particle_array_consistency.cpp(1 hunks)tests/core/numerics/interpolator/test_main.cpp(5 hunks)tests/core/numerics/ion_updater/test_updater.cpp(3 hunks)tests/simulator/per_test.hpp(6 hunks)tools/bench/amr/data/particles/copy_data.cpp(1 hunks)tools/bench/core/numerics/interpolator/bench_main.cpp(2 hunks)tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp(2 hunks)tools/bench/core/numerics/pusher/push_bench.hpp(1 hunks)tools/bench/core/numerics/pusher/push_raw_use.cpp(1 hunks)tools/bench/core/numerics/pusher/pusher.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- src/amr/data/particles/refine/splitter.hpp
✅ Files skipped from review due to trivial changes (2)
- CMakeLists.txt
- tools/bench/core/numerics/pusher/push_bench.hpp
🚧 Files skipped from review as they are similar to previous changes (22)
- src/amr/data/particles/refine/split.hpp
- tools/bench/core/numerics/interpolator/bench_main.cpp
- src/phare_simulator_options.hpp
- tests/core/data/particles/test_particle_array_consistency.cpp
- tools/bench/core/numerics/pusher/push_raw_use.cpp
- tools/bench/amr/data/particles/copy_data.cpp
- tests/amr/tagging/test_tagging.cpp
- tools/bench/core/numerics/pusher/pusher.cpp
- src/python3/data_wrangler.hpp
- tests/core/numerics/interpolator/test_main.cpp
- tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp
- src/core/utilities/meta/meta_utilities.hpp
- src/python3/patch_level.hpp
- src/python3/cpp_simulator.hpp
- tests/core/numerics/ion_updater/test_updater.cpp
- src/simulator/phare_types.hpp
- src/phare_solver.hpp
- tests/simulator/per_test.hpp
- src/phare_amr.hpp
- src/phare_core.hpp
- tests/amr/messengers/test_messengers.cpp
- src/simulator/simulator.hpp
🧰 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:
tests/core/data/ion_population/test_ion_population_fixtures.hpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-10-09T08:32:15.667Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-09-05T17:02:58.784Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#888
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:566-568
Timestamp: 2024-09-09T13:57:02.285Z
Learning: Avoid commenting on draft PRs for PhilipDeegan unless explicitly requested.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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`.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-10-09T08:32:15.667Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-07-26T22:04:34.160Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
.github/workflows/cmake_ubuntu.yml (1)
Learnt from: nicolasaunai
PR: #591
File: res/cmake/options.cmake:73-75
Timestamp: 2025-02-07T14:35:14.630Z
Learning: In CMake configuration for PHARE, lowResourceTests and highResourceTests options must be mutually exclusive and cannot be enabled simultaneously.
🪛 actionlint (1.7.7)
.github/workflows/cmake_ubuntu.yml
98-98: property "workspace" is not defined in object type {arch: string; debug: string; environment: string; name: string; os: string; temp: string; tool_cache: string}
(expression)
🔇 Additional comments (9)
tests/core/data/ion_population/test_ion_population_fixtures.hpp (8)
26-28: LGTM: Clean template parameter consolidation.The introduction of
SimOptsstruct successfully consolidates the dimension and interpolation parameters into a single constexpr object, aligning with the PR's objective to simplify template usage. The updatedPHARE_Typesusage is correct and maintains type safety.
34-36: LGTM: Cleaner type alias syntax.The removal of redundant
typenamekeywords from type alias declarations improves code readability and follows modern C++ best practices. The aliases are correctly defined and maintain proper type relationships.
45-48: LGTM: Consistent type alias improvements.The type alias declarations follow the same clean syntax pattern established earlier in the file. The removal of redundant
typenamekeywords maintains consistency and improves readability.
60-65: LGTM: Proper resource integration.The structured binding correctly captures the new charge density resource and the buffer setup properly associates each density type with its corresponding grid. The implementation follows established patterns and maintains consistency with other resource handling in the constructor.
74-77: LGTM: Consistent member declarations.The member variable declarations properly reflect the constructor initialization, with both density grids correctly typed and the vector/tensor fields using the updated type alias syntax. The organization is clear and maintains consistency throughout the class.
85-86: LGTM: Consistent type alias style.The type alias declarations maintain the same clean syntax pattern used throughout the file, with appropriate removal of redundant
typenamekeywords while preserving correct type relationships.
136-138: LGTM: Consistent member variable declarations.The member variable declarations use the updated type alias syntax consistently with the rest of the file. All variables are properly typed and the declarations align with their usage in the constructor and methods.
54-55: Charge density correctly uses HybridQuantity::Scalar::rho
All instances ofchargeDensity(includingionChargeDensity,protonChargeDensity,alphaChargeDensity) in tests, core ion/electron code, and benchmarks consistently useHybridQuantity::Scalar::rho. No alternate quantity type is in use, so no change is needed..github/workflows/cmake_ubuntu.yml (1)
107-109: Re-configuring in the same build directory can create stale artefactsYou first configure/build with tests on, then re-configure with them off inside the same directory.
CMake removes targets but object files from the first build remain and may be linked accidentally, giving non-deterministic binaries and longer incremental builds. Either:• use a fresh
${{ github.workspace }}/bench-builddirectory, or
• callcmake --preset bench/cmake --build . --target benchwithout re-configuring.
PhilipDeegan
force-pushed
the
simopts
branch
2 times, most recently
from
f568b81 to
fa3de8b
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| #include "pybind11/chrono.h" | ||
| #include "pybind11/complex.h" | ||
| #include "pybind11/functional.h" | ||
| // #include "python3/pybind_def.hpp" |
Check notice
Code scanning / CodeQL
Commented-out code Note
|
|
||
| #include "python3/particles.hpp" | ||
| #include "python3/patch_data.hpp" | ||
| #include "python3/patch_level.hpp" | ||
| #include "python3/data_wrangler.hpp" | ||
|
|
||
|
|
||
| // #include <vector> |
Check notice
Code scanning / CodeQL
Commented-out code Note
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/cmake_macos.yml (1)
71-72: Unify workspace path expansion to avoid confusionThe steps now mix two syntaxes to reach the workflow directory (
${{github.workspace}}vs$GITHUB_WORKSPACE).
While both expand to the same value, sticking to the$GITHUB_WORKSPACEenvironment variable insiderun/working-directoryblocks is the common pattern across GitHub-Actions docs and keeps the YAML expression language out of shell commands.- run: cmake -E make_directory ${{github.workspace}}/build + run: cmake -E make_directory "$GITHUB_WORKSPACE/build" - working-directory: ${{github.workspace}}/build + working-directory: $GITHUB_WORKSPACE/buildCarry the same replacement to the subsequent Build and Test steps for consistency.
Please confirm all other workflow files now reference the workspace path the same way, otherwise cache keys and artefact paths may diverge.
Also applies to: 75-76, 84-85, 88-89
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
.github/workflows/cmake_macos.yml(2 hunks).github/workflows/cmake_ubuntu.yml(2 hunks)CMakeLists.txt(1 hunks)src/amr/data/particles/refine/split.hpp(1 hunks)src/amr/data/particles/refine/splitter.hpp(0 hunks)src/core/utilities/meta/meta_utilities.hpp(4 hunks)src/phare_amr.hpp(1 hunks)src/phare_core.hpp(1 hunks)src/phare_simulator_options.hpp(1 hunks)src/phare_solver.hpp(1 hunks)src/python3/cpp_simulator.hpp(6 hunks)src/python3/data_wrangler.hpp(5 hunks)src/python3/patch_level.hpp(2 hunks)src/simulator/phare_types.hpp(1 hunks)src/simulator/simulator.hpp(13 hunks)tests/amr/messengers/test_messengers.cpp(2 hunks)tests/amr/tagging/test_tagging.cpp(3 hunks)tests/core/data/ion_population/test_ion_population_fixtures.hpp(5 hunks)tests/core/data/particles/test_particle_array_consistency.cpp(1 hunks)tests/core/numerics/interpolator/test_main.cpp(5 hunks)tests/core/numerics/ion_updater/test_updater.cpp(3 hunks)tests/simulator/per_test.hpp(6 hunks)tools/bench/amr/data/particles/CMakeLists.txt(1 hunks)tools/bench/amr/data/particles/copy_data.cpp(1 hunks)tools/bench/core/data/particles/CMakeLists.txt(1 hunks)tools/bench/core/numerics/interpolator/CMakeLists.txt(1 hunks)tools/bench/core/numerics/interpolator/bench_main.cpp(2 hunks)tools/bench/core/numerics/ion_updater/CMakeLists.txt(1 hunks)tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp(2 hunks)tools/bench/core/numerics/pusher/CMakeLists.txt(2 hunks)tools/bench/core/numerics/pusher/push_bench.hpp(1 hunks)tools/bench/core/numerics/pusher/push_raw_use.cpp(1 hunks)tools/bench/core/numerics/pusher/pusher.cpp(1 hunks)tools/bench/hi5/CMakeLists.txt(1 hunks)
💤 Files with no reviewable changes (1)
- src/amr/data/particles/refine/splitter.hpp
✅ Files skipped from review due to trivial changes (6)
- tools/bench/amr/data/particles/CMakeLists.txt
- tools/bench/core/numerics/interpolator/CMakeLists.txt
- tools/bench/core/numerics/pusher/CMakeLists.txt
- tools/bench/core/numerics/ion_updater/CMakeLists.txt
- tools/bench/core/data/particles/CMakeLists.txt
- CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (25)
- .github/workflows/cmake_ubuntu.yml
- tools/bench/core/numerics/pusher/push_bench.hpp
- src/amr/data/particles/refine/split.hpp
- src/phare_simulator_options.hpp
- tools/bench/core/numerics/interpolator/bench_main.cpp
- tests/amr/tagging/test_tagging.cpp
- src/python3/patch_level.hpp
- tools/bench/amr/data/particles/copy_data.cpp
- tools/bench/core/numerics/pusher/push_raw_use.cpp
- src/phare_amr.hpp
- src/phare_core.hpp
- tests/core/numerics/ion_updater/test_updater.cpp
- tools/bench/core/numerics/pusher/pusher.cpp
- src/phare_solver.hpp
- tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp
- src/python3/cpp_simulator.hpp
- tests/core/data/particles/test_particle_array_consistency.cpp
- src/python3/data_wrangler.hpp
- tests/core/numerics/interpolator/test_main.cpp
- tests/amr/messengers/test_messengers.cpp
- src/core/utilities/meta/meta_utilities.hpp
- src/simulator/phare_types.hpp
- src/simulator/simulator.hpp
- tests/core/data/ion_population/test_ion_population_fixtures.hpp
- tests/simulator/per_test.hpp
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-10-09T08:32:15.667Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-09-05T17:02:58.784Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#888
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:566-568
Timestamp: 2024-09-09T13:57:02.285Z
Learning: Avoid commenting on draft PRs for PhilipDeegan unless explicitly requested.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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`.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-10-09T08:32:15.667Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-07-26T22:04:34.160Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
tools/bench/hi5/CMakeLists.txt (3)
Learnt from: PhilipDeegan
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.
Learnt from: nicolasaunai
PR: #591
File: res/cmake/options.cmake:73-75
Timestamp: 2025-02-07T14:35:14.630Z
Learning: In CMake configuration for PHARE, lowResourceTests and highResourceTests options must be mutually exclusive and cannot be enabled simultaneously.
Learnt from: PhilipDeegan
PR: #943
File: res/cmake/dep/highfive.cmake:7-7
Timestamp: 2025-01-16T10:45:48.912Z
Learning: The HighFive library development has moved from BlueBrain/HighFive to highfive-devs/highfive as of December 2024 due to the conclusion of the Blue Brain Project.
.github/workflows/cmake_macos.yml (1)
Learnt from: nicolasaunai
PR: #591
File: res/cmake/options.cmake:73-75
Timestamp: 2025-02-07T14:35:14.630Z
Learning: In CMake configuration for PHARE, lowResourceTests and highResourceTests options must be mutually exclusive and cannot be enabled simultaneously.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-latest)
🔇 Additional comments (1)
tools/bench/hi5/CMakeLists.txt (1)
5-8: Confirmed: the10literal is theexec_levelthreshold, not an iteration count
Theadd_phare_cpp_benchmark(exec_level …)macro simply checks thatexec_levelfalls betweenPHARE_EXEC_LEVEL_MINandPHARE_EXEC_LEVEL_MAXbefore instantiating the benchmark. No other logic depends on the old value of11. Changing to10across all CMakeLists is safe, provided your execution‐level bounds include10.
Replace C++ Simulator class templates with single constexpr class struct
This should simplify MHD Simulator additions, and I'm already using this for GPU