Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pyphare/pyphare/pharein/restarts.py(2 hunks)pyphare/pyphare/pharein/simulation.py(3 hunks)tests/simulator/test_restarts.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-10-09T08:32:15.667Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-07-26T22:04:34.160Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1068
File: src/amr/data/field/coarsening/electric_field_coarsener.hpp:1-2
Timestamp: 2025-09-17T13:35:11.533Z
Learning: PhilipDeegan prefers header guard names that include the full directory path structure, following the pattern PHARE_[PATH_WITH_UNDERSCORES]_HPP. For example, a file at src/amr/data/field/coarsening/electric_field_coarsener.hpp should use PHARE_AMR_DATA_FIELD_COARSENING_ELECTRIC_FIELD_COARSENER_HPP as the header guard.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-09-05T17:02:58.784Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-10-09T08:32:15.667Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
🪛 Ruff (0.14.6)
tests/simulator/test_restarts.py
461-461: Local variable diag_dir0 is assigned to but never used
Remove assignment to unused variable diag_dir0
(F841)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: Analyze (python)
🔇 Additional comments (4)
pyphare/pyphare/pharein/restarts.py (1)
57-76: Clarifyrestart_time/autosemantics with new0default
restart_time()andfind_latest_time_from_restarts()now return0when the key is missing or when no restart directories exist. That makes “no restart data” indistinguishable from “restart at t = 0”, and callers that previously sawNoneor a missing key will now see0. As long as all call sites treat0as “fresh run / no prior restart” and don’t rely on distinguishing the two cases or on failing whenautofinds nothing, this change looks consistent with the updated tests.pyphare/pyphare/pharein/simulation.py (2)
630-690: Deep-copying kwargs and validating restart options earlyUsing
kwargs = deepcopy(kwargs)in the checker wrapper plus assigningkwargs["restart_options"] = check_restart_options(**kwargs)beforecheck_time(**kwargs)is a good cleanup: it avoids mutating the caller’s dicts (tests still usesimput["restart_options"]with"auto") and guaranteescheck_timesees a fully normalizedrestart_time. The added copy cost is negligible for one-time Simulation construction; overall this looks like a solid internal API tightening.
502-538:check_restart_optionsnow always injects arestart_timevalueNormalizing
restart_timeviarestarts.restart_time(restart_options)centralizes and simplifies logic for later stages likecheck_time(). However, this means the internalSimulation.restart_optionswill always contain a"restart_time"key when restart_options are provided—even when the user specified only defaults.If
Simulation.is_from_restart()relies on checking for the presence of"restart_time"key, this change will alter its return value. Confirm whether:
- The old distinction between "has restart_options but not from restart" and "actual restart" is relied upon downstream
is_from_restart()semantics should change, or if the key presence check needs adjustmenttests/simulator/test_restarts.py (1)
429-487: Advanced restart test cleanly exerciseskeep_last+restart_time("auto")The updated
test_advanced_restarts_optionsnicely verifies:
- First run dumps restarts at all steps up to 0.007 with
keep_last=3.- Second run restarts from
auto(asserting 0.007) withtime_step_nbr=3and timestamps 0.007–0.01.- After the second run, only the last three restart directories (0.008–0.01) remain and
restart_time("auto")reflects 0.01.Using
simput["time_step_nbr"]for the initial timestamps and hard-coding the[7, 11)range for the restarted window keeps the intent explicit while still tying the first phase to the configured time-step count. Also, dropping the unusedmodelvariable in favor of a baresetup_model()call is a good cleanup.
PhilipDeegan
force-pushed
the
restarts
branch
from
be3dc21 to
f5f91fb
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
PhilipDeegan
force-pushed
the
restarts
branch
from
f5f91fb to
69f2bb4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
PhilipDeegan
force-pushed
the
restarts
branch
from
69f2bb4 to
523aabd
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
PhilipDeegan
force-pushed
the
restarts
branch
from
523aabd to
7959c17
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
PhilipDeegan
force-pushed
the
restarts
branch
2 times, most recently
from
4ed833d to
b3d4c50
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
PhilipDeegan
force-pushed
the
restarts
branch
from
b3d4c50 to
99c4bfe
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
PhilipDeegan
force-pushed
the
restarts
branch
2 times, most recently
from
5b704c7 to
b5b97c5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
PhilipDeegan
force-pushed
the
restarts
branch
from
b5b97c5 to
9cc9ecd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pyphare/pyphare/pharein/simulation.py`:
- Around line 183-185: The division logic in the block guarded by final_and_dt
can produce time_step_nbr == 0 when total_time < kwargs["time_step"], causing a
ZeroDivisionError; change the computation to ensure a nonzero step count (e.g.
compute time_step_nbr from total_time and kwargs["time_step"] then coerce to at
least 1 using max(1, ...), or use an integer ceil) and then compute time_step =
total_time / time_step_nbr; update the code referencing time_step_nbr and
time_step (symbols: final_and_dt, time_step_nbr, time_step, kwargs["time_step"],
total_time) so it never divides by zero.
🧹 Nitpick comments (1)
pyphare/pyphare/pharein/simulation.py (1)
687-687: Nit:dict(**kwargs_in)is redundant.
kwargs_inis already adict(from**kwargs_inin the function signature), sodeepcopy(dict(**kwargs_in))can be simplified todeepcopy(kwargs_in). The intermediatedict()constructor with unpacking creates an unnecessary shallow copy before the deep copy.
PhilipDeegan
force-pushed
the
restarts
branch
from
38a89ab to
9b29ea7
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRestart gating was relaxed to trigger when the Changes
Sequence Diagram(s)sequenceDiagram
participant Driver
participant Wrapper as Python wrapper
participant RestartSvc as restart_options / restarts module
participant Simulator as C++ Simulator
Driver->>Wrapper: call wrapper(simulation_object, **kwargs_in)
Wrapper->>Wrapper: deepcopy kwargs -> kwargs
Wrapper->>RestartSvc: check_restart_options(kwargs)
alt restart_time key present
RestartSvc->>RestartSvc: resolve restart_time (e.g., "auto")
RestartSvc->>Wrapper: return restart_options (with restart_time)
Wrapper->>RestartSvc: load restart file / deserialize simulation
RestartSvc->>Simulator: provide currentTime_ (restart_time)
Wrapper->>Simulator: construct Simulator(..., currentTime_)
Simulator->>Simulator: finalTime_ += currentTime_
else no restart
Wrapper->>Simulator: construct Simulator(..., currentTime_=0)
end
Wrapper->>Simulator: run simulation loop (time steps using finalTime_)
Simulator-->>Driver: simulation complete / results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pyphare/pyphare/pharein/simulation.py`:
- Around line 1015-1018: is_from_restart currently returns the raw
restart_options["restart_time"], which makes a valid restart at 0 or 0.0
evaluate to False; change is_from_restart(self) to return a boolean (True if
restart_options is not None and "restart_time" in restart_options) and add a new
accessor restart_time(self) that returns restart_options["restart_time"] or
None; update callers (e.g., start_time()) to call restart_time() when they need
the numeric value and use is_from_restart() for boolean checks.
🧹 Nitpick comments (1)
pyphare/pyphare/pharein/simulation.py (1)
679-679: Minor:dict(**kwargs_in)is a redundant copy beforedeepcopy.
kwargs_inis already adict, so wrapping it indict()just creates an unnecessary shallow copy that is immediately deep-copied.Suggested simplification
- kwargs = deepcopy(dict(**kwargs_in)) # local copy - dictionaries are weird + kwargs = deepcopy(kwargs_in) # local copy - dictionaries are weird
PhilipDeegan
force-pushed
the
restarts
branch
from
9b29ea7 to
1062481
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/simulator/test_restarts.py`:
- Line 461: The test uses strict float equality causing fragility; replace
self.assertEqual(0.007, ph.restarts.restart_time(simput["restart_options"]))
with an approximate comparison (e.g., self.assertAlmostEqual(..., places=6) or
delta=1e-8) to tolerate tiny representation differences when parsing restart
timestamps from disk, and make the analogous change for the other assertion at
line 479 so both checks use assertAlmostEqual against
ph.restarts.restart_time(simput["restart_options"]).
| simput["restart_options"]["restart_time"] = "auto" | ||
| self.assertEqual(0.01, ph.restarts.restart_time(simput["restart_options"])) | ||
| simput["restart_options"]["timestamps"] = timestamps | ||
| self.assertEqual(0.007, ph.restarts.restart_time(simput["restart_options"])) |
There was a problem hiding this comment.
Potential floating-point fragility in assertEqual with computed restart times.
assertEqual(0.007, ...) depends on exact float representation of the directory name parsed from disk. If the C++ side writes the timestamp with a slightly different precision (e.g., 0.00700000000000001), this assertion would fail. Consider using assertAlmostEqual for consistency with line 477 which already uses it.
Suggested fix
- self.assertEqual(0.007, ph.restarts.restart_time(simput["restart_options"]))
+ self.assertAlmostEqual(0.007, ph.restarts.restart_time(simput["restart_options"]))Similarly on line 479:
- self.assertEqual(0.01, ph.restarts.restart_time(simput["restart_options"]))
+ self.assertAlmostEqual(0.01, ph.restarts.restart_time(simput["restart_options"]))🤖 Prompt for AI Agents
In `@tests/simulator/test_restarts.py` at line 461, The test uses strict float
equality causing fragility; replace self.assertEqual(0.007,
ph.restarts.restart_time(simput["restart_options"])) with an approximate
comparison (e.g., self.assertAlmostEqual(..., places=6) or delta=1e-8) to
tolerate tiny representation differences when parsing restart timestamps from
disk, and make the analogous change for the other assertion at line 479 so both
checks use assertAlmostEqual against
ph.restarts.restart_time(simput["restart_options"]).
There was a problem hiding this comment.
this is a legit comment @PhilipDeegan we should probably check the abs(time-0.007) is less than 1e-15 or something.
There is this https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertAlmostEqual
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
PhilipDeegan
force-pushed
the
restarts
branch
from
1062481 to
37b3bfd
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pyphare/pyphare/pharein/simulation.py (1)
1029-1032:is_from_restartnow correctly returns a boolean — addresses past review.The redundant inner
is not Noneguard is harmless (sincecheck_restart_optionsnever storesNoneunder the"restart_time"key — it pops the key instead), but can be simplified to match the past review's suggestion:♻️ Optional simplification
def is_from_restart(self): - if self.restart_options is not None and "restart_time" in self.restart_options: - return self.restart_options["restart_time"] is not None - return False + return ( + self.restart_options is not None + and "restart_time" in self.restart_options + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyphare/pyphare/pharein/simulation.py` around lines 1029 - 1032, The is_from_restart method contains a redundant inner "is not None" check for restart_options["restart_time"]; simplify it by returning a boolean that checks only whether restart_options exists and contains the "restart_time" key. Update the is_from_restart function to use a concise expression referencing self.restart_options and the "restart_time" key (e.g., check presence of the key while guarding against self.restart_options being None) and remove the unnecessary value-none check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/simulator/test_restarts.py`:
- Line 461: Replace the fragile float equality checks that use self.assertEqual
on the value returned by ph.restarts.restart_time(simput["restart_options"])
(which ultimately comes from find_latest_time_from_restarts) with
self.assertAlmostEqual to allow for minor serialization precision differences;
update both occurrences that currently call assertEqual (including the one at
the other assertion for the same logic) to assertAlmostEqual and keep the
expected value and any delta/places parameter consistent with the tolerance used
elsewhere in the file (e.g., the pattern at the other test that already uses
assertAlmostEqual).
---
Nitpick comments:
In `@pyphare/pyphare/pharein/simulation.py`:
- Around line 1029-1032: The is_from_restart method contains a redundant inner
"is not None" check for restart_options["restart_time"]; simplify it by
returning a boolean that checks only whether restart_options exists and contains
the "restart_time" key. Update the is_from_restart function to use a concise
expression referencing self.restart_options and the "restart_time" key (e.g.,
check presence of the key while guarding against self.restart_options being
None) and remove the unnecessary value-none check.
37b3bfd to
40ecf8d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/simulator/simulator.hpp (1)
362-362: Misleading comment — clarify intent.The comment
// final time is from timestep * timestep_nbr!describes the initializer-list expression already computed on line 356, not the purpose of this statement. The actual intent is to shift the absolute end-time forward by the restart offset.✏️ Suggested comment fix
- finalTime_ += currentTime_; // final time is from timestep * timestep_nbr! + finalTime_ += currentTime_; // shift absolute end-time by restart offset🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simulator/simulator.hpp` at line 362, The comment on the statement "finalTime_ += currentTime_;" is misleading; update it to clearly state that this line shifts the absolute simulation end time by the restart offset (i.e., adds the current restart time to finalTime_), referencing the variables finalTime_ and currentTime_ so future readers understand this is applying the restart time offset rather than recomputing timestep*count.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/simulator/simulator.hpp`:
- Line 362: The comment on the statement "finalTime_ += currentTime_;" is
misleading; update it to clearly state that this line shifts the absolute
simulation end time by the restart offset (i.e., adds the current restart time
to finalTime_), referencing the variables finalTime_ and currentTime_ so future
readers understand this is applying the restart time offset rather than
recomputing timestep*count.
| simput["restart_options"]["restart_time"] = "auto" | ||
| self.assertEqual(0.01, ph.restarts.restart_time(simput["restart_options"])) | ||
| simput["restart_options"]["timestamps"] = timestamps | ||
| self.assertEqual(0.007, ph.restarts.restart_time(simput["restart_options"])) |
There was a problem hiding this comment.
this is a legit comment @PhilipDeegan we should probably check the abs(time-0.007) is less than 1e-15 or something.
There is this https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertAlmostEqual
| for i, idx in enumerate(range(8, 11)): | ||
| self.assertAlmostEqual(dirs[i], time_step * idx) | ||
|
|
||
| self.assertEqual(0.01, ph.restarts.restart_time(simput["restart_options"])) |
There was a problem hiding this comment.
it's possible, but the tests are also passing, so it would be weird if they suddently stopped
There was a problem hiding this comment.
added a comment why we expect binary ==
PhilipDeegan
force-pushed
the
restarts
branch
from
40ecf8d to
7dfc1b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/simulator/test_restarts.py (1)
468-468:assertEqualfor float directory-name comparisonAlready flagged in a prior review — using
assertAlmostEqual(as at line 484) is more robust against floating-point representation differences when parsing timestamps from directory names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/test_restarts.py` at line 468, The test uses assertEqual for a floating-point comparison of the parsed restart time; change the assertion at the call to ph.restarts.restart_time(simput["restart_options"]) to use assertAlmostEqual (as done at line 484) so the float comparison tolerates minor representation differences when parsing timestamps from directory names; update the assertion in tests/simulator/test_restarts.py accordingly to call self.assertAlmostEqual(expected, ph.restarts.restart_time(simput["restart_options"])) with the same default or an explicit delta/places if desired.pyphare/pyphare/pharein/__init__.py (1)
368-368: Guard change from truthiness to key-presence correctly handlesrestart_time = 0.0The old truthiness check would silently skip the restart-load block for a valid restart at
t = 0. The key-presence check is correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyphare/pyphare/pharein/__init__.py` at line 368, The truthiness check for restart options incorrectly skipped valid restarts at t=0; change any conditional like if restart_options.get("restart_time") or if restart_options["restart_time"] to a key-presence check using if "restart_time" in restart_options (as shown) so that restart_time == 0.0 is handled, and ensure code paths that read restart_options["restart_time"] proceed even when the value is falsy.pyphare/pyphare/pharein/simulation.py (1)
1029-1032:is_from_restart()now correctly returns a booleanReturns
Falseforrestart_options = {}(key absent) andTruefor a resolved numericrestart_time. Properly handlesrestart_time = 0.0edge case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyphare/pyphare/pharein/simulation.py` around lines 1029 - 1032, Replace the old is_from_restart implementation with a robust boolean check that returns True only when restart_options exists, contains the "restart_time" key, and that value is not None (so restart_time == 0.0 returns True), otherwise return False; update the function named is_from_restart in class Simulation to use this logic (check self.restart_options is not None, "restart_time" in self.restart_options, and self.restart_options["restart_time"] is not None).
🧹 Nitpick comments (2)
src/simulator/simulator.hpp (1)
361-362:restart_time(dict)is invoked twice for the same result
currentTime_(constructor body, line 361) andstartTime_(hybrid_init, line 331) both callrestart_time(dict)and read the same dict path. Consider passingcurrentTime_as a parameter tohybrid_initto avoid the redundant lookup.♻️ Suggested refactor sketch
// Declaration in class body: - void hybrid_init(initializer::PHAREDict const&); + void hybrid_init(initializer::PHAREDict const&, double start_time); // Constructor body: currentTime_ = restart_time(dict); finalTime_ += currentTime_; ... if (find_model("HybridModel")) - hybrid_init(dict); + hybrid_init(dict, currentTime_); // hybrid_init definition: -void Simulator<opts>::hybrid_init(initializer::PHAREDict const& dict) +void Simulator<opts>::hybrid_init(initializer::PHAREDict const& dict, double start_time) { ... - startTime_ = restart_time(dict); + startTime_ = start_time;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simulator/simulator.hpp` around lines 361 - 362, restart_time(dict) is being called twice (once to set currentTime_ in the constructor and again inside hybrid_init to set startTime_), causing a redundant lookup; change hybrid_init to accept the already-read time (pass currentTime_ into hybrid_init and use that to set startTime_ instead of calling restart_time(dict) again), update all callers of hybrid_init to pass currentTime_ (or a new parameter name like start_time) and remove the internal call to restart_time(dict) inside hybrid_init so startTime_ is assigned from the passed-in value; ensure finalTime_ logic (finalTime_ += currentTime_) remains correct with the new parameter.tests/simulator/test_restarts.py (1)
486-486: ConsiderassertAlmostEqualfor the new0.01assertionSame floating-point fragility as line 468 — the resolved timestamp comes from a directory name parsed as a float.
- self.assertEqual(0.01, ph.restarts.restart_time(simput["restart_options"])) + self.assertAlmostEqual(0.01, ph.restarts.restart_time(simput["restart_options"]))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/simulator/test_restarts.py` at line 486, The assertion comparing floating-point values is fragile; replace the exact equality check in tests/simulator/test_restarts.py (the line asserting 0.01 == ph.restarts.restart_time(simput["restart_options"])) with a tolerant comparison using unittest.TestCase.assertAlmostEqual (or assertAlmostEqual with an appropriate places/delta) to compare 0.01 and ph.restarts.restart_time(simput["restart_options"]) to avoid failures from float parsing imprecision.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyphare/pyphare/pharein/simulation.py`:
- Around line 535-536: The defaulting of restart_options to {} makes the
existing "is not None" guards stale; change the guards to falsy checks so logic
only runs when restart_options is provided/true: update the
check_restart_options usage and replace any "if restart_options is not None"
(used around validate_restart_options and in populateDict) with "if
restart_options" (or equivalent truthy check) so validate_restart_options only
calls restarts.validate(sim) for real restart options and populateDict only
executes the restart block (including the add_string calls for "filePath" and
"serialized_simulation") when restart_options contains actual restart
configuration, preventing unintended creation of
RestartsManager/RestartsManagerResolver::make_unique for non-restart
simulations; key symbols: restart_options, check_restart_options,
validate_restart_options, populateDict, add_string, serialized_sim,
RestartsManager.
---
Duplicate comments:
In `@pyphare/pyphare/pharein/__init__.py`:
- Line 368: The truthiness check for restart options incorrectly skipped valid
restarts at t=0; change any conditional like if
restart_options.get("restart_time") or if restart_options["restart_time"] to a
key-presence check using if "restart_time" in restart_options (as shown) so that
restart_time == 0.0 is handled, and ensure code paths that read
restart_options["restart_time"] proceed even when the value is falsy.
In `@pyphare/pyphare/pharein/simulation.py`:
- Around line 1029-1032: Replace the old is_from_restart implementation with a
robust boolean check that returns True only when restart_options exists,
contains the "restart_time" key, and that value is not None (so restart_time ==
0.0 returns True), otherwise return False; update the function named
is_from_restart in class Simulation to use this logic (check
self.restart_options is not None, "restart_time" in self.restart_options, and
self.restart_options["restart_time"] is not None).
In `@tests/simulator/test_restarts.py`:
- Line 468: The test uses assertEqual for a floating-point comparison of the
parsed restart time; change the assertion at the call to
ph.restarts.restart_time(simput["restart_options"]) to use assertAlmostEqual (as
done at line 484) so the float comparison tolerates minor representation
differences when parsing timestamps from directory names; update the assertion
in tests/simulator/test_restarts.py accordingly to call
self.assertAlmostEqual(expected,
ph.restarts.restart_time(simput["restart_options"])) with the same default or an
explicit delta/places if desired.
---
Nitpick comments:
In `@src/simulator/simulator.hpp`:
- Around line 361-362: restart_time(dict) is being called twice (once to set
currentTime_ in the constructor and again inside hybrid_init to set startTime_),
causing a redundant lookup; change hybrid_init to accept the already-read time
(pass currentTime_ into hybrid_init and use that to set startTime_ instead of
calling restart_time(dict) again), update all callers of hybrid_init to pass
currentTime_ (or a new parameter name like start_time) and remove the internal
call to restart_time(dict) inside hybrid_init so startTime_ is assigned from the
passed-in value; ensure finalTime_ logic (finalTime_ += currentTime_) remains
correct with the new parameter.
In `@tests/simulator/test_restarts.py`:
- Line 486: The assertion comparing floating-point values is fragile; replace
the exact equality check in tests/simulator/test_restarts.py (the line asserting
0.01 == ph.restarts.restart_time(simput["restart_options"])) with a tolerant
comparison using unittest.TestCase.assertAlmostEqual (or assertAlmostEqual with
an appropriate places/delta) to compare 0.01 and
ph.restarts.restart_time(simput["restart_options"]) to avoid failures from float
parsing imprecision.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pyphare/pyphare/pharein/__init__.pypyphare/pyphare/pharein/simulation.pysrc/simulator/simulator.hpptests/simulator/test_restarts.py
2 participants
better tested