divergence free refluxing #1028
Conversation
📝 WalkthroughWalkthroughThis update introduces AMR refluxing support for electric field fluxes, adapting the coarsening logic for electric fields and integrating new reflux-related methods and fields across the messenger and solver interfaces. It refactors field coarsening, updates test logic, and extends the integrator to invoke refluxing during level synchronization, ensuring correct flux correction in multilevel simulations. Changes
Sequence Diagram(s)sequenceDiagram
participant Integrator as MultiPhysicsIntegrator
participant Solver as ISolver
participant Messenger as IMessenger
Integrator->>Solver: prepareStep(model, level, currentTime)
Integrator->>Messenger: prepareStep(model, level, currentTime)
Integrator->>Solver: advanceLevel(...)
Integrator->>Solver: accumulateFluxSum(model, level, coef)
Integrator->>Messenger: synchronize(level)
alt Not coarsest level
Integrator->>Messenger: reflux(coarserLevel, fineLevel, syncTime)
Integrator->>Solver: reflux(model, coarserLevel, syncTime)
Integrator->>Solver: resetFluxSum(model, fineLevel)
end
Integrator->>Messenger: postSynchronize(level)
sequenceDiagram
participant HybridMessengerStrategy
participant ResourceMgr
participant SAMRAI
HybridMessengerStrategy->>ResourceMgr: register(Eold_)
HybridMessengerStrategy->>ResourceMgr: register(refluxElectric, fluxSumElectric)
HybridMessengerStrategy->>SAMRAI: create RefluxAlgo (electric field coarsener)
HybridMessengerStrategy->>SAMRAI: create PatchGhostRefluxedAlgo (refiner)
HybridMessengerStrategy->>SAMRAI: create refluxSchedules, patchGhostRefluxedSchedules
HybridMessengerStrategy->>HybridMessengerStrategy: reflux(coarserLevel, fineLevel, syncTime)
HybridMessengerStrategy->>SAMRAI: refluxSchedules[fineLevel]->coarsenData()
HybridMessengerStrategy->>SAMRAI: patchGhostRefluxedSchedules[coarserLevel]->fillData()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (18)
src/amr/messengers/hybrid_messenger_info.hpp (1)
70-73: Clarify intent & naming for new reflux vectorsThe two new members (
refluxElectric,fluxSumElectric) are singularVecFieldNames, whereas all neighbouring members that hold lists use either plural names (ghostElectric) orstd::vector<VecFieldNames>.
If each represents exactly one vector field, consider:-VecFieldNames refluxElectric; -VecFieldNames fluxSumElectric; +std::optional<VecFieldNames> refluxElectric; +std::optional<VecFieldNames> fluxSumElectric;or rename to
refluxElectricNames/fluxSumElectricNamesfor consistency.
Otherwise future users may erroneously treat them as collections.src/amr/messengers/hybrid_messenger.hpp (1)
193-199:refluxforwarding added – minor const-correctness noteForwarding is fine, but
HybridMessenger::refluxis non-constwhile it only delegates.
You could safely mark itconstand keep the contract withIMessengerby declaring the interface methodconst, making it clearer that the call is side-effect-free on the messenger wrapper itself:-void reflux(int const coarserLevelNumber, int const fineLevelNumber, - double const syncTime) override +void reflux(int const coarserLevelNumber, int const fineLevelNumber, + double const syncTime) const override(The internal
strat_already allows mutation.)Purely stylistic; no functional impact.
src/amr/messengers/mhd_hybrid_messenger_strategy.hpp (1)
135-138: Mark unused parameters or implement logic
refluxis currently a no-op yet its parameters are named (and thus trigger ‑Wunused-parameter under common warning levels). Either:-void reflux(int const /*coarserLevelNumber*/, int const /*fineLevelNumber*/, - double const /*syncTime*/) override +void reflux([[maybe_unused]] int const /*coarserLevelNumber*/, + [[maybe_unused]] int const /*fineLevelNumber*/, + [[maybe_unused]] double const /*syncTime*/) overrideor add a meaningful implementation if MHD↔Hybrid reflux is actually required.
src/amr/messengers/mhd_messenger.hpp (2)
87-90: Parameter typo:prevCoarserTIme
prevCoarserTImecontains a capital “I”. UseprevCoarserTimefor consistency with other APIs.
115-118: Silence unused-parameter warnings in emptyrefluxSame pattern as other messengers—either mark parameters
[[maybe_unused]]or add logic.src/amr/messengers/hybrid_messenger_strategy.hpp (1)
118-121: Add brief comment explainingrefluxcontractA one-line comment clarifying expected behaviour (coarse–fine flux correction, units, when called) will help implementers satisfy the new interface.
tests/functional/harris/harris_2d.py (2)
10-20: Replace dangling triple-quoted literal with commentThis multiline string is not a module docstring (code precedes it) and is never used, so Python still allocates it. Convert it to a regular
#comment to avoid the unused literal.
36-99: Functionconfigexceeds recommended local variable countPylint flags 16 locals (R0914). Consider splitting magnetic-field helpers into a nested class or separate module to improve readability.
src/amr/solvers/solver_ppc.hpp (1)
304-308: Incorrect log label – copy-paste slip
resetFluxSumstill logs"SolverPPC::accumulateFluxSum", which is misleading during debugging.- PHARE_LOG_SCOPE(1, "SolverPPC::accumulateFluxSum"); + PHARE_LOG_SCOPE(1, "SolverPPC::resetFluxSum");src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (6)
13-17: Possible super-fluous include
field_coarsen_operator.hppis already pulled transitively viadefault_field_coarsener.hpp.
Dropping the extra#includekeeps the header set minimal and shortens compile times.
192-208: Guard clause is brittleThe five
getIDcalls are validated by a single large compoundif.
If any single ID is missing you lose the information about which one failed. Consider:auto ensure = [&](auto name, auto id){ if (!id) throw std::runtime_error( "HybridHybridMessengerStrategy: missing variable ID for " + name); }; ensure(hybridInfo->refluxElectric.xName, ex_reflux_id); ...This pin-points the culprit and avoids a long boolean expression.
253-256: Unnecessary schedule on finest level
ghostRefluxedSchedules[levelNumber]is built unconditionally, yet the comment says it is “technically not needed for finest”.
Skip construction whenlevelNumber == hierarchy->getFinestLevelNumber()to save memory and SAMRAI bookkeeping.
707-714: Stale comment blockThe call to
magPatchGhostsRefineSchedules[levelNumber]->fillData(time);is commented out but the explanatory comment above still references it.
Update the comment or remove the dead code entirely to prevent future confusion.
1070-1073: Inconsistent algorithm construction
RefluxAlgois built with an explicit dimension,GhostRefluxedAlgois not.
For symmetry and clarity:SAMRAI::xfer::RefineAlgorithm GhostRefluxedAlgo{SAMRAI::tbox::Dimension{dimension}};This eliminates any ambiguity about dimensional assumptions inside SAMRAI.
1131-1132: Op lives for entire strategy instance
fluxSumCoarseningOp_is shared across all components and schedules.
Document the lifetime expectations (or make the memberconst) to clarify ownership – accidental modification elsewhere would break every reflux schedule.src/amr/solvers/solver.hpp (3)
84-92: Missing@paramdocumentation for the newprepareStephookThe newly introduced method is only documented with a generic sentence and gives no hint about the three parameters. Future implementers have to open the header to understand what is expected, breaking self-documentation.
Add explicit@paramtags (or an equivalent comment style) formodel,level, andcurrentTime.- * @brief prepareStep is used to prepare internal variable needed for the reflux. It is - * called before the advanceLevel() method. + * @brief Prepare internal data required by the reflux algorithm. + * + * @param model Physical model owning solver resources. + * @param level Patch level that will be advanced. + * @param currentTime Simulation time at the beginning of the step. + * + * Called immediately before advanceLevel().
118-123:advanceLevelcomment not updated after signature changeThe brief still states “advances the given level from t to t+dt”, but the signature now carries both
currentTimeandnewTime. Update the comment (and ideally add@paramtags) so the contract remains unambiguous.
128-134: Stale documentation forallocate— newallocateTimenot describedThe doc-block refers to “at the given time” but doesn’t mention the
allocateTimeparameter explicitly. Align the comment with the code to avoid confusion.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/amr/CMakeLists.txt(1 hunks)src/amr/data/field/coarsening/flux_sum_coarsener.hpp(1 hunks)src/amr/data/field/coarsening/magnetic_field_coarsener.hpp(1 hunks)src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(12 hunks)src/amr/messengers/hybrid_messenger.hpp(2 hunks)src/amr/messengers/hybrid_messenger_info.hpp(1 hunks)src/amr/messengers/hybrid_messenger_strategy.hpp(2 hunks)src/amr/messengers/messenger.hpp(4 hunks)src/amr/messengers/mhd_hybrid_messenger_strategy.hpp(5 hunks)src/amr/messengers/mhd_messenger.hpp(4 hunks)src/amr/multiphysics_integrator.hpp(5 hunks)src/amr/solvers/solver.hpp(2 hunks)src/amr/solvers/solver_mhd.hpp(1 hunks)src/amr/solvers/solver_ppc.hpp(8 hunks)tests/functional/harris/harris_2d.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/amr/data/field/refine/magnetic_refine_patch_strategy.hppsrc/amr/data/field/coarsening/magnetic_field_coarsener.hppsrc/amr/messengers/hybrid_messenger.hppsrc/amr/messengers/mhd_hybrid_messenger_strategy.hppsrc/amr/messengers/hybrid_messenger_info.hppsrc/amr/messengers/mhd_messenger.hppsrc/amr/solvers/solver_mhd.hppsrc/amr/messengers/hybrid_messenger_strategy.hppsrc/amr/data/field/coarsening/flux_sum_coarsener.hppsrc/amr/solvers/solver.hppsrc/amr/solvers/solver_ppc.hppsrc/amr/multiphysics_integrator.hppsrc/amr/messengers/hybrid_hybrid_messenger_strategy.hppsrc/amr/messengers/messenger.hpp
🪛 Pylint (3.3.7)
tests/functional/harris/harris_2d.py
[refactor] 36-36: Too many local variables (16/15)
(R0914)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-13)
🔇 Additional comments (8)
src/amr/data/field/coarsening/magnetic_field_coarsener.hpp (1)
72-74: Typo fix looks goodThe corrected wording (“primal-dual”) removes the distracting typo and matches the terminology used elsewhere in the code base.
src/amr/CMakeLists.txt (1)
14-16: Header added – verify install/export lists
flux_sum_coarsener.hppis header-only, so adding it toSOURCES_INCis sufficient for compilation.
Just double-check that anyinstall()/export()rules picking fromSOURCES_INCstill pick it up; otherwise downstream packages (or IDE symbol navigation) might miss it.src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp (1)
70-71: Comment re-flow only – no action neededsrc/amr/messengers/hybrid_messenger.hpp (1)
356-356: Pointer qualifier change keeps semanticsSwitch from
const std::unique_ptr<T>tostd::unique_ptr<T> constretains immutability of the pointer while allowing the strategy object to mutate—needed forreflux. Good catch.src/amr/solvers/solver_mhd.hpp (1)
45-64: ```shell
#!/bin/bashLocate solver_mhd.hpp and display lines 40-80 to inspect the stub methods
solver_file=$(find . -type f -name 'solver_mhd.hpp')
echo "Found at: $solver_file"
sed -n '40,80p' "$solver_file"</details> <details> <summary>src/amr/messengers/messenger.hpp (1)</summary> `210-213`: **New `reflux` contract looks sound** Interface is consistent with other sync-time APIs and leaves mutability to implementations. No further concerns. </details> <details> <summary>src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (2)</summary> `209-215`: **Verify destination / source order** `registerCoarsen(dst_id, src_id, …)` expects *coarse* destination first and *fine* source second. Here `*ex_reflux_id` is assumed to live on the coarse level and `*ex_fluxsum_id` on the fine one – please double-check the ordering for every component; a silent swap would corrupt the flux. --- `670-678`: **Magnetic synchronizer disabled—intentional?** `magnetoSynchronizers_.sync(levelNumber);` is now commented out. If the new refluxing fully replaces coarse-fine B-field synchronisation this is fine; otherwise div B errors may creep in. Please confirm this is deliberate. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| auto iCoarseLevel = ilvl - 1; | ||
| auto& coarseLevel = *hierarchy->getPatchLevel(iCoarseLevel); | ||
| auto& coarseSolver = getSolver_(iCoarseLevel); | ||
| auto& coarseModel = getModel_(iCoarseLevel); | ||
|
|
||
| toCoarser.reflux(iCoarseLevel, ilvl, syncTime); | ||
| coarseSolver.reflux(coarseModel, coarseLevel, syncTime); | ||
| coarseSolver.resetFluxSum(coarseModel, fineLevel); | ||
|
|
There was a problem hiding this comment.
Reset flux sum on the correct solver/level
resetFluxSum is invoked on coarseSolver but passed the fine level (fineLevel).
If different solver instances serve coarse & fine levels this can silently skip the reset or double-count memory.
- coarseSolver.resetFluxSum(coarseModel, fineLevel);
+ auto& fineSolver = getSolver_(ilvl); // same level as fineLevel
+ auto& fineModel = getModel_(ilvl);
+ fineSolver.resetFluxSum(fineModel, fineLevel);📝 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.
| auto iCoarseLevel = ilvl - 1; | |
| auto& coarseLevel = *hierarchy->getPatchLevel(iCoarseLevel); | |
| auto& coarseSolver = getSolver_(iCoarseLevel); | |
| auto& coarseModel = getModel_(iCoarseLevel); | |
| toCoarser.reflux(iCoarseLevel, ilvl, syncTime); | |
| coarseSolver.reflux(coarseModel, coarseLevel, syncTime); | |
| coarseSolver.resetFluxSum(coarseModel, fineLevel); | |
| auto iCoarseLevel = ilvl - 1; | |
| auto& coarseLevel = *hierarchy->getPatchLevel(iCoarseLevel); | |
| auto& coarseSolver = getSolver_(iCoarseLevel); | |
| auto& coarseModel = getModel_(iCoarseLevel); | |
| toCoarser.reflux(iCoarseLevel, ilvl, syncTime); | |
| coarseSolver.reflux(coarseModel, coarseLevel, syncTime); | |
| - coarseSolver.resetFluxSum(coarseModel, fineLevel); | |
| + auto& fineSolver = getSolver_(ilvl); // same level as fineLevel | |
| + auto& fineModel = getModel_(ilvl); | |
| + fineSolver.resetFluxSum(fineModel, fineLevel); |
🤖 Prompt for AI Agents
In src/amr/multiphysics_integrator.hpp around lines 578 to 586, the resetFluxSum
method is called on the coarseSolver but incorrectly passed the fineLevel
argument. To fix this, change the argument to the corresponding coarseLevel or
its model so that resetFluxSum operates on the correct solver and level,
ensuring proper flux sum resetting without memory errors.
| auto const& coarseLevel = hierarchy->getPatchLevel(levelNumber - 1); | ||
| refluxSchedules[levelNumber] = RefluxAlgo.createSchedule(coarseLevel, level); | ||
|
|
There was a problem hiding this comment.
Potential nullptr dereference
refluxSchedules[levelNumber] = … is only created for non-root levels but later accessed with operator[] in reflux().
Add a defensive check (or use .at() with clear error) before calling coarsenData() to avoid a hard crash if registration somehow failed.
🤖 Prompt for AI Agents
In src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp around lines 274 to
276, refluxSchedules[levelNumber] is assigned only for non-root levels but later
accessed without checks, risking nullptr dereference. Add a defensive check
before calling coarsenData() to verify refluxSchedules[levelNumber] exists, or
replace operator[] with .at() to throw a clear exception if the schedule is
missing, preventing a hard crash.
| void reflux(int const coarserLevelNumber, int const fineLevelNumber, | ||
| double const syncTime) override | ||
| { | ||
| refluxSchedules[fineLevelNumber]->coarsenData(); | ||
| ghostRefluxedSchedules[coarserLevelNumber]->fillData(syncTime); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing existence / null checks
reflux() blindly dereferences refluxSchedules[fineLevelNumber] and ghostRefluxedSchedules[coarserLevelNumber].
Guard against missing entries to avoid UB:
auto rIt = refluxSchedules.find(fineLevelNumber);
if (rIt != refluxSchedules.end() && rIt->second)
rIt->second->coarsenData();🤖 Prompt for AI Agents
In src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp around lines 681 to
686, the reflux() method dereferences refluxSchedules[fineLevelNumber] and
ghostRefluxedSchedules[coarserLevelNumber] without checking if these entries
exist or are non-null. To fix this, add existence checks using find() on both
refluxSchedules and ghostRefluxedSchedules before dereferencing, and only call
coarsenData() or fillData() if the entries are found and non-null to prevent
undefined behavior.
| /** | ||
| * @brief accumulateFluxSum accumulates the flux sum(s) on the given PatchLevel for | ||
| * refluxing later. | ||
| */ | ||
| virtual void accumulateFluxSum(IPhysicalModel<AMR_Types>& model, | ||
| SAMRAI::hier::PatchLevel& level, double const coef) | ||
| = 0; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
coef parameter lacks semantics & naming clarity
The third argument of accumulateFluxSum is named coef, but the comment does not explain what this coefficient represents (Δt? weighting? scaling factor?).
Either rename the parameter to something self-descriptive (e.g. timeStep) or extend the doc-block accordingly.
- virtual void accumulateFluxSum(IPhysicalModel<AMR_Types>& model,
- SAMRAI::hier::PatchLevel& level, double const coef)
+ virtual void accumulateFluxSum(IPhysicalModel<AMR_Types>& model,
+ SAMRAI::hier::PatchLevel& level, double const timeStep)🤖 Prompt for AI Agents
In src/amr/solvers/solver.hpp around lines 93 to 100, the parameter named 'coef'
in the accumulateFluxSum method lacks clarity about its purpose. Update the
doc-block comment to explicitly describe what this coefficient represents, such
as whether it is a time step, weighting factor, or scaling parameter.
Alternatively, rename the parameter to a more descriptive name like 'timeStep'
if that matches its intended use, ensuring the name and comment clearly convey
its semantics.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| #include "messenger_info.hpp" | ||
| //#include "core/data/grid/gridlayout.hpp" | ||
| // #include "core/data/grid/gridlayout.hpp" |
Check notice
Code scanning / CodeQL
Commented-out code
UCaromel
force-pushed
the
refluxing
branch
2 times, most recently
from
1fc018f to
f6bc18a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/functional/harris/harris_2d.py (1)
31-33: Fix deprecated numpy behavior with float argument.The calculation
nt = final_time / dt + 1produces a float which causes deprecated behavior when passed tonp.arange().Apply this fix:
-nt = final_time / dt + 1 +nt = int(final_time / dt) + 1src/amr/data/field/coarsening/flux_sum_coarsener.hpp (1)
35-43: Address unused ratio parameter.The constructor accepts a
ratioparameter but doesn't store or validate it, instead using the hard-codedratio_ = 2.Apply this fix to ensure consistency:
FluxSumCoarsener(std::array<core::QtyCentering, dimension> const centering, SAMRAI::hier::Box const& sourceBox, SAMRAI::hier::Box const& destinationBox, SAMRAI::hier::IntVector const& ratio) : centering_{centering} , sourceBox_{sourceBox} , destinationBox_{destinationBox} { + if (ratio.max() != ratio_) { + throw std::runtime_error("FluxSumCoarsener supports only 2-to-1 coarsening"); + } }src/amr/multiphysics_integrator.hpp (1)
577-585: Fix incorrect solver/level pairing in resetFluxSum.The
resetFluxSumis called oncoarseSolverbut passed thefineLevelparameter, which is inconsistent and could cause incorrect behavior.Apply this fix to ensure proper flux sum resetting:
- coarseSolver.resetFluxSum(coarseModel, fineLevel); + coarseSolver.resetFluxSum(coarseModel, coarseLevel);
🧹 Nitpick comments (1)
tests/functional/harris/harris_2d.py (1)
36-154: Consider refactoring config() to reduce local variable count.The function has 16 local variables, exceeding the recommended limit of 15. Consider extracting helper functions for related functionality (e.g., magnetic field initialization, velocity functions) to improve readability and maintainability.
Example refactoring:
def config(): sim = ph.Simulation( # simulation parameters... ) def create_field_functions(sim): """Create magnetic field and related functions""" # Move bx, by, bz, T functions here return locals() def create_velocity_functions(): """Create velocity and thermal velocity functions""" # Move velocity functions here return locals() field_funcs = create_field_functions(sim) velocity_funcs = create_velocity_functions() # Use the functions...
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/amr/CMakeLists.txt(1 hunks)src/amr/data/field/coarsening/flux_sum_coarsener.hpp(1 hunks)src/amr/data/field/coarsening/magnetic_field_coarsener.hpp(1 hunks)src/amr/data/field/refine/electric_field_refiner.hpp(1 hunks)src/amr/data/field/refine/magnetic_field_refiner.hpp(1 hunks)src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(12 hunks)src/amr/messengers/hybrid_messenger.hpp(2 hunks)src/amr/messengers/hybrid_messenger_info.hpp(1 hunks)src/amr/messengers/hybrid_messenger_strategy.hpp(2 hunks)src/amr/messengers/messenger.hpp(4 hunks)src/amr/messengers/mhd_hybrid_messenger_strategy.hpp(5 hunks)src/amr/messengers/mhd_messenger.hpp(4 hunks)src/amr/multiphysics_integrator.hpp(5 hunks)src/amr/resources_manager/amr_utils.hpp(1 hunks)src/amr/solvers/solver.hpp(2 hunks)src/amr/solvers/solver_mhd.hpp(1 hunks)src/amr/solvers/solver_ppc.hpp(8 hunks)tests/functional/harris/harris_2d.py(4 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/amr/data/field/refine/electric_field_refiner.hpp
- src/amr/messengers/hybrid_messenger_info.hpp
🚧 Files skipped from review as they are similar to previous changes (12)
- src/amr/data/field/coarsening/magnetic_field_coarsener.hpp
- src/amr/CMakeLists.txt
- src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp
- src/amr/messengers/mhd_hybrid_messenger_strategy.hpp
- src/amr/messengers/mhd_messenger.hpp
- src/amr/messengers/hybrid_messenger.hpp
- src/amr/messengers/hybrid_messenger_strategy.hpp
- src/amr/solvers/solver_mhd.hpp
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
- src/amr/messengers/messenger.hpp
- src/amr/solvers/solver.hpp
- src/amr/solvers/solver_ppc.hpp
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/amr/data/field/refine/magnetic_field_refiner.hppsrc/amr/resources_manager/amr_utils.hppsrc/amr/data/field/coarsening/flux_sum_coarsener.hppsrc/amr/multiphysics_integrator.hpp
🪛 Pylint (3.3.7)
tests/functional/harris/harris_2d.py
[refactor] 36-36: Too many local variables (16/15)
(R0914)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- 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)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (9)
src/amr/resources_manager/amr_utils.hpp (1)
112-130: LGTM! Well-implemented coarse index conversion.The algorithm correctly handles both positive and negative indices with the appropriate mathematical formula for refinement ratio 2. The documentation clearly explains the rationale for the negative index handling, and the implementation is clean and efficient.
src/amr/data/field/refine/magnetic_field_refiner.hpp (1)
57-58: LGTM! Good refactoring to use centralized utility function.Replacing the manual coarse index calculation with
toCoarseIndex()improves code consistency and maintainability across the codebase. This aligns well with the DRY principle and reduces potential for calculation errors.tests/functional/harris/harris_2d.py (1)
156-168: LGTM! Clean refactoring to standalone execution.The transformation from class-based to function-based approach with graceful handling of missing dependencies (phlop) is well-implemented. The MPI barrier ensures proper synchronization.
src/amr/data/field/coarsening/flux_sum_coarsener.hpp (3)
70-78: LGTM! Correct 1D coarsening implementation.The 1D coarsening logic correctly handles dual centering (averaging two adjacent fine values) and primal centering (copying single fine value). This preserves flux conservation properties.
82-109: LGTM! Correct 2D coarsening for electric field components.The 2D implementation properly handles all electric field component centerings:
- Ex (dual-X, primal-Y): averages along X direction
- Ey (primal-X, dual-Y): averages along Y direction
- Ez (primal-X, primal-Y): direct copy
The error handling for unexpected centerings is appropriate.
110-146: LGTM! Complete 3D coarsening implementation.The 3D implementation correctly extends the 2D logic to handle all three electric field components with appropriate averaging along the dual-centered dimension. The algorithm maintains flux conservation properties essential for the refluxing mechanism.
src/amr/multiphysics_integrator.hpp (3)
532-532: LGTM! Proper solver preparation integration.Adding
prepareStep()before level advancement ensures the solver is properly configured for the upcoming time step. This is essential for the new refluxing mechanism.
549-554: LGTM! Correct flux accumulation with proper scaling.The flux accumulation logic is well-implemented:
- Only applied to non-coarsest levels (iLevel != 0)
- Coefficient
1/(ratio²)correctly scales for flux conservation- Called after level advancement when flux data is available
583-584: LGTM! Well-structured refluxing sequence.The reflux operations are properly ordered:
- Messenger reflux for inter-level communication
- Solver reflux for field corrections
This maintains the correct data flow in the AMR refluxing process.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
681-686: Critical: Address potential null pointer dereferences.The reflux method accesses
refluxSchedules[fineLevelNumber]andghostRefluxedSchedules[coarserLevelNumber]without verifying these entries exist, which could cause crashes if registration failed or levels are accessed incorrectly.Add defensive checks before dereferencing:
void reflux(int const coarserLevelNumber, int const fineLevelNumber, double const syncTime) override { - refluxSchedules[fineLevelNumber]->coarsenData(); - ghostRefluxedSchedules[coarserLevelNumber]->fillData(syncTime); + auto refluxIt = refluxSchedules.find(fineLevelNumber); + if (refluxIt != refluxSchedules.end() && refluxIt->second) { + refluxIt->second->coarsenData(); + } + + auto ghostIt = ghostRefluxedSchedules.find(coarserLevelNumber); + if (ghostIt != ghostRefluxedSchedules.end() && ghostIt->second) { + ghostIt->second->fillData(syncTime); + } }tests/functional/harris/harris_2d.py (1)
27-33: Fix deprecated float-to-int conversion.The calculation of
ntproduces a float that is passed tonp.arange(), which causes deprecated behavior.Apply this fix:
-nt = final_time / dt + 1 +nt = int(final_time / dt) + 1
🧹 Nitpick comments (1)
tests/functional/harris/harris_2d.py (1)
36-153: Consider refactoring to reduce local variable count.The
config()function has 16 local variables, exceeding the recommended limit of 15. Consider extracting some initialization functions into separate helper functions to improve readability.For example, you could extract the field initialization functions:
def create_field_functions(sim): """Create and return field initialization functions.""" def density(x, y): # ... existing density function def by(x, y): # ... existing by function def bx(x, y): # ... existing bx function # ... other functions return { 'density': density, 'bx': bx, 'by': by, 'bz': bz, 'vx': vx, 'vy': vy, 'vz': vz, 'vthx': vthx, 'vthy': vthy, 'vthz': vthz }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/amr/CMakeLists.txt(1 hunks)src/amr/data/field/coarsening/flux_sum_coarsener.hpp(1 hunks)src/amr/data/field/coarsening/magnetic_field_coarsener.hpp(1 hunks)src/amr/data/field/refine/electric_field_refiner.hpp(1 hunks)src/amr/data/field/refine/magnetic_field_refiner.hpp(1 hunks)src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(12 hunks)src/amr/messengers/hybrid_messenger.hpp(2 hunks)src/amr/messengers/hybrid_messenger_info.hpp(1 hunks)src/amr/messengers/hybrid_messenger_strategy.hpp(2 hunks)src/amr/messengers/messenger.hpp(4 hunks)src/amr/messengers/mhd_hybrid_messenger_strategy.hpp(5 hunks)src/amr/messengers/mhd_messenger.hpp(4 hunks)src/amr/multiphysics_integrator.hpp(5 hunks)src/amr/resources_manager/amr_utils.hpp(1 hunks)src/amr/solvers/solver.hpp(2 hunks)src/amr/solvers/solver_mhd.hpp(1 hunks)src/amr/solvers/solver_ppc.hpp(8 hunks)tests/amr/messengers/test_messengers.cpp(3 hunks)tests/functional/harris/harris_2d.py(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/amr/messengers/hybrid_messenger_info.hpp
🚧 Files skipped from review as they are similar to previous changes (16)
- src/amr/CMakeLists.txt
- src/amr/data/field/coarsening/magnetic_field_coarsener.hpp
- src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp
- src/amr/data/field/refine/magnetic_field_refiner.hpp
- src/amr/data/field/refine/electric_field_refiner.hpp
- src/amr/resources_manager/amr_utils.hpp
- src/amr/messengers/mhd_hybrid_messenger_strategy.hpp
- src/amr/messengers/hybrid_messenger_strategy.hpp
- src/amr/messengers/mhd_messenger.hpp
- src/amr/multiphysics_integrator.hpp
- src/amr/messengers/hybrid_messenger.hpp
- src/amr/solvers/solver_mhd.hpp
- src/amr/solvers/solver.hpp
- src/amr/data/field/coarsening/flux_sum_coarsener.hpp
- src/amr/messengers/messenger.hpp
- src/amr/solvers/solver_ppc.hpp
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
🪛 Pylint (3.3.7)
tests/functional/harris/harris_2d.py
[refactor] 36-36: Too many local variables (16/15)
(R0914)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest, gcc)
🔇 Additional comments (21)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (10)
13-16: LGTM: Clean addition of required includes.The new includes for field coarsening and flux sum coarsening are properly added to support the refluxing functionality.
39-40: LGTM: Required SAMRAI coarsening includes added.The CoarsenAlgorithm and CoarsenSchedule includes are necessary for the new refluxing infrastructure.
105-105: LGTM: Flux sum coarsening operator typedef properly defined.The FluxSumCoarsenOp typedef follows the established pattern for other coarsening operators in the class.
192-226: LGTM: Comprehensive reflux registration implementation.The reflux registration correctly:
- Registers variable IDs for reflux electric fields and flux sum electric fields
- Sets up coarsening operations from flux sum to reflux fields
- Configures refinement operations for ghost cell updates
- Uses appropriate variable fill patterns for each component
The implementation follows SAMRAI patterns and integrates well with the existing codebase.
253-255: LGTM: Proper reflux schedule creation.The schedule creation correctly:
- Creates ghost reflux schedules for all levels (including finest)
- Creates reflux schedules only for non-root levels, which is appropriate since root level cannot reflux to a coarser level
- Uses the correct algorithm instances and level hierarchy
Also applies to: 273-275
616-616: LGTM: Bug fix removing erroneous argument.The prepareStep method call has been corrected by removing an invalid argument, improving code correctness.
674-674: LGTM: Magnetic synchronizer appropriately disabled.Commenting out the magnetic synchronizer call aligns with the PR objective of implementing the new refluxing strategy that replaces the previous Loring (2008) method.
713-713: LGTM: Magnetic patch ghost filling appropriately disabled.Commenting out the magnetic patch ghost refinement aligns with the new refluxing approach that maintains divergence-free properties through the reflux mechanism.
1069-1072: LGTM: Reflux algorithm and schedule member variables properly added.The new member variables follow the established naming conventions and are correctly typed for the refluxing infrastructure.
1131-1131: LGTM: Flux sum coarsening operator member properly added.The flux sum coarsening operator member is correctly declared and follows the pattern of other coarsening operators in the class.
tests/amr/messengers/test_messengers.cpp (3)
293-293: LGTM: Resource registration properly added.The
hybridSolver->registerResources(*models[1])call is correctly placed before messenger registration, ensuring resources are available for the refluxing infrastructure.
305-305: LGTM: MHD solver resource registration properly added.The
mhdSolver->registerResources(*models[0])call follows the same pattern and ensures proper test setup for the enhanced solver interface.
317-317: LGTM: Consistent resource registration pattern.The second
hybridSolver->registerResources(*models[1])call maintains consistency across test cases and ensures proper resource registration before messenger operations.tests/functional/harris/harris_2d.py (8)
7-8: LGTM: Import updates for refactored approach.The addition of
SimulatorandstartMPIimports supports the transition from class-based to function-based test structure.
10-20: LGTM: Comprehensive scope timing documentation.The environment variable setup and detailed documentation provide clear guidance for enabling scope timing functionality. This will be valuable for performance analysis of the new refluxing implementation.
25-25: LGTM: MPI initialization moved to appropriate scope.Moving
startMPI()to the top-level script scope ensures proper MPI setup before any simulation operations.
38-51: LGTM: Updated simulation configuration for enhanced testing.The simulation parameters have been appropriately updated:
- Larger grid size (200x400) for more comprehensive testing
- Specified patch sizes for better AMR control
- Increased time steps (1000) with smaller time step (0.001) for finer resolution
- Updated output directory structure
These changes will better exercise the new refluxing infrastructure.
57-98: LGTM: Improved magnetic field initialization functions.The rewritten
density,by, andbxfunctions now use exponential parameterizations that:
- Provide more physically realistic field configurations
- Replace previous Gaussian perturbations with better-controlled profiles
- Maintain mathematical consistency with Harris sheet equilibrium
These improvements will provide more robust testing of the magnetic field refluxing capabilities.
107-107: LGTM: Temperature scaling constant updated.Changing the temperature scaling constant from 0.7 to 1.0 provides a cleaner physical setup for the Harris sheet configuration.
151-151: LGTM: Streamlined diagnostics setup.Removing fluid diagnostics except for particle count info simplifies the test while maintaining essential monitoring capabilities for the refluxing functionality.
156-168: LGTM: Clean refactoring to standalone function.The transition from class-based to function-based approach:
- Simplifies the test structure
- Includes proper error handling for optional plotting functionality
- Maintains MPI synchronization with barrier call
- Provides clear fallback messaging when plotting dependencies are missing
This refactoring improves maintainability while preserving functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/amr/multiphysics_integrator.hpp (1)
578-586: Critical issue: Reset flux sum on the correct solver/level.
resetFluxSumis invoked oncoarseSolverbut passed the fine level (fineLevel). If different solver instances serve coarse & fine levels this can silently skip the reset or cause memory errors.- coarseSolver.resetFluxSum(coarseModel, fineLevel); + coarseSolver.resetFluxSum(coarseModel, coarseLevel);tests/functional/harris/harris_2d.py (1)
31-33: Fix deprecated float usage in np.arange().
nt = final_time / dt + 1produces a float (101.0) which is then passed tonp.arange(nt). While NumPy currently truncates, this is deprecated behavior. Cast explicitly:-nt = final_time / dt + 1 +nt = int(final_time / dt) + 1src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
681-686: Critical: Add defensive checks before accessing schedule mapsThe
reflux()method directly accessesrefluxSchedules[fineLevelNumber]andghostRefluxedSchedules[coarserLevelNumber]without verifying these entries exist, risking undefined behavior if schedules weren't properly registered.Apply this defensive pattern to prevent crashes:
void reflux(int const coarserLevelNumber, int const fineLevelNumber, double const syncTime) override { - refluxSchedules[fineLevelNumber]->coarsenData(); - ghostRefluxedSchedules[coarserLevelNumber]->fillData(syncTime); + auto rIt = refluxSchedules.find(fineLevelNumber); + if (rIt != refluxSchedules.end() && rIt->second) + rIt->second->coarsenData(); + + auto gIt = ghostRefluxedSchedules.find(coarserLevelNumber); + if (gIt != ghostRefluxedSchedules.end() && gIt->second) + gIt->second->fillData(syncTime); }
🧹 Nitpick comments (7)
tests/simulator/utilities/field_coarsening.py (3)
31-31: Apply static analysis suggestion for better readability.The condition can be simplified using the
inoperator for better readability and maintainability.- if qty == "Bx" or qty == "Ey" or qty == "Ez": + if qty in ("Bx", "Ey", "Ez"):
74-74: Apply static analysis suggestion for better readability.The condition can be simplified using the
inoperator for better readability and maintainability.- if qty == "Bx" or qty == "Ey": + if qty in ("Bx", "Ey"):
91-91: Apply static analysis suggestion for better readability.The condition can be simplified using the
inoperator for better readability and maintainability.- if qty == "By" or qty == "Ex": + if qty in ("By", "Ex"):src/amr/solvers/solver_ppc.hpp (1)
302-327: Fix logging scope name in resetFluxSum.The logging scope incorrectly uses "accumulateFluxSum" instead of "resetFluxSum".
- PHARE_LOG_SCOPE(1, "SolverPPC::accumulateFluxSum"); + PHARE_LOG_SCOPE(1, "SolverPPC::resetFluxSum");src/amr/data/field/coarsening/electric_field_coarsener.hpp (3)
1-2: Fix header guard naming for consistency.The header guard name doesn't match the file purpose. Consider using a more specific name that reflects the electric field coarsening functionality.
-#ifndef PHARE_FLUX_SUM_COARSENER -#define PHARE_FLUX_SUM_COARSENER +#ifndef PHARE_ELECTRIC_FIELD_COARSENER +#define PHARE_ELECTRIC_FIELD_COARSENER
17-30: Update class documentation to reflect electric field purpose.The documentation still references magnetic field coarsening and flux conservation, but this class is specifically for electric field coarsening.
-/** @brief This class gives an operator() that performs the coarsening of N fine nodes onto a - * given coarse node - * - * A MagneticFieldCoarsener object is created each time the refine() method of the - * FieldCoarsenOperator is called and its operator() is called for each coarse index. - * It is the default coarsening policy and used for any field that does not come with - * specific constraints (such as conserving some property in the coarsening process). - * - * - * This coarsening operation is defined so to conserve the magnetic flux. - * This is done by assigning to a magnetic field component on a coarse face, the average - * of the enclosed fine faces - * - */ +/** @brief This class provides coarsening functionality for electric field components + * from fine to coarse AMR levels. + * + * An ElectricFieldCoarsener object performs coarsening based on the centering pattern + * of electric field components (primal/dual in each dimension). The coarsening + * operation averages fine field values for dual-centered components and directly + * assigns values for primal-centered components. + */
35-44: Remove unused ratio parameter from constructor.The constructor accepts a
ratioparameter but doesn't use it, relying instead on the hardcodedratio_member. Either use the parameter or remove it for clarity.ElectricFieldCoarsener(std::array<core::QtyCentering, dimension> const centering, SAMRAI::hier::Box const& sourceBox, - SAMRAI::hier::Box const& destinationBox, - SAMRAI::hier::IntVector const& ratio) + SAMRAI::hier::Box const& destinationBox)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/amr/CMakeLists.txt(1 hunks)src/amr/data/field/coarsening/electric_field_coarsener.hpp(1 hunks)src/amr/data/field/coarsening/magnetic_field_coarsener.hpp(1 hunks)src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(13 hunks)src/amr/messengers/hybrid_messenger.hpp(2 hunks)src/amr/messengers/hybrid_messenger_info.hpp(1 hunks)src/amr/messengers/hybrid_messenger_strategy.hpp(2 hunks)src/amr/messengers/messenger.hpp(4 hunks)src/amr/messengers/mhd_hybrid_messenger_strategy.hpp(5 hunks)src/amr/messengers/mhd_messenger.hpp(4 hunks)src/amr/multiphysics_integrator.hpp(5 hunks)src/amr/solvers/solver.hpp(2 hunks)src/amr/solvers/solver_mhd.hpp(1 hunks)src/amr/solvers/solver_ppc.hpp(8 hunks)tests/amr/messengers/test_messengers.cpp(3 hunks)tests/functional/harris/harris_2d.py(4 hunks)tests/simulator/utilities/field_coarsening.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/amr/messengers/hybrid_messenger_info.hpp
🚧 Files skipped from review as they are similar to previous changes (11)
- src/amr/data/field/coarsening/magnetic_field_coarsener.hpp
- src/amr/CMakeLists.txt
- src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp
- src/amr/messengers/hybrid_messenger.hpp
- src/amr/messengers/mhd_messenger.hpp
- src/amr/messengers/mhd_hybrid_messenger_strategy.hpp
- tests/amr/messengers/test_messengers.cpp
- src/amr/messengers/hybrid_messenger_strategy.hpp
- src/amr/solvers/solver_mhd.hpp
- src/amr/messengers/messenger.hpp
- src/amr/solvers/solver.hpp
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/amr/multiphysics_integrator.hppsrc/amr/messengers/hybrid_hybrid_messenger_strategy.hppsrc/amr/data/field/coarsening/electric_field_coarsener.hppsrc/amr/solvers/solver_ppc.hpp
🪛 Pylint (3.3.7)
tests/simulator/utilities/field_coarsening.py
[refactor] 31-31: Consider merging these comparisons with 'in' by using 'qty in ('Bx', 'Ey', 'Ez')'. Use a set instead if elements are hashable.
(R1714)
[refactor] 74-74: Consider merging these comparisons with 'in' by using 'qty in ('Bx', 'Ey')'. Use a set instead if elements are hashable.
(R1714)
[refactor] 91-91: Consider merging these comparisons with 'in' by using 'qty in ('By', 'Ex')'. Use a set instead if elements are hashable.
(R1714)
tests/functional/harris/harris_2d.py
[refactor] 36-36: Too many local variables (16/15)
(R0914)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
🔇 Additional comments (21)
tests/simulator/utilities/field_coarsening.py (1)
55-71: LGTM! Electric field coarsening logic correctly implemented.The special handling for "Ez" in the 2D primal case correctly implements direct copying from fine to coarse data, consistent with the electric field coarsening requirements for the new refluxing framework.
src/amr/multiphysics_integrator.hpp (3)
439-439: LGTM! Const correctness improvement.The parameter type change enhances const correctness without affecting functionality.
532-532: LGTM! Solver preparation step correctly added.The
prepareStepcall properly prepares the solver before advancing the level, which is essential for the new refluxing workflow.
549-554: LGTM! Flux accumulation logic correctly implemented.The flux accumulation with refinement ratio-based coefficient is properly implemented for non-coarsest levels, supporting the new refluxing framework.
tests/functional/harris/harris_2d.py (3)
7-8: LGTM! Import additions support the new workflow.The import changes correctly support the streamlined simulation execution and MPI initialization.
10-21: LGTM! Clear documentation for timing diagnostics.The environment variable setup and documentation provide clear guidance for enabling scope timing, which supports the new diagnostic capabilities.
156-168: LGTM! Streamlined main function improves maintainability.The refactoring from class-based to function-based approach simplifies the script while properly integrating timing diagnostics and error handling.
src/amr/solvers/solver_ppc.hpp (7)
4-8: LGTM! Necessary includes for refluxing functionality.The additional includes support the new refluxing capabilities and patch-level operations.
59-61: LGTM! New fields correctly declared for refluxing.The new vector fields
Bold_andfluxSumE_along with theoldTime_map properly support the refluxing workflow requirements.
99-108: LGTM! Reflux method signatures correctly defined.The new public methods properly implement the ISolver interface requirements for refluxing operations.
123-131: LGTM! Accessor methods follow consistent pattern.The compile-time resource view accessors properly expose the new fields using the established tuple-based pattern.
252-270: LGTM! Prepare step correctly implements magnetic field snapshotting.The
prepareStepmethod properly captures the current magnetic field state and time for later use in refluxing operations.
273-300: LGTM! Flux accumulation correctly implemented.The
accumulateFluxSummethod properly accumulates electric field flux with the provided coefficient across all components and ghost regions.
330-347: LGTM! Reflux method correctly applies Faraday update.The
refluxmethod properly applies Faraday's law using the stored magnetic field state and accumulated electric field flux to update the magnetic field.src/amr/data/field/coarsening/electric_field_coarsener.hpp (3)
69-80: LGTM! 1D coarsening logic correctly implemented.The 1D implementation properly handles dual-centered (Ex) and primal-centered (Ey, Ez) components with appropriate averaging and direct assignment.
82-110: LGTM! 2D coarsening logic handles all electric field components.The 2D implementation correctly covers all electric field component centering patterns (Ex, Ey, Ez) with appropriate coarsening operations and includes proper error handling.
111-147: LGTM! 3D coarsening logic completes the implementation.The 3D implementation properly handles all three electric field components with correct centering-based coarsening and maintains consistency with lower dimensions.
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (4)
192-226: Well-structured reflux registration with proper error handlingThe reflux registration correctly validates all required variable IDs before proceeding and provides a descriptive error message if any are missing. The registration pattern follows the established SAMRAI algorithm conventions.
102-105: Consistent template alias pattern for new coarsening operatorThe
ElectricFieldCoarsenOpfollows the same template pattern as existing coarsening operators, maintaining code consistency and readability.
1069-1072: Proper SAMRAI algorithm integration for reflux operationsThe new reflux algorithms and schedule maps are correctly integrated with appropriate SAMRAI types and follow the existing pattern used for other algorithms in the class.
674-674: Verify magnetic synchronization disabling aligns with reflux requirementsThe commented-out magnetic synchronizers and patch ghost refinement appear to be intentionally disabled during reflux operations, but ensure this doesn't create inconsistencies in magnetic field boundary conditions.
#!/bin/bash # Search for documentation or comments explaining why magnetic sync is disabled during reflux rg -A 5 -B 5 "magnetoSynchronizers.*sync|magPatchGhostsRefineSchedules.*fillData" --type cppAlso applies to: 713-713
UCaromel
force-pushed
the
refluxing
branch
2 times, most recently
from
f4ad38b to
fdd3aea
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
tests/functional/harris/harris_2d.py (1)
56-98: Validate 2D current‐sheet equilibrium consistencyThe new implementation uses two shifted sech² density peaks and Gaussian‐filament fields rather than the classic tanh/cosh Harris sheet. To ensure this 2D equilibrium remains physically valid and stable, please:
- Verify ∇·B remains zero (to machine precision) for the combined 2D field (bx, by).
- Check total pressure
p(y) + ½ |B(x,y)|²
is spatially constant (i.e. ∇p + J×B = 0).- Add a small diagnostic or unit test in tests/functional/harris/harris_2d.py that computes p + B²/2 over the domain and asserts uniformity within a tight tolerance.
- Update the function docstrings and test description to explain the Gaussian‐filament construction (A_z ∝ exp(–r²/L²) → B = ∇×A) and the rationale for deviating from the standard tanh/cosh profile.
This will catch any inadvertent force‐imbalance and document the intended equilibrium.
♻️ Duplicate comments (2)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
682-687: Critical: Add defensive checks before dereferencing reflux schedulesThe
reflux()method directly dereferencesrefluxSchedules[fineLevelNumber]andghostRefluxedSchedules[coarserLevelNumber]without verifying their existence, risking undefined behavior if entries are missing.Apply defensive checks before dereferencing:
void reflux(int const coarserLevelNumber, int const fineLevelNumber, double const syncTime) override { - refluxSchedules[fineLevelNumber]->coarsenData(); - ghostRefluxedSchedules[coarserLevelNumber]->fillData(syncTime); + auto rIt = refluxSchedules.find(fineLevelNumber); + if (rIt != refluxSchedules.end() && rIt->second) + rIt->second->coarsenData(); + + auto gIt = ghostRefluxedSchedules.find(coarserLevelNumber); + if (gIt != ghostRefluxedSchedules.end() && gIt->second) + gIt->second->fillData(syncTime); }tests/functional/harris/harris_2d.py (1)
31-33: Fix deprecated float usage in np.arangeThe calculation
nt = final_time / dt + 1produces a float that's passed tonp.arange(), which is deprecated behavior.Cast explicitly to integer:
-nt = final_time / dt + 1 +nt = int(final_time / dt) + 1
🧹 Nitpick comments (1)
tests/functional/harris/harris_2d.py (1)
25-25: Consider MPI initialization implicationsCalling
startMPI()at module level may affect test isolation if this script is imported elsewhere. Consider moving MPI initialization into themain()function if test modularity is important.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/amr/CMakeLists.txt(1 hunks)src/amr/data/field/coarsening/electric_field_coarsener.hpp(1 hunks)src/amr/data/field/coarsening/magnetic_field_coarsener.hpp(1 hunks)src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(13 hunks)src/amr/messengers/hybrid_messenger.hpp(2 hunks)src/amr/messengers/hybrid_messenger_info.hpp(1 hunks)src/amr/messengers/hybrid_messenger_strategy.hpp(2 hunks)src/amr/messengers/messenger.hpp(4 hunks)src/amr/messengers/mhd_hybrid_messenger_strategy.hpp(5 hunks)src/amr/messengers/mhd_messenger.hpp(4 hunks)src/amr/multiphysics_integrator.hpp(5 hunks)src/amr/solvers/solver.hpp(2 hunks)src/amr/solvers/solver_mhd.hpp(1 hunks)src/amr/solvers/solver_ppc.hpp(8 hunks)tests/amr/messengers/test_messengers.cpp(3 hunks)tests/functional/harris/harris_2d.py(4 hunks)tests/simulator/test_advance.py(2 hunks)tests/simulator/utilities/field_coarsening.py(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/simulator/test_advance.py
- src/amr/messengers/mhd_hybrid_messenger_strategy.hpp
🚧 Files skipped from review as they are similar to previous changes (15)
- src/amr/CMakeLists.txt
- src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp
- src/amr/data/field/coarsening/magnetic_field_coarsener.hpp
- src/amr/messengers/hybrid_messenger.hpp
- src/amr/messengers/hybrid_messenger_info.hpp
- tests/amr/messengers/test_messengers.cpp
- src/amr/messengers/mhd_messenger.hpp
- tests/simulator/utilities/field_coarsening.py
- src/amr/messengers/hybrid_messenger_strategy.hpp
- src/amr/multiphysics_integrator.hpp
- src/amr/solvers/solver_mhd.hpp
- src/amr/data/field/coarsening/electric_field_coarsener.hpp
- src/amr/solvers/solver.hpp
- src/amr/messengers/messenger.hpp
- src/amr/solvers/solver_ppc.hpp
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: rochSmets
PR: PHAREHUB/PHARE#921
File: src/core/data/ions/ions.hpp:138-142
Timestamp: 2025-02-06T10:52:56.461Z
Learning: In PHARE's physics simulations, handling of zero density cells in velocity calculations must be based on physical requirements rather than simple numerical fixes. The solution must account for population-specific behavior in the bulk velocity computation.
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
Learnt from: rochSmets
PR: PHAREHUB/PHARE#921
File: src/core/data/ions/ions.hpp:138-142
Timestamp: 2025-02-06T10:52:56.461Z
Learning: The bulk velocity calculation in PHARE must maintain physical consistency across both ion and electron calculations. Zero density cases need special handling that preserves the physics, particularly since the calculations are used in level initialization, ion updates, and electron physics.
🔇 Additional comments (6)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (4)
192-226: LGTM: Well-structured reflux algorithm registrationThe electric field reflux algorithm registration follows consistent patterns with proper variable ID validation and clear separation between coarsening and ghost refinement operations.
1070-1073: LGTM: Consistent member variable additionsThe new reflux algorithms, schedules, and electric field coarsening operator are properly declared as member variables, maintaining consistency with existing patterns.
Also applies to: 1132-1132
675-675: Verify intentional disabling of magnetic synchronizationThe magneto synchronizer call and related patch-ghost refine fills have been commented out, while other field synchronizations remain enabled. Confirm this omission is deliberate under the reflux-based implementation and does not compromise div-B consistency.
• File: src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
– Line 675:// magnetoSynchronizers_.sync(levelNumber);
– Line 714:// magPatchGhostsRefineSchedules[levelNumber]->fillData(time);
– Also reviewmagPatchGhostsRefineSchedulesmap declarations andmagnetoSynchronizers_setup (registerLevel/add calls, schedule assignments)
617-617: Variadic setOnPatch signature covers modified arguments—no action needed
The call toresourcesManager_->setOnPatch(*patch, hybridModel.state.J, hybridModel.state.ions, Jold_, NiOld_, ViOld_)still matches the single variadic template definition inResourcesManager::setOnPatch, so removing or adding resource‐view parameters here does not break its signature or behavior.tests/functional/harris/harris_2d.py (2)
107-107: Verify temperature scaling constant changeThe temperature scaling constant
Khas been changed from 0.7 to 1.0. Confirm this change is intentional and maintains pressure balance in the Harris sheet equilibrium.
159-164: LGTM: Robust optional import handlingThe try/except pattern for the optional
phlopplotting module provides good user experience with clear fallback messaging when the dependency is unavailable.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (2)
688-693: Critical: Add null checks for reflux schedule access.The
refluxmethod directly accessesrefluxSchedules[fineLevelNumber]andghostRefluxedSchedules[coarserLevelNumber]without verifying they exist, which could cause crashes if these schedules weren't created properly.void reflux(int const coarserLevelNumber, int const fineLevelNumber, double const syncTime) override { - refluxSchedules[fineLevelNumber]->coarsenData(); - ghostRefluxedSchedules[coarserLevelNumber]->fillData(syncTime); + auto refluxIt = refluxSchedules.find(fineLevelNumber); + if (refluxIt != refluxSchedules.end() && refluxIt->second) + refluxIt->second->coarsenData(); + + auto ghostIt = ghostRefluxedSchedules.find(coarserLevelNumber); + if (ghostIt != ghostRefluxedSchedules.end() && ghostIt->second) + ghostIt->second->fillData(syncTime); }
255-257: Add null checks before dereferencing schedule pointers in refluxTo prevent potential null‐pointer dereferences when
coarserLevelNumber == 0, guard both schedule accesses:• File:
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
Location: thereflux(...)method (around lines 275–278)Suggested change:
void reflux(int const coarserLevelNumber, int const fineLevelNumber, double const syncTime) override { - refluxSchedules[fineLevelNumber]->coarsenData(); - ghostRefluxedSchedules[coarserLevelNumber]->fillData(syncTime); + if (refluxSchedules[fineLevelNumber]) { + refluxSchedules[fineLevelNumber]->coarsenData(); + } + if (ghostRefluxedSchedules[coarserLevelNumber]) { + ghostRefluxedSchedules[coarserLevelNumber]->fillData(syncTime); + } }This ensures that root‐level entries (which remain unset) won’t be dereferenced at runtime.
🧹 Nitpick comments (1)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
681-681: Document the reason for disabling magnetic synchronizers.The magnetic synchronizer is commented out, which may be intentional for the refluxing implementation, but the reason should be documented.
- // magnetoSynchronizers_.sync(levelNumber); + // magnetoSynchronizers_.sync(levelNumber); // Disabled for refluxing - magnetic fields handled separately
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(18 hunks)src/amr/messengers/refiner_pool.hpp(3 hunks)src/amr/solvers/solver_ppc.hpp(9 hunks)tests/simulator/test_advance.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/simulator/test_advance.py
- src/amr/solvers/solver_ppc.hpp
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/amr/messengers/refiner_pool.hppsrc/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: rochSmets
PR: PHAREHUB/PHARE#921
File: src/core/data/ions/ions.hpp:138-142
Timestamp: 2025-02-06T10:52:56.461Z
Learning: In PHARE's physics simulations, handling of zero density cells in velocity calculations must be based on physical requirements rather than simple numerical fixes. The solution must account for population-specific behavior in the bulk velocity computation.
🔇 Additional comments (10)
src/amr/messengers/refiner_pool.hpp (2)
69-74: LGTM! Well-designed overload for flexible ghost-model pairing.The new method signature enables pairing multiple ghost vectors with corresponding model vectors, which is more flexible than the existing single-model approach. The design is consistent with the existing interface patterns.
134-134: LGTM! Consistent constness style.The parameter type change from
const inttoint constmaintains consistency with the codebase style.src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (8)
13-13: LGTM! Appropriate includes for refluxing functionality.The added includes for field coarsening operators and SAMRAI coarsening algorithms are necessary for the new refluxing implementation.
Also applies to: 16-16, 39-40
102-105: LGTM! Consistent coarsening operator type definitions.The new coarsening operator type definitions follow the existing pattern and provide the necessary types for refluxing operations.
121-121: LGTM! Consistent field registration and allocation.The
Eold_field registration and allocation follows the established pattern used for other "old" fields likeJold_,NiOld_, andViOld_.Also applies to: 142-142
194-228: LGTM! Well-structured refluxing algorithm registration.The refluxing algorithm registration is well-organized with proper variable ID validation and consistent algorithm registration patterns. The separation of coarsening and refining algorithms is appropriate for the two-phase refluxing process.
620-620: LGTM! Consistent integration of Eold_ field.The addition of
Eold_field handling inprepareStepfollows the established pattern used for other "old" fields, maintaining consistency with the existing codebase.Also applies to: 625-625, 630-630, 635-635
736-742: LGTM! Proper transition to time refiners with interpolation.The change from static to time refiners for electric field ghost communication is appropriate and uses the correct
Jold_field for time interpolation as described in the AI summary.
804-804: LGTM! Appropriate use of electric field coarsening operator.Using the specialized
electricFieldCoarseningOp_instead of the default coarsening operator is correct for electric field synchronization in the refluxing context.
1044-1044: LGTM! Well-organized member variable additions.The new member variables are appropriately added:
Eold_field for storing old electric field state- Reflux algorithms and schedules for the two-phase refluxing process
- Electric field coarsening operator for specialized handling
All follow consistent naming conventions and initialization patterns.
Also applies to: 1079-1082, 1141-1141
|
This last commit adds time refinement for E, which would break the previous method we used for fine coarse consistency at interface (Loring and Raeder 2008). This is what we obtain for divB with time refinement for E on top of master as of 09/07/2025. |
UCaromel
force-pushed
the
refluxing
branch
3 times, most recently
from
013e644 to
fcb9ad6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/amr/data/field/coarsening/electric_field_coarsener.hpp (1)
17-30: Update outdated comments to reflect electric field coarsening.The comments still reference magnetic field coarsening and magnetic flux conservation, which is incorrect for an electric field coarsener.
-/** @brief This class gives an operator() that performs the coarsening of N fine nodes onto a - * given coarse node - * - * A MagneticFieldCoarsener object is created each time the refine() method of the - * FieldCoarsenOperator is called and its operator() is called for each coarse index. - * It is the default coarsening policy and used for any field that does not come with - * specific constraints (such as conserving some property in the coarsening process). - * - * - * This coarsening operation is defined so to conserve the magnetic flux. - * This is done by assigning to a magnetic field component on a coarse face, the average - * of the enclosed fine faces - * - */ +/** @brief This class gives an operator() that performs the coarsening of electric field + * components from fine nodes onto coarse nodes. + * + * An ElectricFieldCoarsener object is created each time the refine() method of the + * FieldCoarsenOperator is called and its operator() is called for each coarse index. + * + * This coarsening operation handles electric field components based on their centering: + * - Dual-centered components are averaged from fine faces + * - Primal-centered components are directly assigned + * + */
♻️ Duplicate comments (2)
src/amr/data/field/coarsening/electric_field_coarsener.hpp (2)
38-38: LGTM: Ratio parameter correctly commented out.The ratio parameter has been appropriately commented out as suggested in the previous review.
55-58: Consider simplifying the fineStartIndex calculation.The current loop-based approach could be simplified using a more direct calculation as suggested in the previous review.
- for (int i = 0u; i < dimension; ++i) - { - fineStartIndex[i] = coarseIndex[i] * this->ratio_; - } + core::Point<int, dimension> fineStartIndex{generate([&](auto const& coarse){ + return coarse * this->ratio_;}, coarseIndex)};Note: This assumes the
generatefunction is available in the current scope.
🧹 Nitpick comments (1)
src/amr/data/field/coarsening/electric_field_coarsener.hpp (1)
1-2: Update include guards to match the specific purpose.The include guards
PHARE_FLUX_SUM_COARSENERare too generic for a file specifically implementing electric field coarsening. They should be more specific to avoid potential conflicts.-#ifndef PHARE_FLUX_SUM_COARSENER -#define PHARE_FLUX_SUM_COARSENER +#ifndef PHARE_ELECTRIC_FIELD_COARSENER +#define PHARE_ELECTRIC_FIELD_COARSENER
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/amr/CMakeLists.txt(1 hunks)src/amr/data/field/coarsening/electric_field_coarsener.hpp(4 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(18 hunks)src/amr/messengers/refiner_pool.hpp(3 hunks)src/amr/multiphysics_integrator.hpp(5 hunks)src/amr/solvers/solver_ppc.hpp(9 hunks)tests/simulator/test_advance.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/amr/CMakeLists.txt
- src/amr/multiphysics_integrator.hpp
- src/amr/messengers/refiner_pool.hpp
- tests/simulator/test_advance.py
- src/amr/solvers/solver_ppc.hpp
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/amr/data/field/coarsening/electric_field_coarsener.hpp
🧠 Learnings (1)
📓 Common learnings
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.
⏰ 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 (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-14)
🔇 Additional comments (4)
src/amr/data/field/coarsening/electric_field_coarsener.hpp (4)
65-73: LGTM: 1D electric field coarsening logic is correct.The implementation correctly handles both dual-centered (ex) and primal-centered (ey, ez) electric field components in 1D.
78-103: LGTM: 2D electric field coarsening logic is correct.The implementation correctly handles all three electric field components (ex, ey, ez) with their respective centering patterns in 2D:
- ex: dual in x, primal in y (averaged along x)
- ey: primal in x, dual in y (averaged along y)
- ez: primal in both directions (direct assignment)
107-140: LGTM: 3D electric field coarsening implementation is correct.The newly implemented 3D case correctly handles all three electric field components:
- ex: dual in x, primal in y,z (averaged along x)
- ey: primal in x,z, dual in y (averaged along y)
- ez: primal in x,y, dual in z (averaged along z)
The logic is consistent with the lower-dimensional cases and properly implements the electric field coarsening requirements.
148-148: LGTM: Hardcoded ratio is appropriate for AMR.The hardcoded ratio of 2 is standard for AMR applications and eliminates the need for runtime configuration.
| // For the moment we only take the case of field with the same centering | ||
| TBOX_ASSERT(fineField.physicalQuantity() == coarseField.physicalQuantity()); | ||
|
|
||
| core::Point<int, dimension> fineStartIndex; |
There was a problem hiding this comment.
if you initialised this on construction it could be const ;)
BROKEN adding the patch refine strategy for the magnetic field 3D for magnetic regriding the path of the SAMRAI cleanup and comments back to NaNs 3d fixes + refactoring back to iterating on the fine field box (in mpi the fine_box might be different from the patch box) pr comments initial commit, E accumulation BROKEN refluxing without divB new magnetic field refinement implementing Toth and Roe (2002) adding the patch refine strategy for the magnetic field forcing even number of ghost cells 3D for magnetic regriding the path of the SAMRAI cleanup and comments back to NaNs 3d fixes + refactoring back to iterating on the fine field box (in mpi the fine_box might be different from the patch box) pr comments initial commit, E accumulation BROKEN adding the patch refine strategy for the magnetic field 3D for magnetic regriding the path of the SAMRAI cleanup and comments back to NaNs 3d fixes + refactoring back to iterating on the fine field box (in mpi the fine_box might be different from the patch box) pr comments BROKEN refluxing without divB rebase master
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/amr/data/field/coarsening/electric_field_coarsener.hpp (3)
39-39: Commented parameter should be documentedThe
ratioparameter is commented out but this obscures the function signature and parameter documentation.
56-59: Loop initialization could be simplifiedThe loop can be simplified using structured bindings or a more direct approach as suggested in past reviews.
- for (auto i = std::size_t{0}; i < dimension; ++i) - { - fineStartIndex[i] = coarseIndex[i] * this->ratio_; - } + for (auto i = std::size_t{0}; i < dimension; ++i) + fineStartIndex[i] = coarseIndex[i] * ratio_;
58-58: Const member initialization opportunityThe
ratio_could be made const if initialized during construction as suggested in past reviews.src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (2)
302-305: Potential null pointer issue in reflux schedule creationThe reflux schedule is only created for non-root levels, but in the
reflux()method it's accessed without null checks, which could cause crashes.
753-758: Missing null pointer safety in reflux methodThe
reflux()method dereferencesrefluxSchedules[fineLevelNumber]andpatchGhostRefluxedSchedules[coarserLevelNumber]without checking if they exist or are non-null.void reflux(int const coarserLevelNumber, int const fineLevelNumber, double const syncTime) override { - refluxSchedules[fineLevelNumber]->coarsenData(); - patchGhostRefluxedSchedules[coarserLevelNumber]->fillData(syncTime); + auto refluxIt = refluxSchedules.find(fineLevelNumber); + if (refluxIt != refluxSchedules.end() && refluxIt->second) + refluxIt->second->coarsenData(); + + auto ghostIt = patchGhostRefluxedSchedules.find(coarserLevelNumber); + if (ghostIt != patchGhostRefluxedSchedules.end() && ghostIt->second) + ghostIt->second->fillData(syncTime); }
🧹 Nitpick comments (4)
tests/functional/harris/harris_2d.py (1)
3-3: Remove unused import.The
osmodule is imported but never used in the code.-import ossrc/amr/data/field/coarsening/electric_field_coarsener.hpp (3)
1-2: Inconsistent header guard namingThe header guard
PHARE_FLUX_SUM_COARSENERdoesn't match the filenameelectric_field_coarsener.hpp. This creates confusion about what the header contains.-#ifndef PHARE_FLUX_SUM_COARSENER -#define PHARE_FLUX_SUM_COARSENER +#ifndef PHARE_ELECTRIC_FIELD_COARSENER_HPP +#define PHARE_ELECTRIC_FIELD_COARSENER_HPP
18-31: Outdated class documentationThe class documentation still refers to "MagneticFieldCoarsener" and magnetic field operations, but this class is now
ElectricFieldCoarsenerfor electric fields.-/** @brief This class gives an operator() that performs the coarsening of N fine nodes onto a - * given coarse node - * - * A MagneticFieldCoarsener object is created each time the refine() method of the - * FieldCoarsenOperator is called and its operator() is called for each coarse index. - * It is the default coarsening policy and used for any field that does not come with - * specific constraints (such as conserving some property in the coarsening process). - * - * - * This coarsening operation is defined so to conserve the magnetic flux. - * This is done by assigning to a magnetic field component on a coarse face, the average - * of the enclosed fine faces - * - */ +/** @brief This class gives an operator() that performs the coarsening of N fine nodes onto a + * given coarse node + * + * An ElectricFieldCoarsener object is created each time the refine() method of the + * FieldCoarsenOperator is called and its operator() is called for each coarse index. + * It implements coarsening specifically for electric field components with appropriate + * centering patterns. + * + * This coarsening operation handles electric field components by averaging fine field + * values onto coarse nodes based on the field's centering (dual/primal) in each direction. + * + */
149-149: Static constexpr ratio could be const memberMaking
ratio_a const member initialized in constructor would improve const-correctness and allow for potential future flexibility.+ ElectricFieldCoarsener(std::array<core::QtyCentering, dimension> const centering, + SAMRAI::hier::Box const& sourceBox, + SAMRAI::hier::Box const& destinationBox, + SAMRAI::hier::IntVector const& ratio) + : centering_{centering} + , sourceBox_{sourceBox} + , destinationBox_{destinationBox} + , ratio_{ratio[0]} // assuming uniform refinement + { + } private: std::array<core::QtyCentering, dimension> const centering_; SAMRAI::hier::Box const sourceBox_; SAMRAI::hier::Box const destinationBox_; - static int constexpr ratio_ = 2; + int const ratio_;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/amr/CMakeLists.txt(1 hunks)src/amr/data/field/coarsening/electric_field_coarsener.hpp(4 hunks)src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp(1 hunks)src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp(14 hunks)src/amr/messengers/hybrid_messenger.hpp(1 hunks)src/amr/messengers/hybrid_messenger_info.hpp(1 hunks)src/amr/messengers/hybrid_messenger_strategy.hpp(1 hunks)src/amr/messengers/messenger.hpp(4 hunks)src/amr/messengers/mhd_hybrid_messenger_strategy.hpp(1 hunks)src/amr/messengers/mhd_messenger.hpp(4 hunks)src/amr/messengers/refiner_pool.hpp(0 hunks)src/amr/multiphysics_integrator.hpp(5 hunks)src/amr/solvers/solver.hpp(2 hunks)src/amr/solvers/solver_mhd.hpp(1 hunks)src/amr/solvers/solver_ppc.hpp(8 hunks)tests/amr/messengers/test_messengers.cpp(3 hunks)tests/functional/harris/harris_2d.py(3 hunks)tests/simulator/test_advance.py(3 hunks)tests/simulator/utilities/field_coarsening.py(3 hunks)
💤 Files with no reviewable changes (1)
- src/amr/messengers/refiner_pool.hpp
✅ Files skipped from review due to trivial changes (1)
- src/amr/messengers/hybrid_messenger_info.hpp
🚧 Files skipped from review as they are similar to previous changes (14)
- src/amr/CMakeLists.txt
- src/amr/data/field/refine/magnetic_refine_patch_strategy.hpp
- src/amr/messengers/hybrid_messenger_strategy.hpp
- src/amr/messengers/hybrid_messenger.hpp
- src/amr/messengers/mhd_hybrid_messenger_strategy.hpp
- tests/amr/messengers/test_messengers.cpp
- tests/simulator/utilities/field_coarsening.py
- tests/simulator/test_advance.py
- src/amr/solvers/solver.hpp
- src/amr/messengers/mhd_messenger.hpp
- src/amr/multiphysics_integrator.hpp
- src/amr/solvers/solver_mhd.hpp
- src/amr/messengers/messenger.hpp
- src/amr/solvers/solver_ppc.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/field/coarsening/electric_field_coarsener.hppsrc/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
🧠 Learnings (5)
📓 Common learnings
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.
📚 Learning: avoid commenting on draft prs for philipdeegan unless explicitly requested....
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.
Applied to files:
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
📚 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.
Applied to files:
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
📚 Learning: the bulk velocity calculation in phare must maintain physical consistency across both ion and electr...
Learnt from: rochSmets
PR: PHAREHUB/PHARE#921
File: src/core/data/ions/ions.hpp:138-142
Timestamp: 2025-02-06T10:52:56.461Z
Learning: The bulk velocity calculation in PHARE must maintain physical consistency across both ion and electron calculations. Zero density cases need special handling that preserves the physics, particularly since the calculations are used in level initialization, ion updates, and electron physics.
Applied to files:
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
📚 Learning: in phare's physics simulations, handling of zero density cells in velocity calculations must be base...
Learnt from: rochSmets
PR: PHAREHUB/PHARE#921
File: src/core/data/ions/ions.hpp:138-142
Timestamp: 2025-02-06T10:52:56.461Z
Learning: In PHARE's physics simulations, handling of zero density cells in velocity calculations must be based on physical requirements rather than simple numerical fixes. The solution must account for population-specific behavior in the bulk velocity computation.
Applied to files:
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
🪛 Ruff (0.12.2)
tests/functional/harris/harris_2d.py
3-3: os imported but unused
Remove unused import: os
(F401)
⏰ 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: build (macos-latest)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (ubuntu-latest, gcc)
🔇 Additional comments (10)
tests/functional/harris/harris_2d.py (2)
5-5: LGTM! Improved matplotlib backend handling for tests.The explicit matplotlib import and setting the backend to "Agg" is appropriate for headless test environments, replacing the previous
ph.NO_GUI()approach.Also applies to: 15-15
170-172: LGTM! Explicit parameter improves code clarity.Adding the explicit
pop_name="protons"parameter makes the API call more clear and self-documenting.src/amr/data/field/coarsening/electric_field_coarsener.hpp (1)
106-142: LGTM on 3D implementationThe 3D coarsening logic correctly handles all three electric field components (Ex, Ey, Ez) with appropriate centering patterns. The implementation properly averages fine field values for dual-centered components and directly copies for primal-centered components.
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (7)
17-24: Clean include organizationThe includes are well-organized with appropriate electric field coarsening and refinement headers. The removal of magnetic field coarsener and addition of electric field coarsener aligns with the refluxing changes.
118-121: Appropriate template alias for electric field coarseningThe
ElectricFieldCoarsenOptemplate alias correctly uses theElectricFieldCoarsenerpolicy, which is consistent with the new refluxing approach for electric fields.
216-243: Comprehensive reflux variable registrationThe registration of reflux electric field and flux sum variables is well-structured with proper error checking for missing variable IDs. The coarsen algorithm registration correctly maps flux sums to reflux fields.
1106-1106: Proper addition of Eold_ member variableThe
Eold_member variable is correctly declared as aVecFieldTwith appropriate naming convention and electric field quantity type. This supports the time interpolation needed for refluxing.
689-689: Consistent Eold_ integration in prepareStepThe
Eold_field is properly integrated into theprepareStepmethod with correct resource management calls for setOnPatch, setTime, and copyData operations, maintaining consistency with other "old" field variables.Also applies to: 694-694, 699-705
1153-1157: Well-structured reflux algorithm and schedule containersThe reflux coarsen algorithm and patch ghost refine algorithm are properly declared with appropriate SAMRAI dimension initialization. The schedule containers use appropriate key types (int for level numbers).
1209-1209: Correct electric field coarsening operator instantiationThe
electricFieldCoarseningOp_is properly instantiated using theElectricFieldCoarsenOptemplate, which ensures electric field-specific coarsening behavior for refluxing operations.
|
replaced by #1068 |
2 participants
This PR's goal is to implement a refluxing strategy for the magnetic field, i order to guaranty divergence free conservation of the magnetic field with arbitrary order refinement. Currently, PHARE implements the Loring (2008) method to conserve the magnetic flux at the coarse fine boundary, but this assume 0th order refinement for the magnetic field.
Our new refluxing strategy is:
Note: The coarsening of the fluxSum and the following faraday on the part of the coarse level overlapped by the fine must, must give the exact same result as what we would have obtained through coarsening of the magnetic field. This is not automatic, and in fact the default field coarsener in PHARE did not permit that. This PR also implements a specific coarsener for that purpose.
implements #1005