Simulator template struct #1056
Conversation
|
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
3 times, most recently
from
fa3de8b to
2368a10
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/python3/data_wrangler.hpp (2)
173-179: Fix self-referential alias:using SimulatorCaster = SimulatorCaster<opts>is ill-formedThe alias shadows the class template name and becomes self-referential, which won’t compile. Use a different alias name and qualify the template you’re targeting.
- using SimulatorCaster = SimulatorCaster<opts>; + using SimCaster = PHARE::pydata::SimulatorCaster<opts>; @@ - Simulator* simulator_ptr = core::makeAtRuntime<SimulatorCaster>( + Simulator* simulator_ptr = core::makeAtRuntime<SimCaster>( simDict["dimension"].template to<int>(), simDict["interp_order"].template to<int>(), - simDict["refined_particle_nbr"].template to<int>(), SimulatorCaster{simulator}); + simDict["refined_particle_nbr"].template to<int>(), SimCaster{simulator});
75-80: Preserve simulator lifetime: store the baseshared_ptralongside the reference
DataWranglerstores aSimulator&that is bound to the object managed by astd::shared_ptr<ISimulator>passed to the ctor, but it does not keep thatshared_ptr. If the caller does not retain theshared_ptr,simulator_becomes a dangling reference. Keep ashared_ptr<ISimulator>data member to own the lifetime.@@ class DataWrangler - Simulator& simulator_; + // Keep ownership to guarantee lifetime of `simulator_` + std::shared_ptr<ISimulator> simulator_base_; + Simulator& simulator_; @@ DataWrangler(...) - : simulator_{cast_simulator(simulator)} - , hierarchy_{hierarchy} + : simulator_base_{simulator} + , simulator_{cast_simulator(simulator_base_)} + , hierarchy_{hierarchy}Note: declare
simulator_base_beforesimulator_so it is initialized first (and thus alive when the reference is bound).Also applies to: 165-167
tools/bench/core/numerics/pusher/push_bench.hpp (1)
30-31: Fix unknown type name: useParticleArray_t
ParticleArrayis not defined in this scope; the alias introduced above isParticleArray_t.- ParticleArray domainParticles{layout.AMRBox()}; + ParticleArray_t domainParticles{layout.AMRBox()};src/simulator/simulator.hpp (4)
238-254: Guard against dMan being reset before the lambda runsreset_dman() exists and you call release/reset later on errors; the "fine_dump" functor should defensively check dMan before dereferencing to avoid a null dereference if it’s been reset.
if (fine_dump_lvl_max > 0) { // copy for later this->fineDumpLvlMax = static_cast<std::size_t>(fine_dump_lvl_max); functors_["pre_advance"]["fine_dump"] = [&](SimFunctorParams const& params) { - std::size_t level_nbr = params["level_nbr"].template to<int>(); - auto timestamp = params["timestamp"].template to<double>(); - - if (this->fineDumpLvlMax >= level_nbr) - this->dMan->dump_level(level_nbr, timestamp); + if (!this->dMan) + return; + const std::size_t level_nbr = static_cast<std::size_t>( + params["level_nbr"].template to<int>()); + const double timestamp = params["timestamp"].template to<double>(); + if (this->fineDumpLvlMax >= level_nbr) + this->dMan->dump_level(level_nbr, timestamp); }; }
378-385: Initialize: missing throw prevents duplicate-initialization protectionThe std::runtime_error is constructed but never thrown. This silently allows re-initialization to proceed.
- if (isInitialized) - std::runtime_error("cannot initialize - simulator already isInitialized"); + if (isInitialized) + throw std::runtime_error("cannot initialize - simulator already isInitialized");
399-401: Fix leak: release() drops ownership without deletion/flushdMan.release() leaks the manager and won’t guarantee flushing/closing. Use reset() to destroy (and thus flush/close) the manager safely.
- this->dMan.release(); // closes/flushes hdf5 files + this->dMan.reset(); // closes/flushes HDF5 files and releases ownership safelyAlso applies to: 439-441
423-425: Advance Time Tracking to Use Integrator’s Absolute TimeThe integrator’s advance(dt) call returns the new absolute simulation time (dt_new), but the existing code applies the previous step dt to the TimeStamper, which can lead to drift if the integrator adapts dt. Instead, you should:
- Compute the actual time increment as
delta = dt_new – currentTime_- Advance the TimeStamper by
delta- Update
currentTime_todt_newThis ensures both
currentTime_and the stamped index stay in sync with the integrator’s notion of time.Locations to update:
- File: src/simulator/simulator.hpp
- Lines: 423–425
Suggested diff:
- dt_new = integrator_->advance(dt); - currentTime_ = startTime_ + ((*timeStamper) += dt); + // Advance integrator and get the new absolute time: + dt_new = integrator_->advance(dt); + // Compute the true time step and update the stamper accordingly: + const double delta = dt_new - currentTime_; + (*timeStamper) += delta; + currentTime_ = dt_new;Note: the ITimeStamper interface currently only supports operator+=(dt). If you need direct absolute assignment (e.g., for a future adaptive stamper), consider extending ITimeStamper with a setter or absolute‐assignment method.
🧹 Nitpick comments (19)
src/python3/data_wrangler.hpp (5)
91-99: Don’t takeconst&¶meter; accept by value (move) or by const&
const&&prevents moving and is unusual/limiting. Accept by value to enable moves from rvalues (yoursync(input)call creates one), or acceptconst&if you intend to only read.- auto sort_merge_1d(std::vector<PatchData<std::vector<double>, dimension>> const&& input, + auto sort_merge_1d(std::vector<PatchData<std::vector<double>, dimension>> input, bool shared_patch_border = false) { - std::vector<std::pair<double, PatchData<std::vector<double>, dimension> const*>> sorted; + std::vector<std::pair<double, PatchData<std::vector<double>, dimension> const*>> sorted; + sorted.reserve(input.size());Given Philip’s preference for minimal/efficient code, this change also reduces copies when the argument is an rvalue.
119-123: Remove unused lambdareinterpret_arrayThis captureless lambda is never referenced and can be deleted.
- auto reinterpret_array = [&](auto& py_array) { - return reinterpret_cast<std::array<std::size_t, dimension>&>( - *static_cast<std::size_t*>(py_array.request().ptr)); - };
141-151: Type/overload safety oncore::mpi::maxcall
mpi_sizeis anint, whileinput.size()isstd::size_t. Depending on the overload set ofcore::mpi::max, this may select an unintended overload or cause narrowing. Prefer making the template argument explicit or casting deliberately.If
core::mpi::maxis templated:- std::size_t max = core::mpi::max(input.size(), mpi_size); + std::size_t max = core::mpi::max<std::size_t>(input.size(), mpi_size);If not templated, cast the first argument to the expected type. Please confirm the function signature in
core::mpi.
181-181: Typo in exception message“Data Wranger” → “Data Wrangler”.
- throw std::runtime_error("Data Wranger creation error: failed to cast Simulator"); + throw std::runtime_error("Data Wrangler creation error: failed to cast Simulator");
188-188: Header guard trailing comment mismatchEnding comment has
_Hwhile the guard is_HPP. Adjust for consistency/readability.-#endif /*PHARE_PYTHON_DATA_WRANGLER_H*/ +#endif /* PHARE_PYTHON_DATA_WRANGLER_HPP */tests/core/data/ion_population/test_ion_population_fixtures.hpp (2)
54-56: Align density naming for clarity.UsableIonsPopulation exposes particleDensity/chargeDensity, while UsableIons exposes massDensity/chargeDensity. If particleDensity is number density, consider renaming to numberDensity (or Ni) to avoid confusion with massDensity. Otherwise, standardize on massDensity where appropriate.
Also applies to: 110-112, 138-140
60-66: Verify compile-time resource view ordering.Structured bindings assume getCompileTimeResourcesViewList returns resources in the exact [_F, _M, _pd, _cd, _particles] and [_bV, _M, _cd, _md] orders. Any mismatch silently wires buffers incorrectly. Please confirm or expose named getters to prevent order-coupling.
I can generate a minimal static_assert-based traits check (or helper tags) to validate tuple ordering at compile time if the view types are distinguishable.
Also applies to: 115-120
tests/simulator/per_test.hpp (2)
14-15: Static input caching may ignore differing job_py values.StaticIntepreter::input is initialized once and never refreshed. If any test passes a different job_py later, dict() will return the previously read configuration. If that’s intentional, add a note; otherwise, guard on job_py changes and reload.
Apply this minimal guard if needed:
- if (!input) + if (!input || input->path() != job_py) { input = std::make_shared<PHARE::initializer::PythonDataProvider>(job_py); input->read(); }Also applies to: 50-56
60-63: Double dict() call in constructor initializer list.dict(job_py) is evaluated twice (for HierarchyMaker and Simulator). It’s likely cheap, but if it performs any non-idempotent work in the future, it can be surprising. Consider a thin helper base that accepts a DictRef or refactor HierarchyMaker to take a provider instead. Optional.
src/simulator/phare_types.hpp (1)
5-7: Avoid reliance on transitive includes.This header uses core::PHARE_Types and amr::PHARE_Types but only includes phare_solver.hpp. Add explicit includes to keep IWYU-friendly boundaries.
#include "phare_solver.hpp" +#include "phare_core.hpp" +#include "phare_amr.hpp"src/phare_amr.hpp (1)
36-39: Optional: validate opts at compile time.If you expect nbRefinedPart > 0 and supported interp_order/dimension ranges, add static_asserts here to fail early with clear messages. Keeps misconfigurations out of runtime.
src/phare_core.hpp (1)
11-11: Ensure NdArrayView is declared or include its header.ArrayView_t aliases NdArrayView, but this header does not obviously include its definition. If NdArrayView isn’t provided by ndarray_vector.hpp, include the proper header explicitly to avoid transitive-include dependence.
#include "core/data/ndarray/ndarray_vector.hpp" +#include "core/data/ndarray/ndarray_view.hpp"Also applies to: 33-35
src/simulator/simulator.hpp (6)
4-6: Make SimOpts dependency explicitSimulatorMaker uses SimOpts later in this header. To avoid hidden transitive includes and improve IWYU, explicitly include the SimOpts header here.
#include "phare_core.hpp" #include "phare_types.hpp" +#include "phare_simulator_options.hpp"Also applies to: 12-25
92-105: Unify dump() semantics and return valueRight now rMan->dump is called and ignored, while dMan->dump decides the return value. If IRestartsManager::dump returns a boolean, consider OR-ing both results. If it’s void, consider returning whether any manager ran. This keeps dump()’s contract predictable.
Would you confirm the signatures of IRestartsManager::dump and IDiagnosticsManager::dump? If both return bool, I can send a precise patch that aggregates results. Otherwise we can gate on void/bool at compile time.
291-299: Load balancer dimension wiring looks correct; remove stale commentUsing LoadBalancerManager<opts.dimension> aligns with the new opts wiring. The inline “moved on next line” comment is stale.
- auto lbm_id = lbm_->getId(); // moved on next line + auto lbm_id = lbm_->getId();Also applies to: 313-315
386-395: Use plain rethrow for simplicityInside a catch block, prefer throw; over std::rethrow_exception(std::current_exception()); It’s clearer and avoids needless exception_ptr machinery.
- std::rethrow_exception(std::current_exception()); + throw; @@ - std::rethrow_exception(std::current_exception()); + throw;
122-123: Prefer explicit bool and const-ref for find_modelReturn type is bool and the argument needn’t be copied.
- auto find_model(std::string name); + bool find_model(std::string const& name);-auto Simulator<opts>::find_model(std::string name) +bool Simulator<opts>::find_model(std::string const& name) { return std::find(std::begin(modelNames_), std::end(modelNames_), name) != std::end(modelNames_); }Also applies to: 450-453
466-482: Name shadowing: avoid reusing “opts” locally when instantiating SimulatorUsing a local constexpr named opts is easy to confuse with the template parameter name used across the file. A distinct name reads clearer.
- SimOpts constexpr static opts{.dimension = d, .interp_order = io, .nbRefinedPart = nb}; - return std::make_unique<Simulator<opts>>(theDict, hierarchy_); + constexpr static SimOpts kOpts{.dimension = d, .interp_order = io, .nbRefinedPart = nb}; + return std::make_unique<Simulator<kOpts>>(theDict, hierarchy_);src/phare_solver.hpp (1)
18-24: Optional: add a compile-time guard documenting supported dimsIf runtime creation is gated to dimensions < 3 in this PR, a static_assert here can document and catch accidental instantiations at compile time. If 3D builds are intentionally allowed for GPU work, skip this.
struct PHARE_Types { auto static constexpr dimension = opts.dimension; auto static constexpr interp_order = opts.interp_order; auto static constexpr nbRefinedPart = opts.nbRefinedPart; + static_assert(dimension >= 1, "PHARE_Types: dimension must be >= 1"); + // Uncomment if compile-time restriction to 1-2D is desired for now: + // static_assert(dimension <= 2, "PHARE_Types: runtime currently supports only 1D/2D");
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (32)
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(1 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(6 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 as they are similar to previous changes (19)
- tools/bench/amr/data/particles/CMakeLists.txt
- tools/bench/core/data/particles/CMakeLists.txt
- tools/bench/core/numerics/pusher/push_raw_use.cpp
- tools/bench/hi5/CMakeLists.txt
- tests/core/data/particles/test_particle_array_consistency.cpp
- src/phare_simulator_options.hpp
- CMakeLists.txt
- tests/core/numerics/ion_updater/test_updater.cpp
- src/amr/data/particles/refine/split.hpp
- tools/bench/amr/data/particles/copy_data.cpp
- tools/bench/core/numerics/pusher/CMakeLists.txt
- tools/bench/core/numerics/interpolator/bench_main.cpp
- tools/bench/core/numerics/interpolator/CMakeLists.txt
- src/python3/patch_level.hpp
- tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp
- tools/bench/core/numerics/ion_updater/CMakeLists.txt
- tools/bench/core/numerics/pusher/pusher.cpp
- src/core/utilities/meta/meta_utilities.hpp
- src/python3/cpp_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:
tools/bench/core/numerics/pusher/push_bench.hpptests/core/data/ion_population/test_ion_population_fixtures.hppsrc/simulator/simulator.hppsrc/simulator/phare_types.hppsrc/phare_core.hppsrc/python3/data_wrangler.hppsrc/phare_solver.hpptests/simulator/per_test.hppsrc/phare_amr.hpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-07-26T22:04:34.160Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-10-09T08:32:15.667Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
🧬 Code graph analysis (3)
tests/core/numerics/interpolator/test_main.cpp (3)
src/amr/data/particles/refine/particles_data_split.hpp (2)
dimension(73-77)dimension(74-74)src/amr/resources_manager/resources_manager.hpp (1)
dimension(85-290)src/simulator/simulator.hpp (1)
interp_order(84-84)
tests/core/data/ion_population/test_ion_population_fixtures.hpp (1)
src/core/data/electrons/electrons.hpp (8)
layout(107-133)layout(107-107)layout(285-285)layout(285-285)layout(286-286)layout(286-286)layout(309-319)layout(309-309)
src/simulator/simulator.hpp (4)
src/amr/load_balancing/load_balancer_details.hpp (2)
dict(38-52)dict(38-38)src/amr/wrappers/integrator.hpp (4)
dict(36-41)dict(36-36)dt(47-54)dt(47-47)src/diagnostic/diagnostics.hpp (2)
make_unique(54-64)make_unique(54-55)src/restarts/restarts.hpp (2)
make_unique(36-46)make_unique(36-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (11)
tools/bench/core/numerics/pusher/push_bench.hpp (1)
23-27: SimOpts-based rewiring looks goodSwitching to
core::PHARE_Types<SimOpts{dim, 1}>is consistent with the PR’s template consolidation and improves readability.tests/amr/messengers/test_messengers.cpp (1)
201-210: SimOpts-based aliasing looks consistentDefining
optsonce and reusing it acrossSimulatorandPHARE_Typeskeeps the test wiring tight and readable. Nice.tests/simulator/per_test.hpp (1)
10-10: Nice move to a single opts non-type parameter.The SimOpts alias and template parametrization reads cleanly and reduces template noise across tests. LGTM.
Also applies to: 36-46
src/simulator/phare_types.hpp (1)
10-42: SimOpts-driven type wiring looks correct.Core/amr/solver type surfaces are consistently re-exposed via opts. No functional issues spotted.
src/phare_amr.hpp (1)
31-50: LGTM on opts refactor and include hygiene.Types now derive from opts with a clear core_types alias; RefinementParams correctly uses core_types::ParticleArray_t. Includes look sufficient and explicit.
src/phare_core.hpp (1)
27-55: Core PHARE_Types updates look consistent.Aliases map cleanly to the underlying core components, and the Yee/GridLayout chain is intact. Good step toward reducing template arity across the codebase.
src/simulator/simulator.hpp (2)
52-72: Single auto opts parameterization is solidConsolidating template parameters into a single constexpr SimOpts via template is clear and reduces instantiation surface. The static constexpr mirrors (dimension/interp_order/nbRefinedPart) and the updated type wiring read well.
491-495: Factory overload LGTMThe simplified factory template cleanly forwards the dict and hierarchy. No concerns.
src/phare_solver.hpp (3)
5-6: IWYU keep on phare_amr.hpp is appropriateKeeping this include prevents accidental removal by tooling; it’s a common hotspot for transitive types. Looks good.
18-24: opts-derived constexpr mirrors are clearDimension/interp_order/nbRefinedPart reading from opts keeps the surface area minimal and avoids desynchronization.
26-33: Type wiring reads clean and removes redundant typename qualifiersThe new aliases (core_types, amr_types, model/solver aliases, MultiPhysicsIntegrator_t) are cohesive and expressive. Good separation by “core deps” and “amr deps”.
Also applies to: 35-41, 43-45, 50-53
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp (3)
12-12: Prefer an integral literal and size-appropriate type for n_parts
constexpr std::uint32_t n_parts = 1e7;uses a floating literal and a fixed-width 32-bit type. Use an integral literal with digit separators andstd::size_tto match container sizes and avoid narrowing.Apply:
- constexpr std::uint32_t n_parts = 1e7; + constexpr std::size_t n_parts = 10'000'000;
50-51: Introduce a named constant for theparticlestuple indexThe grep run confirms that everywhere
getCompileTimeResourcesViewList()is destructured into five elements—[F, M, d, c, particles]—sostd::get<4>(…)indeed selectsparticles. Defining a local constexpr index will document this intent and guard against future tuple reordering.• File tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp, around lines 50–51
Suggested change:
- auto& pack - = std::get<4>(ions.getRunTimeResourcesViewList()[0].getCompileTimeResourcesViewList()); + constexpr std::size_t idx_particles = 4; + auto& pack = + std::get<idx_particles>( + ions.getRunTimeResourcesViewList()[0].getCompileTimeResourcesViewList() + ); pack.setBuffer(&patch_particles.pack());This makes it clear that index 4 corresponds to
particlesand prevents “magic-number” drift if the tuple order ever changes.
13-13: Prefer consistent specifier order for clarity
- CMake for the bench suite inherits C++20 (
CMAKE_CXX_STANDARD 20in top-level CMakeLists.txt), so using a class-type NTTP is portable across supported toolchains.SimOptsinsrc/phare_simulator_options.hppis a C++20 structural type: it’s a simplestructwith two publicstd::size_tmembers and no user-defined constructor or destructor.Apply this optional style tweak for readability:
-auto static constexpr opts = PHARE::SimOpts{dim, interp}; +static constexpr auto opts = PHARE::SimOpts{dim, interp};
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-07-26T22:04:34.160Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-10-09T08:32:15.667Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
🧬 Code graph analysis (1)
tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp (3)
src/core/data/particles/particle_array.hpp (2)
dim(21-65)dim(251-288)src/core/utilities/box/box.hpp (6)
dim(106-122)dim(124-139)grow(88-93)grow(88-88)grow(272-277)grow(272-272)tests/core/numerics/ion_updater/test_updater.cpp (2)
ions(327-355)ions(327-327)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
🔇 Additional comments (3)
tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp (3)
15-15: Type alias migration to PHARE_Types and cleaner nested aliases: LGTMThe switch to
PHARE_Types<opts>and dropping unnecessarytypenameon nested aliases improves readability without changing semantics. Nice.Also applies to: 18-19
27-27: Verify constructor overload ambiguity for Boxing_t initializationI attempted to locate any competing two-argument constructors in
UpdaterSelectionBoxingbut couldn’t find matching definitions in the codebase. Please manually confirm there’s no overload ambiguity on your compiler.• File: tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp
Line 27:Boxing_t const boxing{layout, {grow(layout.AMRBox(), GridLayout_t::nbrParticleGhosts())}};Suggested explicit form to avoid any unintended overload resolution:
-auto const boxing = Boxing_t{layout, {grow(layout.AMRBox(), GridLayout_t::nbrParticleGhosts())}}; +auto const haloBox = grow(layout.AMRBox(), GridLayout_t::nbrParticleGhosts()); +auto const boxing = Boxing_t{layout, {haloBox}};
66-68: 3D benchmarks are supported as-isAll core components used by
updater_routine<3, …>explicitly allowdim == 3at compile time, and no build-time guard prevents instantiating 3D types:
- Particle dim-check in
particle.hpp:
static_assert(dim > 0 and dim < 4, "Only dimensions 1,2,3 are supported.");- Box utilities in
box.hpp:
static_assert(dim <= 3 and dim > 0);- Field time-interpolator in
field_linear_time_interpolate.hpp:
static_assert(dim > 0 && dim <= 3);- Diagnostic tests in
test_diagnostics.hpp:
static_assert(dim >= 1 and dim <= 3, "Invalid dimension.");
UsableElectromag<3>,UsableIons_t<…, interp>andIonUpdater<Ions, Electromag_t, GridLayout_t>are all templated ondimand have 3D test fixtures in place—nostatic_assertorrequireswill fire fordim=3. There is also no#if PHARE_ENABLE_3D_BENCHES(or equivalent) around these benchmarks, because 3D support is fully implemented.Conclusion: you can safely run the existing
BENCHMARK_TEMPLATE(..., 3, *)lines without additional gating.
nicolasaunai
left a comment
nicolasaunai
left a comment
There was a problem hiding this comment.
you have a bunch of tests going from 11 to 10 what is that for already?
| template<typename dim, typename interp, typename nbRefinedPart> | ||
| constexpr bool valid_simulator() | ||
| { | ||
| return dim{}() < 3; |
There was a problem hiding this comment.
given the name of the function I would have expected checking more than the dimension?
why taking the other parameters then?
There was a problem hiding this comment.
it allows for more parameters, copy paste from https://github.com/PhilipDeegan/PHARE/blob/mkn/src/python3/cpp_simulator.hpp#L175-L183
likely TORM anyway
There was a problem hiding this comment.
I suggested you use this function before to only build the permutation you wanted, the idea being to make the build time quicker, not building things you weren't going to use
you mean the benchmarks? On master, if you turn on benchmarks with cmake options, you also need to set the max exec level from the default of 10, to 11, otherwise they are still skipped @nicolasaunai |
📝 WalkthroughWalkthroughRefactors the codebase to centralize simulator configuration into a single SimOpts options object across core, AMR, solver, simulator, Python bindings, tests, and benchmarks. Adds 3D simulator options to metadata (runtime-gated), adjusts default refined parts lookup to runtime, reorders a CMake include, and updates density handling in specific tests/benches. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Py as Python
participant Bind as pybind11 binding
participant Fact as SimulatorMaker
participant FacFn as makeSimulator<Sim>
participant Sim as Simulator<opts>
participant AMR as AMR/LoadBalancer
Py->>Bind: make_simulator(hier)
Note right of Bind: opts = SimOpts{dim, interp, nb}
Bind->>Fact: SimulatorMaker::operator()(dict)
Fact->>FacFn: makeSimulator<Sim>(hier)
FacFn->>Sim: construct(dict, hier)
Sim->>Sim: initialize()
Sim->>AMR: hybrid_init(dict) using opts.dimension
AMR-->>Sim: setup complete
Sim-->>Bind: shared_ptr<Sim>
Bind-->>Py: return simulator
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
There was a problem hiding this comment.
Actionable comments posted: 6
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)
90-112: defaultNbrRefinedParts: constexpr + runtime assert is brittle; add CT/RT-safe check or drop constexpr
- Unconditional assert in a constexpr function prevents valid constant-evaluation and is compiled out in release, risking a silent 0. Either:
- make the failure hard at runtime (throw or abort) and drop constexpr, or
- gate with std::is_constant_evaluated() and use static_assert in the CT path.
- Minor: once found, we still scan the tuple; guard with if (nbRefinedPart == 0) before assigning to avoid redundant writes.
src/simulator/simulator.hpp (1)
378-381: Bug: missing throw when already initializedThis creates a temporary std::runtime_error but doesn’t throw it. The check silently does nothing.
- if (isInitialized) - std::runtime_error("cannot initialize - simulator already isInitialized"); + if (isInitialized) + throw std::runtime_error("cannot initialize - simulator already initialized");
♻️ Duplicate comments (1)
src/core/utilities/meta/meta_utilities.hpp (1)
125-128: Replace “TORM on 3D PR” with a proper feature gateUse a compile-time flag (e.g., PHARE_ENABLE_3D) or a named constexpr (max_supported_dim) instead of ad‑hoc comments. Keeps intent clear and avoids accidental inclusion later.
Apply this style locally:
- if constexpr (_dim{}() < 3) // TORM on 3D PR + constexpr std::size_t max_supported_dim = 2; // TODO: raise when 3D ready + if constexpr (_dim{}() <= max_supported_dim)Also applies to: 154-158
🧹 Nitpick comments (34)
src/amr/data/particles/refine/split.hpp (2)
4-5: Clarify IWYU keep rationale; verify dimensional coverageGood use of IWYU keep. Please add a brief comment explaining the side‑effect/public API reason these headers must remain (e.g., registration, explicit instantiations), so future readers don’t remove them.
Also, given the PR mentions 3D options elsewhere, confirm that no
split_3d.hppis required here (or that it’s intentionally gated elsewhere). If 3D is (or will be) supported, consider conditional includes via a dimension macro to keep compile time minimal.Apply this minimal doc tweak:
-#include "amr/data/particles/refine/split_1d.hpp" // IWYU pragma: keep -#include "amr/data/particles/refine/split_2d.hpp" // IWYU pragma: keep +// Intentionally kept: required for split specializations/registration. IWYU: keep. +#include "amr/data/particles/refine/split_1d.hpp" // IWYU pragma: keep +#include "amr/data/particles/refine/split_2d.hpp" // IWYU pragma: keep
8-8: Tidy the endif commentThe trailing comment is redundant. Prefer a clean guard terminator.
-#endif // endif PHARE_SPLIT_HPP +#endif // PHARE_SPLIT_HPPsrc/phare_amr.hpp (3)
5-11: Avoid relying on transitive includes; include SimOpts directlyTo decouple from indirect headers, include the SimOpts declaration explicitly.
#include "core/def/phare_mpi.hpp" // IWYU pragma: keep -#include "phare_core.hpp" +#include "phare_core.hpp" +#include "phare_simulator_options.hpp" // for SimOpts #include "amr/wrappers/hierarchy.hpp" #include "amr/data/particles/refine/splitter.hpp" #include "amr/data/particles/refine/particles_data_split.hpp"
13-27: Prune heavy SAMRAI includes here if unused in this headerThese headers don’t appear needed for the type aliases below. Consider removing or moving to a .cpp to cut compile time; keep only what’s directly required.
-#include <SAMRAI/hier/Box.h> -#include <SAMRAI/tbox/Database.h> -#include <SAMRAI/hier/IntVector.h> -#include <SAMRAI/tbox/DatabaseBox.h> -#include <SAMRAI/hier/BoxContainer.h> -#include <SAMRAI/tbox/InputManager.h> -#include <SAMRAI/tbox/MemoryDatabase.h> -#include <SAMRAI/hier/PatchHierarchy.h> -#include <SAMRAI/mesh/BergerRigoutsos.h> -#include <SAMRAI/mesh/GriddingAlgorithm.h> -#include <SAMRAI/mesh/TreeLoadBalancer.h> -#include <SAMRAI/geom/CartesianGridGeometry.h> -#include <SAMRAI/mesh/StandardTagAndInitialize.h> -#include <SAMRAI/algs/TimeRefinementIntegrator.h>
33-50: SimOpts NTTP refactor reads clean; add invariants via static_assertsNice consolidation. Add compile-time checks for common constraints to catch misconfig early.
template<SimOpts opts> struct PHARE_Types { auto static constexpr dimension = opts.dimension; auto static constexpr interp_order = opts.interp_order; auto static constexpr nbRefinedPart = opts.nbRefinedPart; + static_assert(dimension >= 1 && dimension <= 3, "PHARE_Types: dimension must be 1..3"); + static_assert(interp_order >= 1 && interp_order <= 3, "PHARE_Types: interp_order must be 1..3"); + static_assert(nbRefinedPart >= 1, "PHARE_Types: nbRefinedPart must be >= 1"); using core_types = PHARE::core::PHARE_Types<opts>;Also, ensure SimOpts remains a structural type suitable for NTTP (no bases, no virtuals, public constexpr members).
tests/amr/tagging/test_tagging.cpp (1)
173-179: Unify type access via phare_types for cohesion (optional)To keep dependencies centralized, consider also deriving GridLayoutT from phare_types (if exposed) rather than mixing direct GridLayout<> with opts‑based types here.
src/python3/patch_level.hpp (1)
4-7: Missing direct headers for used STL containersThis file uses std::vector and std::unordered_map but doesn’t include them directly. Relying on transitive includes is brittle.
Apply this diff:
#include "patch_data.hpp" #include "phare_solver.hpp" -#include <string> +#include <string> +#include <vector> +#include <unordered_map>tools/bench/core/numerics/interpolator/bench_main.cpp (2)
11-11: Avoid floating literal for integral countUse an integer literal to prevent implicit double→int conversion.
- constexpr std::uint32_t n_parts = 1e7; + constexpr std::uint32_t n_parts = 10'000'000u;
11-17: Include what you use for SimOpts/PHARE_Types (optional)If not already included transitively, add the headers that define SimOpts and PHARE_Types to make this TU self‑sufficient.
#include "tools/bench/core/bench.hpp" #include "core/numerics/interpolator/interpolator.hpp" #include "tests/core/data/gridlayout/test_gridlayout.hpp" +#include "phare_simulator_options.hpp" // for PHARE::SimOpts +// #include "phare_core.hpp" // if PHARE::core::PHARE_Types isn't in the abovesrc/core/utilities/meta/meta_utilities.hpp (2)
102-106: Default nbRefinedPart selection is “first in list” — confirm intentSelecting std::tuple_element_t<2, SimOption> picks the first valid value. If “smallest” is the intended default, add a comment; if not, choose explicitly (e.g., max or policy-driven).
81-87: 3D options are exposed but mostly gated elsewhereYou add 3D entries here, but runtime dispatch currently filters them out. Ensure callers expecting 3D don’t observe inconsistent behavior (e.g., defaults or lookups working while construction is silently disabled).
tests/simulator/per_test.hpp (2)
12-21: Typo: StaticIntepreter → StaticInterpreterFix naming to avoid propagating the misspelling.
-struct __attribute__((visibility("hidden"))) StaticIntepreter +struct __attribute__((visibility("hidden"))) StaticInterpreter @@ - static StaticIntepreter& INSTANCE() + static StaticInterpreter& INSTANCE() @@ - static StaticIntepreter i; + static StaticInterpreter i;Also update call sites (Lines 50, 60).
76-93: Param lists: consider referencing defaults from core to reduce driftIf defaults change, the explicit nbRefinedPart enumerations here may drift. Optionally generate these from core::possibleSimulators() in a single place (helper/type list) to keep tests aligned.
Also applies to: 102-114
tests/amr/messengers/test_messengers.cpp (1)
446-453: Strengthen config validationAdd a static_assert verifying nbRefinePart is valid for (dimension, interpOrder) to catch misconfig at compile time in tests.
struct AfullHybridBasicHierarchy { static constexpr std::size_t interpOrder = 1; static constexpr PHARE::SimOpts opts{dimension, interpOrder, nbRefinePart}; + static_assert(opts.nbRefinedPart == PHARE::core::defaultNbrRefinedParts(dimension, interpOrder), + "nbRefinePart not supported for given (dimension, interpOrder)");src/python3/data_wrangler.hpp (2)
114-124: Remove dead codereinterpret_array is unused.
- auto reinterpret_array = [&](auto& py_array) { - return reinterpret_cast<std::array<std::size_t, dimension>&>( - *static_cast<std::size_t*>(py_array.request().ptr)); - };
171-184: Robust nbRefinedPart lookup + typo fix
- Handle missing simulation.algo.refined_particle_nbr by falling back to core::defaultNbrRefinedParts.
- Fix “Wranger” → “Wrangler”.
static Simulator& cast_simulator(std::shared_ptr<ISimulator> const& simulator) { using SimulatorCaster = SimulatorCaster<opts>; auto const& simDict = initializer::PHAREDictHandler::INSTANCE().dict()["simulation"]; - Simulator* simulator_ptr = core::makeAtRuntime<SimulatorCaster>( - simDict["dimension"].template to<int>(), simDict["interp_order"].template to<int>(), - simDict["refined_particle_nbr"].template to<int>(), SimulatorCaster{simulator}); + const auto userDim = simDict["dimension"].template to<int>(); + const auto userInterpOrder = simDict["interp_order"].template to<int>(); + // Fallback to core default if key is absent + std::size_t userNbRefinedPart = + simDict.contains("refined_particle_nbr") + ? simDict["refined_particle_nbr"].template to<int>() + : PHARE::core::defaultNbrRefinedParts(userDim, userInterpOrder); + + Simulator* simulator_ptr = core::makeAtRuntime<SimulatorCaster>( + userDim, userInterpOrder, userNbRefinedPart, SimulatorCaster{simulator}); if (!simulator_ptr) - throw std::runtime_error("Data Wranger creation error: failed to cast Simulator"); + throw std::runtime_error("Data Wrangler creation error: failed to cast Simulator"); return *simulator_ptr; }tests/core/data/ion_population/test_ion_population_fixtures.hpp (3)
26-29: Prefer designated initialization for SimOpts for clarity and forward‑compatibilityBeing explicit avoids reliance on field ordering if SimOpts evolves.
- SimOpts static constexpr opts{dim, interp_}; + SimOpts static constexpr opts{.dimension = dim, .interp_order = interp_};
74-77: Density quantity tags: double‑check semanticsBoth particleDensity/chargeDensity are created with HybridQuantity::Scalar::rho. If distinct tags exist for number vs charge density, prefer them to prevent misuse by downstream algorithms.
115-126: Avoid reallocation on runtime resources listYou clear then push_back per population; reserve once to avoid potential reallocations.
- Super::getRunTimeResourcesViewList().clear(); - for (auto& pop : populations) - Super::getRunTimeResourcesViewList().emplace_back(*pop); + auto& rt = Super::getRunTimeResourcesViewList(); + rt.clear(); + rt.reserve(populations.size()); + for (auto& pop : populations) + rt.emplace_back(*pop);tests/core/numerics/interpolator/test_main.cpp (5)
186-189: Remove noisy debug prints in testsUnconditional std::cout in tight loops can flood CI logs.
- std::cout << inode << " " << ipos << " " << data.weights[ipos][inode] << " =? " - << weights[inode] << "\n"; + // Uncomment for local debugging: + // std::cout << inode << " " << ipos << " " << data.weights[ipos][inode] + // << " =? " << weights[inode] << "\n";
223-230: Consistently use designated SimOpts initializationSame rationale as elsewhere; it reads better and is robust to field order.
- constexpr static PHARE::SimOpts opts{dimension, interp_order}; + constexpr static PHARE::SimOpts opts{.dimension = dimension, .interp_order = interp_order};Apply similarly at Lines 310, 401, 501, and 694.
668-669: Remove unused variableinterp is not used.
- constexpr auto interp = TypeParam::interp_order; + // (unused)
733-741: Remove unused variable (2D variant)Same as the 1D test; interp isn’t used.
- constexpr auto interp = TypeParam::interp_order; + // (unused)
524-525: weights is never usedConsider removing or marking [[maybe_unused]] to keep warnings clean.
src/phare_core.hpp (1)
27-31: Template parameter style: consider consistency with SimulatorHere you use template; elsewhere you use template. Both are valid if SimOpts is structural. Consider aligning styles for consistency.
src/simulator/simulator.hpp (2)
291-299: Minor: prefer using the local alias for dimensionFor readability, use the declared constant dimension instead of opts.dimension in the template argument.
- auto lbm_ = std::make_unique<amr::LoadBalancerManager<opts.dimension>>(dict); + auto lbm_ = std::make_unique<amr::LoadBalancerManager<dimension>>(dict);
319-327: Optional: initialize diagnostics before integrator to catch config errors earlierIf diagnostics config is invalid, failing before building the Integrator avoids partially initialized state. Consider moving diagnostics_init earlier once hierarchy/model exist.
src/python3/cpp_simulator.hpp (3)
116-121: Method name mismatch: getPopFluxes vs getPopFluxThe binding exposes "getPopFluxes" but points to PL::getPopFlux. Confirm the underlying method name/signature; otherwise rename the binding or call the correct method.
157-159: Avoid redundant std::move on prvaluemakeSimulator returns a prvalue unique_ptr; moving it is unnecessary.
- return std::shared_ptr<Sim>{std::move(makeSimulator<Sim>(hier))}; + return std::shared_ptr<Sim>{makeSimulator<Sim>(hier)};
141-149: Use designated initialization for opts in bindings tooKeeps consistency and clarity with the rest of the codebase.
- constexpr auto opts = SimOpts{dim, interp, nbRefinedPart}; + constexpr auto opts = SimOpts{.dimension = dim, .interp_order = interp, + .nbRefinedPart = nbRefinedPart};Apply similarly at Lines 83-87.
src/phare_solver.hpp (2)
5-5: Include what you use: add phare_core.hppThis header uses PHARE::core::PHARE_Types but relies on transitive includes. Add the direct include for robustness (IWYU).
#include "phare_amr.hpp" // IWYU pragma: keep +#include "phare_core.hpp" // for PHARE::core::PHARE_Types<opts>
21-23: Guard opts at compile timeLightweight checks catch bad SimOpts early.
auto static constexpr dimension = opts.dimension; auto static constexpr interp_order = opts.interp_order; auto static constexpr nbRefinedPart = opts.nbRefinedPart; + static_assert(dimension >= 1 && dimension <= 3, "dimension must be in [1,3]"); + static_assert(interp_order > 0, "interp_order must be > 0");src/simulator/phare_types.hpp (2)
5-7: Include what you use: add direct deps (IWYU)This header refers to core::PHARE_Types and amr::PHARE_Types via aliases—don’t rely on transitive includes.
#include "phare_solver.hpp" +#include "phare_core.hpp" // for core::PHARE_Types<opts> +#include "phare_amr.hpp" // for amr::PHARE_Types<opts>
13-15: Optional: basic SimOpts sanity checksCatch invalid configurations at compile time.
static auto constexpr dimension = opts.dimension; static auto constexpr interp_order = opts.interp_order; static auto constexpr nbRefinedPart = opts.nbRefinedPart; + static_assert(dimension >= 1 && dimension <= 3, "dimension must be in [1,3]"); + static_assert(interp_order > 0, "interp_order must be > 0");
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
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(1 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(6 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
🧰 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/data/particles/refine/split.hppsrc/phare_simulator_options.hppsrc/python3/patch_level.hppsrc/core/utilities/meta/meta_utilities.hpptools/bench/core/numerics/pusher/push_bench.hppsrc/python3/data_wrangler.hppsrc/simulator/phare_types.hppsrc/simulator/simulator.hppsrc/phare_core.hpptests/core/data/ion_population/test_ion_population_fixtures.hppsrc/python3/cpp_simulator.hppsrc/phare_solver.hpptests/simulator/per_test.hppsrc/phare_amr.hpp
🧠 Learnings (4)
📓 Common learnings
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.
📚 Learning: 2025-07-09T17:18:05.771Z
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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:
tools/bench/hi5/CMakeLists.txtCMakeLists.txt
📚 Learning: 2025-02-07T14:35:14.630Z
Learnt from: nicolasaunai
PR: PHAREHUB/PHARE#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:
tools/bench/hi5/CMakeLists.txtCMakeLists.txt
📚 Learning: 2025-09-01T09:41:40.618Z
Learnt from: PhilipDeegan
PR: PHAREHUB/PHARE#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/python3/cpp_simulator.hpp
🧬 Code graph analysis (8)
src/phare_simulator_options.hpp (1)
src/core/utilities/meta/meta_utilities.hpp (2)
defaultNbrRefinedParts(91-112)defaultNbrRefinedParts(91-91)
tools/bench/core/numerics/pusher/pusher.cpp (1)
src/core/data/particles/particle_array.hpp (2)
dim(21-65)dim(251-288)
tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp (3)
src/core/data/particles/particle_array.hpp (2)
dim(21-65)dim(251-288)src/core/utilities/box/box.hpp (6)
dim(106-122)dim(124-139)grow(88-93)grow(88-88)grow(272-277)grow(272-272)tests/core/numerics/ion_updater/test_updater.cpp (2)
ions(327-355)ions(327-327)
tools/bench/core/numerics/interpolator/bench_main.cpp (1)
tools/bench/core/bench.hpp (4)
particle(19-26)particle(19-19)particle(135-138)particle(135-135)
src/simulator/simulator.hpp (6)
src/amr/load_balancing/load_balancer_details.hpp (2)
dict(38-52)dict(38-38)src/amr/wrappers/integrator.hpp (4)
dict(36-41)dict(36-36)dt(47-54)dt(47-47)src/diagnostic/diagnostic_manager.hpp (5)
dict(83-83)dict(86-86)dict(86-86)make_unique(74-80)make_unique(74-75)src/restarts/restarts_manager.hpp (5)
dict(64-64)dict(65-65)dict(65-65)make_unique(50-60)make_unique(50-51)src/diagnostic/diagnostics.hpp (2)
make_unique(54-64)make_unique(54-55)src/restarts/restarts.hpp (2)
make_unique(36-46)make_unique(36-37)
tests/core/data/ion_population/test_ion_population_fixtures.hpp (1)
src/core/data/electrons/electrons.hpp (8)
layout(107-133)layout(107-107)layout(285-285)layout(285-285)layout(286-286)layout(286-286)layout(309-319)layout(309-309)
src/python3/cpp_simulator.hpp (2)
src/simulator/simulator.hpp (2)
makeSimulator(492-495)makeSimulator(492-492)src/core/utilities/types.hpp (2)
apply(97-100)apply(97-97)
tests/core/numerics/interpolator/test_main.cpp (4)
src/amr/data/particles/refine/particles_data_split.hpp (2)
dimension(73-77)dimension(74-74)src/amr/resources_manager/resources_manager.hpp (1)
dimension(85-290)src/simulator/simulator.hpp (1)
interp_order(84-84)pyphare/pyphare/simulator/simulator.py (1)
interp_order(283-285)
⏰ 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: build (macos-13)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
🔇 Additional comments (33)
tools/bench/hi5/CMakeLists.txt (1)
7-7: Bench level 11→10: confirm default-run intent and CI budgetDropping the level to 10 will make this bench run under the default max level. If that’s intentional, LGTM; just confirm CI time/memory are OK. The HighFive guard is correct per project options.
tools/bench/core/numerics/ion_updater/CMakeLists.txt (1)
5-5: Bench level alignment to 10Consistent with the other benches; will run at default max level now. Please verify expected runtime footprint.
tools/bench/core/data/particles/CMakeLists.txt (1)
5-5: Bench level set to 10Looks consistent with the refactor. Confirm that enabling benches no longer requires bumping the max exec level to 11.
CMakeLists.txt (1)
48-49: Moving test.cmake earlier: sensible, but check load-order couplingGood call if bench/test gating vars/macros are needed before bench registration. Please confirm:
- No target redefinition or policy warnings due to the move.
- add_phare_cpp_benchmark still sees the expected gating variables.
tools/bench/amr/data/particles/CMakeLists.txt (1)
5-5: Bench level standardized to 10Consistent with the rest. Please verify local/CI runs execute this bench by default as intended.
tools/bench/core/numerics/interpolator/CMakeLists.txt (1)
5-5: Interpolator bench now at level 10LGTM; aligns with SimOpts-based update. Confirm runtime fits budgets.
tools/bench/core/numerics/pusher/CMakeLists.txt (1)
5-5: Approve: pusher benches lowered to level 10 (verified)
Matches the suite-wide change; ensure CI stays within budget — scan found no remaining add_phare_cpp_benchmark(... 11 ...) occurrences under tools/bench.tests/core/data/particles/test_particle_array_consistency.cpp (1)
45-45: LGTM! Clean migration to SimOpts pattern.The change from
PHARE_Types<dim, interp>toPHARE_Types<SimOpts{dim, interp}>aligns well with the PR's objective of centralizing simulator configuration.tools/bench/core/numerics/pusher/push_bench.hpp (1)
23-26: LGTM! Consistent SimOpts adoption with cleaner type aliases.The migration to
SimOptsis properly implemented, and removing thetypenamekeywords forParticleArray_tandParticle_tmakes the code cleaner since they're no longer dependent type names.tools/bench/core/numerics/pusher/pusher.cpp (1)
6-17: LGTM! Well-structured SimOpts integration.The introduction of the
optsconstant and subsequent type derivations throughPHARE_Types<opts>is clean and consistent. The removal oftypenamequalifiers for nested types improves readability.tools/bench/core/numerics/pusher/push_raw_use.cpp (1)
8-18: LGTM! Clean and consistent SimOpts adoption.The implementation follows the same pattern as other benchmark files, properly introducing
optsand deriving all types throughPHARE_Types<opts>. The removal oftypenamequalifiers is consistent with the refactoring pattern.tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp (2)
50-50: Index change verified — std::get<4> is correct.ions' compile-time tuple for a population is (F, M, d, c, particles), so std::get<4> selects particles (see tests/core/numerics/ion_updater/test_updater.cpp:339 and src/core/data/ions/ions.hpp).
27-27: Initializer list is correct — no action required.UpdaterSelectionBoxing is an aggregate whose second member is std::vector<Box_t> nonLevelGhostBox; using boxing{layout, {grow(...)}} correctly initializes that vector (and preserves layout-first initialization).
tools/bench/amr/data/particles/copy_data.cpp (1)
15-19: Good move to SimOpts-driven typesThis removes template arg duplication and aligns benches with the new options model. No issues spotted.
tests/amr/tagging/test_tagging.cpp (2)
26-27: LGTM on SimOpts adoption in testsLocal constexpr opts keeps the test concise and compile‑time stable.
38-40: LGTM (duplicate pattern)Same as above; consistent usage across tests.
src/python3/patch_level.hpp (1)
14-23: Clean switch to templateType resolution via solver::PHARE_Types and deriving HybridModel/GridLayout from it looks correct and future‑proof.
tools/bench/core/numerics/interpolator/bench_main.cpp (1)
38-38: Confirm new particle→mesh overload availabilityCall interpolator(particles, particleDensity, chargeDensity, flux, layout) assumes an overload accepting both particleDensity and chargeDensity exists for all dims/interps; verify that overload is defined and unambiguous (ADL/namespace) to avoid falling back to older signatures.
tools/bench/core/numerics/interpolator/bench_main.cpp:38
interpolator(particles, particleDensity, chargeDensity, flux, layout);Automated search here failed (path 'core' not found) — run locally, for example:
rg -nP --type=cpp -C2 'operator(\s*[^)]particleDensity[^,],\s*[^)]*chargeDensity' -Ssrc/phare_simulator_options.hpp (1)
12-18: NTTP safety — nbRefinedPart is constexpr (verified)core::defaultNbrRefinedParts and core::possibleSimulators are declared constexpr (src/core/utilities/meta/meta_utilities.hpp) and core::for_N is constexpr (src/core/utilities/types.hpp), so SimOpts::nbRefinedPart is computed in a constant expression and is usable as an NTTP. Optional: add a tiny TU static_assert/UsePHARE::SimOpts{1,1} to pin the check at compile time.
tests/simulator/per_test.hpp (1)
36-48: Nice SimOpts migrationThe test fixture cleanly adopts PHARE::Simulator and PHARE_Types; ctor wiring looks correct.
Also applies to: 60-65
tests/core/numerics/ion_updater/test_updater.cpp (3)
171-173: LGTM: opts-based PHARE_TypesAliases compile cleanly to the new SimOpts path.
207-215: LGTM: ParticleInitializerFactory_t aliasLocal alias keeps downstream code unchanged; good.
366-373: LGTM: opts adoption in IonUpdaterTestConsistent with the fixture types; no functional change.
tests/amr/messengers/test_messengers.cpp (1)
201-210: LGTM: 1D SimOpts wiringAliases and usage align with the new simulator interface.
src/python3/data_wrangler.hpp (2)
29-34: LGTM: SimulatorCasterType-safe downcast keyed by opts; minimal overhead.
Also applies to: 53-56
88-89: PatchLevel: goodReturning PatchLevel specialized on opts is consistent with the rest of the binding layer.
tests/core/data/ion_population/test_ion_population_fixtures.hpp (1)
60-66: Confirm compile‑time resources tuple ordering matches bindingsThe destructure assumes getCompileTimeResourcesViewList returns [_F, _M, _pd, _cd, _particles] in that exact order. A mismatch silently wires wrong buffers.
If the order isn’t guaranteed, consider named accessors or a static_assert on tuple element types.
src/phare_core.hpp (2)
34-42: Verify auxiliary includes for NdArrayViewArrayView_t aliases NdArrayView but this header isn’t included here. If not transitively included, add the proper include to avoid ADL/implicit deps.
[suggest_minor_issue]
#include "core/data/ndarray/ndarray_vector.hpp" +#include "core/data/ndarray/ndarray_view.hpp"
38-51: Confirm SymTensorField/VecField alias template argumentsThe second template parameter is HybridQuantity; many APIs expect a specific category (e.g., HybridQuantity::Vector or ::Tensor). Ensure these aliases match IonPopulation/Electromag expectations to avoid subtle type mismatches.
src/simulator/simulator.hpp (1)
124-147: Environment access style is inconsistent across the filelog_file_name() uses if (auto log = core::get_env(...)) and compares log to strings; logging() uses the init‑stmt form and compares directly. Verify get_env’s return type supports both patterns (truthiness and direct string compare); otherwise, dereference or value() is needed.
src/phare_solver.hpp (3)
43-45: Add missing typename for RefinementParamsRefinementParams is a nested dependent type.
- using RefinementParams = amr_types::RefinementParams; + using RefinementParams = typename amr_types::RefinementParams;⛔ Skipped due to learnings
Learnt from: PhilipDeegan PR: PHAREHUB/PHARE#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.
35-41: No change required — unqualified names resolve in PHARE::solverHybridModel, MHDModel, SolverPPC, SolverMHD and LevelInitializerFactory are declared in namespace PHARE::solver (src/amr/physical_models/hybrid_model.hpp, src/amr/physical_models/mhd_model.hpp, src/amr/solvers/solver_ppc.hpp, src/amr/solvers/solver_mhd.hpp, src/amr/level_initializer/level_initializer_factory.hpp) and the using-aliases in src/phare_solver.hpp are inside namespace PHARE::solver, so the unqualified names resolve correctly.
50-52: Public alias rename — verify downstream/public consumers
- Internal propagation present: alias defined at src/phare_solver.hpp:50–51; propagated in src/simulator/phare_types.hpp:41 and consumed in src/simulator/simulator.hpp (68, 184, 344). amr/multiphysics_integrator.hpp contains the MultiPhysicsIntegrator class; pyphare reference at pyphare/pyphare/pharein/simulation.py:964 may indicate a binding/doc mention.
- Action: confirm all external/public call sites (public headers, language bindings, downstream repos, and docs) are updated to the renamed alias.
| #include "core/data/vecfield/vecfield.hpp" | ||
| #include "core/data/electrons/electrons.hpp" | ||
| #include "core/data/electromag/electromag.hpp" | ||
| #include "core/data/grid/gridlayoutimplyee.hpp" |
There was a problem hiding this comment.
Broken include path for GridLayoutImplYee
"gridlayoutimplyee.hpp" looks misspelled; tests include "core/data/grid/gridlayout_impl.hpp". This will fail to build.
-#include "core/data/grid/gridlayoutimplyee.hpp"
+#include "core/data/grid/gridlayout_impl.hpp"If the actual header is gridlayout_impl_yee.hpp, include that instead.
📝 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.
| #include "core/data/grid/gridlayoutimplyee.hpp" | |
| #include "core/data/grid/gridlayout_impl.hpp" |
🤖 Prompt for AI Agents
In src/phare_core.hpp around line 10, the include "#include
"core/data/grid/gridlayoutimplyee.hpp"" is misspelled and will break builds;
replace it with the correct header path used by the project (either
"core/data/grid/gridlayout_impl.hpp" if that is the canonical header, or
"core/data/grid/gridlayout_impl_yee.hpp" if the Yee-specific implementation is
intended), and ensure the filename and path exactly match the header present in
the repo (fix casing/underscores) so the compiler can locate it.
| using VecField_t = core_types::VecField_t; | ||
| using Grid_t = core_types::Grid_t; | ||
| using Electromag_t = core_types::Electromag_t; | ||
| using Ions_t = core_types::Ions_t; | ||
| using GridLayout_t = core_types::GridLayout_t; | ||
| using Electrons_t = core_types::Electrons_t; |
There was a problem hiding this comment.
Add missing typename on dependent nested types
These aliases reference dependent types and require typename to compile.
Apply:
- using VecField_t = core_types::VecField_t;
- using Grid_t = core_types::Grid_t;
- using Electromag_t = core_types::Electromag_t;
- using Ions_t = core_types::Ions_t;
- using GridLayout_t = core_types::GridLayout_t;
- using Electrons_t = core_types::Electrons_t;
+ using VecField_t = typename core_types::VecField_t;
+ using Grid_t = typename core_types::Grid_t;
+ using Electromag_t = typename core_types::Electromag_t;
+ using Ions_t = typename core_types::Ions_t;
+ using GridLayout_t = typename core_types::GridLayout_t;
+ using Electrons_t = typename core_types::Electrons_t;📝 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.
| using VecField_t = core_types::VecField_t; | |
| using Grid_t = core_types::Grid_t; | |
| using Electromag_t = core_types::Electromag_t; | |
| using Ions_t = core_types::Ions_t; | |
| using GridLayout_t = core_types::GridLayout_t; | |
| using Electrons_t = core_types::Electrons_t; | |
| using VecField_t = typename core_types::VecField_t; | |
| using Grid_t = typename core_types::Grid_t; | |
| using Electromag_t = typename core_types::Electromag_t; | |
| using Ions_t = typename core_types::Ions_t; | |
| using GridLayout_t = typename core_types::GridLayout_t; | |
| using Electrons_t = typename core_types::Electrons_t; |
🤖 Prompt for AI Agents
In src/phare_solver.hpp around lines 27 to 32, the type aliases reference
dependent nested types and must be prefixed with the keyword typename; update
each alias to use "typename core_types::..." (e.g., change using VecField_t =
core_types::VecField_t; to using VecField_t = typename core_types::VecField_t;)
for all six aliases so the code compiles when core_types is a dependent type.
| template<SimOpts opts> | ||
| struct PHARE_Types |
There was a problem hiding this comment.
Unknown SimOpts here (and inconsistent with solver header)
This file uses template but doesn’t include the SimOpts definition, and solver uses template. Either include the SimOpts header or switch to auto for consistency.
Option A (match solver, no extra include):
-template<SimOpts opts>
+template<auto opts>Option B (keep typed NTTP): add the include where SimOpts is defined.
#include "phare_solver.hpp"
+#include "phare_simulator_options.hpp" // for SimOpts📝 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.
| template<SimOpts opts> | |
| struct PHARE_Types | |
| template<auto opts> | |
| struct PHARE_Types |
🤖 Prompt for AI Agents
In src/simulator/phare_types.hpp around lines 10-11, the template parameter uses
SimOpts (template<SimOpts opts>) but SimOpts is not included here and the solver
uses template<auto opts>, causing inconsistency; fix by either (A) change the
declaration to use an untyped NTTP (template<auto opts>) to match the solver and
remove any missing include, or (B) keep the typed NTTP and add the header that
defines SimOpts (include the exact header where SimOpts is declared) so the type
is visible; pick one option and update the template and includes accordingly to
restore consistency.
| using Array_t = core_types::Array_t; | ||
| using Grid_t = core_types::Grid_t; | ||
| using Field_t = core_types::Field_t; | ||
| using VecField_t = core_types::VecField_t; | ||
| using Electromag_t = core_types::Electromag_t; | ||
| using Ions_t = core_types::Ions_t; | ||
| using GridLayout_t = core_types::GridLayout_t; | ||
| using ParticleArray_t = core_types::ParticleArray_t; | ||
| using IonPopulation_t = core_types::IonPopulation_t; | ||
| using Electrons_t = core_types::Electrons_t; | ||
| using ParticleInitializerFactory = core_types::ParticleInitializerFactory_t; | ||
|
|
There was a problem hiding this comment.
Add typename to all core_types dependent aliases
Required for dependent nested types.
- using Array_t = core_types::Array_t;
- using Grid_t = core_types::Grid_t;
- using Field_t = core_types::Field_t;
- using VecField_t = core_types::VecField_t;
- using Electromag_t = core_types::Electromag_t;
- using Ions_t = core_types::Ions_t;
- using GridLayout_t = core_types::GridLayout_t;
- using ParticleArray_t = core_types::ParticleArray_t;
- using IonPopulation_t = core_types::IonPopulation_t;
- using Electrons_t = core_types::Electrons_t;
- using ParticleInitializerFactory = core_types::ParticleInitializerFactory_t;
+ using Array_t = typename core_types::Array_t;
+ using Grid_t = typename core_types::Grid_t;
+ using Field_t = typename core_types::Field_t;
+ using VecField_t = typename core_types::VecField_t;
+ using Electromag_t = typename core_types::Electromag_t;
+ using Ions_t = typename core_types::Ions_t;
+ using GridLayout_t = typename core_types::GridLayout_t;
+ using ParticleArray_t = typename core_types::ParticleArray_t;
+ using IonPopulation_t = typename core_types::IonPopulation_t;
+ using Electrons_t = typename core_types::Electrons_t;
+ using ParticleInitializerFactory = typename core_types::ParticleInitializerFactory_t;📝 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.
| using Array_t = core_types::Array_t; | |
| using Grid_t = core_types::Grid_t; | |
| using Field_t = core_types::Field_t; | |
| using VecField_t = core_types::VecField_t; | |
| using Electromag_t = core_types::Electromag_t; | |
| using Ions_t = core_types::Ions_t; | |
| using GridLayout_t = core_types::GridLayout_t; | |
| using ParticleArray_t = core_types::ParticleArray_t; | |
| using IonPopulation_t = core_types::IonPopulation_t; | |
| using Electrons_t = core_types::Electrons_t; | |
| using ParticleInitializerFactory = core_types::ParticleInitializerFactory_t; | |
| using Array_t = typename core_types::Array_t; | |
| using Grid_t = typename core_types::Grid_t; | |
| using Field_t = typename core_types::Field_t; | |
| using VecField_t = typename core_types::VecField_t; | |
| using Electromag_t = typename core_types::Electromag_t; | |
| using Ions_t = typename core_types::Ions_t; | |
| using GridLayout_t = typename core_types::GridLayout_t; | |
| using ParticleArray_t = typename core_types::ParticleArray_t; | |
| using IonPopulation_t = typename core_types::IonPopulation_t; | |
| using Electrons_t = typename core_types::Electrons_t; | |
| using ParticleInitializerFactory = typename core_types::ParticleInitializerFactory_t; |
🤖 Prompt for AI Agents
In src/simulator/phare_types.hpp around lines 18 to 29, the type aliases
referencing nested types within the dependent type core_types must be prefixed
with the keyword typename; update each using declaration (Array_t, Grid_t,
Field_t, VecField_t, Electromag_t, Ions_t, GridLayout_t, ParticleArray_t,
IonPopulation_t, Electrons_t, ParticleInitializerFactory) to use "typename
core_types::..." so the compiler recognizes these as dependent types.
| using hierarchy_t = amr_types::hierarchy_t; | ||
| using Splitter = amr_types::Splitter_t; | ||
| using RefinementParams = amr_types::RefinementParams; | ||
|
|
There was a problem hiding this comment.
Add typename to amr_types nested aliases
[hierarchy_t, Splitter_t, RefinementParams] are dependent types.
- using hierarchy_t = amr_types::hierarchy_t;
- using Splitter = amr_types::Splitter_t;
- using RefinementParams = amr_types::RefinementParams;
+ using hierarchy_t = typename amr_types::hierarchy_t;
+ using Splitter = typename amr_types::Splitter_t;
+ using RefinementParams = typename amr_types::RefinementParams;📝 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.
| using hierarchy_t = amr_types::hierarchy_t; | |
| using Splitter = amr_types::Splitter_t; | |
| using RefinementParams = amr_types::RefinementParams; | |
| using hierarchy_t = typename amr_types::hierarchy_t; | |
| using Splitter = typename amr_types::Splitter_t; | |
| using RefinementParams = typename amr_types::RefinementParams; |
🤖 Prompt for AI Agents
In src/simulator/phare_types.hpp around lines 31 to 34, the three using aliases
reference dependent nested types from the template-dependent amr_types and
therefore must be prefixed with typename; update the declarations to use
'typename amr_types::hierarchy_t', 'typename amr_types::Splitter_t', and
'typename amr_types::RefinementParams' respectively so the compiler recognizes
them as types.
| using solver_types = solver::PHARE_Types<opts>; | ||
| using HybridModel_t = solver_types::HybridModel_t; | ||
| using MHDModel_t = solver_types::MHDModel_t; | ||
| using SolverPPC_t = solver_types::SolverPPC_t; | ||
| using SolverMHD_t = solver_types::SolverMHD_t; | ||
| using MessengerFactory = solver_types::MessengerFactory; | ||
| using MultiPhysicsIntegrator = solver_types::MultiPhysicsIntegrator_t; | ||
| }; |
There was a problem hiding this comment.
Add typename to solver_types nested aliases
Same dependent-type rule applies here.
- using HybridModel_t = solver_types::HybridModel_t;
- using MHDModel_t = solver_types::MHDModel_t;
- using SolverPPC_t = solver_types::SolverPPC_t;
- using SolverMHD_t = solver_types::SolverMHD_t;
- using MessengerFactory = solver_types::MessengerFactory;
- using MultiPhysicsIntegrator = solver_types::MultiPhysicsIntegrator_t;
+ using HybridModel_t = typename solver_types::HybridModel_t;
+ using MHDModel_t = typename solver_types::MHDModel_t;
+ using SolverPPC_t = typename solver_types::SolverPPC_t;
+ using SolverMHD_t = typename solver_types::SolverMHD_t;
+ using MessengerFactory = typename solver_types::MessengerFactory;
+ using MultiPhysicsIntegrator = typename solver_types::MultiPhysicsIntegrator_t;📝 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.
| using solver_types = solver::PHARE_Types<opts>; | |
| using HybridModel_t = solver_types::HybridModel_t; | |
| using MHDModel_t = solver_types::MHDModel_t; | |
| using SolverPPC_t = solver_types::SolverPPC_t; | |
| using SolverMHD_t = solver_types::SolverMHD_t; | |
| using MessengerFactory = solver_types::MessengerFactory; | |
| using MultiPhysicsIntegrator = solver_types::MultiPhysicsIntegrator_t; | |
| }; | |
| using solver_types = solver::PHARE_Types<opts>; | |
| using HybridModel_t = typename solver_types::HybridModel_t; | |
| using MHDModel_t = typename solver_types::MHDModel_t; | |
| using SolverPPC_t = typename solver_types::SolverPPC_t; | |
| using SolverMHD_t = typename solver_types::SolverMHD_t; | |
| using MessengerFactory = typename solver_types::MessengerFactory; | |
| using MultiPhysicsIntegrator = typename solver_types::MultiPhysicsIntegrator_t; | |
| }; |
🤖 Prompt for AI Agents
In src/simulator/phare_types.hpp around lines 35 to 42, the using aliases
reference dependent nested types from solver::PHARE_Types<opts> without the
required typename keyword; add "typename" before each dependent nested type
(e.g., typename solver_types::HybridModel_t, typename solver_types::MHDModel_t,
typename solver_types::SolverPPC_t, typename solver_types::SolverMHD_t, typename
solver_types::MessengerFactory, typename solver_types::MultiPhysicsIntegrator_t)
so the compiler treats them as types.
2 participants
Replace C++ Simulator class templates with single constexpr class struct
This should simplify MHD Simulator additions, and I'm already using this for GPU