force elapsed time test to save restart after first advance #786
Conversation
WalkthroughWalkthroughThe changes involve updates to the configuration and testing of a simulation environment. The Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- tests/simulator/config.py (1 hunks)
- tests/simulator/test_restarts.py (4 hunks)
Additional comments: 7
tests/simulator/test_restarts.py (7)
3-7:
The addition oftimeanddatetimeimports aligns with the changes in the test behavior.276-276:
The change totime_step_nbrensures that the simulation will restart after the first time step, as intended.281-282:
The addition of a RuntimeError to handle the absence of a refinement box version is a good practice to alert about potential regridding issues.292-294:
Settingrestart_idxto 1 correctly indicates that the restart should happen after the first advancement.303-307:
Thesecondsvariable andelapsed_timestampssetup align with the intended test behavior to dump diagnostics on the first advance.320-321:
Please verify the necessity of thetime.sleep(2)call, as it introduces a delay in the test execution. If it's not essential for the test logic, consider removing it to keep the code efficient, in line with your preference for minimal code.328-328:
The removal ofelapsed_timestampsfromsimput["restart_options"]ensures that the second simulation behaves differently from the first, which is necessary for the test's logic.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- tests/simulator/config.py (1 hunks)
- tests/simulator/test_restarts.py (5 hunks)
Additional comments: 9
tests/simulator/test_restarts.py (9)
3-4: The addition of
timeanddatetimeimports is appropriate for the new time-based features introduced in the tests.140-142: The early return based on
cpp.mpi_rank()is a standard practice in MPI-based parallel computing to ensure that only the master rank or a specific rank performs the diagnostics check.284-285: The addition of a
RuntimeErrorwhen "refinement" is not present insimputenforces stricter input requirements for the simulation tests, which is a good practice for ensuring that necessary configuration is provided.279-279: Updating
simput["time_step_nbr"]to 2 is intended to force a restart after the first advance, which is a valid test case for the restart functionality.295-295: The update of
restart_idxto 1 is consistent with the test's intention to check the restart functionality after a single time step.297-297: The
timestampslist now contains only one element, which is consistent with the updated test case that checks for a single restart event.306-306: The update of the
secondsvalue to 1 is likely related to the timing of the restart test, ensuring that the simulation dumps diagnostics on the first advance.310-311: The updates to
simput["restart_options"]["dir"]andsimput["diag_options"]["options"]["dir"]are necessary to reflect the new directory paths used in the tests.292-314: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [270-339]
The removal of the
_find_restart_timefunction and related code indicates a simplification of the restart time determination logic, which is a positive change for maintainability.
|
|
||
| time.sleep(seconds + 1) | ||
| time.sleep(5) |
There was a problem hiding this comment.
The time.sleep(5) call has been reduced to time.sleep(2), which may still be necessary for the test setup. However, considering the user's preference for minimal and efficient code, it's worth verifying if this delay is essential or if it can be further optimized.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- tests/simulator/config.py (1 hunks)
- tests/simulator/test_restarts.py (5 hunks)
Additional comments: 1
tests/simulator/test_restarts.py (1)
- 325-327: Given your preference for minimal and efficient code, please verify if the
time.sleep(5)is essential or if it can be further optimized.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- tests/simulator/config.py (1 hunks)
- tests/simulator/test_restarts.py (5 hunks)
Additional comments: 8
tests/simulator/config.py (2)
- 3-7: The
atvariable is still present in the code, which contradicts the previous comments about it being removed. Consider inlining the placeholder string to align with the preference for minimal and efficient code.- at = "@" # try to work without cmake config - if project_root == f"{at}PHARE_PROJECT_DIR{at}": + if project_root == "@PHARE_PROJECT_DIR@":
- 6-6: Verify that the
Pathclass frompathlibis not used elsewhere in the file outside of this conditional block. If it is, consider moving the import to the top of the file.tests/simulator/test_restarts.py (6)
3-4: The addition of
timeanddatetimeimports aligns with the changes made to handle timestamps and restart behavior.140-142: The early return for non-zero MPI ranks has been removed from the
check_diagsfunction. Ensure that this change is intentional and does not affect parallel execution of tests.284-285: The addition of a
RuntimeErrorwhen no refinement box is present due to regridding is a good practice for early failure and clear messaging.325-325: The
time.sleep(5)call has been removed, which aligns with the preference for minimal and efficient code. Confirm that this removal does not affect the test's reliability.295-296: The change in
restart_idxfrom 4 to 1 andsecondsfrom 1 to 9 is consistent with the summary's mention of adjustments to restart behavior.279-279: The
time_step_nbrhas been changed from 2 to 10 in thetest_restartsfunction. Verify that this change is intentional and aligns with the desired test behavior.
|
|
||
| if project_root == f"{at}PHARE_PROJECT_DIR{at}": | ||
| from pathlib import Path | ||
| project_root = Path(__file__).resolve().parent.parent.parent |
There was a problem hiding this comment.
Consider adding a check to verify that the new project_root path exists to prevent potential runtime errors.
if not project_root.exists():
raise RuntimeError(f"The project root path '{project_root}' does not exist.")
passing on my fork PhilipDeegan#79
Summary by CodeRabbit
New Features
Chores