Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pyphare/pyphare/pharein/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ def as_paths(rb):
if "dir" in restart_options:
restart_file_path = restart_options["dir"]

if "restart_time" in restart_options and restart_options["restart_time"] > 0:
if "restart_time" in restart_options:
from pyphare.cpp import cpp_etc_lib

restart_time = restart_options["restart_time"]
Expand Down
21 changes: 13 additions & 8 deletions pyphare/pyphare/pharein/simulation.py
Original file line number Diff line number Diff line change
Expand Up @@ -531,9 +531,10 @@ def check_restart_options(**kwargs):
"restart_time", # number or "auto"
"keep_last", # delete obsolete
]
restart_options = kwargs.get("restart_options", None)

if restart_options is not None:
restart_options = kwargs.get("restart_options", {})

if "restart_options" in kwargs:
for key in restart_options.keys():
if key not in valid_keys:
raise ValueError(
Expand All @@ -553,8 +554,11 @@ def check_restart_options(**kwargs):
f"Invalid restart mode {mode}, valid modes are {valid_modes}"
)

if restart_time := restarts.restart_time(restart_options):
restart_time = restarts.restart_time(restart_options)
if restart_time is not None:
restart_options["restart_time"] = restart_time
elif "restart_time" in restart_options:
restart_options.pop("restart_time") # auto with no existing file to use

return restart_options

Expand Down Expand Up @@ -650,7 +654,7 @@ def check_clustering(**kwargs):


def checker(func):
def wrapper(simulation_object, **kwargs):
def wrapper(simulation_object, **kwargs_in):
accepted_keywords = [
"domain_size",
"cells",
Expand Down Expand Up @@ -684,6 +688,7 @@ def wrapper(simulation_object, **kwargs):
"write_reports",
]

kwargs = deepcopy(kwargs_in) # local copy - dictionaries are weird
accepted_keywords += check_optional_keywords(**kwargs)

wrong_kwds = phare_utilities.not_in_keywords_list(accepted_keywords, **kwargs)
Expand All @@ -702,6 +707,7 @@ def wrapper(simulation_object, **kwargs):

kwargs["clustering"] = check_clustering(**kwargs)

kwargs["restart_options"] = check_restart_options(**kwargs)
time_step_nbr, time_step, final_time = check_time(**kwargs)
kwargs["time_step_nbr"] = time_step_nbr
kwargs["time_step"] = time_step
Expand All @@ -716,7 +722,6 @@ def wrapper(simulation_object, **kwargs):

ndim = compute_dimension(cells)
kwargs["diag_options"] = check_diag_options(**kwargs)
kwargs["restart_options"] = check_restart_options(**kwargs)

kwargs["boundary_types"] = check_boundaries(ndim, **kwargs)

Expand Down Expand Up @@ -1022,9 +1027,9 @@ def start_time(self):
return 0

def is_from_restart(self):
return (
self.restart_options is not None and "restart_time" in self.restart_options
)
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
Comment thread
coderabbitai[bot] marked this conversation as resolved.

def __getattr__(
self, name
Expand Down
1 change: 1 addition & 0 deletions src/simulator/simulator.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,7 @@ Simulator<opts>::Simulator(PHARE::initializer::PHAREDict const& dict,
{
resman_ptr = std::make_shared<ResourceManager_t>();
currentTime_ = restart_time(dict);
finalTime_ += currentTime_; // final time is from timestep * timestep_nbr!

if (dict["simulation"].contains("restarts"))
rMan = restarts::RestartsManagerResolver::make_unique(*hierarchy_, *resman_ptr,
Expand Down
18 changes: 14 additions & 4 deletions tests/simulator/test_restarts.py
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,7 @@ def test_advanced_restarts_options(self):
dup(
dict(
cells=10,
time_step_nbr=10,
time_step_nbr=7,
max_nbr_levels=1,
refinement="tagging",
)
Expand All @@ -440,22 +440,30 @@ def test_advanced_restarts_options(self):

simput["interp_order"] = interp
time_step = simput["time_step"]
time_step_nbr = simput["time_step_nbr"]

timestamps = time_step * np.arange(time_step_nbr + 1)
timestamps = time_step * np.arange(simput["time_step_nbr"] + 1)
local_out = self.unique_diag_dir_for_test_case(f"{out}/test", ndim, interp)
simput["restart_options"]["dir"] = local_out
simput["restart_options"]["keep_last"] = 3
simput["restart_options"]["timestamps"] = timestamps
simput["restart_options"]["restart_time"] = "auto"

ph.global_vars.sim = None
ph.global_vars.sim = ph.Simulation(**simput)
setup_model()
Simulator(ph.global_vars.sim).run().reset()
self.register_diag_dir_for_cleanup(local_out)

# restarted
timestamps = time_step * np.arange(7, 11)
simput["time_step_nbr"] = 3
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"]))
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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"]).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

ph.global_vars.sim = None
ph.global_vars.sim = ph.Simulation(**simput)
setup_model()
Simulator(ph.global_vars.sim).run().reset()

dirs = []
for path_object in Path(local_out).iterdir():
Expand All @@ -469,6 +477,8 @@ def test_advanced_restarts_options(self):
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"]))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same remark here

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's possible, but the tests are also passing, so it would be weird if they suddently stopped

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a comment why we expect binary ==



if __name__ == "__main__":
unittest.main()