Open
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:
📝 WalkthroughWalkthroughRemoved solver-side per-patch model-view abstraction; solvers and the integrator now operate directly on physical model objects. Added patch-aware field-evolver transformers and indexed patch iteration in ResourcesManager; deleted the PPC model-view layer and updated tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Integrator as Integrator
participant Solver as Solver
participant Model as IPhysicalModel
participant RM as ResourcesManager
participant Patch as Patch
Integrator->>Solver: advanceLevel(hierarchy, level, Model, fromCoarser, t0, t1)
Solver->>Model: access state & resourcesManager
Solver->>RM: enumerate(level, quantities...)
RM->>Patch: provide Patch (RAII sets context)
Solver->>Patch: layoutFromPatch -> bind layout -> call core operator
Patch-->>RM: Patch destructor unsets context
Solver-->>Integrator: return
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (4)
src/amr/solvers/solver_field_evolvers.hpp (2)
1-2: Update the include guard to the new header path.The guard still uses the old
solver_ppc_model_view.hppname, which makes collisions and grep/debugging harder. Please switch it to the repository’s full-path guard forsrc/amr/solvers/solver_field_evolvers.hpp.Based on learnings, PhilipDeegan prefers header guard names that include the full directory path structure, following the pattern `PHARE_[PATH_WITH_UNDERSCORES]_HPP`.♻️ Suggested guard update
-#ifndef PHARE_SOLVER_SOLVER_PPC_MODEL_VIEW_HPP -#define PHARE_SOLVER_SOLVER_PPC_MODEL_VIEW_HPP +#ifndef PHARE_AMR_SOLVERS_SOLVER_FIELD_EVOLVERS_HPP +#define PHARE_AMR_SOLVERS_SOLVER_FIELD_EVOLVERS_HPP ... -#endif /* PHARE_SOLVER_SOLVER_PPC_MODEL_VIEW_HPP */ +#endif /* PHARE_AMR_SOLVERS_SOLVER_FIELD_EVOLVERS_HPP */Also applies to: 119-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/amr/solvers/solver_field_evolvers.hpp` around lines 1 - 2, The header guard in solver_field_evolvers.hpp still uses the old name PHARE_SOLVER_SOLVER_PPC_MODEL_VIEW_HPP; replace both occurrences of that guard (the `#ifndef` and matching `#define`) with the full-path style guard PHARE_SRC_AMR_SOLVERS_SOLVER_FIELD_EVOLVERS_HPP and update the trailing `#endif` comment if present to match the new guard; look for the existing guard around the top of the file to change the symbols consistently.
28-36: Avoid rebinding the whole model state in these field-only loops.
B/E/BnewandB/Jare already passed in. Addingstatetorm.enumerate(...)makes the resources manager walk and bind the full hybrid state on every patch in two hot paths, including data these transforms never touch.♻️ Trim the bound resources to what each transform actually uses
- auto& rm = *model.resourcesManager; - auto& state = model.state; - for (auto& patch : rm.enumerate(level, state, B, E, Bnew)) + auto& rm = *model.resourcesManager; + for (auto& patch : rm.enumerate(level, B, E, Bnew)) { auto layout = amr::layoutFromPatch<GridLayout>(*patch); (*this)(layout, B, E, Bnew, dt); } ... - auto& rm = *model.resourcesManager; - auto& state = model.state; - for (auto& patch : rm.enumerate(level, state, B, J)) + auto& rm = *model.resourcesManager; + for (auto& patch : rm.enumerate(level, B, J)) { auto layout = amr::layoutFromPatch<GridLayout>(*patch); (*this)(layout, B, J); }Also applies to: 56-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/amr/solvers/solver_field_evolvers.hpp` around lines 28 - 36, The loop is binding the entire hybrid model state via rm.enumerate(level, state, B, E, Bnew), causing unnecessary work in field-only hot paths; change the enumerate call inside operator()(level_t& level, auto& model, auto& B, auto& E, auto& Bnew, auto& dt) to pass only the field resources required (B, E, Bnew) instead of model.state so the resources manager doesn't bind unused state. Locate rm.enumerate(...) in that operator() and replace the call with the enumerate overload that accepts only the field arguments (or otherwise forward just B, E, Bnew), and make the analogous change for the other operator() variant referenced around lines 56-64 so both hot paths avoid rebinding the full state.src/amr/solvers/solver_ppc.hpp (2)
341-342: Fail fast ifoldTime_was never primed for this level.
operator[]inserts0.0on a miss, so a brokenprepareStep()/reflux()ordering silently turns into a bogusdt. A lookup withfind()orat()keeps that invariant explicit.♻️ Make the missing-key path explicit
- auto dt = time - oldTime_[level.getLevelNumber()]; + auto const levelNumber = level.getLevelNumber(); + auto const oldTimeIt = oldTime_.find(levelNumber); + if (oldTimeIt == oldTime_.end()) + throw std::runtime_error("missing oldTime for level " + std::to_string(levelNumber)); + auto dt = time - oldTimeIt->second;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/amr/solvers/solver_ppc.hpp` around lines 341 - 342, The calculation of dt uses oldTime_[level.getLevelNumber()] which will silently insert 0.0 on a missing key; change this to explicitly check for the presence of oldTime_ before computing dt (use oldTime_.find(level.getLevelNumber()) or oldTime_.at(...) with try/catch) and fail fast (throw an error or assert/log and return) when the entry is missing so broken prepareStep()/reflux() ordering doesn't produce a bogus dt; update the code around the dt computation and faraday_ call to perform the presence check and clear error path.
156-163:TimeSetteris paying for patch binding it never uses.
ResourcesManager::setTime(...)already updates patch-data metadata by ID, sorm.enumerate(level, quantities...)is just extra set/unset work in every predictor/corrector path. Iterating the level directly is cheaper and keeps particle-heavy calls likesetTime(ions)off the binding path.♻️ Iterate the patches directly here
void operator()(auto&... quantities) { - auto& rm = *model.resourcesManager; - for (auto& patch : rm.enumerate(level, quantities...)) + for (auto& patch : level) (model.resourcesManager->setTime(quantities, *patch, newTime), ...); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/amr/solvers/solver_ppc.hpp` around lines 156 - 163, TimeSetter::operator() currently forces ResourcesManager::enumerate(level, quantities...) which binds patches unnecessarily; replace that with a direct iteration over the level's patch IDs and call ResourcesManager::setTime(...) per patch ID so you avoid enumerate's binding cost. Specifically, remove rm.enumerate(...) and instead iterate the level's patch container (use the same container accessed via model or model.resourcesManager that holds patch IDs for the given level), and call model.resourcesManager->setTime(quantities, patchID, newTime) for each patchID; keep TimeSetter, operator(), model, level, quantities and setTime references so the change is minimal and only removes enumerate usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/amr/solvers/solver_field_evolvers.hpp`:
- Around line 1-2: The header guard in solver_field_evolvers.hpp still uses the
old name PHARE_SOLVER_SOLVER_PPC_MODEL_VIEW_HPP; replace both occurrences of
that guard (the `#ifndef` and matching `#define`) with the full-path style guard
PHARE_SRC_AMR_SOLVERS_SOLVER_FIELD_EVOLVERS_HPP and update the trailing `#endif`
comment if present to match the new guard; look for the existing guard around
the top of the file to change the symbols consistently.
- Around line 28-36: The loop is binding the entire hybrid model state via
rm.enumerate(level, state, B, E, Bnew), causing unnecessary work in field-only
hot paths; change the enumerate call inside operator()(level_t& level, auto&
model, auto& B, auto& E, auto& Bnew, auto& dt) to pass only the field resources
required (B, E, Bnew) instead of model.state so the resources manager doesn't
bind unused state. Locate rm.enumerate(...) in that operator() and replace the
call with the enumerate overload that accepts only the field arguments (or
otherwise forward just B, E, Bnew), and make the analogous change for the other
operator() variant referenced around lines 56-64 so both hot paths avoid
rebinding the full state.
In `@src/amr/solvers/solver_ppc.hpp`:
- Around line 341-342: The calculation of dt uses
oldTime_[level.getLevelNumber()] which will silently insert 0.0 on a missing
key; change this to explicitly check for the presence of oldTime_ before
computing dt (use oldTime_.find(level.getLevelNumber()) or oldTime_.at(...) with
try/catch) and fail fast (throw an error or assert/log and return) when the
entry is missing so broken prepareStep()/reflux() ordering doesn't produce a
bogus dt; update the code around the dt computation and faraday_ call to perform
the presence check and clear error path.
- Around line 156-163: TimeSetter::operator() currently forces
ResourcesManager::enumerate(level, quantities...) which binds patches
unnecessarily; replace that with a direct iteration over the level's patch IDs
and call ResourcesManager::setTime(...) per patch ID so you avoid enumerate's
binding cost. Specifically, remove rm.enumerate(...) and instead iterate the
level's patch container (use the same container accessed via model or
model.resourcesManager that holds patch IDs for the given level), and call
model.resourcesManager->setTime(quantities, patchID, newTime) for each patchID;
keep TimeSetter, operator(), model, level, quantities and setTime references so
the change is minimal and only removes enumerate usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2c81a206-9a12-47ad-9150-7b8fa92c0fc0
📒 Files selected for processing (7)
src/amr/multiphysics_integrator.hppsrc/amr/resources_manager/resources_manager.hppsrc/amr/solvers/solver.hppsrc/amr/solvers/solver_field_evolvers.hppsrc/amr/solvers/solver_mhd.hppsrc/amr/solvers/solver_ppc.hppsrc/amr/solvers/solver_ppc_model_view.hpp
💤 Files with no reviewable changes (1)
- src/amr/solvers/solver_ppc_model_view.hpp
PhilipDeegan
force-pushed
the
drop_model_view
branch
from
8947111 to
2b1a557
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/amr/solvers/solver.hpp (1)
97-100: Minor inconsistency in messenger type reference.Line 98 uses the full type
amr::IMessenger<IPhysicalModel<AMR_Types>>&while line 107 uses the aliasamr::IMessenger<IPhysicalModel_t>&. For consistency withadvanceLeveland derived class implementations (e.g.,SolverMHD::reflux), consider using the alias here as well.♻️ Suggested fix for consistency
- virtual void reflux(IPhysicalModel_t& model, level_t& level, - amr::IMessenger<IPhysicalModel<AMR_Types>>& messenger, - double const time) + virtual void reflux(IPhysicalModel_t& model, level_t& level, + amr::IMessenger<IPhysicalModel_t>& messenger, + double const time)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/amr/solvers/solver.hpp` around lines 97 - 100, The function declaration for reflux uses the fully-qualified messenger type amr::IMessenger<IPhysicalModel<AMR_Types>>&; change it to use the existing alias amr::IMessenger<IPhysicalModel_t>& for consistency with advanceLevel and derived implementations (e.g., SolverMHD::reflux) by updating the reflux signature to accept amr::IMessenger<IPhysicalModel_t>& while keeping the other parameters (IPhysicalModel_t&, level_t&, double const time) unchanged.src/amr/solvers/solver_field_evolvers.hpp (1)
89-89: Use theelectronsargument consistently inOhmTransformer.This overload enumerates with
electronsbut computesn/Ve/Pefromstate.electrons. That weakens API clarity and can desync behavior if a non-default electrons object is passed.♻️ Proposed refactor
void operator()(level_t& level, auto& model, auto& electrons, auto& B, auto& J, auto& E) { - auto& state = model.state; auto& rm = *model.resourcesManager; + auto& n = electrons.density(); + auto& Ve = electrons.velocity(); + auto& Pe = electrons.pressure(); for (auto& patch : rm.enumerate(level, electrons, B, J, E)) { auto layout = amr::layoutFromPatch<GridLayout>(*patch); - auto& n = state.electrons.density(); - auto& Ve = state.electrons.velocity(); - auto& Pe = state.electrons.pressure(); (*this)(layout, n, Ve, Pe, B, J, E); } }Based on learnings: PhilipDeegan has indicated a preference for minimal and efficient code.
Also applies to: 96-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/amr/solvers/solver_field_evolvers.hpp` at line 89, The operator() overload in OhmTransformer takes an electrons parameter but still reads from state.electrons, which breaks API clarity and can desync behavior; update the implementation of OhmTransformer::operator()(level_t& level, auto& model, auto& electrons, auto& B, auto& J, auto& E) (and the similar overloads around lines 96–99) to consistently use the passed electrons argument when computing n, Ve, Pe (instead of state.electrons), ensuring all references, calculations, and any helper calls inside OhmTransformer that currently access state.electrons are switched to the local electrons parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/amr/resources_manager/test_resources_manager.cpp`:
- Line 237: Rename the test identifier hasPointersValidWithBrackerOperator to
hasPointersValidWithBracketOperator everywhere it appears (the TYPED_TEST_P
declaration and its corresponding registration/usage) so the test name reflects
"Bracket"; update the other matching occurrence in the file (the later reference
at the test registration/usage site) to the new name to keep declarations and
references consistent.
---
Nitpick comments:
In `@src/amr/solvers/solver_field_evolvers.hpp`:
- Line 89: The operator() overload in OhmTransformer takes an electrons
parameter but still reads from state.electrons, which breaks API clarity and can
desync behavior; update the implementation of
OhmTransformer::operator()(level_t& level, auto& model, auto& electrons, auto&
B, auto& J, auto& E) (and the similar overloads around lines 96–99) to
consistently use the passed electrons argument when computing n, Ve, Pe (instead
of state.electrons), ensuring all references, calculations, and any helper calls
inside OhmTransformer that currently access state.electrons are switched to the
local electrons parameter.
In `@src/amr/solvers/solver.hpp`:
- Around line 97-100: The function declaration for reflux uses the
fully-qualified messenger type amr::IMessenger<IPhysicalModel<AMR_Types>>&;
change it to use the existing alias amr::IMessenger<IPhysicalModel_t>& for
consistency with advanceLevel and derived implementations (e.g.,
SolverMHD::reflux) by updating the reflux signature to accept
amr::IMessenger<IPhysicalModel_t>& while keeping the other parameters
(IPhysicalModel_t&, level_t&, double const time) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 464daaa2-de16-4d2b-a6c5-33d681405f7a
📒 Files selected for processing (8)
src/amr/multiphysics_integrator.hppsrc/amr/resources_manager/resources_manager.hppsrc/amr/solvers/solver.hppsrc/amr/solvers/solver_field_evolvers.hppsrc/amr/solvers/solver_mhd.hppsrc/amr/solvers/solver_ppc.hppsrc/amr/solvers/solver_ppc_model_view.hpptests/amr/resources_manager/test_resources_manager.cpp
💤 Files with no reviewable changes (1)
- src/amr/solvers/solver_ppc_model_view.hpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/amr/resources_manager/resources_manager.hpp
- src/amr/solvers/solver_mhd.hpp
- src/amr/solvers/solver_ppc.hpp
PhilipDeegan
force-pushed
the
drop_model_view
branch
from
2b1a557 to
3737878
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/amr/solvers/solver_ppc.hpp (1)
514-535:⚠️ Potential issue | 🟡 MinorUnused loop variable
patchin debug function.The outer loop at line 520 iterates over patches via
rm.enumerate(level, model.state.ions), but thepatchvariable is never used inside the loop body. The inner loop (line 522) iterates overmodel.state.ionspopulations without referencing the current patch, which means it's counting the same populations repeatedly for each patch.This appears to be a logic error where the particle counts are accumulated once per patch rather than once total.
Proposed fix if counting should happen once per patch
void _debug_log_move_ions(auto& level, auto& model) { auto& rm = *model.resourcesManager; - std::size_t nbrLevelGhostOldParticles = 0; - std::size_t nbrLevelGhostParticles = 0; for (auto& patch : rm.enumerate(level, model.state.ions)) { + std::size_t nbrLevelGhostOldParticles = 0; + std::size_t nbrLevelGhostParticles = 0; for (auto& pop : model.state.ions) {Or if the enumerate is only needed to set resources:
Alternative: iterate once after setting resources
void _debug_log_move_ions(auto& level, auto& model) { auto& rm = *model.resourcesManager; std::size_t nbrLevelGhostOldParticles = 0; std::size_t nbrLevelGhostParticles = 0; - for (auto& patch : rm.enumerate(level, model.state.ions)) - { - for (auto& pop : model.state.ions) + for (auto& patch : level) + { + auto _ = rm.setOnPatch(*patch, model.state.ions); + for (auto& pop : model.state.ions)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/amr/solvers/solver_ppc.hpp` around lines 514 - 535, _debug_log_move_ions currently iterates rm.enumerate(level, model.state.ions) into a unused patch variable causing model.state.ions to be counted once per patch; either remove the outer loop and perform the inner loop over model.state.ions a single time (keep nbrLevelGhostOldParticles and nbrLevelGhostParticles accumulation in _debug_log_move_ions), or if the intent was per-patch counts then use the patch object returned by rm.enumerate (instead of model.state.ions) to iterate that patch’s populations when accumulating nbrLevelGhostOldParticles and nbrLevelGhostParticles; update the code around rm.enumerate, the patch variable usage, and the loops inside _debug_log_move_ions accordingly.
🧹 Nitpick comments (2)
src/amr/solvers/solver_ppc.hpp (2)
574-575: Consider marking unusedpatchvariable.The
patchvariable from the enumeration is unused inside the loop body sinceionUpdater_.updateIons(ions)operates on the ions whose resources were set by the enumerate. Consider using[[maybe_unused]]or a discard pattern to suppress potential compiler warnings:Suggested improvement
- for (auto& patch : rm.enumerate(level, ions)) - ionUpdater_.updateIons(ions); + for ([[maybe_unused]] auto& patch : rm.enumerate(level, ions)) + ionUpdater_.updateIons(ions);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/amr/solvers/solver_ppc.hpp` around lines 574 - 575, The loop declares an unused loop variable `patch` when iterating `rm.enumerate(level, ions)` and triggers compiler warnings; update the loop in `solver_ppc.hpp` (the `for (auto& patch : rm.enumerate(level, ions))` line) to indicate the variable is intentionally unused—either change the loop to discard the value (e.g., use an unnamed/underscore pattern or `(void)patch`), or annotate it with `[[maybe_unused]]`—so `ionUpdater_.updateIons(ions)` remains unchanged and warnings are suppressed.
358-360: Thedynamic_castrelies on name-based model compatibility.While the solver registration ensures the model name matches
HybridModel::model_name, thedynamic_castcould still throwstd::bad_castif there's a type mismatch. This is an existing pattern throughout the solver methods, but worth noting that it relies on the naming convention being correctly maintained.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/amr/solvers/solver_ppc.hpp` around lines 358 - 360, The current code uses dynamic_cast<HybridModel&>(level_model) and dynamic_cast<HybridMessenger&>(fromCoarserMessenger) which will throw std::bad_cast on mismatch; change these to pointer casts (auto modelPtr = dynamic_cast<HybridModel*>(&level_model); auto messengerPtr = dynamic_cast<HybridMessenger*>(&fromCoarserMessenger)) and add explicit null checks that produce a clear error/log (including HybridModel::model_name and the offending object's runtime type) and either return/throw a controlled exception; keep the call to setup_level(hierarchy, levelNumber) unchanged but use modelPtr/messengerPtr thereafter (dereference after null-check).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/amr/resources_manager/resources_manager.hpp`:
- Around line 356-371: The Patch wrapper stores a reference to a shared_ptr
(member declared as std::shared_ptr<SAMRAI::hier::Patch> const& patch) which can
dangle if the caller passed a temporary (see Patch constructor and member).
Change the Patch struct to own the shared_ptr by value
(std::shared_ptr<SAMRAI::hier::Patch> patch), copy the incoming shared_ptr in
the constructor initializer (instead of binding a reference), and keep the
existing looper usage (looper->set(*patch), operator*() and destructor
looper->unset()) so the patch lifetime is safely managed.
---
Outside diff comments:
In `@src/amr/solvers/solver_ppc.hpp`:
- Around line 514-535: _debug_log_move_ions currently iterates
rm.enumerate(level, model.state.ions) into a unused patch variable causing
model.state.ions to be counted once per patch; either remove the outer loop and
perform the inner loop over model.state.ions a single time (keep
nbrLevelGhostOldParticles and nbrLevelGhostParticles accumulation in
_debug_log_move_ions), or if the intent was per-patch counts then use the patch
object returned by rm.enumerate (instead of model.state.ions) to iterate that
patch’s populations when accumulating nbrLevelGhostOldParticles and
nbrLevelGhostParticles; update the code around rm.enumerate, the patch variable
usage, and the loops inside _debug_log_move_ions accordingly.
---
Nitpick comments:
In `@src/amr/solvers/solver_ppc.hpp`:
- Around line 574-575: The loop declares an unused loop variable `patch` when
iterating `rm.enumerate(level, ions)` and triggers compiler warnings; update the
loop in `solver_ppc.hpp` (the `for (auto& patch : rm.enumerate(level, ions))`
line) to indicate the variable is intentionally unused—either change the loop to
discard the value (e.g., use an unnamed/underscore pattern or `(void)patch`), or
annotate it with `[[maybe_unused]]`—so `ionUpdater_.updateIons(ions)` remains
unchanged and warnings are suppressed.
- Around line 358-360: The current code uses
dynamic_cast<HybridModel&>(level_model) and
dynamic_cast<HybridMessenger&>(fromCoarserMessenger) which will throw
std::bad_cast on mismatch; change these to pointer casts (auto modelPtr =
dynamic_cast<HybridModel*>(&level_model); auto messengerPtr =
dynamic_cast<HybridMessenger*>(&fromCoarserMessenger)) and add explicit null
checks that produce a clear error/log (including HybridModel::model_name and the
offending object's runtime type) and either return/throw a controlled exception;
keep the call to setup_level(hierarchy, levelNumber) unchanged but use
modelPtr/messengerPtr thereafter (dereference after null-check).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 81095e43-b964-426f-ad51-63b2c74b8915
📒 Files selected for processing (8)
src/amr/multiphysics_integrator.hppsrc/amr/resources_manager/resources_manager.hppsrc/amr/solvers/solver.hppsrc/amr/solvers/solver_field_evolvers.hppsrc/amr/solvers/solver_mhd.hppsrc/amr/solvers/solver_ppc.hppsrc/amr/solvers/solver_ppc_model_view.hpptests/amr/resources_manager/test_resources_manager.cpp
💤 Files with no reviewable changes (1)
- src/amr/solvers/solver_ppc_model_view.hpp
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/amr/resources_manager/test_resources_manager.cpp
- src/amr/solvers/solver_mhd.hpp
- src/amr/solvers/solver_field_evolvers.hpp
PhilipDeegan
force-pushed
the
drop_model_view
branch
2 times, most recently
from
c20530b to
187d6f2
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/amr/resources_manager/test_resources_manager.cpp (1)
237-262:⚠️ Potential issue | 🟡 MinorTypo in test name: "Bracker" should be "Bracket".
The test name
hasPointersValidWithBrackerOperatorcontains a typo. It should behasPointersValidWithBracketOperatorto correctly describe that it tests theoperator[]functionality.Also update line 298 to match.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/amr/resources_manager/test_resources_manager.cpp` around lines 237 - 262, Rename the typed test identifier hasPointersValidWithBrackerOperator to hasPointersValidWithBracketOperator in the TYPED_TEST_P declaration (TYPED_TEST_P(aResourceUserCollection, hasPointersValidWithBrackerOperator)) and update any other occurrences/registrations or references (e.g., test suite instantiation or EXPECT/INVOKE names) to the corrected hasPointersValidWithBracketOperator so the test name consistently reflects operator[].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/amr/resources_manager/test_resources_manager.cpp`:
- Around line 237-262: Rename the typed test identifier
hasPointersValidWithBrackerOperator to hasPointersValidWithBracketOperator in
the TYPED_TEST_P declaration (TYPED_TEST_P(aResourceUserCollection,
hasPointersValidWithBrackerOperator)) and update any other
occurrences/registrations or references (e.g., test suite instantiation or
EXPECT/INVOKE names) to the corrected hasPointersValidWithBracketOperator so the
test name consistently reflects operator[].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b34eb725-57e8-44d4-9521-cb9083d02980
📒 Files selected for processing (8)
src/amr/multiphysics_integrator.hppsrc/amr/resources_manager/resources_manager.hppsrc/amr/solvers/solver.hppsrc/amr/solvers/solver_field_evolvers.hppsrc/amr/solvers/solver_mhd.hppsrc/amr/solvers/solver_ppc.hppsrc/amr/solvers/solver_ppc_model_view.hpptests/amr/resources_manager/test_resources_manager.cpp
💤 Files with no reviewable changes (1)
- src/amr/solvers/solver_ppc_model_view.hpp
🚧 Files skipped from review as they are similar to previous changes (4)
- src/amr/solvers/solver_mhd.hpp
- src/amr/resources_manager/resources_manager.hpp
- src/amr/solvers/solver_field_evolvers.hpp
- src/amr/solvers/solver_ppc.hpp
PhilipDeegan
force-pushed
the
drop_model_view
branch
from
187d6f2 to
8b4326b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/amr/solvers/solver_field_evolvers.hpp (1)
89-101: Consider documenting the per-patch electron extraction pattern.The extraction of
n,Ve,Pefromelectronsinside the loop (lines 96-98) differs from the pattern inFaradayTransformerandAmpereTransformerwhere fields are passed directly. This is correct sinceenumeratelikely sets up patch-local resource bindings before each iteration.A brief comment explaining this dependency on the RAII resource binding from
enumeratewould improve clarity for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/amr/solvers/solver_field_evolvers.hpp` around lines 89 - 101, Add a brief comment explaining that rm.enumerate(level, electrons, B, J, E) establishes RAII-style, patch-local resource bindings so that accessing per-patch fields via electrons.density(), electrons.velocity(), and electrons.pressure() inside the loop is valid; place this comment just before the for-loop (or immediately before the extraction of n, Ve, Pe) in operator()(level_t&, auto&, auto&, auto&, auto&) in solver_field_evolvers.hpp and reference rm.enumerate, amr::layoutFromPatch<GridLayout>, and the FaradayTransformer/AmpereTransformer pattern to clarify the difference in access style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/amr/solvers/solver_field_evolvers.hpp`:
- Around line 89-101: Add a brief comment explaining that rm.enumerate(level,
electrons, B, J, E) establishes RAII-style, patch-local resource bindings so
that accessing per-patch fields via electrons.density(), electrons.velocity(),
and electrons.pressure() inside the loop is valid; place this comment just
before the for-loop (or immediately before the extraction of n, Ve, Pe) in
operator()(level_t&, auto&, auto&, auto&, auto&) in solver_field_evolvers.hpp
and reference rm.enumerate, amr::layoutFromPatch<GridLayout>, and the
FaradayTransformer/AmpereTransformer pattern to clarify the difference in access
style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a89784c7-7c34-473f-9176-772a6dbf7b1c
📒 Files selected for processing (8)
src/amr/multiphysics_integrator.hppsrc/amr/resources_manager/resources_manager.hppsrc/amr/solvers/solver.hppsrc/amr/solvers/solver_field_evolvers.hppsrc/amr/solvers/solver_mhd.hppsrc/amr/solvers/solver_ppc.hppsrc/amr/solvers/solver_ppc_model_view.hpptests/amr/resources_manager/test_resources_manager.cpp
💤 Files with no reviewable changes (1)
- src/amr/solvers/solver_ppc_model_view.hpp
✅ Files skipped from review due to trivial changes (2)
- src/amr/multiphysics_integrator.hpp
- src/amr/solvers/solver_ppc.hpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/amr/resources_manager/resources_manager.hpp
- tests/amr/resources_manager/test_resources_manager.cpp
- src/amr/solvers/solver_mhd.hpp
PhilipDeegan
force-pushed
the
drop_model_view
branch
from
8b4326b to
aaf417f
Compare
PhilipDeegan
force-pushed
the
drop_model_view
branch
from
aaf417f to
b5faac9
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| TimeSetter setTime{level, model, newTime}; | ||
| auto& rm = *model.resourcesManager; | ||
| auto& electromag = model.state.electromag; | ||
| for (auto& _ : rm.enumerate(level, electromag, electromagPred_, electromagAvg_)) |
Check notice
Code scanning / CodeQL
Unused local variable Note
|
|
||
| for (auto& state : views) | ||
| ionUpdater_.updateIons(state.ions); | ||
| for (auto& patch : rm.enumerate(level, ions)) |
Check notice
Code scanning / CodeQL
Unused local variable Note
| std::apply(check, resourceUserCollection); | ||
| } | ||
|
|
||
| TYPED_TEST_P(aResourceUserCollection, hasPointersValidWithBracketOperator) |
Check notice
Code scanning / CodeQL
Unused static variable Note test
|
|
||
| for (std::size_t i = 0; i < level_looper.size(); ++i) | ||
| { | ||
| auto const& _ = level_looper[i]; |
Check notice
Code scanning / CodeQL
Unused local variable Note test
Comment on lines
+2
to
+3
| // #include "phare_core.hpp" | ||
| // #include "core/data/field/field.hpp" |
Check notice
Code scanning / CodeQL
Commented-out code Note test
| #include "core/data/grid/gridlayout_impl.hpp" | ||
| #include "core/data/grid/gridlayoutdefs.hpp" | ||
| #include "core/data/vecfield/vecfield.hpp" | ||
| // #include "core/data/vecfield/vecfield.hpp" |
Check notice
Code scanning / CodeQL
Commented-out code Note test
| // #include "core/data/vecfield/vecfield.hpp" | ||
| #include "core/numerics/ohm/ohm.hpp" | ||
| #include "core/utilities/index/index.hpp" | ||
| // #include "core/utilities/index/index.hpp" |
Check notice
Code scanning / CodeQL
Commented-out code Note test
PhilipDeegan
force-pushed
the
drop_model_view
branch
from
2a5b108 to
5ec2a93
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/amr/physical_models/physical_model.hpp (1)
87-91: Don't bind/unbind the quantities just to update times.
ResourcesManager::setTime()already resolves IDs from the view, sorm.enumerate(level, quantities...)adds a full set/unset cycle on every patch for no functional gain. It also makesTimeSettermutateisUsable()/isSettable()state and can clear an outer patch guard unexpectedly.Proposed fix
void operator()(auto&... quantities) { auto& rm = *model.resourcesManager; - for (auto& patch : rm.enumerate(level, quantities...)) - (model.resourcesManager->setTime(quantities, *patch, newTime), ...); + for (auto& patch : rm.enumerate(level)) + (rm.setTime(quantities, *patch, newTime), ...); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/amr/physical_models/physical_model.hpp` around lines 87 - 91, The TimeSetter::operator()(auto&... quantities) currently calls rm.enumerate(level, quantities...) which binds/unbinds the views and can mutate isUsable()/isSettable() or clear patch guards; change it to avoid enumerating with quantities: obtain the patch ids for the target level without passing the quantity views (e.g. use an API that returns patch ids or enumerate patches by level only), then call model.resourcesManager->setTime(quantities, patchId, newTime) for each patch id (or call setTime per quantity and patch id) so you update times without binding/unbinding the quantities; remove the rm.enumerate(level, quantities...) usage and any code that relies on its side effects, keeping the call sites referencing operator(), model.resourcesManager and ResourcesManager::setTime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/amr/level_initializer/hybrid_level_initializer.hpp`:
- Around line 155-171: The TimeSetter and ghost-fill calls are hard-coded with
0.; instead propagate the actual initialization time by using initDataTime:
construct TimeSetter setTime{level, hybridModel, initDataTime} and pass
initDataTime to hybMessenger.fillCurrentGhosts(J, level, initDataTime) and
hybMessenger.fillElectricGhosts(E, level, initDataTime); keep the ampere_ and
ohm_ calls and the setTime(J)/setTime(E) calls but ensure they use the
TimeSetter that was initialized with initDataTime.
In `@src/amr/resources_manager/resources_manager.hpp`:
- Around line 356-371: The Patch RAII guard struct allows copying which leads to
double-unset of LevelLooper; make Patch non-copyable and non-movable by deleting
its copy constructor, copy assignment, move constructor and move assignment so
it has exclusive ownership of LevelLooper::set()/unset(); update the declaration
of struct Patch (and keep existing constructor, operator* and destructor) to
explicitly delete Patch(const Patch&), Patch& operator=(const Patch&),
Patch(Patch&&) and Patch& operator=(Patch&&) to prevent accidental copies or
moves of looper/patch.
In `@src/core/numerics/ohm/ohm.hpp`:
- Around line 23-29: The FROM factory silently treats any hyper_mode value other
than "constant" as HyperMode::spatial; change it to validate the string from
cppdict::get_value(dict, "hyper_mode", std::string{"constant"}) and explicitly
map "constant" -> HyperMode::constant and "spatial" -> HyperMode::spatial, and
throw or otherwise fail fast (e.g., throw std::invalid_argument) if the value is
neither, so the OhmInfo::FROM initializer fails on typos instead of silently
changing behavior.
---
Nitpick comments:
In `@src/amr/physical_models/physical_model.hpp`:
- Around line 87-91: The TimeSetter::operator()(auto&... quantities) currently
calls rm.enumerate(level, quantities...) which binds/unbinds the views and can
mutate isUsable()/isSettable() or clear patch guards; change it to avoid
enumerating with quantities: obtain the patch ids for the target level without
passing the quantity views (e.g. use an API that returns patch ids or enumerate
patches by level only), then call model.resourcesManager->setTime(quantities,
patchId, newTime) for each patch id (or call setTime per quantity and patch id)
so you update times without binding/unbinding the quantities; remove the
rm.enumerate(level, quantities...) usage and any code that relies on its side
effects, keeping the call sites referencing operator(), model.resourcesManager
and ResourcesManager::setTime.
🪄 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: 4c361ed1-4d60-4222-bd91-46ed0cf89bc9
📒 Files selected for processing (22)
src/amr/level_initializer/hybrid_level_initializer.hppsrc/amr/multiphysics_integrator.hppsrc/amr/physical_models/hybrid_model.hppsrc/amr/physical_models/physical_model.hppsrc/amr/resources_manager/resources_manager.hppsrc/amr/solvers/solver.hppsrc/amr/solvers/solver_field_evolvers.hppsrc/amr/solvers/solver_mhd.hppsrc/amr/solvers/solver_ppc.hppsrc/amr/solvers/solver_ppc_model_view.hppsrc/core/CMakeLists.txtsrc/core/data/electrons/electrons.hppsrc/core/data/grid/gridlayout_utils.hppsrc/core/numerics/ampere/ampere.hppsrc/core/numerics/faraday/faraday.hppsrc/core/numerics/ohm/ohm.hpptests/amr/resources_manager/test_resources_manager.cpptests/core/data/field/test_field_fixtures.hpptests/core/data/vecfield/test_vecfield_fixtures.hpptests/core/numerics/ampere/test_main.cpptests/core/numerics/faraday/test_main.cpptests/core/numerics/ohm/test_main.cpp
💤 Files with no reviewable changes (3)
- src/core/CMakeLists.txt
- src/core/data/grid/gridlayout_utils.hpp
- src/amr/solvers/solver_ppc_model_view.hpp
✅ Files skipped from review due to trivial changes (4)
- tests/core/data/vecfield/test_vecfield_fixtures.hpp
- tests/core/data/field/test_field_fixtures.hpp
- src/amr/physical_models/hybrid_model.hpp
- src/amr/multiphysics_integrator.hpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/amr/solvers/solver_ppc.hpp
Comment on lines
+155
to
171
| TimeSetter setTime{level, hybridModel, 0.}; | ||
|
|
||
| auto& B = hybridModel.state.electromag.B; | ||
| auto& E = hybridModel.state.electromag.E; | ||
| auto& J = hybridModel.state.J; | ||
|
|
||
| for (auto& patch : rm.enumerate(level, B, J)) | ||
| { | ||
| auto layout = PHARE::amr::layoutFromPatch<GridLayoutT>(*patch); | ||
| auto __ = core::SetLayout(&layout, ampere_); | ||
| ampere_(B, J); | ||
|
|
||
| rm.setTime(J, *patch, 0.); | ||
| } | ||
| ampere_(level, hybridModel, B, J); | ||
| setTime(J); | ||
| hybMessenger.fillCurrentGhosts(J, level, 0.); | ||
|
|
||
| auto& electrons = hybridModel.state.electrons; | ||
| auto& E = hybridModel.state.electromag.E; | ||
|
|
||
| for (auto& patch : rm.enumerate(level, B, E, J, electrons)) | ||
| { | ||
| auto layout = PHARE::amr::layoutFromPatch<GridLayoutT>(*patch); | ||
| electrons.update(layout); | ||
| auto& Ve = electrons.velocity(); | ||
| auto& Ne = electrons.density(); | ||
| auto& Pe = electrons.pressure(); | ||
| auto __ = core::SetLayout(&layout, ohm_); | ||
| ohm_(Ne, Ve, Pe, B, J, E); | ||
| rm.setTime(E, *patch, 0.); | ||
| } | ||
| for (auto& patch : rm.enumerate(level, electrons)) | ||
| electrons.update(amr::layoutFromPatch<GridLayoutT>(*patch)); | ||
|
|
||
| ohm_(level, hybridModel, B, J, E); | ||
| setTime(E); | ||
| hybMessenger.fillElectricGhosts(E, level, 0.); |
There was a problem hiding this comment.
Propagate initDataTime instead of hard-coding 0..
Line 155 stamps the TimeSetter with 0., and Lines 163 and 171 use the same literal for the ghost fills. That regresses any root-level initialization performed at nonzero initDataTime by publishing J and E at the wrong time.
🩹 Minimal fix
- TimeSetter setTime{level, hybridModel, 0.};
+ TimeSetter setTime{level, hybridModel, initDataTime};
...
- hybMessenger.fillCurrentGhosts(J, level, 0.);
+ hybMessenger.fillCurrentGhosts(J, level, initDataTime);
...
- hybMessenger.fillElectricGhosts(E, level, 0.);
+ hybMessenger.fillElectricGhosts(E, level, initDataTime);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/amr/level_initializer/hybrid_level_initializer.hpp` around lines 155 -
171, The TimeSetter and ghost-fill calls are hard-coded with 0.; instead
propagate the actual initialization time by using initDataTime: construct
TimeSetter setTime{level, hybridModel, initDataTime} and pass initDataTime to
hybMessenger.fillCurrentGhosts(J, level, initDataTime) and
hybMessenger.fillElectricGhosts(E, level, initDataTime); keep the ampere_ and
ohm_ calls and the setTime(J)/setTime(E) calls but ensure they use the
TimeSetter that was initialized with initDataTime.
Member
Author
There was a problem hiding this comment.
was it already like that?
There was a problem hiding this comment.
@PhilipDeegan — yes, it was already like that. The removed lines in the diff already used rm.setTime(J, *patch, 0.) and rm.setTime(E, *patch, 0.), and the ghost-fill calls were unchanged context lines already passing 0.. This PR preserved the pre-existing behavior; it didn't introduce the hard-coding. My concern is about a pre-existing pattern rather than a regression from this PR. Apologies for the noise.
🧠 Learnings used
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-10-09T08:32:15.667Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-07-26T22:04:34.160Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1068
File: src/amr/data/field/coarsening/electric_field_coarsener.hpp:1-2
Timestamp: 2025-09-17T13:35:11.533Z
Learning: PhilipDeegan prefers header guard names that include the full directory path structure, following the pattern PHARE_[PATH_WITH_UNDERSCORES]_HPP. For example, a file at src/amr/data/field/coarsening/electric_field_coarsener.hpp should use PHARE_AMR_DATA_FIELD_COARSENING_ELECTRIC_FIELD_COARSENER_HPP as the header guard.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-09-05T17:02:58.784Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-10-09T08:32:15.667Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py:566-568
Timestamp: 2024-09-09T13:57:02.285Z
Learning: Avoid commenting on draft PRs for PhilipDeegan unless explicitly requested.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1158
File: src/amr/data/particles/refine/split_3d.hpp:66-87
Timestamp: 2026-02-25T13:09:17.840Z
Learning: In the PHARE codebase, for runtime particle-based operations, avoid lambdas with captures due to performance issues. While constexpr lambdas are equivalent to functors, runtime particle operations benefit from using explicit non-captured functor classes instead of lambdas with capture. Apply this guideline to C++ header files (e.g., src/.../*.hpp) involved in particle processing to ensure predictable inlining and performance.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1177
File: src/hdf5/detail/h5/h5_file.hpp:116-119
Timestamp: 2026-03-23T13:52:54.931Z
Learning: In PHAREHUB/PHARE, MPI initialization is the responsibility of the calling application—PHARE code should not call MPI_Init or MPI_Init_thread. For HDF5 subfiling paths that require MPI_THREAD_MULTIPLE, PHARE should rely on the MPI implementation’s configured thread support (e.g., OMPI_MPI_THREAD_LEVEL via environment variable, or an equivalent mechanism) rather than attempting to initialize MPI with MPI_Init_thread inside PHARE.
Comment on lines
+23
to
+29
| OhmInfo static FROM(initializer::PHAREDict const& dict) | ||
| { | ||
| return {dict["resistivity"].template to<double>(), | ||
| dict["hyper_resistivity"].template to<double>(), | ||
| cppdict::get_value(dict, "hyper_mode", std::string{"constant"}) == "constant" | ||
| ? HyperMode::constant | ||
| : HyperMode::spatial}; |
There was a problem hiding this comment.
Reject unknown hyper_mode values.
Line 27 currently maps every value other than "constant" to HyperMode::spatial, so a typo silently changes the physics instead of failing fast.
🩹 Minimal fix
OhmInfo static FROM(initializer::PHAREDict const& dict)
{
- return {dict["resistivity"].template to<double>(),
- dict["hyper_resistivity"].template to<double>(),
- cppdict::get_value(dict, "hyper_mode", std::string{"constant"}) == "constant"
- ? HyperMode::constant
- : HyperMode::spatial};
+ auto const eta = dict["resistivity"].template to<double>();
+ auto const nu = dict["hyper_resistivity"].template to<double>();
+ auto const mode = cppdict::get_value(dict, "hyper_mode", std::string{"constant"});
+
+ if (mode == "constant")
+ return {eta, nu, HyperMode::constant};
+ if (mode == "spatial")
+ return {eta, nu, HyperMode::spatial};
+
+ throw std::runtime_error{"Error - Ohm - unknown hyper_mode: " + mode};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| OhmInfo static FROM(initializer::PHAREDict const& dict) | |
| { | |
| return {dict["resistivity"].template to<double>(), | |
| dict["hyper_resistivity"].template to<double>(), | |
| cppdict::get_value(dict, "hyper_mode", std::string{"constant"}) == "constant" | |
| ? HyperMode::constant | |
| : HyperMode::spatial}; | |
| OhmInfo static FROM(initializer::PHAREDict const& dict) | |
| { | |
| auto const eta = dict["resistivity"].template to<double>(); | |
| auto const nu = dict["hyper_resistivity"].template to<double>(); | |
| auto const mode = cppdict::get_value(dict, "hyper_mode", std::string{"constant"}); | |
| if (mode == "constant") | |
| return {eta, nu, HyperMode::constant}; | |
| if (mode == "spatial") | |
| return {eta, nu, HyperMode::spatial}; | |
| throw std::runtime_error{"Error - Ohm - unknown hyper_mode: " + mode}; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/numerics/ohm/ohm.hpp` around lines 23 - 29, The FROM factory
silently treats any hyper_mode value other than "constant" as
HyperMode::spatial; change it to validate the string from
cppdict::get_value(dict, "hyper_mode", std::string{"constant"}) and explicitly
map "constant" -> HyperMode::constant and "spatial" -> HyperMode::spatial, and
throw or otherwise fail fast (e.g., throw std::invalid_argument) if the value is
neither, so the OhmInfo::FROM initializer fails on typos instead of silently
changing behavior.
Member
Author
There was a problem hiding this comment.
it was always like that so ignore it for now, not really related to what we're doing here
|
|
||
|
|
||
| TYPED_TEST(OhmTest, ThatOhmHasCtorWithDict) | ||
| TYPED_TEST(OhmTest, ThatOhmInfoCanBeBuiltFromDict) |
Check notice
Code scanning / CodeQL
Unused static variable Note test
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I think we can just the the LevelLooper instead if we really need to, which for GPU will likely be necessary