Open
Conversation
📝 WalkthroughWalkthroughExtracts simulation/test setup into new HybridAdvanceTest and MHDAdvanceTest classes, reworks many tests to dispatch via permute helpers and dynamic superclass casting, centralizes diag-dir management on Simulation instances, and adds/updates initialization, advance, and particle tests that build/run sims and load HDF5 diagnostic hierarchies. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Test Runner
participant TestModule as getHierarchy (Hybrid/MHD)
participant Simulator as Simulation
participant Diagnostics as Diagnostics
participant H5 as HDF5 Output
participant Loader as Hierarchy Loader
participant Verifier as Verification
Runner->>TestModule: request hierarchy(qty, interp_order, refinement_boxes)
TestModule->>TestModule: configure domain, density, fields, models
TestModule->>Simulator: instantiate Simulation (models + diag_options)
Simulator->>Diagnostics: collect snapshots during run
Diagnostics->>H5: write HDF5 diagnostic files
Loader->>H5: read diagnostics (fields/moments/particles)
Loader->>TestModule: assemble data hierarchy
TestModule->>Runner: return datahier
Runner->>Verifier: run checks (overlap, conservation, domain/ghost consistency)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/simulator/advance/test_advance_hybrid.py`:
- Around line 8-17: The test references undefined symbols merge_particles,
hierarchy_overlaps, and boxm causing NameError; add the missing imports at the
top of the module for these symbols so the test can access them (e.g., import
merge_particles from its particles module, import hierarchy_overlaps from
pyphare.pharesee.hierarchy, and import boxm from pyphare.pharein.boxm) and
ensure the names used in the test (merge_particles, hierarchy_overlaps, boxm)
match the imported identifiers.
In `@tests/simulator/advance/test_advance_mhd.py`:
- Around line 142-147: The branch that returns eb_hier incorrectly includes "e"
in the return condition so qty="e" can return None; update the logic in the
block handling qty to either (A) load an E hierarchy by calling hierarchy_from
with h5_filename=diag_outputs + "/EM_E.h5" into a new e_hier and return it when
qty contains "e", or (B) explicitly reject unsupported "e" by removing "e" from
the return condition and raising a ValueError (or assertion) when qty == "e";
adjust the conditions around eb_hier, hierarchy_from, and the return so "e" is
handled deterministically.
- Around line 27-47: The getHierarchy signature in test_advance_mhd.py places
refinement_boxes as the second positional parameter which conflicts with
AdvanceTestBase callers that pass interp_order as the second positional arg; to
fix, reorder the parameters so interp_order (default 2) comes immediately after
ndim and before refinement_boxes in the getHierarchy definition (preserve all
other defaults and kwargs), update any internal uses of the parameter name if
necessary, and run the existing tests to ensure no other call sites require
adjustments.
In `@tests/simulator/advance/test_fields_advance_1d.py`:
- Around line 72-90: The test function
test_overlaped_fields_are_equal_with_min_max_patch_size_of_max_ghosts calls
getHierarchy without forwarding interp_order and refinement_boxes, which causes
HybridAdvanceTest.getHierarchy to fail on the hybrid path; update the
getHierarchy invocation in this test to pass interp_order=interp_order and
refinement_boxes=kwargs.get("refinement_boxes") (or the appropriate
refinement_boxes variable from the test harness) so that
HybridAdvanceTest.getHierarchy receives both required arguments.
- Around line 33-40: The function permute_mhd contains a reference to an
undefined name interp_order which raises NameError; update permute_mhd to accept
an interp_order parameter with a sensible default (e.g., interp_order=2 or None)
and use that parameter when constructing the dict
(test=test_mhd.MHDAdvanceTest(), interp_order=interp_order, ...), and adjust any
callers of permute_mhd to pass an explicit interp_order where needed so the MHD
permutations no longer reference an undefined variable.
In `@tests/simulator/advance/test_fields_advance_2d.py`:
- Around line 74-93: The test function
test_overlaped_fields_are_equal_with_min_max_patch_size_of_max_ghosts calls
test.getHierarchy without forwarding the interp_order argument, which causes a
TypeError on paths where getHierarchy requires it; fix by passing
interp_order=interp_order into the test.getHierarchy(...) call (keep other args
the same) so interp_order is propagated to getHierarchy.
- Around line 35-42: The function permute_mhd references an undefined
interp_order causing a NameError; change permute_mhd to accept an interp_order
parameter (e.g., def permute_mhd(boxes={}, interp_order=2) or interp_order=None)
and use that parameter in the returned dict, or alternatively remove the
interp_order key if not needed; update any call sites that invoke permute_mhd
without the new argument to supply the desired interpolation order so
test_mhd.MHDAdvanceTest receives a defined interp_order.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/simulator/advance/test_advance_hybrid.pytests/simulator/advance/test_advance_mhd.pytests/simulator/advance/test_fields_advance_1d.pytests/simulator/advance/test_fields_advance_2d.pytests/simulator/advance/test_particles_advance_1d.pytests/simulator/advance/test_particles_advance_2d.pytests/simulator/test_advance.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
tests/simulator/advance/test_fields_advance_2d.py (1)
74-93:⚠️ Potential issue | 🔴 CriticalForward
interp_orderintogetHierarchyin this test path.
interp_orderis unpacked into the test method (Line 75) but dropped in thegetHierarchycall (Line 85), causing missing-argument failures for hybrid cases.Suggested fix
datahier = test.getHierarchy( ndim, qty="eb", + interp_order=interp_order, smallest_patch_size=smallest_patch_size, largest_patch_size=smallest_patch_size, time_step=time_step, time_step_nbr=time_step_nbr, **kwargs, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/advance/test_fields_advance_2d.py` around lines 74 - 93, The test method test_overlaped_fields_are_equal_with_min_max_patch_size_of_max_ghosts accepts interp_order but does not pass it into the call to getHierarchy; update the getHierarchy(...) invocation to include interp_order=interp_order so the hierarchy is constructed with the requested interpolation order (refer to function getHierarchy and the test method name to locate the call).tests/simulator/advance/test_fields_advance_1d.py (1)
72-90:⚠️ Potential issue | 🔴 Critical
getHierarchycall drops required arguments on this 1D test path.Line 83 omits both
interp_orderand**kwargs; this losesrefinement_boxesand breaks hybridgetHierarchycalls.Suggested fix
datahier = test.getHierarchy( ndim, qty="eb", + interp_order=interp_order, smallest_patch_size=smallest_patch_size, largest_patch_size=smallest_patch_size, time_step=time_step, time_step_nbr=time_step_nbr, + **kwargs, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/advance/test_fields_advance_1d.py` around lines 72 - 90, The getHierarchy call in test_overlaped_fields_are_equal_with_min_max_patch_size_of_max_ghosts drops required arguments; update the call to include interp_order and pass through **kwargs so any refinement_boxes or other options are preserved (i.e. call getHierarchy(..., interp_order=interp_order, ..., **kwargs)), referencing the test function name and variables interp_order, **kwargs and ensuring refinement_boxes is not lost.tests/simulator/advance/test_advance_mhd.py (2)
23-43:⚠️ Potential issue | 🟠 MajorAlign
MHDAdvanceTest.getHierarchyparameter order with shared base callers.
AdvanceTestBase._test_field_coarsening_via_subcyclespassesinterp_orderas the second positional argument. With the current signature, that value binds torefinement_boxes, andrefinement_boxesfrom**kwargscollides (multiple values for argument 'refinement_boxes').Suggested fix
def getHierarchy( self, ndim, + interp_order, refinement_boxes, qty, density=_density, @@ diag_outputs="", extra_diag_options={}, timestamps=None, - interp_order=2, # hax **kwargs ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/advance/test_advance_mhd.py` around lines 23 - 43, The getHierarchy signature currently places interp_order near the end so callers that pass interp_order as the second positional arg (e.g. AdvanceTestBase._test_field_coarsening_via_subcycles) end up binding it to refinement_boxes; fix this by reordering parameters so interp_order comes immediately after ndim (e.g. def getHierarchy(self, ndim, interp_order=2, refinement_boxes, qty, ...)), update any default placement accordingly, and run tests to ensure no other callers rely on the old positional order; reference getHierarchy and AdvanceTestBase._test_field_coarsening_via_subcycles when locating the change.
138-143:⚠️ Potential issue | 🟠 MajorHandle
qty="e"explicitly instead of returningNone.Line 142 includes
"e"in the return condition, but no E hierarchy is loaded in this method.qty="e"currently returnsNone.Suggested fix
- if qty in ["e", "b", "eb"]: + if qty in ["b", "eb"]: return eb_hier + if qty == "e": + raise ValueError("qty='e' is unsupported in MHDAdvanceTest")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/advance/test_advance_mhd.py` around lines 138 - 143, The code returns eb_hier for qty values including "e" but never loads an E hierarchy, causing qty="e" to yield None; fix by explicitly loading an E hierarchy (e_hier) using hierarchy_from with h5_filename=diag_outputs + "/EM_E.h5" when qty includes "e" (and adjust the return logic so qty=="e" returns e_hier, while qty=="eb" or combined cases still return the appropriate eb_hier/e_hier). Update the branches around the existing hierarchy_from call and the return condition that references qty, using the variables hierarchy_from, eb_hier, e_hier, qty, and diag_outputs to locate and modify the code.
🤖 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/simulator/advance/test_advance_hybrid.py`:
- Around line 39-42: The test helper getHierarchy currently uses mutable default
arguments model_init={} and extra_diag_options={}, which allows
extra_diag_options to be mutated across calls and leak state; change
getHierarchy signature to use model_init=None and extra_diag_options=None and
inside the function set model_init = {} if model_init is None and
extra_diag_options = {} if extra_diag_options is None, then continue to mutate
the local dicts as needed (this prevents cross-test state bleed while preserving
existing logic that mutates extra_diag_options).
In `@tests/simulator/advance/test_advance_mhd.py`:
- Line 39: The parameter extra_diag_options is declared with a mutable default
({}), causing shared state across calls; change the function signature to use
extra_diag_options=None and inside the function initialize it with if
extra_diag_options is None: extra_diag_options = {} before any mutations (where
the code updates or assigns into extra_diag_options), and apply the same change
for any other mutable defaults in this file to avoid state leakage across test
invocations.
---
Duplicate comments:
In `@tests/simulator/advance/test_advance_mhd.py`:
- Around line 23-43: The getHierarchy signature currently places interp_order
near the end so callers that pass interp_order as the second positional arg
(e.g. AdvanceTestBase._test_field_coarsening_via_subcycles) end up binding it to
refinement_boxes; fix this by reordering parameters so interp_order comes
immediately after ndim (e.g. def getHierarchy(self, ndim, interp_order=2,
refinement_boxes, qty, ...)), update any default placement accordingly, and run
tests to ensure no other callers rely on the old positional order; reference
getHierarchy and AdvanceTestBase._test_field_coarsening_via_subcycles when
locating the change.
- Around line 138-143: The code returns eb_hier for qty values including "e" but
never loads an E hierarchy, causing qty="e" to yield None; fix by explicitly
loading an E hierarchy (e_hier) using hierarchy_from with
h5_filename=diag_outputs + "/EM_E.h5" when qty includes "e" (and adjust the
return logic so qty=="e" returns e_hier, while qty=="eb" or combined cases still
return the appropriate eb_hier/e_hier). Update the branches around the existing
hierarchy_from call and the return condition that references qty, using the
variables hierarchy_from, eb_hier, e_hier, qty, and diag_outputs to locate and
modify the code.
In `@tests/simulator/advance/test_fields_advance_1d.py`:
- Around line 72-90: The getHierarchy call in
test_overlaped_fields_are_equal_with_min_max_patch_size_of_max_ghosts drops
required arguments; update the call to include interp_order and pass through
**kwargs so any refinement_boxes or other options are preserved (i.e. call
getHierarchy(..., interp_order=interp_order, ..., **kwargs)), referencing the
test function name and variables interp_order, **kwargs and ensuring
refinement_boxes is not lost.
In `@tests/simulator/advance/test_fields_advance_2d.py`:
- Around line 74-93: The test method
test_overlaped_fields_are_equal_with_min_max_patch_size_of_max_ghosts accepts
interp_order but does not pass it into the call to getHierarchy; update the
getHierarchy(...) invocation to include interp_order=interp_order so the
hierarchy is constructed with the requested interpolation order (refer to
function getHierarchy and the test method name to locate the call).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/simulator/advance/test_advance_hybrid.pytests/simulator/advance/test_advance_mhd.pytests/simulator/advance/test_fields_advance_1d.pytests/simulator/advance/test_fields_advance_2d.pytests/simulator/test_advance.pytests/simulator/test_diagnostics.pytests/simulator/test_vtk_diagnostics.py
💤 Files with no reviewable changes (2)
- tests/simulator/test_diagnostics.py
- tests/simulator/test_vtk_diagnostics.py
PhilipDeegan
force-pushed
the
pre-mhd
branch
2 times, most recently
from
7b671c9 to
50b9aba
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
tests/simulator/advance/test_advance_hybrid.py (1)
39-41:⚠️ Potential issue | 🟠 MajorAvoid shared mutable defaults in
getHierarchy.
model_initandextra_diag_optionsare shared across calls, andextra_diag_optionsis mutated on Line 55. This can leak state between test invocations.Proposed fix
- model_init={}, + model_init=None, @@ - extra_diag_options={}, + extra_diag_options=None, @@ - extra_diag_options["mode"] = "overwrite" + if model_init is None: + model_init = {} + if extra_diag_options is None: + extra_diag_options = {} + else: + extra_diag_options = dict(extra_diag_options) + + extra_diag_options["mode"] = "overwrite"#!/bin/bash rg -nP 'model_init=\{\}|extra_diag_options=\{\}|extra_diag_options\["mode"\]' tests/simulator/advance/test_advance_hybrid.pyBased on learnings: Mutable default arguments can lead to unexpected behavior because defaults are shared between calls; replace them with
Noneand initialize inside the function.Also applies to: 55-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/advance/test_advance_hybrid.py` around lines 39 - 41, getHierarchy currently uses mutable defaults (model_init and extra_diag_options) which allows state to leak between calls; change the function signature of getHierarchy to accept model_init=None and extra_diag_options=None, then inside the function set model_init = {} if None and extra_diag_options = {} if None (or copy inputs if callers pass dicts) and when you mutate extra_diag_options (e.g. extra_diag_options["mode"] on the code path referenced), operate on the new local dict to avoid altering a shared default; update any callers/tests that rely on the old signature if needed.
🧹 Nitpick comments (4)
tests/simulator/test_initialization.py (1)
39-39: Mutable default argumentextra_diag_options={}.Same pattern as other files - using
{}as default can cause shared state. Consider usingNoneand initializing inside the function for consistency with Python best practices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/test_initialization.py` at line 39, The parameter extra_diag_options currently uses a mutable default (extra_diag_options={}); change the signature to use extra_diag_options=None and inside the function (where extra_diag_options is referenced) initialize it with a safe default, e.g. if extra_diag_options is None: extra_diag_options = {}, to avoid shared mutable state across calls while keeping behavior identical.tests/simulator/advance/test_fields_advance_2d.py (2)
82-84: Unused variablelargest_patch_size.The unpacked variable
largest_patch_sizeis never used. Prefix it with an underscore to indicate it's intentionally ignored.Suggested fix
- largest_patch_size, smallest_patch_size = check_patch_size( + _largest_patch_size, smallest_patch_size = check_patch_size( ndim, interp_order=interp_order, cells=[60] * ndim )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/advance/test_fields_advance_2d.py` around lines 82 - 84, The variable largest_patch_size returned from check_patch_size is unused; update the unpacking to use a discarded name (e.g., _largest_patch_size, smallest_patch_size = check_patch_size(...)) so the intent to ignore the first return value is clear and linter warnings are avoided; make this change where check_patch_size is called in the test (function/method name: check_patch_size, current variables: largest_patch_size and smallest_patch_size).
23-48: Mutable default arguments inpermute_*functions.While
boxes={}is used as a read-only configuration and not mutated within these functions, using mutable defaults is generally discouraged. Since these helpers are called with explicit arguments from test decorators, the risk is low, but consider usingNonefor consistency with Python best practices.Suggested fix
-def permute_hybrid(boxes={}): +def permute_hybrid(boxes=None): + if boxes is None: + boxes = {} return [ ... ] -def permute_mhd(boxes={}): +def permute_mhd(boxes=None): + if boxes is None: + boxes = {} return [ ... ] -def permute(boxes={}, hybrid=True, mhd=False): +def permute(boxes=None, hybrid=True, mhd=False): + if boxes is None: + boxes = {} return (permute_hybrid(boxes) if hybrid else []) + ( permute_mhd(boxes) if mhd else [] )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/advance/test_fields_advance_2d.py` around lines 23 - 48, The three helper functions permute_hybrid, permute_mhd, and permute currently use a mutable default argument boxes={}, change their signatures to use boxes=None and inside each function set boxes = {} if boxes is None else boxes (or boxes = boxes or {} if you prefer) to avoid mutable default pitfalls; update permute to accept boxes=None as well and pass the resolved boxes into permute_hybrid/permute_mhd calls so behavior is unchanged while following best practices.tests/simulator/advance/test_fields_advance_1d.py (1)
22-23: Replace mutable default arguments in permutation helpers.Using
boxes={}as a default argument can cause unexpected behavior if the dict is modified, as the same instance persists across function calls. Initialize mutable defaults inside the function instead.Proposed fix
-def permute_hybrid(boxes={}): +def permute_hybrid(boxes=None): + if boxes is None: + boxes = {} return [ dict( test=test_hybrid.HybridAdvanceTest(), interp_order=interp_order, refinement_boxes=boxes, ) for interp_order in interp_orders ] -def permute_mhd(boxes={}): +def permute_mhd(boxes=None): + if boxes is None: + boxes = {} return [ dict( test=test_mhd.MHDAdvanceTest(), interp_order=2, # hax todo refinement_boxes=boxes, ) ] -def permute(boxes={}, hybrid=True, mhd=False): +def permute(boxes=None, hybrid=True, mhd=False): + if boxes is None: + boxes = {} return (permute_hybrid(boxes) if hybrid else []) + ( permute_mhd(boxes) if mhd else [] )Also applies to: lines 33-34, 43-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/advance/test_fields_advance_1d.py` around lines 22 - 23, The helper function permute_hybrid currently uses a mutable default argument boxes={}, which can persist state across calls; change the signature to use boxes=None and inside the function initialize boxes = {} if boxes is None, then proceed unchanged; apply the same pattern to the other permutation helper functions in this file that use mutable defaults so each call gets a fresh dict.
🤖 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/simulator/advance/test_advance_hybrid.py`:
- Around line 24-25: Replace assigned lambdas with proper named functions and
factor duplicated logic into a single helper: create a helper function (e.g.,
neg_sq_diff(i, xyz, hL) or neg_sq_sum(xyz, hL)) that returns -((xyz[i] - hL[i])
** 2) or the sum over indices as needed, then replace the assigned lambda "_"
and the identical lambdas used by by, bz, vx, vy, vz with calls to that helper;
update references in the return expressions and any list comprehensions to call
the named helper or a small named wrapper function (e.g., def compute_term(i):
return neg_sq_diff(i, xyz, hL)) so no assigned lambdas remain and duplicate
logic is centralized.
In `@tests/simulator/advance/test_advance_mhd.py`:
- Around line 17-21: The _density function is defined as an instance method but
lacks the self parameter; update its signature to include self as the first
parameter (def _density(self, *xyz):) or explicitly mark it as a `@staticmethod`
if it should not access instance state, and adjust any references in
getHierarchy that pass it as the default density to match the chosen approach
(use _density or self._density accordingly).
In `@tests/simulator/test_diagnostics.py`:
- Around line 217-219: The code calls an undefined variable and wrong helper
name: replace the undefined `sim` with the `simulation` instance returned by
`ph.Simulation(**simInput)` and call the correct helper `unique_diag_dir_update`
(not `unique_diag_update`) to produce `diag_path`; keep the subsequent
`register_diag_dir_for_cleanup(diag_path)` call unchanged so the cleanup still
uses the produced `diag_path`.
In `@tests/simulator/test_load_balancing.py`:
- Around line 198-204: The run_sim function currently uses a mutable default
argument dic={} and passes diags_dir into config() before diags_dir is defined;
fix by changing the signature to def run_sim(self, dic=None): and inside set dic
= {} if None, then compute the diagnostic directory first (call
self.unique_diag_dir_update(diag_outputs, ...) or otherwise create the diag
path) into diags_dir before calling config, pass that diags_dir as the first arg
to config(sim) so config receives the actual diag_dir, then call
self.register_diag_dir_for_cleanup(diags_dir) and finally run
Simulator(sim).run(); ensure you reference run_sim, config,
unique_diag_dir_update, register_diag_dir_for_cleanup, and Simulator.run when
making the changes.
In `@tests/simulator/test_restarts.py`:
- Around line 270-273: The restart directory/option is being set on simput then
ph.Simulation is created, but local_out (via unique_diag_dir_update) is computed
after creation causing a use-before-define; move the call to
unique_diag_dir_update(f"{out}/test_n2", ...) to before instantiating
ph.Simulation so restart_options/restart_time on simput are finalized when
passed into ph.Simulation, or alternatively set ph.global_vars.sim =
ph.Simulation(**simput) first and then explicitly set the simulation's restart
directory on ph.global_vars.sim (e.g., update its restart path attribute) before
computing local_out—update either the order around simput/restart_options,
ph.Simulation, and unique_diag_dir_update to ensure the restart directory is
defined prior to use.
- Around line 257-261: The code uses local_out before it is defined: move the
call that sets local_out via self.unique_diag_dir_update(f"{out}/test",
ph.global_vars.sim) so it occurs before assigning
simput["restart_options"]["dir"] = local_out, or alternatively set
simput["restart_options"]["dir"] to a temporary path and after creating
ph.global_vars.sim = ph.Simulation(**simput) update
ph.global_vars.sim.restart_options["dir"] = local_out; reference simput,
local_out, ph.global_vars.sim, ph.Simulation and unique_diag_dir_update to
locate where to reorder or update the restart_options in place.
---
Duplicate comments:
In `@tests/simulator/advance/test_advance_hybrid.py`:
- Around line 39-41: getHierarchy currently uses mutable defaults (model_init
and extra_diag_options) which allows state to leak between calls; change the
function signature of getHierarchy to accept model_init=None and
extra_diag_options=None, then inside the function set model_init = {} if None
and extra_diag_options = {} if None (or copy inputs if callers pass dicts) and
when you mutate extra_diag_options (e.g. extra_diag_options["mode"] on the code
path referenced), operate on the new local dict to avoid altering a shared
default; update any callers/tests that rely on the old signature if needed.
---
Nitpick comments:
In `@tests/simulator/advance/test_fields_advance_1d.py`:
- Around line 22-23: The helper function permute_hybrid currently uses a mutable
default argument boxes={}, which can persist state across calls; change the
signature to use boxes=None and inside the function initialize boxes = {} if
boxes is None, then proceed unchanged; apply the same pattern to the other
permutation helper functions in this file that use mutable defaults so each call
gets a fresh dict.
In `@tests/simulator/advance/test_fields_advance_2d.py`:
- Around line 82-84: The variable largest_patch_size returned from
check_patch_size is unused; update the unpacking to use a discarded name (e.g.,
_largest_patch_size, smallest_patch_size = check_patch_size(...)) so the intent
to ignore the first return value is clear and linter warnings are avoided; make
this change where check_patch_size is called in the test (function/method name:
check_patch_size, current variables: largest_patch_size and
smallest_patch_size).
- Around line 23-48: The three helper functions permute_hybrid, permute_mhd, and
permute currently use a mutable default argument boxes={}, change their
signatures to use boxes=None and inside each function set boxes = {} if boxes is
None else boxes (or boxes = boxes or {} if you prefer) to avoid mutable default
pitfalls; update permute to accept boxes=None as well and pass the resolved
boxes into permute_hybrid/permute_mhd calls so behavior is unchanged while
following best practices.
In `@tests/simulator/test_initialization.py`:
- Line 39: The parameter extra_diag_options currently uses a mutable default
(extra_diag_options={}); change the signature to use extra_diag_options=None and
inside the function (where extra_diag_options is referenced) initialize it with
a safe default, e.g. if extra_diag_options is None: extra_diag_options = {}, to
avoid shared mutable state across calls while keeping behavior identical.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
tests/simulator/__init__.pytests/simulator/advance/test_advance_hybrid.pytests/simulator/advance/test_advance_mhd.pytests/simulator/advance/test_fields_advance_1d.pytests/simulator/advance/test_fields_advance_2d.pytests/simulator/test_advance.pytests/simulator/test_diagnostics.pytests/simulator/test_initialization.pytests/simulator/test_load_balancing.pytests/simulator/test_restarts.pytests/simulator/test_vtk_diagnostics.py
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/simulator/test_load_balancing.py (1)
32-45:⚠️ Potential issue | 🔴 CriticalFix undefined
diag_dirreference inconfig.Line 44 uses
diag_dir, butconfigno longer defines/accepts it after the Line 32 signature change.run_sim(Line 200) now fails withNameErrorbefore the simulation starts.Suggested fix
def config(loadbalancing={}): sim = ph.Simulation( @@ diag_options={ "format": "phareh5", - "options": {"dir": diag_dir, "mode": "overwrite"}, + "options": {"mode": "overwrite"}, }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/test_load_balancing.py` around lines 32 - 45, config currently references diag_dir but no longer accepts or defines it, causing a NameError when run_sim calls it; update the config function (config(...)) to accept a diag_dir parameter (e.g., add diag_dir with a sensible default) or define diag_dir inside config before it is used, and ensure any callers such as run_sim pass the diag_dir through to config so the diag_options block inside the ph.Simulation constructor can use a defined diag_dir value.
♻️ Duplicate comments (3)
tests/simulator/test_diagnostics.py (1)
218-218:⚠️ Potential issue | 🔴 CriticalCall the correct helper:
unique_diag_dir_update.Line 218 calls
self.unique_diag_update(...), which is not defined onSimulatorTest; this will raiseAttributeError.Suggested fix
- diag_path = self.unique_diag_update(f"{out}/test", simulation) + diag_path = self.unique_diag_dir_update(f"{out}/test", simulation)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/test_diagnostics.py` at line 218, The test calls a non-existent helper self.unique_diag_update(...) causing an AttributeError; update the call in tests/simulator/test_diagnostics.py to use the correct helper name unique_diag_dir_update so that diag_path = self.unique_diag_dir_update(f"{out}/test", simulation) (refer to the SimulatorTest helper unique_diag_dir_update to locate and replace the incorrect call).tests/simulator/advance/test_advance_mhd.py (2)
133-138:⚠️ Potential issue | 🟠 Major
qty="e"currently returnsNone.Line 137 includes
"e"in the return condition, but no E hierarchy is loaded, so this path is non-deterministic.Suggested fix (explicit reject)
- if qty in ["e", "b", "eb"]: + if qty in ["b", "eb"]: return eb_hier + if qty == "e": + raise ValueError("qty='e' is unsupported in MHDAdvanceTest")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/advance/test_advance_mhd.py` around lines 133 - 138, The branch returns eb_hier when qty is in ["e","b","eb"] but eb_hier is only set when qty includes "b"/"eb", so qty="e" can return None; update the logic in the function handling qty to either (a) load an e_hier via hierarchy_from when qty includes "e" (e.g., call hierarchy_from with EM_E.h5 into an e_hier variable before the return) or (b) remove "e" from the return condition and explicitly reject/raise an error for qty=="e" so you never return an unset eb_hier; locate the code around the existing hierarchy_from call and the return eb_hier statement to implement one of these fixes.
23-42:⚠️ Potential issue | 🟠 MajorAlign
getHierarchypositional parameters with base callers.
AdvanceTestBasehelper calls passinterp_orderas the second positional argument. With the current signature, that value is bound torefinement_boxesinstead.Suggested fix
def getHierarchy( self, ndim, + interp_order, refinement_boxes, qty, density=_density, @@ diag_outputs="", extra_diag_options=None, timestamps=None, - interp_order=2, # hax **kwargs ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/advance/test_advance_mhd.py` around lines 23 - 42, The getHierarchy signature currently places interp_order late so callers in AdvanceTestBase that pass interp_order as the second positional argument end up binding it to refinement_boxes; change the getHierarchy parameter order so interp_order is the second parameter (e.g., def getHierarchy(self, ndim, interp_order=2, refinement_boxes, qty, ...)) or otherwise accept/forward positional args to preserve existing caller behavior; update the function definition in getHierarchy to move interp_order immediately after ndim (and adjust any default values or callers if necessary) to ensure positional binding matches AdvanceTestBase usage.
🤖 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/simulator/advance/test_advance_mhd.py`:
- Around line 83-85: The test calls the removed helper
unique_diag_dir_for_test_case which will raise AttributeError; update the call
in test_advance_mhd.py to use the current helper unique_diag_dir_update instead,
passing the same arguments (the string "phare_outputs/advance", sim,
diag_outputs) and preserving the assignment to diag_outputs so the SimulatorTest
flow uses the new helper method.
In `@tests/simulator/advance/test_fields_advance_1d.py`:
- Around line 22-46: permute_hybrid, permute_mhd, and permute are creating
TestCase instances at import time (test=test_hybrid.HybridAdvanceTest() and
test=test_mhd.MHDAdvanceTest()) and use a mutable default boxes={}, which causes
diag/assert state to attach to detached instances and shared defaults; change
the signature to boxes=None and inside set boxes = {} if None, and stop
instantiating tests at import — pass the class or a zero-arg factory (e.g.,
test=test_hybrid.HybridAdvanceTest without parentheses or test_factory=lambda:
HybridAdvanceTest()) so the AdvanceTest is constructed at runtime by the test
runner; apply the same fixes to the other permute_* helpers noted (lines 57-66,
73-94, 112-129).
In `@tests/simulator/advance/test_fields_advance_2d.py`:
- Around line 23-48: Replace mutable defaults and lazy-instantiate test helpers:
change the functions permute_hybrid, permute_mhd and permute to use boxes=None
(not boxes={}) and inside each function do boxes = {} if boxes is None; ensure
HybridAdvanceTest() and MHDAdvanceTest() are created per-entry (i.e.,
instantiated inside the list comprehension or just before creating each dict) so
a new helper is used for each variant and no single helper instance accumulates
cleanup state; also update related test method signatures that currently use
boxes={} to boxes=None and instantiate their helper objects locally before use
so tearDown is not skipped on shared instances.
---
Outside diff comments:
In `@tests/simulator/test_load_balancing.py`:
- Around line 32-45: config currently references diag_dir but no longer accepts
or defines it, causing a NameError when run_sim calls it; update the config
function (config(...)) to accept a diag_dir parameter (e.g., add diag_dir with a
sensible default) or define diag_dir inside config before it is used, and ensure
any callers such as run_sim pass the diag_dir through to config so the
diag_options block inside the ph.Simulation constructor can use a defined
diag_dir value.
---
Duplicate comments:
In `@tests/simulator/advance/test_advance_mhd.py`:
- Around line 133-138: The branch returns eb_hier when qty is in ["e","b","eb"]
but eb_hier is only set when qty includes "b"/"eb", so qty="e" can return None;
update the logic in the function handling qty to either (a) load an e_hier via
hierarchy_from when qty includes "e" (e.g., call hierarchy_from with EM_E.h5
into an e_hier variable before the return) or (b) remove "e" from the return
condition and explicitly reject/raise an error for qty=="e" so you never return
an unset eb_hier; locate the code around the existing hierarchy_from call and
the return eb_hier statement to implement one of these fixes.
- Around line 23-42: The getHierarchy signature currently places interp_order
late so callers in AdvanceTestBase that pass interp_order as the second
positional argument end up binding it to refinement_boxes; change the
getHierarchy parameter order so interp_order is the second parameter (e.g., def
getHierarchy(self, ndim, interp_order=2, refinement_boxes, qty, ...)) or
otherwise accept/forward positional args to preserve existing caller behavior;
update the function definition in getHierarchy to move interp_order immediately
after ndim (and adjust any default values or callers if necessary) to ensure
positional binding matches AdvanceTestBase usage.
In `@tests/simulator/test_diagnostics.py`:
- Line 218: The test calls a non-existent helper self.unique_diag_update(...)
causing an AttributeError; update the call in
tests/simulator/test_diagnostics.py to use the correct helper name
unique_diag_dir_update so that diag_path =
self.unique_diag_dir_update(f"{out}/test", simulation) (refer to the
SimulatorTest helper unique_diag_dir_update to locate and replace the incorrect
call).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
tests/simulator/__init__.pytests/simulator/advance/test_advance_hybrid.pytests/simulator/advance/test_advance_mhd.pytests/simulator/advance/test_fields_advance_1d.pytests/simulator/advance/test_fields_advance_2d.pytests/simulator/advance/test_particles_advance_1d.pytests/simulator/advance/test_particles_advance_2d.pytests/simulator/test_advance.pytests/simulator/test_diagnostics.pytests/simulator/test_initialization.pytests/simulator/test_load_balancing.pytests/simulator/test_restarts.pytests/simulator/test_vtk_diagnostics.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/simulator/advance/test_particles_advance_2d.py
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/simulator/test_load_balancing.py (1)
32-45:⚠️ Potential issue | 🔴 Critical
config()will fail at runtime due to undefineddiag_dir(and has a mutable default).
diag_diris referenced on line 44 but is never defined; the correct variable isdiag_outputs(defined at line 28). This causesNameErrorwhenconfig()is called. Also,loadbalancing={}should be replaced withNoneto avoid shared-state defaults.Proposed fix
-def config(loadbalancing={}): +def config(loadbalancing=None): + if loadbalancing is None: + loadbalancing = {} sim = ph.Simulation( @@ diag_options={ "format": "phareh5", - "options": {"dir": diag_dir, "mode": "overwrite"}, + "options": {"dir": diag_outputs, "mode": "overwrite"}, }, )Additionally,
run_simat line 198 has the same mutable default issue withdic={}and should use the same pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/test_load_balancing.py` around lines 32 - 45, In config(), replace the incorrect diag_dir reference with the existing diag_outputs variable (use diag_outputs in the diag_options "dir" value) and change the mutable default loadbalancing={} to loadbalancing=None and then inside the function set loadbalancing = loadbalancing or {} to avoid shared-state; likewise, in run_sim change the mutable default dic={} to dic=None and set dic = dic or {} at the start of the function to prevent shared mutable defaults—update references to the config() and run_sim() signatures accordingly.
♻️ Duplicate comments (5)
tests/simulator/test_load_balancing.py (1)
198-198:⚠️ Potential issue | 🟡 MinorAvoid mutable default in
run_sim.
dic={}can leak state across calls. UseNoneand initialize inside the method.Proposed fix
- def run_sim(self, dic={}): + def run_sim(self, dic=None): + if dic is None: + dic = {} ph.global_vars.sim = None sim = config(dic)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/test_load_balancing.py` at line 198, The run_sim function currently uses a mutable default argument dic={}, which can leak state between calls; change the signature to def run_sim(self, dic=None): and inside the method initialize with if dic is None: dic = {} so each call gets a fresh dict; update any callers accordingly if they rely on passing None explicitly.tests/simulator/advance/test_fields_advance_2d.py (1)
22-45: 🛠️ Refactor suggestion | 🟠 MajorReplace mutable defaults and consider test helper lifecycle.
Three issues:
boxes={}mutable defaults at lines 26, 38, 42 can cause unexpected behavior across calls- Module-level
mhd_testandhybrid_testinstances are shared across all test variants- Cleanup registrations on helper instances won't be executed since only
AdvanceTest.tearDown()is calledSuggested fix for mutable defaults
-mhd_test = test_advance_mhd.MHDAdvanceTest() -hybrid_test = test_advance_hybrid.HybridAdvanceTest() +# Consider lazy instantiation or ensure cleanup is shared -def permute_hybrid(boxes={}): +def permute_hybrid(boxes=None): + boxes = boxes if boxes is not None else {} return [ dict( test=hybrid_test, interp_order=interp_order, refinement_boxes=boxes, nbr_part_per_cell=ppc, ) for interp_order in interp_orders ] -def permute_mhd(boxes={}): # interp_order hax todo +def permute_mhd(boxes=None): # interp_order hax todo + boxes = boxes if boxes is not None else {} return [dict(test=mhd_test, interp_order=2, refinement_boxes=boxes)] -def permute(boxes={}, hybrid=True, mhd=False): +def permute(boxes=None, hybrid=True, mhd=False): + boxes = boxes if boxes is not None else {} return (permute_hybrid(boxes) if hybrid else []) + ( permute_mhd(boxes) if mhd else [] )Based on learnings: Mutable default arguments like lists or dictionaries can lead to unexpected behavior because the same instance of the mutable object is shared across all calls to the function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/advance/test_fields_advance_2d.py` around lines 22 - 45, Replace the mutable default boxes={} with boxes=None in permute_hybrid, permute_mhd, and permute, and initialize boxes = {} inside each function; stop using module-level singletons mhd_test and hybrid_test—construct new instances (e.g., new test_advance_mhd.MHDAdvanceTest() and test_advance_hybrid.HybridAdvanceTest()) inside permute_mhd/permute_hybrid (or within permute when selecting type) so each permutation gets a fresh helper; finally ensure helper cleanup is wired up by either returning the fresh test instance in the dict (so the caller/test harness can call its tearDown) or by registering the helper’s cleanup with the main test (so helper-specific cleanup runs rather than relying only on AdvanceTest.tearDown()).tests/simulator/advance/test_advance_mhd.py (1)
133-138:⚠️ Potential issue | 🟠 Major
qty="e"returnsNonesince E-field hierarchy is never loaded.When
qty="e", the condition at line 133 (qty in ["b", "eb", "fields"]) is false, soeb_hierremainsNone. Then line 137-138 returns thisNonevalue. UnlikeHybridAdvanceTestwhich loadsEM_E.h5for the "e" case, this implementation doesn't.Suggested fix (either load E or reject explicitly)
eb_hier = None + if qty in ["e", "eb", "fields"]: + eb_hier = hierarchy_from( + h5_filename=diag_outputs + "/EM_E.h5", hier=eb_hier + ) if qty in ["b", "eb", "fields"]: eb_hier = hierarchy_from( h5_filename=diag_outputs + "/EM_B.h5", hier=eb_hier ) if qty in ["e", "b", "eb"]: return eb_hierOr if E diagnostics are not available in MHD mode:
- if qty in ["e", "b", "eb"]: + if qty == "e": + raise ValueError("qty='e' is not supported in MHDAdvanceTest") + if qty in ["b", "eb"]: return eb_hier🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/advance/test_advance_mhd.py` around lines 133 - 138, The test currently never loads the E-field HDF5 so when qty == "e" it returns None; update the branching where qty is checked to ensure EM_E.h5 is read with hierarchy_from when qty includes "e" (e.g., add a branch or extend the existing condition to call hierarchy_from(h5_filename=diag_outputs + "/EM_E.h5", hier=eb_hier) or a new e_hier variable) and then return the appropriate hierarchy for "e" vs "b" vs "eb" (use the existing eb_hier/e_hier variables and the hierarchy_from call to mirror HybridAdvanceTest behavior).tests/simulator/advance/test_fields_advance_1d.py (1)
21-39: 🛠️ Refactor suggestion | 🟠 MajorReplace mutable defaults and consider test helper lifecycle.
Same issues as the 2D counterpart:
boxes={}mutable defaults at lines 25, 32, 36- Module-level
mhd_testandhybrid_testinstances shared across test variants- Cleanup registrations won't execute on helper instances
Suggested fix for mutable defaults
-mhd_test = test_advance_mhd.MHDAdvanceTest() -hybrid_test = test_advance_hybrid.HybridAdvanceTest() -def permute_hybrid(boxes={}): +def permute_hybrid(boxes=None): + boxes = boxes if boxes is not None else {} return [ dict(test=hybrid_test, interp_order=interp_order, refinement_boxes=boxes) for interp_order in interp_orders ] -def permute_mhd(boxes={}): # interp_order hax todo +def permute_mhd(boxes=None): # interp_order hax todo + boxes = boxes if boxes is not None else {} return [dict(test=mhd_test, interp_order=2, refinement_boxes=boxes)] -def permute(boxes={}, hybrid=True, mhd=False): +def permute(boxes=None, hybrid=True, mhd=False): + boxes = boxes if boxes is not None else {} return (permute_hybrid(boxes) if hybrid else []) + ( permute_mhd(boxes) if mhd else [] )Based on learnings: Mutable default arguments like lists or dictionaries can lead to unexpected behavior because the same instance of the mutable object is shared across all calls to the function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/advance/test_fields_advance_1d.py` around lines 21 - 39, The functions permute_hybrid, permute_mhd, and permute currently use a mutable default boxes={} and share module-level instances mhd_test and hybrid_test; change the signature of permute_hybrid(boxes=None), permute_mhd(boxes=None) and permute(boxes=None, hybrid=True, mhd=False) to default boxes to None and treat None as an empty dict inside the function, and stop using module-level test instances by instantiating fresh test objects inside the helpers (e.g., create new test_advance_hybrid.HybridAdvanceTest() in permute_hybrid and new test_advance_mhd.MHDAdvanceTest() in permute_mhd) so each variant gets its own lifecycle and any cleanup/registration on the instance will run per test.tests/simulator/test_diagnostics.py (1)
218-218:⚠️ Potential issue | 🔴 CriticalUse
unique_diag_dir_updatehere (helper name mismatch).Line 218 calls
self.unique_diag_update(...); the shared helper exposed intests/simulator/__init__.pyisunique_diag_dir_update(...). If no alias exists, this is a runtimeAttributeError.Suggested fix
- diag_path = self.unique_diag_update(f"{out}/test", simulation) + diag_path = self.unique_diag_dir_update(f"{out}/test", simulation)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/test_diagnostics.py` at line 218, Replace the incorrect helper call self.unique_diag_update(...) with the exposed helper name self.unique_diag_dir_update(...); locate the invocation in test_diagnostics.py (the line calling unique_diag_update) and update the symbol to unique_diag_dir_update so it matches the helper exported from tests/simulator/__init__.py and avoids a runtime AttributeError.
🧹 Nitpick comments (2)
tests/simulator/advance/test_fields_advance_2d.py (1)
79-79: Prefix unused variable with underscore.
largest_patch_sizeis unpacked but never used.Suggested fix
- largest_patch_size, smallest_patch_size = check_patch_size( + _largest_patch_size, smallest_patch_size = check_patch_size( ndim, interp_order=interp_order, cells=[60] * ndim )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/advance/test_fields_advance_2d.py` at line 79, The test unpacks two values from check_patch_size but never uses largest_patch_size; rename the unused variable to _largest_patch_size (or prefix with an underscore) so the tuple unpack becomes _largest_patch_size, smallest_patch_size = check_patch_size(...) and keep the call to check_patch_size intact; update any references to largest_patch_size (none expected) to use the underscore-prefixed name to satisfy linters.tests/simulator/advance/test_fields_advance_1d.py (1)
73-73: Prefix unused variable with underscore.
largest_patch_sizeis unpacked but never used.Suggested fix
- largest_patch_size, smallest_patch_size = check_patch_size( + _largest_patch_size, smallest_patch_size = check_patch_size( ndim, interp_order=interp_order, cells=[60] * ndim )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/advance/test_fields_advance_1d.py` at line 73, The variable largest_patch_size returned from check_patch_size is unused; change the unpacking to prefix the unused variable with an underscore (e.g., _largest_patch_size, smallest_patch_size) so the intent is clear and linters won't warn; update the assignment where check_patch_size(...) is called to use the underscore-prefixed name.
🤖 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/simulator/test_restarts.py`:
- Around line 450-451: After setting local_out and updating
ph.global_vars.sim.restart_options["dir"], also update the simulation input
dictionary so restart_time("auto") reads the correct directory; specifically
assign local_out into ph.global_vars.sim.simput["restart_options"]["dir"] (or
the sim.simput variable that holds sim inputs) right after the existing lines so
both ph.global_vars.sim.restart_options and simput["restart_options"]["dir"]
stay in sync for restart_time("auto").
- Around line 327-334: The restart directory is only set on
ph.global_vars.sim.restart_options["dir"] but the restarted simulation is
constructed from simput, so propagate the chosen directory to simput before
building the second simulation: after calling unique_diag_dir_update and
assigning diag_dir0/diag_dir1, also set simput.restart_options["dir"] (or the
equivalent restart option on the simput object) to the same diag_dir0/diag_dir1
so the restarted simulation built from simput reads the correct restart
directory.
---
Outside diff comments:
In `@tests/simulator/test_load_balancing.py`:
- Around line 32-45: In config(), replace the incorrect diag_dir reference with
the existing diag_outputs variable (use diag_outputs in the diag_options "dir"
value) and change the mutable default loadbalancing={} to loadbalancing=None and
then inside the function set loadbalancing = loadbalancing or {} to avoid
shared-state; likewise, in run_sim change the mutable default dic={} to dic=None
and set dic = dic or {} at the start of the function to prevent shared mutable
defaults—update references to the config() and run_sim() signatures accordingly.
---
Duplicate comments:
In `@tests/simulator/advance/test_advance_mhd.py`:
- Around line 133-138: The test currently never loads the E-field HDF5 so when
qty == "e" it returns None; update the branching where qty is checked to ensure
EM_E.h5 is read with hierarchy_from when qty includes "e" (e.g., add a branch or
extend the existing condition to call hierarchy_from(h5_filename=diag_outputs +
"/EM_E.h5", hier=eb_hier) or a new e_hier variable) and then return the
appropriate hierarchy for "e" vs "b" vs "eb" (use the existing eb_hier/e_hier
variables and the hierarchy_from call to mirror HybridAdvanceTest behavior).
In `@tests/simulator/advance/test_fields_advance_1d.py`:
- Around line 21-39: The functions permute_hybrid, permute_mhd, and permute
currently use a mutable default boxes={} and share module-level instances
mhd_test and hybrid_test; change the signature of permute_hybrid(boxes=None),
permute_mhd(boxes=None) and permute(boxes=None, hybrid=True, mhd=False) to
default boxes to None and treat None as an empty dict inside the function, and
stop using module-level test instances by instantiating fresh test objects
inside the helpers (e.g., create new test_advance_hybrid.HybridAdvanceTest() in
permute_hybrid and new test_advance_mhd.MHDAdvanceTest() in permute_mhd) so each
variant gets its own lifecycle and any cleanup/registration on the instance will
run per test.
In `@tests/simulator/advance/test_fields_advance_2d.py`:
- Around line 22-45: Replace the mutable default boxes={} with boxes=None in
permute_hybrid, permute_mhd, and permute, and initialize boxes = {} inside each
function; stop using module-level singletons mhd_test and hybrid_test—construct
new instances (e.g., new test_advance_mhd.MHDAdvanceTest() and
test_advance_hybrid.HybridAdvanceTest()) inside permute_mhd/permute_hybrid (or
within permute when selecting type) so each permutation gets a fresh helper;
finally ensure helper cleanup is wired up by either returning the fresh test
instance in the dict (so the caller/test harness can call its tearDown) or by
registering the helper’s cleanup with the main test (so helper-specific cleanup
runs rather than relying only on AdvanceTest.tearDown()).
In `@tests/simulator/test_diagnostics.py`:
- Line 218: Replace the incorrect helper call self.unique_diag_update(...) with
the exposed helper name self.unique_diag_dir_update(...); locate the invocation
in test_diagnostics.py (the line calling unique_diag_update) and update the
symbol to unique_diag_dir_update so it matches the helper exported from
tests/simulator/__init__.py and avoids a runtime AttributeError.
In `@tests/simulator/test_load_balancing.py`:
- Line 198: The run_sim function currently uses a mutable default argument
dic={}, which can leak state between calls; change the signature to def
run_sim(self, dic=None): and inside the method initialize with if dic is None:
dic = {} so each call gets a fresh dict; update any callers accordingly if they
rely on passing None explicitly.
---
Nitpick comments:
In `@tests/simulator/advance/test_fields_advance_1d.py`:
- Line 73: The variable largest_patch_size returned from check_patch_size is
unused; change the unpacking to prefix the unused variable with an underscore
(e.g., _largest_patch_size, smallest_patch_size) so the intent is clear and
linters won't warn; update the assignment where check_patch_size(...) is called
to use the underscore-prefixed name.
In `@tests/simulator/advance/test_fields_advance_2d.py`:
- Line 79: The test unpacks two values from check_patch_size but never uses
largest_patch_size; rename the unused variable to _largest_patch_size (or prefix
with an underscore) so the tuple unpack becomes _largest_patch_size,
smallest_patch_size = check_patch_size(...) and keep the call to
check_patch_size intact; update any references to largest_patch_size (none
expected) to use the underscore-prefixed name to satisfy linters.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
tests/simulator/__init__.pytests/simulator/advance/test_advance_hybrid.pytests/simulator/advance/test_advance_mhd.pytests/simulator/advance/test_fields_advance_1d.pytests/simulator/advance/test_fields_advance_2d.pytests/simulator/advance/test_particles_advance_1d.pytests/simulator/advance/test_particles_advance_2d.pytests/simulator/test_advance.pytests/simulator/test_diagnostics.pytests/simulator/test_initialization.pytests/simulator/test_load_balancing.pytests/simulator/test_restarts.pytests/simulator/test_vtk_diagnostics.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/simulator/init.py
- tests/simulator/test_vtk_diagnostics.py
- tests/simulator/test_initialization.py
PhilipDeegan
force-pushed
the
pre-mhd
branch
12 times, most recently
from
c0d0dbd to
0168cac
Compare
Member
Author
|
@coderabbitai did you review this? |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
tests/simulator/advance/test_advance_mhd.py (1)
137-142:⚠️ Potential issue | 🟠 Major
qty="e"currently resolves toNoneinstead of deterministic behavior.At Line 142,
"e"is included in the return branch, but no E hierarchy is loaded before returning.Suggested fix (explicitly reject unsupported `e`)
- if qty in ["e", "b", "eb"]: + if qty in ["b", "eb"]: return eb_hier + if qty == "e": + raise ValueError("qty='e' is unsupported in MHDAdvanceTest")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/advance/test_advance_mhd.py` around lines 137 - 142, The branch wrongly treats qty == "e" as if an E-field hierarchy exists: update the logic around the qty checks so "e" is not included in the return branch (remove "e" from the if condition that returns eb_hier) and add an explicit guard for unsupported qty values (e.g., raise a ValueError or Assertion for qty == "e" with a clear message). Adjust the code that calls hierarchy_from and the variable eb_hier (and the filename "EM_B.h5") accordingly so only supported quantities return a hierarchy and unsupported "e" fails deterministically.tests/simulator/test_load_balancing.py (1)
31-31:⚠️ Potential issue | 🟡 MinorReplace mutable default dicts with
Nonedefaults.Line 31 and Line 181 use
{}as default values, which creates shared state across calls.Suggested fix
-def config(sim, loadbalancing={}): +def config(sim, loadbalancing=None): + if loadbalancing is None: + loadbalancing = {} @@ - def run_sim(self, dic={}): + def run_sim(self, dic=None): + if dic is None: + dic = {} ph.global_vars.sim = None#!/bin/bash rg -nP 'def\s+\w+\([^)]*=\{\}' tests/simulator/test_load_balancing.pyAlso applies to: 181-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/test_load_balancing.py` at line 31, The function config currently uses a mutable default argument (loadbalancing={}); change the default to None and inside the function initialize loadbalancing = {} if it's None to avoid shared state across calls; update any other functions in this file that use {} defaults (the other occurrence noted at line 181) in the same way so both use None defaults and internal assignment.tests/simulator/test_restarts.py (1)
323-348:⚠️ Potential issue | 🟠 MajorSync
simput["restart_options"]["dir"]before constructing the restarted simulation.At Line 347, the second simulation is built from
simput, but onlysim.restart_options["dir"]is updated at Line 326. This can make the restart source directory stale for the second run.Suggested fix
sim = self.simulation(**simput) diag_dir0 = sim.diag_options["options"]["dir"] diag_dir1 = f"{diag_dir0}_n2" sim.restart_options["dir"] = diag_dir0 + simput["restart_options"]["dir"] = diag_dir0 self.assertEqual([seconds], sim.restart_options["elapsed_timestamps"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/test_restarts.py` around lines 323 - 348, The restart source directory in simput isn't being updated before creating the restarted Simulation: copy the current source dir into simput by setting simput["restart_options"]["dir"] = sim.restart_options["dir"] (or explicitly simput["restart_options"]["dir"] = diag_dir0) after you modify sim.restart_options and before calling ph.Simulation(**simput) so the restarted Simulation uses the correct restart dir; update the block that sets simput["diag_options"] and simput["restart_options"] accordingly.tests/simulator/advance/test_advance_hybrid.py (1)
25-25:⚠️ Potential issue | 🟡 MinorAvoid mutable default for
refinement_boxesingetHierarchy.Use a
Nonedefault to prevent cross-test state carry-over.Suggested fix
- refinement_boxes={}, + refinement_boxes=None, @@ - if smallest_patch_size is None: + if refinement_boxes is None: + refinement_boxes = {} + + if smallest_patch_size is None: from pyphare.pharein.simulation import check_patch_size🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/advance/test_advance_hybrid.py` at line 25, The getHierarchy call currently uses a mutable default refinement_boxes={} which can leak state across tests; change the function signature/usage to use refinement_boxes=None and inside getHierarchy (or its caller) set refinement_boxes = {} if refinement_boxes is None, then proceed, and update any tests calling getHierarchy accordingly to pass nothing or explicit dicts as needed.tests/simulator/advance/test_fields_advance_2d.py (1)
21-21:⚠️ Potential issue | 🟡 MinorReplace mutable default dicts in permutation helpers.
boxes={}is shared between calls. Switch toNonedefaults and initialize per call.Suggested fix
-def permute_hybrid(boxes={}): +def permute_hybrid(boxes=None): + boxes = {} if boxes is None else boxes return [ dict( super_class=HybridAdvanceTest, interp_order=interp_order, refinement_boxes=boxes, nbr_part_per_cell=ppc, ) for interp_order in interp_orders ] -def permute_mhd(boxes={}): # interp_order hax todo +def permute_mhd(boxes=None): # interp_order hax todo + boxes = {} if boxes is None else boxes return [dict(super_class=MHDAdvanceTest, interp_order=2, refinement_boxes=boxes)] -def permute(boxes={}, hybrid=True, mhd=False): +def permute(boxes=None, hybrid=True, mhd=False): + boxes = {} if boxes is None else boxes return (permute_hybrid(boxes) if hybrid else []) + ( permute_mhd(boxes) if mhd else [] )Also applies to: 33-33, 37-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/advance/test_fields_advance_2d.py` at line 21, The helper function permute_hybrid uses a mutable default argument (boxes={}) which is shared across calls; change the signature to use boxes=None and inside the function set boxes = {} if boxes is None, and do the same fix for the other permutation helper functions flagged in the review (the helpers on the other commented lines), ensuring each call gets a fresh dict instead of a shared default.tests/simulator/advance/test_fields_advance_1d.py (1)
20-20:⚠️ Potential issue | 🟡 MinorReplace mutable default dicts in permutation helpers.
boxes={}creates shared state across calls. UseNonedefaults and initialize inside each function.Suggested fix
-def permute_hybrid(boxes={}): +def permute_hybrid(boxes=None): + boxes = {} if boxes is None else boxes return [ dict( super_class=HybridAdvanceTest, interp_order=interp_order, refinement_boxes=boxes, ) for interp_order in interp_orders ] -def permute_mhd(boxes={}): # interp_order hax todo +def permute_mhd(boxes=None): # interp_order hax todo + boxes = {} if boxes is None else boxes return [dict(super_class=MHDAdvanceTest, interp_order=2, refinement_boxes=boxes)] -def permute(boxes={}, hybrid=True, mhd=False): +def permute(boxes=None, hybrid=True, mhd=False): + boxes = {} if boxes is None else boxes return (permute_hybrid(boxes) if hybrid else []) + ( permute_mhd(boxes) if mhd else [] )Also applies to: 31-31, 35-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/advance/test_fields_advance_1d.py` at line 20, Replace the mutable default dict argument in permute_hybrid (and the other permutation helper functions in this test file) by using boxes=None in the signature and initializing boxes = {} inside the function when None is passed; update all permutation helper defs that currently use boxes={} to follow this pattern so each call gets a fresh dict and no shared state is created across calls.
🧹 Nitpick comments (3)
tests/simulator/initialize/test_fields_init_1d.py (1)
41-42: Use direct superclass method calls instead of changingself.__class__.The runtime class reassignment pattern is error-prone in test runners. Call the chosen class method explicitly with
self.Proposed pattern
- self.__class__ = super_class # cast to super class - self._test_B_is_as_provided_by_user(ndim, **kwargs) + super_class._test_B_is_as_provided_by_user(self, ndim, **kwargs)Apply similarly to the other test methods.
Also applies to: 48-49, 55-56, 62-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/initialize/test_fields_init_1d.py` around lines 41 - 42, Replace the dynamic runtime class reassignment (the lines setting self.__class__ = super_class) with direct superclass method calls: instead of changing self.__class__, call the superclass method explicitly using super_class._test_B_is_as_provided_by_user(self, ndim, **kwargs) (and similarly for the other test methods referenced at lines 48-49, 55-56, 62-63), ensuring you pass self as the first argument so the instance is used without mutating its __class__.tests/simulator/test_diagnostics.py (1)
25-28: Makesetup_modelexplicit by passingsimulationin.Using
pyphare.pharein.global_vars.simhere makes the helper order-dependent and harder to reuse safely.Proposed refactor
-def setup_model(ppc=100): - from pyphare.pharein.global_vars import sim - - L = sim.simulation_domain() +def setup_model(sim, ppc=100): + L = sim.simulation_domain()And update call sites accordingly:
dump_all_diags(setup_model(simulation).populations)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/test_diagnostics.py` around lines 25 - 28, Change the helper to accept an explicit simulation object instead of pulling pyphare.pharein.global_vars.sim internally: modify setup_model to take a simulation parameter (e.g., def setup_model(simulation): ...) and use that simulation for L = simulation.simulation_domain() (or equivalent), and update all call sites (e.g., tests calling dump_all_diags) to pass the current simulation instance (e.g., dump_all_diags(setup_model(simulation).populations)) rather than relying on global_vars.sim so the helper is order-independent and reusable.tests/simulator/initialize/test_fields_init_2d.py (1)
43-44: Avoid mutatingself.__class__for dispatch.Reassigning
self.__class__at runtime is fragile. Prefer calling the selected class method directly withself.Proposed pattern
- self.__class__ = super_class # cast to super class - self._test_B_is_as_provided_by_user(ndim, **kwargs, nbr_part_per_cell=ppc) + super_class._test_B_is_as_provided_by_user( + self, ndim, **kwargs, nbr_part_per_cell=ppc + )Apply the same pattern to the other three test methods in this class.
Also applies to: 50-51, 57-58, 64-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/initialize/test_fields_init_2d.py` around lines 43 - 44, Do not reassign self.__class__; instead call the selected superclass method directly, e.g. invoke super_class._test_B_is_as_provided_by_user(self, ndim, **kwargs, nbr_part_per_cell=ppc) rather than setting self.__class__ = super_class, and apply the same change to the other three test methods in this class so each dispatch calls the target class method with self passed explicitly instead of mutating self.__class__.
🤖 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/simulator/advance/test_advance_mhd.py`:
- Line 23: The test function currently uses a mutable default argument
refinement_boxes={} which can share state across calls; change the parameter
default to refinement_boxes=None and inside the function initialize it with
refinement_boxes = {} if refinement_boxes is None (following the same pattern
used for smallest_patch_size and extra_diag_options in this file) so all
references to refinement_boxes within the function use the newly-initialized
local dict.
In `@tests/simulator/initialize/test_init_mhd.py`:
- Around line 136-142: The code returns eb_hier when qty == "e" even though
eb_hier was never initialized; change the branch handling so that when qty
includes "e" but you do not load an E diagnostic (i.e. eb_hier is still None),
you raise an explicit error instead of returning None. Concretely, in the block
using qty, after the checks that call hierarchy_from (symbol: hierarchy_from)
and before returning eb_hier, add a guard that if "e" in qty (or qty == "e") and
eb_hier is None then raise a ValueError/RuntimeError with a clear message that E
diagnostics are not configured; otherwise proceed to return eb_hier. This
ensures callers never get a silent None for eb_hier.
- Line 22: Change the mutable default for refinement_boxes from {} to None in
the function signature where refinement_boxes is defined (the parameter named
refinement_boxes in this test/fixture), and inside that function initialize it
with an empty dict when None (e.g., if refinement_boxes is None:
refinement_boxes = {}) following the same pattern used at the other
initialization site around line 52 so state is not shared across calls.
---
Duplicate comments:
In `@tests/simulator/advance/test_advance_hybrid.py`:
- Line 25: The getHierarchy call currently uses a mutable default
refinement_boxes={} which can leak state across tests; change the function
signature/usage to use refinement_boxes=None and inside getHierarchy (or its
caller) set refinement_boxes = {} if refinement_boxes is None, then proceed, and
update any tests calling getHierarchy accordingly to pass nothing or explicit
dicts as needed.
In `@tests/simulator/advance/test_advance_mhd.py`:
- Around line 137-142: The branch wrongly treats qty == "e" as if an E-field
hierarchy exists: update the logic around the qty checks so "e" is not included
in the return branch (remove "e" from the if condition that returns eb_hier) and
add an explicit guard for unsupported qty values (e.g., raise a ValueError or
Assertion for qty == "e" with a clear message). Adjust the code that calls
hierarchy_from and the variable eb_hier (and the filename "EM_B.h5") accordingly
so only supported quantities return a hierarchy and unsupported "e" fails
deterministically.
In `@tests/simulator/advance/test_fields_advance_1d.py`:
- Line 20: Replace the mutable default dict argument in permute_hybrid (and the
other permutation helper functions in this test file) by using boxes=None in the
signature and initializing boxes = {} inside the function when None is passed;
update all permutation helper defs that currently use boxes={} to follow this
pattern so each call gets a fresh dict and no shared state is created across
calls.
In `@tests/simulator/advance/test_fields_advance_2d.py`:
- Line 21: The helper function permute_hybrid uses a mutable default argument
(boxes={}) which is shared across calls; change the signature to use boxes=None
and inside the function set boxes = {} if boxes is None, and do the same fix for
the other permutation helper functions flagged in the review (the helpers on the
other commented lines), ensuring each call gets a fresh dict instead of a shared
default.
In `@tests/simulator/test_load_balancing.py`:
- Line 31: The function config currently uses a mutable default argument
(loadbalancing={}); change the default to None and inside the function
initialize loadbalancing = {} if it's None to avoid shared state across calls;
update any other functions in this file that use {} defaults (the other
occurrence noted at line 181) in the same way so both use None defaults and
internal assignment.
In `@tests/simulator/test_restarts.py`:
- Around line 323-348: The restart source directory in simput isn't being
updated before creating the restarted Simulation: copy the current source dir
into simput by setting simput["restart_options"]["dir"] =
sim.restart_options["dir"] (or explicitly simput["restart_options"]["dir"] =
diag_dir0) after you modify sim.restart_options and before calling
ph.Simulation(**simput) so the restarted Simulation uses the correct restart
dir; update the block that sets simput["diag_options"] and
simput["restart_options"] accordingly.
---
Nitpick comments:
In `@tests/simulator/initialize/test_fields_init_1d.py`:
- Around line 41-42: Replace the dynamic runtime class reassignment (the lines
setting self.__class__ = super_class) with direct superclass method calls:
instead of changing self.__class__, call the superclass method explicitly using
super_class._test_B_is_as_provided_by_user(self, ndim, **kwargs) (and similarly
for the other test methods referenced at lines 48-49, 55-56, 62-63), ensuring
you pass self as the first argument so the instance is used without mutating its
__class__.
In `@tests/simulator/initialize/test_fields_init_2d.py`:
- Around line 43-44: Do not reassign self.__class__; instead call the selected
superclass method directly, e.g. invoke
super_class._test_B_is_as_provided_by_user(self, ndim, **kwargs,
nbr_part_per_cell=ppc) rather than setting self.__class__ = super_class, and
apply the same change to the other three test methods in this class so each
dispatch calls the target class method with self passed explicitly instead of
mutating self.__class__.
In `@tests/simulator/test_diagnostics.py`:
- Around line 25-28: Change the helper to accept an explicit simulation object
instead of pulling pyphare.pharein.global_vars.sim internally: modify
setup_model to take a simulation parameter (e.g., def setup_model(simulation):
...) and use that simulation for L = simulation.simulation_domain() (or
equivalent), and update all call sites (e.g., tests calling dump_all_diags) to
pass the current simulation instance (e.g.,
dump_all_diags(setup_model(simulation).populations)) rather than relying on
global_vars.sim so the helper is order-independent and reusable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 903d05b9-04b6-47bd-9aaf-739dc301993f
📒 Files selected for processing (24)
tests/simulator/__init__.pytests/simulator/advance/test_advance_hybrid.pytests/simulator/advance/test_advance_mhd.pytests/simulator/advance/test_fields_advance_1d.pytests/simulator/advance/test_fields_advance_2d.pytests/simulator/advance/test_particles_advance_1d.pytests/simulator/advance/test_particles_advance_2d.pytests/simulator/initialize/test_fields_init_1d.pytests/simulator/initialize/test_fields_init_2d.pytests/simulator/initialize/test_init_hybrid.pytests/simulator/initialize/test_init_mhd.pytests/simulator/initialize/test_particles_init_1d.pytests/simulator/initialize/test_particles_init_2d.pytests/simulator/refinement/test_2d_10_core.pytests/simulator/refinement/test_2d_2_core.pytests/simulator/test_advance.pytests/simulator/test_diagnostics.pytests/simulator/test_exceptions.pytests/simulator/test_initialization.pytests/simulator/test_load_balancing.pytests/simulator/test_restarts.pytests/simulator/test_run.pytests/simulator/test_validation.pytests/simulator/test_vtk_diagnostics.py
💤 Files with no reviewable changes (2)
- tests/simulator/test_validation.py
- tests/simulator/test_exceptions.py
✅ Files skipped from review due to trivial changes (2)
- tests/simulator/refinement/test_2d_10_core.py
- tests/simulator/refinement/test_2d_2_core.py
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/simulator/test_diagnostics.py (1)
213-244:⚠️ Potential issue | 🟡 MinorSwitching to
self.simulation()also needs superclass cleanup here.These tests now register unique diag dirs through
SimulatorTest.simulation()(tests/simulator/__init__.py:257-271), butDiagnosticsTest.tearDown()never callssuper().tearDown(). Successful runs will keep accumulating diagnostic directories.Follow-up outside this hunk
def tearDown(self): super().tearDown() if self.simulator is not None: self.simulator.reset() self.simulator = None ph.global_vars.sim = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/test_diagnostics.py` around lines 213 - 244, The DiagnosticsTest class needs to invoke its superclass teardown so directories registered by SimulatorTest.simulation() are cleaned up; update DiagnosticsTest.tearDown() to call super().tearDown() at the start (or ensure it's invoked), then continue existing cleanup (resetting self.simulator and clearing ph.global_vars.sim) so diagnostic directories and simulator state do not accumulate across tests.tests/simulator/initialize/test_particles_init_2d.py (1)
26-30:⚠️ Potential issue | 🟡 MinorThis still exercises the helper's default PPC, not the new 2D value.
_test_nbr_particles_per_cell_is_as_provided()intests/simulator/initialize/test_init_hybrid.py:235-249defaultsdefault_ppcto100. Because Line 30 does not pass this module'sppcfrom Line 18, the refactor no longer checks the intended 10-particle configuration.Suggested fix
def test_nbr_particles_per_cell_is_as_provided(self, interp_order): print(f"{self._testMethodName}_{ndim}d") - self._test_nbr_particles_per_cell_is_as_provided(ndim, interp_order) + self._test_nbr_particles_per_cell_is_as_provided(ndim, interp_order, ppc)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/initialize/test_particles_init_2d.py` around lines 26 - 30, The test method Initialization2DTest.test_nbr_particles_per_cell_is_as_provided is calling the helper _test_nbr_particles_per_cell_is_as_provided(ndim, interp_order) but not passing this module's ppc, so the helper uses its default_ppc=100; update the call to include the ppc variable (i.e., _test_nbr_particles_per_cell_is_as_provided(ndim, interp_order, ppc)) so the test exercises the intended 2D ppc value.
♻️ Duplicate comments (2)
tests/simulator/initialize/test_init_mhd.py (1)
136-142:⚠️ Potential issue | 🟠 MajorHandle
qty="e"explicitly.Only
EM_B.h5is loaded here, so theqty == "e"branch returnsNone. Either load an E hierarchy here or reject"e"explicitly; the same branch is duplicated intests/simulator/advance/test_advance_mhd.py.Proposed fix
- if qty in ["e", "b", "eb"]: + if qty in ["b", "eb"]: return eb_hier + if qty == "e": + raise ValueError("qty='e' is unsupported in MHDInitializationTest")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/initialize/test_init_mhd.py` around lines 136 - 142, The current logic only loads the B-field hierarchy (hierarchy_from(..., h5_filename=diag_outputs + "/EM_B.h5", hier=eb_hier)) and then returns eb_hier when qty in ["e","b","eb"], which leaves qty=="e" returning None; fix by handling qty=="e" explicitly: either load the E-field hierarchy (call hierarchy_from with h5_filename=diag_outputs + "/EM_E.h5" into a new e_hier and return it when qty == "e") or raise a clear error (e.g., ValueError) rejecting "e" if E data is unsupported; apply the same change to the duplicated branch in tests/simulator/advance/test_advance_mhd.py and ensure you reference qty, hierarchy_from, eb_hier (and e_hier if added) consistently.tests/simulator/advance/test_advance_mhd.py (1)
136-142:⚠️ Potential issue | 🟠 Major
qty="e"still falls through withNone.This copy still only loads
EM_B.h5, so the"e"branch cannot return data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/advance/test_advance_mhd.py` around lines 136 - 142, The code only constructs eb_hier from "EM_B.h5" and then returns eb_hier for qty values including "e", causing qty="e" to return None; fix by separately loading the electron field hierarchy when qty includes "e" (e.g., create e_hier via hierarchy_from with "EM_E.h5") and return the appropriate hierarchy based on qty, or load both hierarchies (e_hier and eb_hier) and select the correct one; update the logic around the qty checks and the use of eb_hier, e_hier, hierarchy_from, and diag_outputs so "e" returns the data from "EM_E.h5" instead of None.
🧹 Nitpick comments (3)
pyphare/pyphare/core/phare_utilities.py (1)
188-190: Constraincast_toto the class changes this test harness actually supports.Bare
obj.__class__ = clsrelies on Python's runtime compatibility checks, so an unexpected caller gets a lateTypeErrorwith no indication of whether this helper only supports upcasts or arbitrary reassignment. If the intent is to switch a test instance to one of its base classes, make that explicit here and fail fast with a clearer error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyphare/pyphare/core/phare_utilities.py` around lines 188 - 190, The helper cast_to currently assigns obj.__class__ = cls unconditionally; constrain it to only allow switching an instance to one of its base classes by checking that issubclass(obj.__class__, cls) (i.e., current type is a subclass of the target cls) and raise a clear TypeError if not supported, and no-op or return the object unchanged when cls is the same as the current class; update the cast_to function to perform this validation and fail fast with a descriptive error message referencing cast_to and the obj.__class__ assignment.tests/simulator/test_restarts.py (1)
318-339: Trim the fixed elapsed-time wait.
Line 336sleeps 5 seconds before the first advance, which dominates this test block across the three permutations. A smaller threshold or an injected clock would keep the elapsed-restart coverage without paying a hard wall-clock tax.Based on learnings: 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.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/test_restarts.py` around lines 318 - 339, The test currently uses a hard 5-second sleep before the first advance (the call to sleep(...) before simulator.advance()), which unnecessarily slows the test; replace the real-time sleep with a much smaller wait (e.g., 0.05s) or, better, inject or mock the clock used by Simulator/Simulator.initialize so you can simulate elapsed time without wall-clock delay; update references around Simulator(...).initialize(), simulator.advance().dump(), and simulator.reset() to rely on the mocked/injected clock or reduced sleep so the restart elapsed-timestamp behavior is exercised without a 5s hard wait.tests/simulator/advance/test_fields_advance_2d.py (1)
22-41: Replace mutable default arguments withNone.The
boxes={}default on lines 22, 34, and 38 uses a mutable data structure as a default argument. While the functions don't mutateboxes, this is a Python anti-pattern that can cause subtle bugs if the code evolves.Suggested fix
-def permute_hybrid(boxes={}): +def permute_hybrid(boxes=None): + boxes = boxes if boxes is not None else {} return [ dict( super_class=HybridAdvanceTest, interp_order=interp_order, refinement_boxes=boxes, nbr_part_per_cell=ppc, ) for interp_order in interp_orders ] -def permute_mhd(boxes={}): # interp_order hax todo +def permute_mhd(boxes=None): # interp_order hax todo + boxes = boxes if boxes is not None else {} return [dict(super_class=MHDAdvanceTest, interp_order=2, refinement_boxes=boxes)] -def permute(boxes={}, hybrid=True, mhd=False): +def permute(boxes=None, hybrid=True, mhd=False): + boxes = boxes if boxes is not None else {} return (permute_hybrid(boxes) if hybrid else []) + ( permute_mhd(boxes) if mhd else [] )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/advance/test_fields_advance_2d.py` around lines 22 - 41, The three functions permute_hybrid, permute_mhd, and permute currently use a mutable default argument boxes={}, replace that default with boxes=None and inside each function set boxes = {} if boxes is None to avoid shared-state bugs; update permute_hybrid, permute_mhd, and permute to accept boxes=None and perform the local fallback before using boxes in the returned dicts.
🤖 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/simulator/__init__.py`:
- Around line 257-271: The function simulation() dereferences
sim.diag_options["options"]["dir"] without ensuring diag_options and its
"options"/"dir" keys exist; change the code to safely handle missing blocks by
first ensuring sim.diag_options is a dict and sim.diag_options["options"] exists
(create them if absent), then read base with something like base =
sim.diag_options.get("options", {}).get("dir", ".") so it won’t KeyError, and
finally set sim.diag_options["options"]["dir"] to the computed per-test
test_output; also keep the existing guard for sim.restart_options before
modifying it (refer to simulation, unique_diag_dir,
register_diag_dir_for_cleanup, and restart_options in your patch).
In `@tests/simulator/advance/test_advance_hybrid.py`:
- Around line 20-39: The getHierarchy helper currently uses a mutable default
refinement_boxes={} which can lead to shared state across tests; change the
parameter default to refinement_boxes=None and inside getHierarchy (function
name getHierarchy) initialize it with refinement_boxes = {} if refinement_boxes
is None so each call gets a fresh mapping; ensure all uses of refinement_boxes
in the function continue to work with this newly-initialized local dictionary.
In `@tests/simulator/initialize/test_init_hybrid.py`:
- Around line 235-249: The helper _test_nbr_particles_per_cell_is_as_provided
currently never forwards default_ppc into getHierarchy and calls
np.histogramdd(icells) with default 10 bins per axis; fix by passing default_ppc
into getHierarchy (e.g., via the particle-per-cell / PPC argument used by
getHierarchy) so the hierarchy is created with the requested PPC, and change the
histogram call to specify bins equal to the number of cells per axis (e.g.,
bins=tuple(n_cells_per_axis) where n_cells_per_axis is derived from the patch
box/grid like patch.box.nCells or patch.patch_grid.nCells) so
np.histogramdd(icells, bins=tuple(n_cells_per_axis)) counts particles per cell
correctly.
In `@tests/simulator/initialize/test_init_mhd.py`:
- Around line 128-132: Replace the non-existent ph.MHDDiagnostics calls with the
correct ph.FluidDiagnostics class: change the loop that currently does
ph.MHDDiagnostics(quantity=quantity, write_timestamps=timestamps) to
ph.FluidDiagnostics(quantity=quantity, write_timestamps=timestamps) (and any
other occurrences of ph.MHDDiagnostics, e.g., in the advance test file) so the
tests use the exported diagnostics API (ensure all occurrences in tests
referencing MHDDiagnostics are updated to FluidDiagnostics).
In `@tests/simulator/test_load_balancing.py`:
- Around line 181-199: run_sim uses self.simulation (implemented in
SimulatorTest) which appends generated diag dirs to self.diag_dirs but
LoadBalancingTest.tearDown currently doesn't trigger that cleanup; update
LoadBalancingTest.tearDown to invoke the parent cleanup (call
SimulatorTest.tearDown or super().tearDown() that resolves to
SimulatorTest.tearDown) or explicitly call clean_up_diags_dirs() before
resetting ph.global_vars.sim so the directories recorded by
SimulatorTest.simulation are removed (refer to run_sim, simulation,
self.diag_dirs, LoadBalancingTest.tearDown, SimulatorTest.tearDown,
clean_up_diags_dirs).
In `@tests/simulator/test_run.py`:
- Around line 251-255: The new plots directory (plot_dir) created when
cpp.mpi_rank() == 0 is not registered with the test teardown, so it persists
between runs; after creating plot_dir (the block that calls plot(diag_dir,
plot_dir)) register it with the same cleanup/teardown mechanism used for
diag_dir in SimulatorTest—e.g., add plot_dir to the SimulatorTest tracking or
call self.addCleanup to remove the directory—so the plots folder is removed
during test teardown.
- Around line 17-20: The test's timestamp array only contains 0.0 because
timestamps is built with a 0.05 step and final_time is 0.005, so the test never
checks post-advance outputs; update the timestamp generation so it includes
final_time (or uses the simulation time step) — e.g., replace the current
np.arange(0, final_time + 0.01, 0.05) logic in tests/simulator/test_run.py with
a construction based on time_step or time_step_nbr (for example using
np.arange(0, final_time + time_step, time_step) or np.linspace(0, final_time,
time_step_nbr + 1)) so that timestamps[-1] equals final_time and the test
verifies outputs produced after Simulator(...).run().
---
Outside diff comments:
In `@tests/simulator/initialize/test_particles_init_2d.py`:
- Around line 26-30: The test method
Initialization2DTest.test_nbr_particles_per_cell_is_as_provided is calling the
helper _test_nbr_particles_per_cell_is_as_provided(ndim, interp_order) but not
passing this module's ppc, so the helper uses its default_ppc=100; update the
call to include the ppc variable (i.e.,
_test_nbr_particles_per_cell_is_as_provided(ndim, interp_order, ppc)) so the
test exercises the intended 2D ppc value.
In `@tests/simulator/test_diagnostics.py`:
- Around line 213-244: The DiagnosticsTest class needs to invoke its superclass
teardown so directories registered by SimulatorTest.simulation() are cleaned up;
update DiagnosticsTest.tearDown() to call super().tearDown() at the start (or
ensure it's invoked), then continue existing cleanup (resetting self.simulator
and clearing ph.global_vars.sim) so diagnostic directories and simulator state
do not accumulate across tests.
---
Duplicate comments:
In `@tests/simulator/advance/test_advance_mhd.py`:
- Around line 136-142: The code only constructs eb_hier from "EM_B.h5" and then
returns eb_hier for qty values including "e", causing qty="e" to return None;
fix by separately loading the electron field hierarchy when qty includes "e"
(e.g., create e_hier via hierarchy_from with "EM_E.h5") and return the
appropriate hierarchy based on qty, or load both hierarchies (e_hier and
eb_hier) and select the correct one; update the logic around the qty checks and
the use of eb_hier, e_hier, hierarchy_from, and diag_outputs so "e" returns the
data from "EM_E.h5" instead of None.
In `@tests/simulator/initialize/test_init_mhd.py`:
- Around line 136-142: The current logic only loads the B-field hierarchy
(hierarchy_from(..., h5_filename=diag_outputs + "/EM_B.h5", hier=eb_hier)) and
then returns eb_hier when qty in ["e","b","eb"], which leaves qty=="e" returning
None; fix by handling qty=="e" explicitly: either load the E-field hierarchy
(call hierarchy_from with h5_filename=diag_outputs + "/EM_E.h5" into a new
e_hier and return it when qty == "e") or raise a clear error (e.g., ValueError)
rejecting "e" if E data is unsupported; apply the same change to the duplicated
branch in tests/simulator/advance/test_advance_mhd.py and ensure you reference
qty, hierarchy_from, eb_hier (and e_hier if added) consistently.
---
Nitpick comments:
In `@pyphare/pyphare/core/phare_utilities.py`:
- Around line 188-190: The helper cast_to currently assigns obj.__class__ = cls
unconditionally; constrain it to only allow switching an instance to one of its
base classes by checking that issubclass(obj.__class__, cls) (i.e., current type
is a subclass of the target cls) and raise a clear TypeError if not supported,
and no-op or return the object unchanged when cls is the same as the current
class; update the cast_to function to perform this validation and fail fast with
a descriptive error message referencing cast_to and the obj.__class__
assignment.
In `@tests/simulator/advance/test_fields_advance_2d.py`:
- Around line 22-41: The three functions permute_hybrid, permute_mhd, and
permute currently use a mutable default argument boxes={}, replace that default
with boxes=None and inside each function set boxes = {} if boxes is None to
avoid shared-state bugs; update permute_hybrid, permute_mhd, and permute to
accept boxes=None and perform the local fallback before using boxes in the
returned dicts.
In `@tests/simulator/test_restarts.py`:
- Around line 318-339: The test currently uses a hard 5-second sleep before the
first advance (the call to sleep(...) before simulator.advance()), which
unnecessarily slows the test; replace the real-time sleep with a much smaller
wait (e.g., 0.05s) or, better, inject or mock the clock used by
Simulator/Simulator.initialize so you can simulate elapsed time without
wall-clock delay; update references around Simulator(...).initialize(),
simulator.advance().dump(), and simulator.reset() to rely on the mocked/injected
clock or reduced sleep so the restart elapsed-timestamp behavior is exercised
without a 5s hard wait.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b8667ac9-0981-45a2-b065-b112510e95c3
📒 Files selected for processing (25)
pyphare/pyphare/core/phare_utilities.pytests/simulator/__init__.pytests/simulator/advance/test_advance_hybrid.pytests/simulator/advance/test_advance_mhd.pytests/simulator/advance/test_fields_advance_1d.pytests/simulator/advance/test_fields_advance_2d.pytests/simulator/advance/test_particles_advance_1d.pytests/simulator/advance/test_particles_advance_2d.pytests/simulator/initialize/test_fields_init_1d.pytests/simulator/initialize/test_fields_init_2d.pytests/simulator/initialize/test_init_hybrid.pytests/simulator/initialize/test_init_mhd.pytests/simulator/initialize/test_particles_init_1d.pytests/simulator/initialize/test_particles_init_2d.pytests/simulator/refinement/test_2d_10_core.pytests/simulator/refinement/test_2d_2_core.pytests/simulator/test_advance.pytests/simulator/test_diagnostics.pytests/simulator/test_exceptions.pytests/simulator/test_initialization.pytests/simulator/test_load_balancing.pytests/simulator/test_restarts.pytests/simulator/test_run.pytests/simulator/test_validation.pytests/simulator/test_vtk_diagnostics.py
💤 Files with no reviewable changes (2)
- tests/simulator/test_exceptions.py
- tests/simulator/test_validation.py
✅ Files skipped from review due to trivial changes (1)
- tests/simulator/refinement/test_2d_2_core.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/simulator/test_vtk_diagnostics.py
- tests/simulator/initialize/test_particles_init_1d.py
- tests/simulator/refinement/test_2d_10_core.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
split out hybrid and mhd simulation/hierarchy setup