[SPH] add the option of stopping evolve_until according to walltime#1874
Draft
tdavidcl wants to merge 1 commit into
Draft
[SPH] add the option of stopping evolve_until according to walltime#1874tdavidcl wants to merge 1 commit into
evolve_until according to walltime#1874tdavidcl wants to merge 1 commit into
Conversation
Contributor
|
Thanks @tdavidcl for opening this PR! You can do multiple things directly here: Once the workflow completes a message will appear displaying informations related to the run. Also the PR gets automatically reviewed by gemini, you can: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces walltime and global walltime limits to the SPH solver's evolve_until method, allowing simulations to stop gracefully when limits are reached. The implementation dynamically calculates the next check interval to minimize overhead. Feedback focuses on preventing undefined behavior in C++: first, by adding a bounds check before casting floating-point remaining iterations to an integer; and second, by performing a safe addition when calculating the next check iteration to prevent signed integer overflow.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+304
to
+309
| auto get_remaining_iters = [&](f64 delta_walltime, f64 factor) -> i32 { | ||
| if (sec_per_iter > 0) { | ||
| return static_cast<i32>(factor * delta_walltime / sec_per_iter); | ||
| } | ||
| return 1000; // default to 1000 iterations if sec_per_iter is 0 | ||
| }; |
Contributor
There was a problem hiding this comment.
Casting a floating-point value to an integer type when the value exceeds the range of the target type (e.g., if sec_per_iter is extremely small, making factor * delta_walltime / sec_per_iter larger than INT_MAX) results in undefined behavior in C++. To prevent this, we should check if the calculated value exceeds std::numeric_limits<i32>::max() before casting.
Suggested change
| auto get_remaining_iters = [&](f64 delta_walltime, f64 factor) -> i32 { | |
| if (sec_per_iter > 0) { | |
| return static_cast<i32>(factor * delta_walltime / sec_per_iter); | |
| } | |
| return 1000; // default to 1000 iterations if sec_per_iter is 0 | |
| }; | |
| auto get_remaining_iters = [&](f64 delta_walltime, f64 factor) -> i32 { | |
| if (sec_per_iter > 0) { | |
| f64 remaining = factor * delta_walltime / sec_per_iter; | |
| if (remaining >= static_cast<f64>(std::numeric_limits<i32>::max())) { | |
| return std::numeric_limits<i32>::max(); | |
| } | |
| return static_cast<i32>(remaining); | |
| } | |
| return 1000; // default to 1000 iterations if sec_per_iter is 0 | |
| }; |
| } | ||
| } | ||
|
|
||
| next_walltime_check_iter = iter_count + std::max(1, iters_to_next_check); |
Contributor
There was a problem hiding this comment.
If iters_to_next_check is very large (e.g., std::numeric_limits<i32>::max()), adding it to iter_count will cause signed integer overflow, which is undefined behavior in C++. Furthermore, if the overflow results in a negative value, iter_count >= next_walltime_check_iter will evaluate to true on every subsequent iteration. This will trigger the collective allreduce_max operation on every single step, severely degrading performance. We should perform a safe addition to prevent overflow.
i32 steps_to_check = std::max(1, iters_to_next_check);
if (std::numeric_limits<i32>::max() - iter_count < steps_to_check) {
next_walltime_check_iter = std::numeric_limits<i32>::max();
} else {
next_walltime_check_iter = iter_count + steps_to_check;
}
Contributor
Workflow reportworkflow report corresponding to commit 7b45dea Pre-commit check reportPre-commit check: ✅ Test pipeline can run. Clang-tidy diff reportDoxygen diff with
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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.
No description provided.