Conversation
📝 WalkthroughWalkthroughRefactors field-border operations to generic operation templates, adds ion-border filling across the messenger/refiner/solver stack with new max-border refiners, adjusts related transaction/functor APIs (SetMax/PlusEquals), and makes minor simulator, MPI utility, and test baseline tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant Solver as SolverPPC
participant Messenger as HybridMessenger
participant Strategy as HybridHybridMessengerStrategy
participant Refiner as RefinerPool
participant Field as FieldData/TensorFieldData
participant Transaction as FieldBorderOpTransaction
Solver->>Messenger: request fillIonBorders(ions, level, time)
Messenger->>Strategy: strat_->fillIonBorders(ions, level, time)
Strategy->>Refiner: select ionDensity/ionFlux max-refiner pool
Refiner->>Field: invoke operate<SetMaxOp>/operate<PlusEqualsOp> on patch data
Field->>Transaction: pack/unpack using templated Operation
Transaction->>Field: apply Operation during unpackStreamAnd<Operation>
Transaction-->>Strategy: border data applied
Strategy-->>Messenger: ion borders filled
Messenger-->>Solver: return control
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
PhilipDeegan
force-pushed
the
maxbattle
branch
from
7dc5cbd to
c53a4e0
Compare
PhilipDeegan
force-pushed
the
maxbattle
branch
from
c53a4e0 to
6a4f250
Compare
Updated smallest_patch_size calculation to use interpolation order.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pyphare/pyphare/pharein/simulation.py`:
- Around line 409-413: The first assignment to smallest_patch_size (using
phare_utilities.np_array_ify(max_ghosts, ndim) * 2 + 1) is dead because it is
overwritten unconditionally by the SAMRAI workaround assignment using
min_per_interp; remove or comment out the first assignment to avoid confusion,
or wrap it behind a clear conditional/feature flag (eg. use a constant like
USE_SAMRAI_WORKAROUND) so that smallest_patch_size is assigned from
phare_utilities.np_array_ify(max_ghosts, ndim) * 2 + 1 when the SAMRAI bug is
resolved; references to smallest_patch_size, phare_utilities.np_array_ify,
max_ghosts, min_per_interp, interp and ndim should remain to locate the change.
🧹 Nitpick comments (3)
src/core/utilities/types.hpp (1)
563-568:std::maxrequires identical types — consider explicit cast orstd::max<D>.Unlike
operator+=(inPlusEquals) which allows implicit conversions,std::max(d, d0)requires both arguments to have the exact same type for template deduction. Ifd0ever has a slightly different type thanD, this will fail to compile. Usingstd::max<D>(d, d0)would be safer and consistent with the implicit conversion behavior of the sibling functors.Suggested fix
template<typename D> struct SetMax { - void operator()(auto& d0) { d = std::max(d, d0); } + void operator()(auto& d0) { d = std::max<D>(d, d0); } D& d; };src/amr/messengers/refiner.hpp (1)
122-123: Duplicate comments for two distinct branches.Lines 122–123 and 135–136 have identical comments. The second block handles
PatchVecFieldBorderMax(vector fields), so it would be clearer to differentiate, e.g. "schedule used to set max of vec field density and flux…".Also, the phrasing "== max" is a bit ambiguous — consider "set to max" for clarity.
Also applies to: 135-136
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
876-891: Hard-coded size assertions onmaxBorderFieldsandmaxBorderVecFieldsmay be brittle.Lines 878 and 885 assert exact sizes (
== 2and== 1). If a future change adds additional max-border quantities, these assertions will fire at runtime with no compile-time indication. Consider adding a comment explaining why exactly 2 fields and 1 vec field are expected, so maintainers know the contract.The in-place registration pattern (
field, field, nullptr, field) is correct for max operations sincemax(a, b)is idempotent — unlike+=, repeated overlaps don't corrupt the result.
| # to prevent primal ghost box overlaps of non adjacent patches, we need smallest_patch_size * 2 + 1 | ||
| smallest_patch_size = phare_utilities.np_array_ify(max_ghosts, ndim) * 2 + 1 | ||
| # TORM next lines after https://github.com/llnl/SAMRAI/issues/311 | ||
| min_per_interp = [6, 9, 9] # SAMRAI BORDER BUG | ||
| smallest_patch_size = phare_utilities.np_array_ify(min_per_interp[interp - 1], ndim) |
There was a problem hiding this comment.
Dead assignment on Line 410 — value is immediately overwritten on Line 413.
The smallest_patch_size computed on Line 410 is never used because it's unconditionally reassigned on Line 413. This was also flagged by CodeQL. If the Line 410 computation is kept as documentation of the "correct" formula (for when the SAMRAI bug is fixed), consider commenting it out to make the intent clearer and avoid confusion.
Suggested cleanup
- # to prevent primal ghost box overlaps of non adjacent patches, we need smallest_patch_size * 2 + 1
- smallest_patch_size = phare_utilities.np_array_ify(max_ghosts, ndim) * 2 + 1
+ # to prevent primal ghost box overlaps of non adjacent patches, we'd need smallest_patch_size * 2 + 1
+ # smallest_patch_size = phare_utilities.np_array_ify(max_ghosts, ndim) * 2 + 1
# TORM next lines after https://github.com/llnl/SAMRAI/issues/311
min_per_interp = [6, 9, 9] # SAMRAI BORDER BUG
smallest_patch_size = phare_utilities.np_array_ify(min_per_interp[interp - 1], ndim)📝 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.
| # to prevent primal ghost box overlaps of non adjacent patches, we need smallest_patch_size * 2 + 1 | |
| smallest_patch_size = phare_utilities.np_array_ify(max_ghosts, ndim) * 2 + 1 | |
| # TORM next lines after https://github.com/llnl/SAMRAI/issues/311 | |
| min_per_interp = [6, 9, 9] # SAMRAI BORDER BUG | |
| smallest_patch_size = phare_utilities.np_array_ify(min_per_interp[interp - 1], ndim) | |
| # to prevent primal ghost box overlaps of non adjacent patches, we'd need smallest_patch_size * 2 + 1 | |
| # smallest_patch_size = phare_utilities.np_array_ify(max_ghosts, ndim) * 2 + 1 | |
| # TORM next lines after https://github.com/llnl/SAMRAI/issues/311 | |
| min_per_interp = [6, 9, 9] # SAMRAI BORDER BUG | |
| smallest_patch_size = phare_utilities.np_array_ify(min_per_interp[interp - 1], ndim) |
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 409-409: Variable defined multiple times
This assignment to 'smallest_patch_size' is unnecessary as it is redefined before this value is used.
🤖 Prompt for AI Agents
In `@pyphare/pyphare/pharein/simulation.py` around lines 409 - 413, The first
assignment to smallest_patch_size (using
phare_utilities.np_array_ify(max_ghosts, ndim) * 2 + 1) is dead because it is
overwritten unconditionally by the SAMRAI workaround assignment using
min_per_interp; remove or comment out the first assignment to avoid confusion,
or wrap it behind a clear conditional/feature flag (eg. use a constant like
USE_SAMRAI_WORKAROUND) so that smallest_patch_size is assigned from
phare_utilities.np_array_ify(max_ghosts, ndim) * 2 + 1 when the SAMRAI bug is
resolved; references to smallest_patch_size, phare_utilities.np_array_ify,
max_ghosts, min_per_interp, interp and ndim should remain to locate the change.
There was a problem hiding this comment.
in future it will only require removing code, this is clear.
There was a problem hiding this comment.
@PhilipDeegan, understood! The current structure makes the future fix straightforward—just remove the SAMRAI workaround lines and the correct computation will take effect. Thanks for clarifying.
✏️ Learnings added
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1146
File: pyphare/pyphare/pharein/simulation.py:409-413
Timestamp: 2026-02-09T17:54:56.848Z
Learning: In Python file pyphare/pyphare/pharein/simulation.py, PhilipDeegan prefers to keep the correct/intended implementation uncommented above temporary workarounds (even if it results in dead code), as this makes future fixes clearer—simply removing the workaround section activates the correct code.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-10-09T08:32:15.667Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-07-26T22:04:34.160Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1068
File: src/amr/data/field/coarsening/electric_field_coarsener.hpp:1-2
Timestamp: 2025-09-17T13:35:11.533Z
Learning: PhilipDeegan prefers header guard names that include the full directory path structure, following the pattern PHARE_[PATH_WITH_UNDERSCORES]_HPP. For example, a file at src/amr/data/field/coarsening/electric_field_coarsener.hpp should use PHARE_AMR_DATA_FIELD_COARSENING_ELECTRIC_FIELD_COARSENER_HPP as the header guard.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-10-09T08:32:15.667Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-09-05T17:02:58.784Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:566-568
Timestamp: 2024-09-09T13:57:02.285Z
Learning: Avoid commenting on draft PRs for PhilipDeegan unless explicitly requested.
max battle : overlapped patch values are assigned to their max to avoid spurious floating point errors mismatches. Also updated smallest_patch_size calculation to use interpolation order.
2 participants
No description provided.