refactor(mhd): demote Resistivity/HyperResistivity/TimeIntegrator from compile-time to runtime - #1233
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughResistivity and hyper-resistivity move to runtime configuration, while time integrators are selected through a dictionary-backed factory. RK integrators share stage resources, and solver, module-generation, permutation, and simulation configuration paths are updated. ChangesMHD Runtime Selection Refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SimulationConfig
participant TimeIntegrator
participant RKIntegrator
participant BaseMHDTimestepper
participant ComputeFluxes
SimulationConfig->>TimeIntegrator: provide time_integrator_type
TimeIntegrator->>RKIntegrator: construct selected integrator
RKIntegrator->>BaseMHDTimestepper: allocate extra stage states
RKIntegrator->>ComputeFluxes: evaluate RK stages
ComputeFluxes->>ComputeFluxes: select current updates from Hall and eta/nu
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/numerics/godunov_fluxes/godunov_fluxes.hpp (1)
111-158: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftPopulate
jt/rhotfor Hall-disabled resistive runs.When
Hall == falsebutetaornuis non-zero, the ideal branch saves onlyvt/coefficients intoct_state. The later resistive block readsct_state.getJt<direction>(), and spatial hyper-resistivity also readsgetRhot<direction>(), so those values can be stale or uninitialized. Restore a runtime resistive/hyper-resistive path that reconstructs/saves the transverse current and density before line 167 uses them.Also applies to: 167-199
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/numerics/godunov_fluxes/godunov_fluxes.hpp` around lines 111 - 158, The Hall-disabled path in Godunov flux computation only stores vt/coefficients in ct_state, but the resistive and hyper-resistive logic later depends on jt and rhot being populated. Update the Godunov flux handling in godunov_fluxes.hpp, especially the Hall branch and the Ideal else branch in the flux solver, so that when resistivity_ or hyper_resistivity_ is enabled you also reconstruct and save the transverse current and density into ct_state before the later resistive/hyper-resistive code reads getJt<direction>() and getRhot<direction>(). Make sure the fix preserves the existing Hall path behavior while adding the missing runtime storage for non-Hall resistive runs.
🧹 Nitpick comments (1)
src/core/numerics/godunov_fluxes/godunov_fluxes.hpp (1)
50-52: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winGate the extra grow layer with
hyper_resistivity_.
evalOnBiggerBoxnow expands every runtime mode by the hyper-resistive laplacian layer, so ideal and resistive-only runs pay extra reconstruction work. Pass the runtime flag intogetGrowand add the extra layer only when hyper-resistivity is enabled.♻️ Proposed refactor
-template<auto direction, size_t dim> -auto getGrow(int const nghosts) +template<auto direction, size_t dim> +auto getGrow(int const nghosts, bool const hyper_resistivity) { Point<std::uint32_t, dim> p{}; @@ - p[dir] += 1; + if (hyper_resistivity) + p[dir] += 1; return p; } @@ - getGrow<direction, dimension>(Reconstruction_t::nghosts), + getGrow<direction, dimension>(Reconstruction_t::nghosts, hyper_resistivity_),As per path instructions,
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.Also applies to: 107-110
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/numerics/godunov_fluxes/godunov_fluxes.hpp` around lines 50 - 52, The grow-size logic in godunov_fluxes.hpp is unconditionally adding the extra laplacian layer, which makes ideal and resistive-only modes do unnecessary work. Update getGrow to accept the runtime hyper_resistivity_ flag and only apply the extra increment to p[dir] when hyper-resistivity is enabled; then make evalOnBiggerBox pass that flag through so the expansion matches the selected runtime mode.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/amr/solvers/time_integrator/base_mhd_timestepper.hpp`:
- Around line 18-20: Thread the MessengerT template through the full
time-integrator stack so the generic messenger type is preserved. Update
BaseMHDTimestepper uses in TimeIntegrator and the concrete integrators
(EulerIntegrator, TVDRK2Integrator, TVDRK3Integrator, SSPRK4_5Integrator) to
accept and forward MessengerT instead of hard-coding
BaseMHDTimestepper<MHDModel>. Make sure the SolverMHD<..., Messenger> path
instantiates these templates with the same messenger type all the way down.
In `@src/amr/solvers/time_integrator/compute_fluxes.hpp`:
- Around line 51-56: The refresh condition for `state.J` in `compute_fluxes.hpp`
only checks `fVMethodInfo_`, so CT-only resistive setups can skip `Ampere_t` and
leave `J` stale. Update the `if constexpr (Hall)` / `else if` logic in
`compute_fluxes` to also consider `constrainedTransportInfo_`’s resistive
coefficients (`eta`/`nu`) when deciding whether to run `Ampere_t` and
`TimeSetter`, so constrained-transport enabled resistive paths trigger the same
refresh.
In `@src/amr/solvers/time_integrator/ssprk4_5_integrator.hpp`:
- Around line 112-127: Restore the resource setup for compute_fluxes_ in the
SSPRK4_5 integrator: the current registerResources and allocate implementations
only initialize euler_, but ComputeFluxes also owns fvm_ and ct_ and is invoked
repeatedly from operator(). Re-enable or add the
compute_fluxes_.registerResources(model) and compute_fluxes_.allocate(model,
patch, allocateTime) calls inside SSPRK4_5_Integrator::registerResources and
SSPRK4_5_Integrator::allocate so those resources are properly prepared.
In `@src/amr/solvers/time_integrator/time_integrator.hpp`:
- Around line 4-6: The header relies on a transitive include for std::string,
which makes parse_time_integrator_type and the time_integrator constructor
default fragile. Add the direct <string> include in this header alongside the
existing includes so the declarations in time_integrator remain self-contained
and independent of other headers.
- Around line 20-26: The parser in parse_time_integrator_type is only matching a
couple of hardcoded spellings per enum value, so mixed-case runtime config still
fails. Normalize the input string first (for example, transform it to a
consistent case) and then compare against the canonical integrator names before
returning MHDOpts::TimeIntegratorType values. Keep the unknown-value exception
for anything that still does not match.
In `@src/core/numerics/MHD_equations/MHD_equations.hpp`:
- Around line 10-16: Restore runtime resistivity selection in MHDEquations by
wiring the stored eta_ and nu_ into the flux/computation path instead of leaving
the resistive branch disabled. Update the compute(u, J) logic and any related
overloads in MHDEquations so resistive contributions are conditionally applied
based on runtime state, and reintroduce the Laplacian-based path if
hyper-resistivity is still supported, guarded by a hyperResistivity_ flag. Use
the existing MHDEquations constructor and member state (hall, eta_, nu_) to keep
the behavior selectable without the removed template parameters.
---
Outside diff comments:
In `@src/core/numerics/godunov_fluxes/godunov_fluxes.hpp`:
- Around line 111-158: The Hall-disabled path in Godunov flux computation only
stores vt/coefficients in ct_state, but the resistive and hyper-resistive logic
later depends on jt and rhot being populated. Update the Godunov flux handling
in godunov_fluxes.hpp, especially the Hall branch and the Ideal else branch in
the flux solver, so that when resistivity_ or hyper_resistivity_ is enabled you
also reconstruct and save the transverse current and density into ct_state
before the later resistive/hyper-resistive code reads getJt<direction>() and
getRhot<direction>(). Make sure the fix preserves the existing Hall path
behavior while adding the missing runtime storage for non-Hall resistive runs.
---
Nitpick comments:
In `@src/core/numerics/godunov_fluxes/godunov_fluxes.hpp`:
- Around line 50-52: The grow-size logic in godunov_fluxes.hpp is
unconditionally adding the extra laplacian layer, which makes ideal and
resistive-only modes do unnecessary work. Update getGrow to accept the runtime
hyper_resistivity_ flag and only apply the extra increment to p[dir] when
hyper-resistivity is enabled; then make evalOnBiggerBox pass that flag through
so the expansion matches the selected runtime mode.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f9b5cbbd-91b7-4140-a023-1c39e5eb1e1e
📒 Files selected for processing (22)
pyphare/pyphare/cpp/__init__.pypyphare/pyphare/pharein/initialize/mhd.pyres/sim/all.txtsrc/amr/solvers/solver_mhd.hppsrc/amr/solvers/solver_mhd_field_evolvers.hppsrc/amr/solvers/time_integrator/base_mhd_timestepper.hppsrc/amr/solvers/time_integrator/compute_fluxes.hppsrc/amr/solvers/time_integrator/euler_integrator.hppsrc/amr/solvers/time_integrator/ssprk4_5_integrator.hppsrc/amr/solvers/time_integrator/time_integrator.hppsrc/amr/solvers/time_integrator/tvdrk2_integrator.hppsrc/amr/solvers/time_integrator/tvdrk3_integrator.hppsrc/core/numerics/MHD_equations/MHD_equations.hppsrc/core/numerics/constrained_transport/constrained_transport.hppsrc/core/numerics/constrained_transport/upwind_constrained_transport.hppsrc/core/numerics/constrained_transport/upwind_constrained_transport_utils.hppsrc/core/numerics/godunov_fluxes/godunov_fluxes.hppsrc/core/numerics/godunov_fluxes/godunov_utils.hppsrc/phare_simulator_options.hppsrc/python3/CMakeLists.txtsrc/python3/mhd_defaults/mhd_defaults.hppsrc/python3/mhd_resolver.hpp
| template<bool Hall> | ||
| class MHDEquations | ||
| { | ||
| public: | ||
| constexpr static bool hall = Hall; | ||
| constexpr static bool resistivity = Resistivity; | ||
| constexpr static bool hyperResistivity = HyperResistivity; | ||
| constexpr static bool hall = Hall; | ||
|
|
||
| MHDEquations(double const gamma, double const eta, double const nu) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wire the removed resistivity flags to runtime state.
After dropping the Resistivity/HyperResistivity template parameters, this class still only exposes hall; eta_/nu_ are stored but not used to select resistive contributions, and the compute(u, J) resistive branch remains commented. That breaks runtime eta/nu selection for paths that rely on MHDEquations for resistive fluxes.
Proposed direction
MHDEquations(double const gamma, double const eta, double const nu)
: gamma_{gamma}
, eta_{eta}
, nu_{nu}
+ , resistivity_{eta != 0.0}
+ , hyperResistivity_{nu != 0.0}
{
}
@@
if constexpr (Hall)
hall_contribution_<direction>(u.rho, u.B, J, f.B, f.P);
- // if constexpr (Resistivity)
- // resistive_contributions_<direction>(eta_, u.B, J, f.B, f.P);
+ if (resistivity_)
+ resistive_contributions<direction>(eta_, u.B, J, f.B, f.P);
@@
double const gamma_;
double const eta_;
double const nu_;
+ bool const resistivity_;
+ bool const hyperResistivity_;If hyper-resistivity is also owned here, restore the Laplacian overload and guard it with hyperResistivity_.
Also applies to: 83-88, 133-136
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/numerics/MHD_equations/MHD_equations.hpp` around lines 10 - 16,
Restore runtime resistivity selection in MHDEquations by wiring the stored eta_
and nu_ into the flux/computation path instead of leaving the resistive branch
disabled. Update the compute(u, J) logic and any related overloads in
MHDEquations so resistive contributions are conditionally applied based on
runtime state, and reintroduce the Laplacian-based path if hyper-resistivity is
still supported, guarded by a hyperResistivity_ flag. Use the existing
MHDEquations constructor and member state (hall, eta_, nu_) to keep the behavior
selectable without the removed template parameters.
There was a problem hiding this comment.
Resistive fluxes are not applied through MHDEquations::compute(u, J) — that commented branch was already dead on master, before this PR. The runtime path is elsewhere by design: godunov_fluxes.hpp applies resistive_contributions(eta, ...) and the hyper-resistive term post-Riemann on transverse-averaged fields (guarded by the runtime resistivity_/hyper_resistivity_ flags introduced here), and constrained transport adds E += eta*J / the Laplacian term itself. So runtime eta/nu selection works; nothing regressed.
Cleaned up in c9c39f3: deleted the dead commented blocks and the now-unused eta_/nu_ members from MHDEquations so the class no longer suggests it owns resistivity.
There was a problem hiding this comment.
@igirault Thanks for the clarification — that makes sense. The commented block in compute(u, J) was indeed dead code carried over from master, and the actual runtime eta/nu selection is correctly handled downstream in godunov_fluxes.hpp and constrained transport via the resistivity_/hyper_resistivity_ flags. Appreciate the cleanup in c9c39f3 removing the dead code and now-unused eta_/nu_ members — that removes the misleading suggestion that MHDEquations owns resistivity handling.
✏️ Learnings added
Learnt from: igirault
Repo: PHAREHUB/PHARE PR: 1233
File: src/core/numerics/MHD_equations/MHD_equations.hpp:10-16
Timestamp: 2026-07-16T07:55:31.199Z
Learning: In PHARE's MHD solver code, resistive and hyper-resistive flux contributions are not applied through `MHDEquations::compute(u, J)` (that path is dead/commented code, pre-existing on master). The actual runtime application of `eta`/`nu`-based resistivity is done in `src/core/numerics/godunov_fluxes/godunov_fluxes.hpp` via `resistive_contributions(eta, ...)` and the hyper-resistive Laplacian term post-Riemann on transverse-averaged fields, guarded by runtime `resistivity_`/`hyper_resistivity_` flags, and in the constrained transport code (`E += eta*J` and the Laplacian term). `MHDEquations` (in `src/core/numerics/MHD_equations/MHD_equations.hpp`) only owns the `Hall` term after the runtime refactor.
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.
- compute_fluxes: refresh J when constrained-transport resistivity/ hyper-resistivity is enabled, not just when fv_method's is - SSPRK4_5: register/allocate resources for its own ComputeFluxes instance (fvm_/ct_ were never set up) - time_integrator: add missing <string> include, make parse_time_integrator_type case-insensitive Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Lots of potentially unrelated TimeStepper stuff in the diff |
|
I think it is on purpose, to select the time integration scheme at runtime. But maybe this is not done the way you would expect, we can discuss it if necessary |
BaseMHDTimestepper abstracted the messenger type but TimeIntegrator and the concrete integrators hard-coded the default, pinning the call path to amr::MHDMessenger and breaking SolverMHD's generic Messenger parameter. Forward MessengerT through all integrators; SolverMHD now derives its messenger from the strategy and static_asserts a match. Also drop the dead commented resistive branch and unused eta_/nu_ from MHDEquations: resistive fluxes are applied at runtime in godunov_fluxes and constrained transport, not via MHDEquations. Refs PHAREHUB#1233 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/amr/solvers/time_integrator/compute_fluxes.hpp (1)
51-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate the condition to avoid code duplication.
As per path instructions for clean code and expressiveness, the identical
Ampere_tandTimeSetterinvocations in both branches can be consolidated. SinceHallis aconstexprboolean,if (Hall || ...)will naturally short-circuit and optimize at compile time whenHallis true, avoiding both the runtime evaluation of the resistive parameters and the duplicated method calls.♻️ Proposed refactor
- auto const needsCurrent = fVMethodInfo_.eta != 0.0 || fVMethodInfo_.nu != 0.0 - || constrainedTransportInfo_.eta != 0.0 - || constrainedTransportInfo_.nu != 0.0; - - if constexpr (Hall) - { - Ampere_t{level, model}(state.B, state.J); - TimeSetter{level, model, newTime}(state.B, state.J); - } - else if (needsCurrent) + if (Hall || fVMethodInfo_.eta != 0.0 || fVMethodInfo_.nu != 0.0 + || constrainedTransportInfo_.eta != 0.0 || constrainedTransportInfo_.nu != 0.0) { Ampere_t{level, model}(state.B, state.J); TimeSetter{level, model, newTime}(state.B, state.J); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/amr/solvers/time_integrator/compute_fluxes.hpp` around lines 51 - 60, Consolidate the current-computation branches in the surrounding flux computation logic: replace the separate Hall and needsCurrent paths with one condition using Hall || needsCurrent, then perform the shared Ampere_t and TimeSetter calls once inside that block. Preserve constexpr short-circuiting so resistive-parameter evaluation is skipped when Hall is true.Source: Path instructions
src/amr/solvers/solver_mhd.hpp (1)
37-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider removing the
Messengertemplate parameter.If
Messengermust always strictly matchTimeIntegratorStrategy::Messenger, consider removing it as a template parameter fromSolverMHDentirely to simplify the class signature. You could then define it as an inner alias:using Messenger = typename TimeIntegratorStrategy::Messenger;.If it's being kept to preserve backward compatibility with existing 4-argument template instantiations across the codebase, the
static_assertis a robust safeguard. As per path instructions, removing redundant parameters improves clean code and expressiveness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/amr/solvers/solver_mhd.hpp` around lines 37 - 42, Remove the redundant Messenger template parameter from SolverMHD and define Messenger as an inner alias to TimeIntegratorStrategy::Messenger. Update SolverMHD template declarations and any internal references accordingly, while preserving compatibility only if existing four-argument instantiations require it; otherwise eliminate the now-unnecessary static_assert.Source: Path instructions
src/amr/solvers/time_integrator/tvdrk2_integrator.hpp (2)
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
w0_for the first stage Butcher flux accumulation.Although
w0_andw1_both equal0.5, usingw0_for the first stage accumulation conveys the specific mathematical intent of the TVDRK2 integration scheme and improves code expressiveness. As per path instructions, prioritize expressiveness in C++ headers.💡 Proposed fix
- this->accumulateButcherFluxes_(model, state.E, fluxes, level, w1_); + this->accumulateButcherFluxes_(model, state.E, fluxes, level, w0_);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/amr/solvers/time_integrator/tvdrk2_integrator.hpp` at line 42, Update the first-stage accumulateButcherFluxes_ call in the TVDRK2 integrator to pass w0_ instead of w1_. Leave the second-stage accumulation and the rest of the integration logic unchanged.Source: Path instructions
19-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused type aliases.
These private type aliases are never used within their respective classes. Removing them improves code clarity and reduces leftover clutter from past refactoring. As per path instructions, focus on clean code principles.
src/amr/solvers/time_integrator/tvdrk2_integrator.hpp#L19-L21: Remove the unusedGridLayoutT,Dispatchers_t, andRKUtils_taliases.src/amr/solvers/time_integrator/ssprk4_5_integrator.hpp#L20-L20: Remove the unusedGridLayoutTalias.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/amr/solvers/time_integrator/tvdrk2_integrator.hpp` around lines 19 - 21, Remove the unused GridLayoutT, Dispatchers_t, and RKUtils_t aliases from the TVDRK2 integrator class in src/amr/solvers/time_integrator/tvdrk2_integrator.hpp (lines 19-21), and remove the unused GridLayoutT alias from the SSPRK4_5 integrator class in src/amr/solvers/time_integrator/ssprk4_5_integrator.hpp (line 20).Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/amr/solvers/solver_mhd.hpp`:
- Around line 37-42: Remove the redundant Messenger template parameter from
SolverMHD and define Messenger as an inner alias to
TimeIntegratorStrategy::Messenger. Update SolverMHD template declarations and
any internal references accordingly, while preserving compatibility only if
existing four-argument instantiations require it; otherwise eliminate the
now-unnecessary static_assert.
In `@src/amr/solvers/time_integrator/compute_fluxes.hpp`:
- Around line 51-60: Consolidate the current-computation branches in the
surrounding flux computation logic: replace the separate Hall and needsCurrent
paths with one condition using Hall || needsCurrent, then perform the shared
Ampere_t and TimeSetter calls once inside that block. Preserve constexpr
short-circuiting so resistive-parameter evaluation is skipped when Hall is true.
In `@src/amr/solvers/time_integrator/tvdrk2_integrator.hpp`:
- Line 42: Update the first-stage accumulateButcherFluxes_ call in the TVDRK2
integrator to pass w0_ instead of w1_. Leave the second-stage accumulation and
the rest of the integration logic unchanged.
- Around line 19-21: Remove the unused GridLayoutT, Dispatchers_t, and RKUtils_t
aliases from the TVDRK2 integrator class in
src/amr/solvers/time_integrator/tvdrk2_integrator.hpp (lines 19-21), and remove
the unused GridLayoutT alias from the SSPRK4_5 integrator class in
src/amr/solvers/time_integrator/ssprk4_5_integrator.hpp (line 20).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eaf2649f-cf4b-4e3f-bfaa-9e4638eb837f
📒 Files selected for processing (10)
src/amr/solvers/solver_mhd.hppsrc/amr/solvers/time_integrator/compute_fluxes.hppsrc/amr/solvers/time_integrator/euler_integrator.hppsrc/amr/solvers/time_integrator/ssprk4_5_integrator.hppsrc/amr/solvers/time_integrator/time_integrator.hppsrc/amr/solvers/time_integrator/tvdrk2_integrator.hppsrc/amr/solvers/time_integrator/tvdrk3_integrator.hppsrc/core/numerics/MHD_equations/MHD_equations.hppsrc/core/numerics/godunov_fluxes/godunov_fluxes.hppsrc/python3/mhd_defaults/mhd_defaults.hpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/amr/solvers/time_integrator/time_integrator.hpp
- src/amr/solvers/time_integrator/tvdrk3_integrator.hpp
- src/core/numerics/godunov_fluxes/godunov_fluxes.hpp
BaseMHDTimestepper abstracted the messenger type but TimeIntegrator and the concrete integrators hard-coded the default, pinning the call path to amr::MHDMessenger and breaking SolverMHD's generic Messenger parameter. Forward MessengerT through all integrators; SolverMHD now derives its messenger from the strategy and static_asserts a match. Also drop the dead commented resistive branch and unused eta_/nu_ from MHDEquations: resistive fluxes are applied at runtime in godunov_fluxes and constrained transport, not via MHDEquations. Refs PHAREHUB#1233 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
c9c39f3 to
752f510
Compare
BaseMHDTimestepper abstracted the messenger type but TimeIntegrator and the concrete integrators hard-coded the default, pinning the call path to amr::MHDMessenger and breaking SolverMHD's generic Messenger parameter. Forward MessengerT through all integrators; SolverMHD now derives its messenger from the strategy and static_asserts a match. Also drop the dead commented resistive branch and unused eta_/nu_ from MHDEquations: resistive fluxes are applied at runtime in godunov_fluxes and constrained transport, not via MHDEquations. Refs PHAREHUB#1233 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
752f510 to
66cc3cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/numerics/ohm/ohm.hpp`:
- Around line 23-24: Update Ohm::E_Eq_() to short-circuit zero-resistivity terms
before invoking resistive_() or hyperresistive_(): use the existing resistive()
and hyperResistive() checks for eta and nu, returning zero or skipping each
corresponding computation while preserving nonzero-term behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: adccb127-3b9d-422f-a2ab-945bb52cbfca
📒 Files selected for processing (13)
src/amr/solvers/solver_mhd.hppsrc/amr/solvers/time_integrator/base_mhd_timestepper.hppsrc/amr/solvers/time_integrator/compute_fluxes.hppsrc/amr/solvers/time_integrator/euler_integrator.hppsrc/amr/solvers/time_integrator/ssprk4_5_integrator.hppsrc/amr/solvers/time_integrator/time_integrator.hppsrc/amr/solvers/time_integrator/tvdrk2_integrator.hppsrc/amr/solvers/time_integrator/tvdrk3_integrator.hppsrc/core/numerics/MHD_equations/MHD_equations.hppsrc/core/numerics/constrained_transport/upwind_constrained_transport.hppsrc/core/numerics/godunov_fluxes/godunov_fluxes.hppsrc/core/numerics/ohm/ohm.hppsrc/python3/mhd_defaults/mhd_defaults.hpp
🚧 Files skipped from review as they are similar to previous changes (12)
- src/python3/mhd_defaults/mhd_defaults.hpp
- src/amr/solvers/time_integrator/time_integrator.hpp
- src/core/numerics/constrained_transport/upwind_constrained_transport.hpp
- src/amr/solvers/time_integrator/euler_integrator.hpp
- src/core/numerics/MHD_equations/MHD_equations.hpp
- src/amr/solvers/time_integrator/base_mhd_timestepper.hpp
- src/amr/solvers/time_integrator/ssprk4_5_integrator.hpp
- src/amr/solvers/time_integrator/compute_fluxes.hpp
- src/amr/solvers/time_integrator/tvdrk2_integrator.hpp
- src/amr/solvers/solver_mhd.hpp
- src/core/numerics/godunov_fluxes/godunov_fluxes.hpp
- src/amr/solvers/time_integrator/tvdrk3_integrator.hpp
|
|
||
| namespace detail | ||
| { | ||
| inline MHDOpts::TimeIntegratorType parse_time_integrator_type(std::string s) |
There was a problem hiding this comment.
to define along with MHDOpts::TimeIntegratorType
|
|
||
| template<auto direction, size_t dim, bool HyperResistivity> | ||
| template<auto direction, size_t dim> | ||
| auto getGrow(int const nghosts) |
There was a problem hiding this comment.
rajouter un booléen pour remplacer HyperResistivity
|
|
||
| template<auto direction, size_t dim, bool HyperResistivity> | ||
| template<auto direction, size_t dim> | ||
| auto getGrow(int const nghosts) |
|
|
||
| // for energy ExB term | ||
| if constexpr (Resistivity || HyperResistivity) | ||
| if (resistivity_ || hyper_resistivity_) |
There was a problem hiding this comment.
if dans une boucle
| , eta_{eta} | ||
| , nu_{nu} | ||
| , hyper_mode_{hyper_mode} | ||
| , resistivity_{eta != 0.0} |
| double const nu; | ||
| HyperMode const hyper_mode; | ||
|
|
||
| bool resistive() const { return eta != 0.0; } |
| @@ -13,7 +13,7 @@ | |||
| namespace PHARE::core | |||
| UpwindConstrainedTransport(UpwindConstrainedTransportInfo const& info, GridLayout const& layout) | ||
| : Super{info} | ||
| , layout_{layout} | ||
| , resistivity_{info.resistive()} |
There was a problem hiding this comment.
booléen pas explicite, toujours avoir un nom type is...
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/numerics/godunov_fluxes/godunov_fluxes.hpp`:
- Around line 147-162: Update the `Ideal` branch and the resistive contribution
flow around `ct_state` so non-Hall runs with `eta` or `nu` enabled reconstruct
and save interface `Jt` and `rhot` before the contributions at the later reads
are applied. Add a tagged non-Hall resistive path using the existing
reconstruction/state-saving mechanisms, while preserving the current ideal
behavior when no resistive terms are active and the Hall-specific path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5e62e7b5-7859-4e0e-8459-0857bf492dd1
📒 Files selected for processing (5)
src/amr/solvers/time_integrator/compute_fluxes.hppsrc/core/numerics/constrained_transport/constrained_transport.hppsrc/core/numerics/constrained_transport/upwind_constrained_transport.hppsrc/core/numerics/godunov_fluxes/godunov_fluxes.hppsrc/core/numerics/ohm/ohm.hpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/numerics/constrained_transport/upwind_constrained_transport.hpp
- src/core/numerics/constrained_transport/constrained_transport.hpp
- src/amr/solvers/time_integrator/compute_fluxes.hpp
4e1bae2 to
5058ff5
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| {indices...}); | ||
| if constexpr (mustSaveBt) | ||
| { | ||
| auto const& [jL, jR] = Reconstructor_t::template center_reconstruct< |
- compute_fluxes: refresh J when constrained-transport resistivity/ hyper-resistivity is enabled, not just when fv_method's is - SSPRK4_5: register/allocate resources for its own ComputeFluxes instance (fvm_/ct_ were never set up) - time_integrator: add missing <string> include, make parse_time_integrator_type case-insensitive Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
BaseMHDTimestepper abstracted the messenger type but TimeIntegrator and the concrete integrators hard-coded the default, pinning the call path to amr::MHDMessenger and breaking SolverMHD's generic Messenger parameter. Forward MessengerT through all integrators; SolverMHD now derives its messenger from the strategy and static_asserts a match. Also drop the dead commented resistive branch and unused eta_/nu_ from MHDEquations: resistive fluxes are applied at runtime in godunov_fluxes and constrained transport, not via MHDEquations. Refs PHAREHUB#1233 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
8fdd8eb to
13f5d98
Compare
6781d4b to
14328d1
Compare
|
it's not clear to me how the time integrator changes related to the resistivity/hyper-resistivity changes could be two PRs? |
|
Two changes are independent, could be split indeed |
| return std::forward_as_tuple(jt_x, rhot_x, jt_y, rhot_y, jt_z, rhot_z); | ||
| } | ||
|
|
||
| VecField jt_x{"j_t_x", MHDQuantity::Vector::VecFlux_x}; |
There was a problem hiding this comment.
Explaining what these transverse field/vecfields are
There was a problem hiding this comment.
Would actually need you for that 😃
| return std::forward_as_tuple(bt_x, bt_y); | ||
| else if constexpr (dimension == 3) | ||
| return std::forward_as_tuple(bt_x, bt_y, bt_z); | ||
| bt_.emplace_back("b_t_x", MHDQuantity::Vector::VecFlux_x); |
| std::visit( | ||
| [&](auto isResistiveTag, auto isHyperResistiveTag, auto hyperModeTag) { | ||
| constexpr bool isResistive = decltype(isResistiveTag)::value; |
There was a problem hiding this comment.
Very nice, much simpler to use. Here is an extension by Claude that also accepts enums https://coliru.stacked-crooked.com/a/969b5615acb3bdd4 It builds on the introduced trait for enum-string conversions.
| // bool -> asBoolConstant; scoped enum -> asEnumConstant (needs an EnumTraits<T> | ||
| // specialization, see enum.hpp). Anything else is a programming error at the call site. | ||
| template<typename T> | ||
| auto toConstexprVariant(T const& value) |
There was a problem hiding this comment.
I would think this should be two functions
auto toConstexprVariant(bool const value){}
template<typename T>
requires(std::is_enum_v<T>)
auto toConstexprVariant(T const& value){}
this way, other implementations could exist as needed without conflicts
There was a problem hiding this comment.
In this case, should toConstexprVariant overloads replace the definitions of toBoolConstant/toEnumConstant ? Or do we keep these last two and declare overloads in meta_utilities.hpp as
namespace detail
{
template<typename Bool>
auto toConstexprVariant(Bool const& value)
requires(std::is_same_v<Bool, bool>)
{
return asBoolConstant(value);
}
template<typename Enum>
auto toConstexprVariant(Enum const& value)
requires(std::is_enum_v<Enum>)
{
return asEnumConstant(value);
}
template<typename T>
constexpr bool always_false_v = false;
template<typename T>
auto toConstexprVariant(T const&)
{
static_assert(always_false_v<T>,
"Constexprifier only supports bool and scoped enum types");
}
} // namespace detail
There was a problem hiding this comment.
template<typename Bool>
auto toConstexprVariant(Bool const& value)
requires(std::is_same_v<Bool, bool>)
I don't know why you would do this when you can just let the type system work and set the parameter as a bool
template<typename T>
constexpr bool always_false_v = false;
template<typename T>
auto toConstexprVariant(T const&)
{
static_assert(always_false_v<T>,
"Constexprifier only supports bool and scoped enum types");
}
this reintroduces the problem which I said we could avoid
There was a problem hiding this comment.
this function
inline auto toConstexprVariant(bool const value) { return asBoolConstant(value); }
will accept integers and converts them to bools, this was to avoid that
ok for the second point
There was a problem hiding this comment.
ok I see, that's annoying, we typically expect Werrors for narrowing, but this one doesn't seem to count
|
A possibly simpler approach than needing all this enum to/from string magic is to expose the enum to python, pretty sure you get it for free there, so you could convert the python string, to a direct C++ runtime enum value not sure it helps converting it to constexpr tho, not sure exactly you need any strings for that either, probably just ints |
|
Yup but this does not spare the manual declaration of string names, as your example shows. And not a big deal, but you loose the ability to print meaningful names on the cpp side. For constexprification, it it enough to assume the enum includes a count member at its end to define a generic On this, I would let you decide |
This isn't great either as the count will increase the total set of possible parameter permutations |
|
So what do we do regarding enum <-> string capability ? Otherwise I addressed all review comments |
Enums are not strings, they are ints, so you should be passing integers from python to C++ |
5 participants
Summary
Demotes three MHD compile-time template parameters to runtime selection
Resistivity/HyperResistivity→ runtimebools. They become runtime params initialized from the value ofeta/nu. No runtime if introduced in compute loops.2
TimeIntegratorType→ runtime selection Virtual dispatch replaces the integrator selection:TimeIntegratorholds astd::unique_ptr<BaseMHDTimestepper>chosen by a factory; RK intermediate states move into the base asextra_states_exposed viagetRunTimeResourcesViewList. A generic mechanism is introduced forenumfrom/tostringconversions.