Draft
Conversation
📝 WalkthroughWalkthroughRemoves abstract Pusher and factory; refactors Boris pusher into a boris namespace with helpers/exceptions; replaces dynamic pusher with a value-based BorisPusher in IonUpdater; adds particle/cell utilities, interpolator per‑particle helper, box/indexer tweaks, and updates tests/bench code paths. Changes
Sequence Diagram(s)sequenceDiagram
participant Updater as IonUpdater
participant Pusher as BorisPusher
participant Interp as Interpolator
participant EM as Electromag
participant Cells as ParticleArray/CellMap
Updater->>Pusher: move<copy_flag>(Population, EM, Interp, Boxing)
Pusher->>Interp: sample fields / compute weights
Pusher->>EM: read E/B at particle positions
Pusher->>Pusher: boris::advance / boris::accelerate (update vel/pos)
Pusher->>Cells: change_icell / swap_last_reduce_by_one / swap (update cell mappings)
Pusher-->>Updater: return updated population state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/bench/core/numerics/pusher/push_raw_use.cpp (1)
30-42:⚠️ Potential issue | 🟠 MajorBenchmark is pushing an empty ion population.
Particles are read into a local
domainParticles, butmove<copy_particle>runs onions[0], which is never populated from that buffer.Proposed fix
- PHARE::core::UsableIons<ParticleArray, interp> ions{layout}; - - ParticleArray domainParticles{layout.AMRBox()}; + PHARE::core::UsableIons<ParticleArray, interp> ions{layout}; + auto& domainParticles = ions[0].domainParticles(); std::stringstream ss; ss << "unsorted_particles_" << dim << ".raw"; PHARE::core::bench::read_raw_from_file(domainParticles, ss.str()); - ParticleArray tmpDomain{layout.AMRBox()}; - tmpDomain.vector() = domainParticles.vector();Also applies to: 47-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/bench/core/numerics/pusher/push_raw_use.cpp` around lines 30 - 42, The benchmark reads particles into domainParticles but never populates the ions container before calling move<copy_particle> on ions[0]; update the code to copy the loaded particles into the ions population (e.g., assign domainParticles.vector() into ions[0] or into tmpDomain and then set ions[0].vector() = tmpDomain.vector()) so that ions[0] contains the data from read_raw_from_file before move<copy_particle> is invoked on it.
🧹 Nitpick comments (9)
src/core/data/particles/particle_array.hpp (1)
170-176: Preferstd::swapover manual swap.The manual swap pattern using a temporary variable is less expressive than
std::swapand may miss move optimizations for particles with movable members.♻️ Suggested improvement
void swap(std::size_t const a, std::size_t const b) { cellMap_.swap(particles_[a].iCell, particles_[b].iCell, a, b); - auto const tmp = particles_[a]; - particles_[a] = particles_[b]; - particles_[b] = tmp; + std::swap(particles_[a], particles_[b]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/data/particles/particle_array.hpp` around lines 170 - 176, In the particle_array::swap method replace the manual three-step element swap with std::swap to get move optimizations: keep the existing cellMap_.swap(particles_[a].iCell, particles_[b].iCell, a, b) call, then call std::swap(particles_[a], particles_[b]) instead of the tmp assignment; add the appropriate header (e.g. <utility>) if not already included.src/core/numerics/pusher/boris.hpp (2)
300-307: Lambda with capture in partition call.The lambda at line 302 captures
boxingby reference. While this is in the partition operation (not per-particle iteration), based on learnings for the PHARE codebase, lambdas with captures in particle-related operations may impact performance. Consider extracting to a predicate functor if profiling shows this is significant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/numerics/pusher/boris.hpp` around lines 300 - 307, The lambda passed to domain.partition (the predicate calling core::isIn(cell, boxing.domainBox)) captures boxing by reference which can hurt performance; extract it into a small predicate functor (e.g., struct IsInDomainBox { Box box; bool operator()(Cell const& c) const { return core::isIn(c, box); } };) and use it in the domain.partition call so the domain.partition invocation no longer uses a capturing lambda — keep the predicate name (e.g., IsInDomainBox) and construct it with boxing.domainBox where the partition is called, then leave the subsequent use of not_in_domain, patch_ghost and domain.erase unchanged.
127-129: Remove unusedOnChangeIcelltype alias.The
OnChangeIcelltype alias is defined at lines 127-129 but is not used anywhere in the codebase. Remove this dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/numerics/pusher/boris.hpp` around lines 127 - 129, Remove the dead type alias OnChangeIcell: delete the using OnChangeIcell = std::function<...> declaration (the lines defining OnChangeIcell that reference ParticleArray_t and Particle_t). After removing it, verify there are no remaining references to OnChangeIcell and remove any now-unused includes (e.g., <functional>) if they become unused.src/core/utilities/box/box.hpp (1)
262-277: Simplify redundant return logic.The function has redundant control flow: after the loop,
pointInBoxis checked and returned if true, then false is returned unconditionally. This can be simplified.♻️ Suggested simplification
template<template<typename, std::size_t> typename Point, typename Type, std::size_t SIZE> NO_DISCARD bool isIn(Point<Type, SIZE> const& point, Box<Type, SIZE> const& box) { - auto isIn1D = [](auto const pos, auto const lower, auto const upper) { - return pos >= lower && pos <= upper; - }; - - bool pointInBox = true; - - for (auto iDim = 0u; iDim < SIZE; ++iDim) - pointInBox = pointInBox && isIn1D(point[iDim], box.lower[iDim], box.upper[iDim]); - if (pointInBox) - return pointInBox; - - return false; + for (auto iDim = 0u; iDim < SIZE; ++iDim) + if (point[iDim] < box.lower[iDim] || point[iDim] > box.upper[iDim]) + return false; + return true; }This also enables early exit on the first out-of-bounds dimension, which may improve performance in hot paths. Based on learnings, avoiding lambdas in runtime particle operations is preferred for predictable inlining.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/utilities/box/box.hpp` around lines 262 - 277, The isIn function contains redundant final returns and a lambda; simplify by removing the isIn1D lambda and the pointInBox accumulation, and instead iterate dimensions in the template function isIn (Point<Type,SIZE> and Box<Type,SIZE>) performing an immediate comparison per dimension (if point[iDim] < box.lower[iDim] || point[iDim] > box.upper[iDim] return false) and return true after the loop; this yields early exit on first out-of-bounds and eliminates the unnecessary conditional and extra return.src/core/numerics/ion_updater/ion_updater.hpp (1)
117-151: Selectors usestd::functionwith lambda captures.The selector lambdas (e.g.,
inDomainBox,inGhostBox, etc.) are stored asstd::function, which introduces type erasure overhead compared to direct function objects. The copy-captures for boxes (e.g.,[domainBox = domainBox]) are intentional for object lifetime safety.If profiling indicates these selectors are performance-critical, consider using template-based selectors or compile-time polymorphism instead of
std::function. Based on learnings, avoiding lambdas with captures in runtime particle operations is preferred for predictable inlining.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/numerics/ion_updater/ion_updater.hpp` around lines 117 - 151, Selectors like inDomainBox/inGhostBox are stored in Selector_t (std::function) which causes runtime type erasure and prevents inlining; replace these stored std::function selectors with concrete functor types or templates: change Selector_t usage to accept/propagate template functor parameters or define small capture-owning struct functors (e.g., struct InDomainBox { Box domainBox; auto operator()(auto& particleRange) const { ... } };) and use those types instead of std::function so the lambdas (inDomainBox, inGhostBox, inNonLevelGhostBox, inGhostLayer, outsideGhostBox) become inlinable and avoid heap/type-erasure overhead while keeping the existing copy-captures for lifetime safety.tests/core/numerics/pusher/test_pusher.cpp (1)
100-102: EmptyparticleToMeshstub may mask issues.The empty
particleToMeshstub silently ignores all arguments. While acceptable for trajectory tests that don't validate deposition, this could mask interface mismatches if the productionInterpolator::particleToMeshsignature changes.Consider adding a minimal assertion or comment documenting why deposition is intentionally ignored in these tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/numerics/pusher/test_pusher.cpp` around lines 100 - 102, The test-local stub particleToMesh currently swallows all args which can hide signature/API regressions; update the stub in tests/core/numerics/pusher/test_pusher.cpp by either adding a static_assert or runtime assertion that verifies the expected signature (e.g., check sizeof...(args) or use std::is_invocable with the production signature) or, at minimum, add a clear comment explaining why deposition is intentionally ignored for these trajectory-only tests and document the expected production signature (reference particleToMesh). This will ensure interface mismatches are flagged while preserving the test intent.src/core/numerics/interpolator/interpolator.hpp (1)
471-497: Consider replacing lambdas with explicit functors for hot particle paths.The per-particle deposition uses lambdas for attribute extraction. While these lambdas are simple and capture-less, based on learnings for the PHARE codebase, runtime particle operations may benefit from explicit functor classes instead of lambdas for more predictable inlining in hot paths.
Given the PR's focus on reducing particle loops and memory usage, this is worth considering but may be acceptable as-is since the lambdas are trivial.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/numerics/interpolator/interpolator.hpp` around lines 471 - 497, Replace the capture-less lambdas used as attribute extractors in particleToMesh (the calls to particleToMesh_ for particleDensity, chargeDensity, xFlux, yFlux, zFlux) with small named functor structs or stateless classes (e.g., PartOneFunctor, ChargeFunctor, VxFunctor, VyFunctor, VzFunctor) that implement operator() and return the same values; update the five particleToMesh_ invocations to pass these functor types instead of lambdas so the hot particle path is explicit and more likely to inline/predictably optimize.src/core/utilities/cellmap.hpp (1)
293-300: Unused parameteritemsin erase overload.The
itemsparameter is declared but not used in the function body. This may be intentional for API consistency with othereraseoverloads, but consider either:
- Using
[[maybe_unused]]to document the intent- Removing the parameter if not needed for this overload
♻️ Suggested fix if parameter is intentionally unused
template<std::size_t dim, typename cell_index_t> template<typename Array, typename> -inline void CellMap<dim, cell_index_t>::erase(Array const& items, cell_t const oldCell, +inline void CellMap<dim, cell_index_t>::erase([[maybe_unused]] Array const& items, cell_t const oldCell, std::size_t const itemIndex) { auto& blist = cellIndexes_(local_(oldCell)); blist.remove(itemIndex); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/utilities/cellmap.hpp` around lines 293 - 300, The erase overload in CellMap (template<typename Array, typename> inline void CellMap<dim, cell_index_t>::erase(Array const& items, cell_t const oldCell, std::size_t const itemIndex)) declares items but never uses it; either mark the parameter as intentionally unused by annotating it with [[maybe_unused]] (e.g. [[maybe_unused]] Array const& items) or remove the items parameter from this overload signature and all callers that rely on it, keeping the body that calls cellIndexes_(local_(oldCell)).remove(itemIndex) unchanged; update declarations/overloads consistently to avoid ambiguity.tools/bench/core/numerics/pusher/pusher.cpp (1)
8-10: Benchmark only exercises the copy path.With
copy_particlefixed totrue, this benchmark cannot quantify the no-copy behavior introduced by this PR. Consider adding a second variant (or parameterizing the benchmark) forcopy_particle=false.Also applies to: 48-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/bench/core/numerics/pusher/pusher.cpp` around lines 8 - 10, The benchmark currently hardcodes bool constexpr copy_particle = true so it only measures the copy code path; change the harness to run two variants (copy_particle=true and copy_particle=false) or parameterize the benchmark flag so both paths are exercised (update the constexpr/templated setting used in pusher.cpp and the other occurrence of copy_particle), run the benchmark for each variant (keeping cells and n_parts the same) and collect separate results so the no-copy behavior introduced by the PR is properly measured.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tools/bench/core/numerics/pusher/push_raw_use.cpp`:
- Around line 30-42: The benchmark reads particles into domainParticles but
never populates the ions container before calling move<copy_particle> on
ions[0]; update the code to copy the loaded particles into the ions population
(e.g., assign domainParticles.vector() into ions[0] or into tmpDomain and then
set ions[0].vector() = tmpDomain.vector()) so that ions[0] contains the data
from read_raw_from_file before move<copy_particle> is invoked on it.
---
Nitpick comments:
In `@src/core/data/particles/particle_array.hpp`:
- Around line 170-176: In the particle_array::swap method replace the manual
three-step element swap with std::swap to get move optimizations: keep the
existing cellMap_.swap(particles_[a].iCell, particles_[b].iCell, a, b) call,
then call std::swap(particles_[a], particles_[b]) instead of the tmp assignment;
add the appropriate header (e.g. <utility>) if not already included.
In `@src/core/numerics/interpolator/interpolator.hpp`:
- Around line 471-497: Replace the capture-less lambdas used as attribute
extractors in particleToMesh (the calls to particleToMesh_ for particleDensity,
chargeDensity, xFlux, yFlux, zFlux) with small named functor structs or
stateless classes (e.g., PartOneFunctor, ChargeFunctor, VxFunctor, VyFunctor,
VzFunctor) that implement operator() and return the same values; update the five
particleToMesh_ invocations to pass these functor types instead of lambdas so
the hot particle path is explicit and more likely to inline/predictably
optimize.
In `@src/core/numerics/ion_updater/ion_updater.hpp`:
- Around line 117-151: Selectors like inDomainBox/inGhostBox are stored in
Selector_t (std::function) which causes runtime type erasure and prevents
inlining; replace these stored std::function selectors with concrete functor
types or templates: change Selector_t usage to accept/propagate template functor
parameters or define small capture-owning struct functors (e.g., struct
InDomainBox { Box domainBox; auto operator()(auto& particleRange) const { ... }
};) and use those types instead of std::function so the lambdas (inDomainBox,
inGhostBox, inNonLevelGhostBox, inGhostLayer, outsideGhostBox) become inlinable
and avoid heap/type-erasure overhead while keeping the existing copy-captures
for lifetime safety.
In `@src/core/numerics/pusher/boris.hpp`:
- Around line 300-307: The lambda passed to domain.partition (the predicate
calling core::isIn(cell, boxing.domainBox)) captures boxing by reference which
can hurt performance; extract it into a small predicate functor (e.g., struct
IsInDomainBox { Box box; bool operator()(Cell const& c) const { return
core::isIn(c, box); } };) and use it in the domain.partition call so the
domain.partition invocation no longer uses a capturing lambda — keep the
predicate name (e.g., IsInDomainBox) and construct it with boxing.domainBox
where the partition is called, then leave the subsequent use of not_in_domain,
patch_ghost and domain.erase unchanged.
- Around line 127-129: Remove the dead type alias OnChangeIcell: delete the
using OnChangeIcell = std::function<...> declaration (the lines defining
OnChangeIcell that reference ParticleArray_t and Particle_t). After removing it,
verify there are no remaining references to OnChangeIcell and remove any
now-unused includes (e.g., <functional>) if they become unused.
In `@src/core/utilities/box/box.hpp`:
- Around line 262-277: The isIn function contains redundant final returns and a
lambda; simplify by removing the isIn1D lambda and the pointInBox accumulation,
and instead iterate dimensions in the template function isIn (Point<Type,SIZE>
and Box<Type,SIZE>) performing an immediate comparison per dimension (if
point[iDim] < box.lower[iDim] || point[iDim] > box.upper[iDim] return false) and
return true after the loop; this yields early exit on first out-of-bounds and
eliminates the unnecessary conditional and extra return.
In `@src/core/utilities/cellmap.hpp`:
- Around line 293-300: The erase overload in CellMap (template<typename Array,
typename> inline void CellMap<dim, cell_index_t>::erase(Array const& items,
cell_t const oldCell, std::size_t const itemIndex)) declares items but never
uses it; either mark the parameter as intentionally unused by annotating it with
[[maybe_unused]] (e.g. [[maybe_unused]] Array const& items) or remove the items
parameter from this overload signature and all callers that rely on it, keeping
the body that calls cellIndexes_(local_(oldCell)).remove(itemIndex) unchanged;
update declarations/overloads consistently to avoid ambiguity.
In `@tests/core/numerics/pusher/test_pusher.cpp`:
- Around line 100-102: The test-local stub particleToMesh currently swallows all
args which can hide signature/API regressions; update the stub in
tests/core/numerics/pusher/test_pusher.cpp by either adding a static_assert or
runtime assertion that verifies the expected signature (e.g., check
sizeof...(args) or use std::is_invocable with the production signature) or, at
minimum, add a clear comment explaining why deposition is intentionally ignored
for these trajectory-only tests and document the expected production signature
(reference particleToMesh). This will ensure interface mismatches are flagged
while preserving the test intent.
In `@tools/bench/core/numerics/pusher/pusher.cpp`:
- Around line 8-10: The benchmark currently hardcodes bool constexpr
copy_particle = true so it only measures the copy code path; change the harness
to run two variants (copy_particle=true and copy_particle=false) or parameterize
the benchmark flag so both paths are exercised (update the constexpr/templated
setting used in pusher.cpp and the other occurrence of copy_particle), run the
benchmark for each variant (keeping cells and n_parts the same) and collect
separate results so the no-copy behavior introduced by the PR is properly
measured.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/core/CMakeLists.txtsrc/core/data/particles/particle_array.hppsrc/core/errors.hppsrc/core/numerics/interpolator/interpolator.hppsrc/core/numerics/ion_updater/ion_updater.hppsrc/core/numerics/pusher/boris.hppsrc/core/numerics/pusher/pusher.hppsrc/core/numerics/pusher/pusher_factory.hppsrc/core/utilities/box/box.hppsrc/core/utilities/cellmap.hpptests/core/data/gridlayout/test_gridlayout.hpptests/core/data/ion_population/test_ion_population_fixtures.hpptests/core/numerics/ion_updater/test_updater.cpptests/core/numerics/pusher/test_pusher.cpptools/bench/core/numerics/ion_updater/bench_ion_updater.cpptools/bench/core/numerics/pusher/push_raw_use.cpptools/bench/core/numerics/pusher/pusher.cpp
💤 Files with no reviewable changes (4)
- src/core/CMakeLists.txt
- src/core/numerics/pusher/pusher_factory.hpp
- tests/core/numerics/ion_updater/test_updater.cpp
- src/core/numerics/pusher/pusher.hpp
PhilipDeegan
force-pushed
the
updater_realloc
branch
from
5e54e81 to
5c9d3c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/bench/core/numerics/pusher/push_raw_use.cpp (1)
30-47:⚠️ Potential issue | 🟠 MajorParticles read from file are never used in the push operation.
The benchmark reads particles from a file into
domainParticles(lines 32-36) and copies them totmpDomain(lines 40-41), but the actual push at line 47 operates onions[0], which is populated by theUsableIonsconstructor—not the file data.This appears to be an incomplete refactoring. Either:
- The file-loaded particles should be transferred to
ions[0].domainParticles(), or- The unused
domainParticlesandtmpDomainvariables should be removed if the benchmark intent changed.🛠️ If file data should be used
PHARE::core::UsableIons<ParticleArray, interp> ions{layout}; - ParticleArray domainParticles{layout.AMRBox()}; - std::stringstream ss; ss << "unsorted_particles_" << dim << ".raw"; - PHARE::core::bench::read_raw_from_file(domainParticles, ss.str()); - - // std::sort(domainParticles); - - ParticleArray tmpDomain{layout.AMRBox()}; - tmpDomain.vector() = domainParticles.vector(); + PHARE::core::bench::read_raw_from_file(ions[0].domainParticles(), ss.str()); BorisPusher_t pusher;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/bench/core/numerics/pusher/push_raw_use.cpp` around lines 30 - 47, The code reads particles into domainParticles/tmpDomain but then pushes ions[0] (created by UsableIons) instead, so either copy the loaded data into the ion used for pushing or remove the unused variables; specifically, if you want to use the file data, assign tmpDomain.vector() (or domainParticles.vector()) into ions[0].domainParticles() (or otherwise set ions[0]’s particle buffer) before calling pusher.template move<copy_particle>(ions[0], em, interpolator, boxing); otherwise delete domainParticles/tmpDomain and their file-read calls to avoid dead code.tests/core/numerics/pusher/test_pusher.cpp (1)
309-313:⚠️ Potential issue | 🟡 MinorUse deterministic RNG seeding in this unit test.
std::random_deviceseeding makes failures non-reproducible. A fixed seed is safer for CI reliability.Proposed change
- std::random_device rd; - std::mt19937 gen(rd()); + std::mt19937 gen(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/numerics/pusher/test_pusher.cpp` around lines 309 - 313, Replace the non-deterministic std::random_device seeding with a fixed seed to make the test reproducible: instead of creating std::random_device rd and std::mt19937 gen(rd()), initialize std::mt19937 gen with a constant seed (e.g. a named const unsigned SEED = 12345 or similar) so the distributions dis and delta produce deterministic sequences; update the variables referenced (rd, gen, dis, delta) accordingly and remove the std::random_device usage.
🧹 Nitpick comments (6)
src/core/utilities/box/box.hpp (1)
262-277: Redundant checks and missing short-circuit inisIn.The current implementation has a few inefficiencies:
pointInBox && isIn1D(...)evaluatesisIn1Deven whenpointInBoxis alreadyfalse- Lines 273-274 check
if (pointInBox) return pointInBox;then unconditionallyreturn false;— this is equivalent to justreturn pointInBox;Consider simplifying with early exit:
♻️ Suggested simplification
template<template<typename, std::size_t> typename Point, typename Type, std::size_t SIZE> NO_DISCARD bool isIn(Point<Type, SIZE> const& point, Box<Type, SIZE> const& box) { - auto isIn1D = [](auto const pos, auto const lower, auto const upper) { - return pos >= lower && pos <= upper; - }; - - bool pointInBox = true; - - for (auto iDim = 0u; iDim < SIZE; ++iDim) - pointInBox = pointInBox && isIn1D(point[iDim], box.lower[iDim], box.upper[iDim]); - if (pointInBox) - return pointInBox; - - return false; + for (auto iDim = 0u; iDim < SIZE; ++iDim) + if (point[iDim] < box.lower[iDim] || point[iDim] > box.upper[iDim]) + return false; + return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/utilities/box/box.hpp` around lines 262 - 277, The isIn function currently computes pointInBox across all dimensions and then redundantly checks/returns it; change it to short-circuit: inside isIn (and using the existing isIn1D lambda) iterate dimensions and immediately return false when isIn1D(point[iDim], box.lower[iDim], box.upper[iDim]) is false, and after the loop return true; remove the pointInBox accumulation and the redundant if (pointInBox) / return false sequence so the function returns as soon as a dimension fails and returns true only if all dimensions pass.src/core/numerics/pusher/boris.hpp (1)
128-129: Unused type aliasOnChangeIcell.The
OnChangeIcelltype alias usingstd::functionis declared but never used in the class. Consider removing it to reduce code clutter, or if it's intended for future use, add a comment explaining its purpose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/numerics/pusher/boris.hpp` around lines 128 - 129, Remove the unused type alias OnChangeIcell (currently defined as std::function<bool(ParticleArray_t&, Particle_t&, std::array<int, dim> const&, std::size_t const)>) to eliminate dead code; if it was intended to be kept for future API use, instead add a brief comment above the typedef explaining its intended purpose and why it's currently unused so reviewers understand its presence. Ensure the change targets the OnChangeIcell alias declaration in boris.hpp and that no references to OnChangeIcell remain elsewhere before removal.tools/bench/core/numerics/pusher/pusher.cpp (1)
45-48: Unused variableno_op.The
no_oplambda is declared but never used after the refactoring to the new pusher interface. It can be removed.Interpolator interpolator; - auto const no_op = [](auto& particleRange) { return particleRange; }; while (state.KeepRunningBatch(1)) pusher.template move<copy_particle>(ions[0], em, interpolator, boxing);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/bench/core/numerics/pusher/pusher.cpp` around lines 45 - 48, Remove the dead no_op lambda declaration (auto const no_op = [](auto& particleRange) { return particleRange; };) since it is unused after the pusher interface refactor; delete that line and ensure nothing else references no_op (verify usages around pusher.template move<copy_particle>(ions[0], em, interpolator, boxing) and the surrounding loop remain unchanged).src/core/utilities/cellmap.hpp (1)
300-307: Unuseditemsparameter in erase overload.The
itemsparameter is never used in this function body—onlyoldCellanditemIndexare accessed. Consider removing it or marking it[[maybe_unused]]if it's kept for API consistency.template<std::size_t dim, typename cell_index_t> template<typename Array, typename> -inline void CellMap<dim, cell_index_t>::erase(Array const& items, cell_t const oldCell, +inline void CellMap<dim, cell_index_t>::erase(Array const& /*items*/, cell_t const oldCell, std::size_t const itemIndex) { auto& blist = cellIndexes_(local_(oldCell)); blist.remove(itemIndex); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/utilities/cellmap.hpp` around lines 300 - 307, The erase overload CellMap<dim, cell_index_t>::erase has an unused parameter items; either remove the parameter from the function signature and all call sites or explicitly mark it as unused to avoid warnings. Update the declaration/definition for template<typename Array, typename> inline void CellMap<dim, cell_index_t>::erase(...) to drop the Array const& items parameter (and adjust callers) or change it to [[maybe_unused]] Array const& items (or add (void)items; / static_cast<void>(items); at the top) so the body that uses local_(oldCell), cellIndexes_ and blist.remove(itemIndex) compiles without unused-parameter warnings.src/core/data/particles/particle_array.hpp (1)
170-176: Consider usingstd::swapfor particle swap.The manual copy-based swap works but
std::swapis more idiomatic and potentially more efficient ifParticle_thas move semantics.void swap(std::size_t const a, std::size_t const b) { cellMap_.swap(particles_[a].iCell, particles_[b].iCell, a, b); - auto const tmp = particles_[a]; - particles_[a] = particles_[b]; - particles_[b] = tmp; + std::swap(particles_[a], particles_[b]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/data/particles/particle_array.hpp` around lines 170 - 176, Replace the manual copy-based swap in particle_array::swap with std::swap to leverage move semantics: keep the existing call to cellMap_.swap(particles_[a].iCell, particles_[b].iCell, a, b) and then call std::swap(particles_[a], particles_[b]) instead of the tmp-copy sequence; ensure the header that provides std::swap (e.g., <utility>) is included if not already.tests/core/numerics/pusher/test_pusher.cpp (1)
163-219: Trajectory tests bypassBorisPusher::move, reducing integration-path coverage.Line 211 implements a local integrator (
boris::advance/accelerate) while Line 169 constructs/configurespusher. This can letBorisPusher::moveregress without failing these tests. Consider either usingpusher->move(...)in this fixture or removingpusherentirely to keep test intent explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/numerics/pusher/test_pusher.cpp` around lines 163 - 219, The test fixture constructs a BorisPusher (pusher) but the move() helper calls boris::advance and boris::accelerate directly, so BorisPusher::move isn't exercised; either call pusher->move(...) from the fixture's move() so the pusher implementation is tested (pass the same particles, interpolator, em, layout, dt/mass as BorisPusher expects) or remove the pusher member entirely and delete its setup to make the test intent explicit; update references to pusher, BorisPusher::move, boris::advance, and boris::accelerate accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/utilities/indexer.hpp`:
- Line 62: The update() method in indexer.hpp currently writes to
indexes_[offset] without bounds checking; modify the method (void
update(std::size_t const offset, std::size_t const index)) to validate that
offset < indexes_.size() and if not throw a std::out_of_range (or similar) with
a clear message containing offset and indexes_.size(), otherwise perform the
assignment to indexes_[offset] as before; this ensures the public API is safe
even when callers omit external assertions.
- Around line 64-72: findIndex currently dereferences std::find without checking
for end(), causing UB in release builds; change findIndex and position to use
explicit return types and runtime checks: have findIndex(std::size_t index)
search with auto it = std::find(std::begin(indexes_), std::end(indexes_),
index), if it == std::end(indexes_) throw std::out_of_range or return an
optional, otherwise return the found value (prefer returning std::size_t by
value or an iterator instead of a reference); update position(std::size_t) to
accept the same runtime-checked result and compute the position with
std::distance(std::begin(indexes_), it) and return a std::size_t, removing
reliance on pointer arithmetic against findIndex and eliminating unchecked
dereference of end().
In `@tests/core/numerics/pusher/test_pusher.cpp`:
- Around line 354-364: The test currently only uses predicate checks that can be
vacuously true; ensure at least one explicit assertion that a split happened
after calling pusher->move in the loop (nt times). For example, after the loop
that calls pusher->move(ions[0], em, interpolator, boxing) check a concrete
change: assert that patchGhost (ions[0].patchGhostParticles()) is non-empty OR
that the total particle count in ions[0] (compare size before the moves to size
after) increased, or assert that at least one particle in patchGhost is not
present in the original particlesIn set; add this single explicit assertion
alongside the existing predicate checks so the test fails when no split occurs.
---
Outside diff comments:
In `@tests/core/numerics/pusher/test_pusher.cpp`:
- Around line 309-313: Replace the non-deterministic std::random_device seeding
with a fixed seed to make the test reproducible: instead of creating
std::random_device rd and std::mt19937 gen(rd()), initialize std::mt19937 gen
with a constant seed (e.g. a named const unsigned SEED = 12345 or similar) so
the distributions dis and delta produce deterministic sequences; update the
variables referenced (rd, gen, dis, delta) accordingly and remove the
std::random_device usage.
In `@tools/bench/core/numerics/pusher/push_raw_use.cpp`:
- Around line 30-47: The code reads particles into domainParticles/tmpDomain but
then pushes ions[0] (created by UsableIons) instead, so either copy the loaded
data into the ion used for pushing or remove the unused variables; specifically,
if you want to use the file data, assign tmpDomain.vector() (or
domainParticles.vector()) into ions[0].domainParticles() (or otherwise set
ions[0]’s particle buffer) before calling pusher.template
move<copy_particle>(ions[0], em, interpolator, boxing); otherwise delete
domainParticles/tmpDomain and their file-read calls to avoid dead code.
---
Nitpick comments:
In `@src/core/data/particles/particle_array.hpp`:
- Around line 170-176: Replace the manual copy-based swap in
particle_array::swap with std::swap to leverage move semantics: keep the
existing call to cellMap_.swap(particles_[a].iCell, particles_[b].iCell, a, b)
and then call std::swap(particles_[a], particles_[b]) instead of the tmp-copy
sequence; ensure the header that provides std::swap (e.g., <utility>) is
included if not already.
In `@src/core/numerics/pusher/boris.hpp`:
- Around line 128-129: Remove the unused type alias OnChangeIcell (currently
defined as std::function<bool(ParticleArray_t&, Particle_t&, std::array<int,
dim> const&, std::size_t const)>) to eliminate dead code; if it was intended to
be kept for future API use, instead add a brief comment above the typedef
explaining its intended purpose and why it's currently unused so reviewers
understand its presence. Ensure the change targets the OnChangeIcell alias
declaration in boris.hpp and that no references to OnChangeIcell remain
elsewhere before removal.
In `@src/core/utilities/box/box.hpp`:
- Around line 262-277: The isIn function currently computes pointInBox across
all dimensions and then redundantly checks/returns it; change it to
short-circuit: inside isIn (and using the existing isIn1D lambda) iterate
dimensions and immediately return false when isIn1D(point[iDim],
box.lower[iDim], box.upper[iDim]) is false, and after the loop return true;
remove the pointInBox accumulation and the redundant if (pointInBox) / return
false sequence so the function returns as soon as a dimension fails and returns
true only if all dimensions pass.
In `@src/core/utilities/cellmap.hpp`:
- Around line 300-307: The erase overload CellMap<dim, cell_index_t>::erase has
an unused parameter items; either remove the parameter from the function
signature and all call sites or explicitly mark it as unused to avoid warnings.
Update the declaration/definition for template<typename Array, typename> inline
void CellMap<dim, cell_index_t>::erase(...) to drop the Array const& items
parameter (and adjust callers) or change it to [[maybe_unused]] Array const&
items (or add (void)items; / static_cast<void>(items); at the top) so the body
that uses local_(oldCell), cellIndexes_ and blist.remove(itemIndex) compiles
without unused-parameter warnings.
In `@tests/core/numerics/pusher/test_pusher.cpp`:
- Around line 163-219: The test fixture constructs a BorisPusher (pusher) but
the move() helper calls boris::advance and boris::accelerate directly, so
BorisPusher::move isn't exercised; either call pusher->move(...) from the
fixture's move() so the pusher implementation is tested (pass the same
particles, interpolator, em, layout, dt/mass as BorisPusher expects) or remove
the pusher member entirely and delete its setup to make the test intent
explicit; update references to pusher, BorisPusher::move, boris::advance, and
boris::accelerate accordingly.
In `@tools/bench/core/numerics/pusher/pusher.cpp`:
- Around line 45-48: Remove the dead no_op lambda declaration (auto const no_op
= [](auto& particleRange) { return particleRange; };) since it is unused after
the pusher interface refactor; delete that line and ensure nothing else
references no_op (verify usages around pusher.template
move<copy_particle>(ions[0], em, interpolator, boxing) and the surrounding loop
remain unchanged).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/core/CMakeLists.txtsrc/core/data/particles/particle_array.hppsrc/core/errors.hppsrc/core/numerics/interpolator/interpolator.hppsrc/core/numerics/ion_updater/ion_updater.hppsrc/core/numerics/pusher/boris.hppsrc/core/numerics/pusher/pusher.hppsrc/core/numerics/pusher/pusher_factory.hppsrc/core/utilities/box/box.hppsrc/core/utilities/cellmap.hppsrc/core/utilities/indexer.hpptests/core/data/gridlayout/test_gridlayout.hpptests/core/data/ion_population/test_ion_population_fixtures.hpptests/core/numerics/ion_updater/test_updater.cpptests/core/numerics/pusher/test_pusher.cpptools/bench/core/numerics/ion_updater/bench_ion_updater.cpptools/bench/core/numerics/pusher/push_raw_use.cpptools/bench/core/numerics/pusher/pusher.cpp
💤 Files with no reviewable changes (4)
- src/core/numerics/pusher/pusher_factory.hpp
- src/core/CMakeLists.txt
- tests/core/numerics/ion_updater/test_updater.cpp
- src/core/numerics/pusher/pusher.hpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/errors.hpp
- tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp
- tests/core/data/gridlayout/test_gridlayout.hpp
| } | ||
| } | ||
|
|
||
| void update(std::size_t const offset, std::size_t const index) { indexes_[offset] = index; } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/core/utilities/indexer.hppRepository: PHAREHUB/PHARE
Length of output: 3733
🏁 Script executed:
rg "\.update\(" --type cpp -B 2 -A 2Repository: PHAREHUB/PHARE
Length of output: 4191
🏁 Script executed:
rg -A 10 -B 10 "indexesA.update|indexesB.update" src/core/utilities/cellmap.hppRepository: PHAREHUB/PHARE
Length of output: 691
🏁 Script executed:
grep -r "\.update(" src/core/utilities/cellmap.hpp -B 5 -A 2 | head -30Repository: PHAREHUB/PHARE
Length of output: 241
Guard update() against out-of-range offsets.
The update() method writes to indexes_[offset] without validation. While the sole current usage in cellmap.hpp is protected by assertions checking is_indexed(), these assertions are stripped in release builds. The public method itself remains unsafe and can cause undefined behavior if invoked with an invalid offset. Add explicit bounds checking to the method.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/utilities/indexer.hpp` at line 62, The update() method in
indexer.hpp currently writes to indexes_[offset] without bounds checking; modify
the method (void update(std::size_t const offset, std::size_t const index)) to
validate that offset < indexes_.size() and if not throw a std::out_of_range (or
similar) with a clear message containing offset and indexes_.size(), otherwise
perform the assignment to indexes_[offset] as before; this ensures the public
API is safe even when callers omit external assertions.
Comment on lines
+64
to
+72
| auto& findIndex(std::size_t const index) const | ||
| { | ||
| // assumes exists | ||
| return *std::find(std::begin(indexes_), std::end(indexes_), index); | ||
| } | ||
| auto position(std::size_t const index) const | ||
| { | ||
| return std::distance(indexes_.data(), &findIndex(index)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat src/core/utilities/indexer.hppRepository: PHAREHUB/PHARE
Length of output: 3019
🏁 Script executed:
rg -n "findIndex|position" --type cpp --type hpp | grep -i indexer | head -20Repository: PHAREHUB/PHARE
Length of output: 85
🏁 Script executed:
rg -n "\.findIndex\(|\.position\(" --type cpp --type hpp | head -20Repository: PHAREHUB/PHARE
Length of output: 85
🏁 Script executed:
rg "\.findIndex\(|\.position\(" --max-count=20Repository: PHAREHUB/PHARE
Length of output: 209
🏁 Script executed:
rg -B 5 -A 5 "\.position\(" src/core/utilities/cellmap.hppRepository: PHAREHUB/PHARE
Length of output: 316
🏁 Script executed:
rg "position\(|findIndex\(" --max-count=50Repository: PHAREHUB/PHARE
Length of output: 468
findIndex/position dereference end() when missing index without release-build protection.
Line 64's findIndex() blindly dereferences std::find() without checking for end(). Line 68's position() depends on this. While the only current callers in cellmap.hpp validate preconditions with assert(), these asserts are stripped in release builds (-DNDEBUG), leaving undefined behavior unprotected. Future callers may not validate preconditions. Replace with runtime checks and explicit return types:
Proposed fix
- auto& findIndex(std::size_t const index) const
+ std::size_t const& findIndex(std::size_t const index) const
{
- // assumes exists
- return *std::find(std::begin(indexes_), std::end(indexes_), index);
+ auto it = std::find(std::begin(indexes_), std::end(indexes_), index);
+ assert(it != std::end(indexes_));
+ if (it == std::end(indexes_))
+ throw std::out_of_range("Indexer::findIndex index not found");
+ return *it;
}
- auto position(std::size_t const index) const
+ std::size_t position(std::size_t const index) const
{
- return std::distance(indexes_.data(), &findIndex(index));
+ auto it = std::find(std::begin(indexes_), std::end(indexes_), index);
+ assert(it != std::end(indexes_));
+ if (it == std::end(indexes_))
+ throw std::out_of_range("Indexer::position index not found");
+ return static_cast<std::size_t>(std::distance(std::begin(indexes_), it));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/utilities/indexer.hpp` around lines 64 - 72, findIndex currently
dereferences std::find without checking for end(), causing UB in release builds;
change findIndex and position to use explicit return types and runtime checks:
have findIndex(std::size_t index) search with auto it =
std::find(std::begin(indexes_), std::end(indexes_), index), if it ==
std::end(indexes_) throw std::out_of_range or return an optional, otherwise
return the found value (prefer returning std::size_t by value or an iterator
instead of a reference); update position(std::size_t) to accept the same
runtime-checked result and compute the position with
std::distance(std::begin(indexes_), it) and return a std::size_t, removing
reliance on pointer arithmetic against findIndex and eliminating unchecked
dereference of end().
PhilipDeegan
force-pushed
the
updater_realloc
branch
from
5c9d3c8 to
dca64d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/bench/core/numerics/pusher/push_raw_use.cpp (1)
29-47:⚠️ Potential issue | 🟠 MajorBenchmark currently pushes
ions[0]without the raw-file particles.The raw data is loaded into local arrays, but
move<copy_particle>is called onions[0]. This makes the benchmark measure the wrong workload.💡 Proposed fix
- ParticleArray domainParticles{layout.AMRBox()}; + auto& domainParticles = ions[0].domainParticles(); std::stringstream ss; ss << "unsorted_particles_" << dim << ".raw"; PHARE::core::bench::read_raw_from_file(domainParticles, ss.str()); - ParticleArray tmpDomain{layout.AMRBox()}; - tmpDomain.vector() = domainParticles.vector(); - BorisPusher_t pusher; pusher.setMeshAndTimeStep(layout.meshSize(), .001); Interpolator interpolator; pusher.template move<copy_particle>(ions[0], em, interpolator, boxing);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/bench/core/numerics/pusher/push_raw_use.cpp` around lines 29 - 47, The benchmark loads raw particles into domainParticles/tmpDomain but then calls pusher.template move<copy_particle>(ions[0], ...) so it measures pushing the preexisting ions[0] instead of the loaded data; replace the use of ions[0] with the ParticleArray containing the loaded particles (e.g., move the data from tmpDomain/domainParticles into the usable ion slot or call pusher.move with tmpDomain) so that pusher.template move<copy_particle> is invoked on the array populated from the .raw file (referencing domainParticles, tmpDomain, ions, and pusher.template move<copy_particle> to locate where to change).
🧹 Nitpick comments (3)
src/core/utilities/box/box.hpp (1)
262-277: SimplifyisIn(point, box)with early return.This predicate is on a hot path; you can avoid extra ops and remove the redundant branch by returning early on first out-of-range dimension.
♻️ Suggested simplification
template<template<typename, std::size_t> typename Point, typename Type, std::size_t SIZE> NO_DISCARD bool isIn(Point<Type, SIZE> const& point, Box<Type, SIZE> const& box) { - auto isIn1D = [](auto const pos, auto const lower, auto const upper) { - return pos >= lower && pos <= upper; - }; - - bool pointInBox = true; - - for (auto iDim = 0u; iDim < SIZE; ++iDim) - pointInBox = pointInBox && isIn1D(point[iDim], box.lower[iDim], box.upper[iDim]); - if (pointInBox) - return pointInBox; - - return false; + for (std::size_t iDim = 0; iDim < SIZE; ++iDim) + if (point[iDim] < box.lower[iDim] || point[iDim] > box.upper[iDim]) + return false; + return true; }As per coding guidelines:
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/utilities/box/box.hpp` around lines 262 - 277, The isIn(Point, Box) implementation keeps a running pointInBox flag and a final redundant branch; change it to check each dimension with the local isIn1D predicate and return false immediately when a coordinate is out of range, returning true at the end if all dims pass. Update the function named isIn (templated on Point, Type, SIZE) to remove the pointInBox accumulation and the trailing if/return pattern, using an early return inside the for loop when isIn1D(point[iDim], box.lower[iDim], box.upper[iDim]) is false.src/core/numerics/pusher/boris.hpp (1)
300-303: Avoid capture lambdas in the partition hot path.This predicate is in runtime particle processing. Prefer a concrete predicate/functor object over
[&]capture to keep inlining behavior predictable and avoid closure overhead in this header path.Based on learnings: In the PHARE codebase, for runtime particle-based operations, avoid lambdas with captures due to performance issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/numerics/pusher/boris.hpp` around lines 300 - 303, The partition call currently uses a capture lambda which causes closure overhead in the hot particle-processing path; replace it with a small concrete functor type (e.g., struct IsInBox { const BoxT& domainBox; bool operator()(CellT const& cell) const { return core::isIn(cell, domainBox); } };) and pass an instance to domain.partition instead of the [&] lambda used around makeIndexRange and domain.partition, referencing makeIndexRange, domain.partition, core::isIn, and boxing.domainBox so the predicate is inlinable and has no hidden captures.src/core/numerics/ion_updater/ion_updater.hpp (1)
110-151:Selector_t+ captured lambdas add avoidable runtime overhead here.For these selector paths, consider replacing
std::function-stored captured lambdas with concrete functors or direct member functions to keep dispatch/inlining tighter.Based on learnings: In the PHARE codebase, for runtime particle-based operations, avoid lambdas with captures due to performance issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/numerics/ion_updater/ion_updater.hpp` around lines 110 - 151, The current use of Selector_t = std::function<ParticleRange(ParticleRange&)> and the captured lambdas (noop, inDomainBox, inGhostBox, inNonLevelGhostBox, inGhostLayer, outsideGhostBox) incurs runtime dispatch and prevents inlining; replace them with concrete callables to restore static dispatch: either (a) define small functor structs (e.g., InDomainBox, InGhostBox, InNonLevelGhostBox, InGhostLayer, OutsideGhostBox) that store the needed boxes as members and implement operator()(ParticleRange&), or (b) convert these selectors into class member functions that take ParticleRange& and use the existing domainBox/ghostBox/nonLevelGhostBox members; remove std::function usage and update all call sites to use the concrete functors or member methods so calls can be inlined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/errors.hpp`:
- Line 23: Make the single-argument constructor explicit and match the parameter
type expected by operator(): change DictionaryException(auto const& cause) to an
explicit constructor that takes a string type (e.g., explicit
DictionaryException(std::string const& cause) or explicit
DictionaryException(std::string_view cause) and convert to std::string) and then
call (*this)("cause", cause); to ensure the API contract and prevent implicit
conversions; update the constructor signature in the DictionaryException
declaration accordingly.
---
Outside diff comments:
In `@tools/bench/core/numerics/pusher/push_raw_use.cpp`:
- Around line 29-47: The benchmark loads raw particles into
domainParticles/tmpDomain but then calls pusher.template
move<copy_particle>(ions[0], ...) so it measures pushing the preexisting ions[0]
instead of the loaded data; replace the use of ions[0] with the ParticleArray
containing the loaded particles (e.g., move the data from
tmpDomain/domainParticles into the usable ion slot or call pusher.move with
tmpDomain) so that pusher.template move<copy_particle> is invoked on the array
populated from the .raw file (referencing domainParticles, tmpDomain, ions, and
pusher.template move<copy_particle> to locate where to change).
---
Nitpick comments:
In `@src/core/numerics/ion_updater/ion_updater.hpp`:
- Around line 110-151: The current use of Selector_t =
std::function<ParticleRange(ParticleRange&)> and the captured lambdas (noop,
inDomainBox, inGhostBox, inNonLevelGhostBox, inGhostLayer, outsideGhostBox)
incurs runtime dispatch and prevents inlining; replace them with concrete
callables to restore static dispatch: either (a) define small functor structs
(e.g., InDomainBox, InGhostBox, InNonLevelGhostBox, InGhostLayer,
OutsideGhostBox) that store the needed boxes as members and implement
operator()(ParticleRange&), or (b) convert these selectors into class member
functions that take ParticleRange& and use the existing
domainBox/ghostBox/nonLevelGhostBox members; remove std::function usage and
update all call sites to use the concrete functors or member methods so calls
can be inlined.
In `@src/core/numerics/pusher/boris.hpp`:
- Around line 300-303: The partition call currently uses a capture lambda which
causes closure overhead in the hot particle-processing path; replace it with a
small concrete functor type (e.g., struct IsInBox { const BoxT& domainBox; bool
operator()(CellT const& cell) const { return core::isIn(cell, domainBox); } };)
and pass an instance to domain.partition instead of the [&] lambda used around
makeIndexRange and domain.partition, referencing makeIndexRange,
domain.partition, core::isIn, and boxing.domainBox so the predicate is inlinable
and has no hidden captures.
In `@src/core/utilities/box/box.hpp`:
- Around line 262-277: The isIn(Point, Box) implementation keeps a running
pointInBox flag and a final redundant branch; change it to check each dimension
with the local isIn1D predicate and return false immediately when a coordinate
is out of range, returning true at the end if all dims pass. Update the function
named isIn (templated on Point, Type, SIZE) to remove the pointInBox
accumulation and the trailing if/return pattern, using an early return inside
the for loop when isIn1D(point[iDim], box.lower[iDim], box.upper[iDim]) is
false.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/core/CMakeLists.txtsrc/core/data/particles/particle_array.hppsrc/core/errors.hppsrc/core/numerics/interpolator/interpolator.hppsrc/core/numerics/ion_updater/ion_updater.hppsrc/core/numerics/pusher/boris.hppsrc/core/numerics/pusher/pusher.hppsrc/core/numerics/pusher/pusher_factory.hppsrc/core/utilities/box/box.hppsrc/core/utilities/cellmap.hppsrc/core/utilities/indexer.hpptests/core/data/gridlayout/test_gridlayout.hpptests/core/data/ion_population/test_ion_population_fixtures.hpptests/core/numerics/ion_updater/test_updater.cpptests/core/numerics/pusher/test_pusher.cpptools/bench/core/numerics/ion_updater/bench_ion_updater.cpptools/bench/core/numerics/pusher/push_raw_use.cpptools/bench/core/numerics/pusher/pusher.cpp
💤 Files with no reviewable changes (4)
- src/core/numerics/pusher/pusher.hpp
- src/core/numerics/pusher/pusher_factory.hpp
- src/core/CMakeLists.txt
- tests/core/numerics/ion_updater/test_updater.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- src/core/numerics/interpolator/interpolator.hpp
- tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp
- tests/core/data/gridlayout/test_gridlayout.hpp
- src/core/utilities/indexer.hpp
| public: | ||
| DictionaryException() = default; | ||
| DictionaryException(auto const& k, auto const& v) { (*this)(k, v); } | ||
| DictionaryException(auto const& cause) { (*this)("cause", cause); } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/core/errors.hppRepository: PHAREHUB/PHARE
Length of output: 3070
🏁 Script executed:
#!/bin/bash
# Search for DictionaryException constructor call sites
rg -n 'DictionaryException\s*\(' --type=cpp --type=c --type=hpp --type=h -A 2Repository: PHAREHUB/PHARE
Length of output: 85
🏁 Script executed:
#!/bin/bash
# Search for DictionaryException constructor call sites
rg -n 'DictionaryException\s*\(' --type=cpp --type=c --type=h -A 2Repository: PHAREHUB/PHARE
Length of output: 402
🏁 Script executed:
#!/bin/bash
# Search for instantiation/throw patterns
rg -n 'throw\s+.*DictionaryException|DictionaryException\s*{' --type=cpp --type=c --type=h -B 1 -A 3Repository: PHAREHUB/PHARE
Length of output: 227
🏁 Script executed:
#!/bin/bash
# Search for throw statements with DictionaryException
rg -n 'throw.*DictionaryException' --type=cpp --type=c --type=h -A 2Repository: PHAREHUB/PHARE
Length of output: 1027
🏁 Script executed:
#!/bin/bash
# Search for usage of the two-arg constructor pattern
rg -n 'DictionaryException\s*\([^)]*,\s*[^)]*\)' --type=cpp --type=c --type=h -B 1 -A 1Repository: PHAREHUB/PHARE
Length of output: 308
🏁 Script executed:
#!/bin/bash
# Get more context from the boris.hpp usage
sed -n '155,170p' src/core/numerics/pusher/boris.hppRepository: PHAREHUB/PHARE
Length of output: 646
Make the single-argument constructor explicit and type-aligned.
Line 23 uses a generic constructor signature (auto const&) but forwards to operator()(std::string const, std::string const), which only accepts strings. This mismatch obscures the actual API contract. Additionally, the non-explicit constructor allows unintended implicit conversions.
Suggested change
- DictionaryException(auto const& cause) { (*this)("cause", cause); }
+ explicit DictionaryException(std::string const& cause) { (*this)("cause", cause); }📝 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.
Suggested change
| DictionaryException(auto const& cause) { (*this)("cause", cause); } | |
| explicit DictionaryException(std::string const& cause) { (*this)("cause", cause); } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/errors.hpp` at line 23, Make the single-argument constructor
explicit and match the parameter type expected by operator(): change
DictionaryException(auto const& cause) to an explicit constructor that takes a
string type (e.g., explicit DictionaryException(std::string const& cause) or
explicit DictionaryException(std::string_view cause) and convert to std::string)
and then call (*this)("cause", cause); to ensure the API contract and prevent
implicit conversions; update the constructor signature in the
DictionaryException declaration accordingly.
dca64d3 to
1b69fc0
Compare
PhilipDeegan
force-pushed
the
updater_realloc
branch
from
1b69fc0 to
1a763a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/core/errors.hpp (1)
23-23:⚠️ Potential issue | 🟡 MinorMake the single-argument constructor
explicitand use a concrete type.This single-argument constructor should be marked
explicitto prevent unintended implicit conversions. Additionally, usingauto const&obscures the fact thatoperator()only acceptsstd::stringarguments—callers may be confused when non-string types cause compilation failures.Suggested fix
- DictionaryException(auto const& cause) { (*this)("cause", cause); } + explicit DictionaryException(std::string const& cause) { (*this)("cause", cause); },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/errors.hpp` at line 23, Mark the single-argument constructor DictionaryException as explicit and change its parameter from auto const& to a concrete std::string const& (since operator() accepts std::string), i.e. update DictionaryException(auto const& cause) to explicit DictionaryException(std::string const& cause) and keep the body calling (*this)("cause", cause); to avoid implicit conversions and make the type expectation clear to callers.
🧹 Nitpick comments (2)
src/core/numerics/pusher/boris.hpp (1)
299-306: Use explicit functors instead of captured lambdas in the partition calls.Both partition calls (lines 301 and 305) in this runtime particle path capture by reference
[&]. Replacing these captured lambdas with explicit functor classes will improve performance and maintain code clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/numerics/pusher/boris.hpp` around lines 299 - 306, Replace the two captured lambdas passed to domain.partition with small explicit predicate functor classes: one functor (e.g., IsInDomainBox) that stores a reference to boxing.domainBox and implements bool operator()(Cell const& cell) { return core::isIn(cell, domainBox_); } and another (e.g., IsInNonLevelGhostBox) that stores a reference to boxing and calls boxing_.isInNonLevelGhostBox(cell). In the code around makeIndexRange(domain) / domain.partition(...) and the subsequent makeRange(...) / domain.partition(...) calls, instantiate and pass these functor objects instead of using [&] lambdas so captures become explicit members initialized in the functor constructors and the partition calls use the functor types directly.src/core/numerics/ion_updater/ion_updater.hpp (1)
110-151: Consider eliminating unusedSelector_tlambdas or refactoring them into explicit functor classes if needed for external APIs.The
Selector_tlambdas with captured data (lines 124, 127, 130, 134, 140) are defined but not invoked in the hot path. The actual particle-pushing loop inmove_domainandmove_level_ghostuses simple boolean member functions (isInDomainBox,isInNonLevelGhostBox) instead. If theseSelector_tfunctions are intended for external consumption, replace them with non-captured explicit functor classes to avoid unnecessarystd::functiontype-erasure overhead; otherwise, remove dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/numerics/ion_updater/ion_updater.hpp` around lines 110 - 151, The file defines several unused Selector_t lambdas (noop, inDomainBox, inGhostBox, inNonLevelGhostBox, inGhostLayer, outsideGhostBox) while move_domain and move_level_ghost use the boolean members isInDomainBox/isInNonLevelGhostBox; either remove these dead lambdas to eliminate clutter or convert them into explicit non-capturing functor types (e.g., structs with operator()) instead of std::function-erased lambdas so they can be used by external APIs without capture overhead; update any external API consumers to take the functor type (or templates) or keep only the boolean member helpers if no external use exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/data/particles/particle_array.hpp`:
- Around line 151-167: Add explicit precondition checks at the start of
swap_last_reduce_by_one: verify size() > 0 and that the provided idx is < size()
before using size() - 1; if either check fails, return false or handle as
appropriate. Locate the checks inside the swap_last_reduce_by_one function
(which uses size(), particles_, cellMap_.swap, cellMap_.erase and resize) so you
avoid underflowing size() - 1 and prevent out-of-range access when calling
particles_[size() - 1] and performing cellMap_.swap/erase.
In `@src/core/numerics/pusher/boris.hpp`:
- Around line 10-17: Add direct standard headers to boris.hpp for symbols used:
include <sstream> to provide std::stringstream, <cstdint> to provide
std::uint16_t, and <string> to provide std::to_string. Locate the top-of-file
include block in src/core/numerics/pusher/boris.hpp (near the existing includes)
and add these three headers so the uses of std::stringstream (lines ~159,173),
std::uint16_t (line ~179) and std::to_string (lines ~180,182,183) do not rely on
transitive includes.
---
Duplicate comments:
In `@src/core/errors.hpp`:
- Line 23: Mark the single-argument constructor DictionaryException as explicit
and change its parameter from auto const& to a concrete std::string const&
(since operator() accepts std::string), i.e. update DictionaryException(auto
const& cause) to explicit DictionaryException(std::string const& cause) and keep
the body calling (*this)("cause", cause); to avoid implicit conversions and make
the type expectation clear to callers.
---
Nitpick comments:
In `@src/core/numerics/ion_updater/ion_updater.hpp`:
- Around line 110-151: The file defines several unused Selector_t lambdas (noop,
inDomainBox, inGhostBox, inNonLevelGhostBox, inGhostLayer, outsideGhostBox)
while move_domain and move_level_ghost use the boolean members
isInDomainBox/isInNonLevelGhostBox; either remove these dead lambdas to
eliminate clutter or convert them into explicit non-capturing functor types
(e.g., structs with operator()) instead of std::function-erased lambdas so they
can be used by external APIs without capture overhead; update any external API
consumers to take the functor type (or templates) or keep only the boolean
member helpers if no external use exists.
In `@src/core/numerics/pusher/boris.hpp`:
- Around line 299-306: Replace the two captured lambdas passed to
domain.partition with small explicit predicate functor classes: one functor
(e.g., IsInDomainBox) that stores a reference to boxing.domainBox and implements
bool operator()(Cell const& cell) { return core::isIn(cell, domainBox_); } and
another (e.g., IsInNonLevelGhostBox) that stores a reference to boxing and calls
boxing_.isInNonLevelGhostBox(cell). In the code around makeIndexRange(domain) /
domain.partition(...) and the subsequent makeRange(...) / domain.partition(...)
calls, instantiate and pass these functor objects instead of using [&] lambdas
so captures become explicit members initialized in the functor constructors and
the partition calls use the functor types directly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/core/CMakeLists.txtsrc/core/data/particles/particle_array.hppsrc/core/errors.hppsrc/core/numerics/interpolator/interpolator.hppsrc/core/numerics/ion_updater/ion_updater.hppsrc/core/numerics/pusher/boris.hppsrc/core/numerics/pusher/pusher.hppsrc/core/numerics/pusher/pusher_factory.hppsrc/core/utilities/box/box.hppsrc/core/utilities/cellmap.hppsrc/core/utilities/indexer.hpptests/core/data/gridlayout/test_gridlayout.hpptests/core/data/ion_population/test_ion_population_fixtures.hpptests/core/numerics/ion_updater/test_updater.cpptests/core/numerics/pusher/test_pusher.cpptools/bench/core/numerics/ion_updater/bench_ion_updater.cpptools/bench/core/numerics/pusher/push_raw_use.cpptools/bench/core/numerics/pusher/pusher.cpp
💤 Files with no reviewable changes (4)
- src/core/numerics/pusher/pusher.hpp
- src/core/CMakeLists.txt
- src/core/numerics/pusher/pusher_factory.hpp
- tests/core/numerics/ion_updater/test_updater.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- src/core/utilities/indexer.hpp
- src/core/utilities/cellmap.hpp
- tools/bench/core/numerics/pusher/push_raw_use.cpp
- src/core/numerics/interpolator/interpolator.hpp
- tools/bench/core/numerics/ion_updater/bench_ion_updater.cpp
- tests/core/data/gridlayout/test_gridlayout.hpp
Comment on lines
+151
to
+167
| bool swap_last_reduce_by_one(Cell const& oldCell, std::size_t const idx) | ||
| { | ||
| // swap last to index | ||
| // swap idx to last | ||
| // --size | ||
| // return true if you need to repeat the current index == expected | ||
|
|
||
| bool const idx_is_last = idx == size() - 1; | ||
| if (!idx_is_last) | ||
| { | ||
| cellMap_.swap(oldCell, particles_[size() - 1].iCell, idx, size() - 1); | ||
| particles_[idx] = particles_[size() - 1]; | ||
| } | ||
|
|
||
| cellMap_.erase(*this, oldCell, size() - 1); // doesn't erase from particles vector | ||
| resize(size() - 1); | ||
| return !idx_is_last; |
There was a problem hiding this comment.
Add explicit precondition guards in swap_last_reduce_by_one.
If this helper is ever called with an empty array or an out-of-range index, size() - 1 underflows and indexing is UB.
🔧 Proposed guard
template<typename Cell>
bool swap_last_reduce_by_one(Cell const& oldCell, std::size_t const idx)
{
+ assert(size() > 0);
+ assert(idx < size());
+
bool const idx_is_last = idx == size() - 1;
if (!idx_is_last)
{
cellMap_.swap(oldCell, particles_[size() - 1].iCell, idx, size() - 1);
particles_[idx] = particles_[size() - 1];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/data/particles/particle_array.hpp` around lines 151 - 167, Add
explicit precondition checks at the start of swap_last_reduce_by_one: verify
size() > 0 and that the provided idx is < size() before using size() - 1; if
either check fails, return false or handle as appropriate. Locate the checks
inside the swap_last_reduce_by_one function (which uses size(), particles_,
cellMap_.swap, cellMap_.erase and resize) so you avoid underflowing size() - 1
and prevent out-of-range access when calling particles_[size() - 1] and
performing cellMap_.swap/erase.
Comment on lines
10
to
17
| #include <array> | ||
| #include <cmath> | ||
| #include <cstddef> | ||
| #include <sstream> | ||
| #include <iterator> | ||
| #include <algorithm> | ||
| #include <exception> | ||
| #include <functional> | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for symbol usage in boris.hpp
echo "=== Checking for symbol usage ==="
rg -n 'stringstream|to_string|uint16_t' src/core/numerics/pusher/boris.hpp
echo -e "\n=== Checking current includes ==="
rg -n '^#include' src/core/numerics/pusher/boris.hpp
echo -e "\n=== File structure overview ==="
wc -l src/core/numerics/pusher/boris.hppRepository: PHAREHUB/PHARE
Length of output: 849
Add direct standard headers for symbols used in this header.
std::stringstream (lines 159, 173), std::uint16_t (line 179), and std::to_string (lines 180, 182, 183) are used but lack direct includes. Relying on transitive includes is brittle.
📦 Include hygiene fix
`#include` <algorithm>
+#include <cstdint>
`#include` <exception>
`#include` <functional>
+#include <sstream>
+#include <string>📝 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.
Suggested change
| #include <array> | |
| #include <cmath> | |
| #include <cstddef> | |
| #include <sstream> | |
| #include <iterator> | |
| #include <algorithm> | |
| #include <exception> | |
| #include <functional> | |
| `#include` <array> | |
| `#include` <cmath> | |
| `#include` <cstddef> | |
| `#include` <iterator> | |
| `#include` <algorithm> | |
| `#include` <cstdint> | |
| `#include` <exception> | |
| `#include` <functional> | |
| `#include` <sstream> | |
| `#include` <string> | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/numerics/pusher/boris.hpp` around lines 10 - 17, Add direct standard
headers to boris.hpp for symbols used: include <sstream> to provide
std::stringstream, <cstdint> to provide std::uint16_t, and <string> to provide
std::to_string. Locate the top-of-file include block in
src/core/numerics/pusher/boris.hpp (near the existing includes) and add these
three headers so the uses of std::stringstream (lines ~159,173), std::uint16_t
(line ~179) and std::to_string (lines ~180,182,183) do not rely on transitive
includes.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| { | ||
| auto const& startIndex_ = primal_startIndex_; | ||
| auto const& weights_ = primal_weights_; | ||
| auto const& [xFlux, yFlux, zFlux] = flux(); |
Check notice
Code scanning / CodeQL
Unused local variable
| // now advance the particles from t=n+1/2 to t=n+1 using v_{n+1} just calculated | ||
| // and get a pointer to the first leaving particle | ||
| firstLeaving = pushStep_(rangeOut, rangeOut, particleIsNotLeaving, PushStep::PostPush); | ||
| auto& [pE, pB] = eb; |
Check notice
Code scanning / CodeQL
Unused local variable
| // and get a pointer to the first leaving particle | ||
| firstLeaving = pushStep_(rangeOut, rangeOut, particleIsNotLeaving, PushStep::PostPush); | ||
| auto& [pE, pB] = eb; | ||
| auto& [pEx, pEy, pEz] = pE; |
Check notice
Code scanning / CodeQL
Unused local variable
| firstLeaving = pushStep_(rangeOut, rangeOut, particleIsNotLeaving, PushStep::PostPush); | ||
| auto& [pE, pB] = eb; | ||
| auto& [pEx, pEy, pEz] = pE; | ||
| auto& [pBx, pBy, pBz] = pB; |
Check notice
Code scanning / CodeQL
Unused local variable
| throw DictionaryException{}("cause", ss.str()); | ||
| DictionaryException ex{ss.str()}; | ||
|
|
||
| auto const& [E, B] = local_em; |
Check notice
Code scanning / CodeQL
Unused local variable
| else | ||
| { | ||
| level_ghost.swap_last_reduce_by_one(oldCell, i); | ||
| --i; // redo current index as last is now i |
Check notice
Code scanning / CodeQL
For loop variable changed in body
| double const myy = 1. + ry2 - rx2 - rz2; | ||
| double const myz = 2. * (ryrz + rx); | ||
| auto& domain = pop.domainParticles(); | ||
| auto& patch_ghost = pop.patchGhostParticles(); |
Check notice
Code scanning / CodeQL
Unused local variable
| if constexpr (!copy_particle) | ||
| { | ||
| domain.swap_last_reduce_by_one(oldCell, i); | ||
| --i; // redo current index as last is now i |
Check notice
Code scanning / CodeQL
For loop variable changed in body
Comment on lines
+337
to
+339
| // BoundaryCondition<1, 1> bc; | ||
| // ParticleArray<1> particlesOut1; | ||
| // ParticleArray<1> particlesOut2; |
Check notice
Code scanning / CodeQL
Commented-out code
| // ParticleArray<1> particlesOut1; | ||
| // ParticleArray<1> particlesOut2; | ||
|
|
||
| // TestIons<GridLayout_t, ParticleArray<dimension>> ions{layout}; |
Check notice
Code scanning / CodeQL
Commented-out code
| { | ||
| auto const& startIndex_ = primal_startIndex_; | ||
| auto const& weights_ = primal_weights_; | ||
| auto const& [xFlux, yFlux, zFlux] = flux(); |
Check notice
Code scanning / CodeQL
Unused local variable Note
| // now advance the particles from t=n+1/2 to t=n+1 using v_{n+1} just calculated | ||
| // and get a pointer to the first leaving particle | ||
| firstLeaving = pushStep_(rangeOut, rangeOut, particleIsNotLeaving, PushStep::PostPush); | ||
| auto& [pE, pB] = eb; |
Check notice
Code scanning / CodeQL
Unused local variable Note
| // and get a pointer to the first leaving particle | ||
| firstLeaving = pushStep_(rangeOut, rangeOut, particleIsNotLeaving, PushStep::PostPush); | ||
| auto& [pE, pB] = eb; | ||
| auto& [pEx, pEy, pEz] = pE; |
Check notice
Code scanning / CodeQL
Unused local variable Note
| firstLeaving = pushStep_(rangeOut, rangeOut, particleIsNotLeaving, PushStep::PostPush); | ||
| auto& [pE, pB] = eb; | ||
| auto& [pEx, pEy, pEz] = pE; | ||
| auto& [pBx, pBy, pBz] = pB; |
Check notice
Code scanning / CodeQL
Unused local variable Note
| throw DictionaryException{}("cause", ss.str()); | ||
| DictionaryException ex{ss.str()}; | ||
|
|
||
| auto const& [E, B] = local_em; |
Check notice
Code scanning / CodeQL
Unused local variable Note
| else | ||
| { | ||
| level_ghost.swap_last_reduce_by_one(oldCell, i); | ||
| --i; // redo current index as last is now i |
Check notice
Code scanning / CodeQL
For loop variable changed in body Note
| double const myy = 1. + ry2 - rx2 - rz2; | ||
| double const myz = 2. * (ryrz + rx); | ||
| auto& domain = pop.domainParticles(); | ||
| auto& patch_ghost = pop.patchGhostParticles(); |
Check notice
Code scanning / CodeQL
Unused local variable Note
| if constexpr (!copy_particle) | ||
| { | ||
| domain.swap_last_reduce_by_one(oldCell, i); | ||
| --i; // redo current index as last is now i |
Check notice
Code scanning / CodeQL
For loop variable changed in body Note
Comment on lines
+337
to
+339
| // BoundaryCondition<1, 1> bc; | ||
| // ParticleArray<1> particlesOut1; | ||
| // ParticleArray<1> particlesOut2; |
Check notice
Code scanning / CodeQL
Commented-out code Note test
| // ParticleArray<1> particlesOut1; | ||
| // ParticleArray<1> particlesOut2; | ||
|
|
||
| // TestIons<GridLayout_t, ParticleArray<dimension>> ions{layout}; |
Check notice
Code scanning / CodeQL
Commented-out code Note test
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In the default scenario where we don't specify a
largest_patch_sizeL0 will typically allocate 1 patch per rank.
This means that the required RAM for hybrid is at least
domain_box.size() * ppc * 2for a 100**3 @ ppc=100 that is an additional 7.6GB
for a 1000**3 @ ppc=100 that is an additional 7600GB
this also affects refined levels but only as much as the largest patch per rank
the following is provided to assuage the requirement for copying the entire particle vector on the first push.
secondarily, it has fewer particle loops
PusherFactory is dropped as IonUpdater/Boris are completely coupled and it should be up to the solver to choose the right "updater", and not for the updater to choose a "pusher"
Draft as low priority, nightly build artifacts look ok tho