prevent solvers from getting stuck at the same timepoint without giving an error #416
Conversation
![ai-maintainer[bot]](https://avatars.githubusercontent.com/in/339956?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI-Maintainer Review for PR - Prevent solvers from getting stuck at the same timepoint without giving an error
Title and Description 👍
The title and description are clear and concise
The title of the pull request is clear and effectively communicates the purpose of the changes. The description, while brief, provides some context about the motivation behind the changes. However, it would be beneficial to include more details about the problem and how the changes address it.
Scope of Changes 👍
The changes are narrowly focused
The changes in this pull request are narrowly focused on addressing a specific issue related to solvers getting stuck at the same timepoint without producing an error. There are no indications that the author is attempting to resolve multiple issues simultaneously.
Testing ⚠️
Testing details are missing
The description does not provide details on how the changes were tested. It would be beneficial for the author to include information about the testing methodology followed, any test cases used, and the results of the tests.
Code Changes 👍
The code changes are appropriate
The code changes appear to be appropriate for addressing the issue at hand. The addition of a check for whether the current timepoint is the same as the previous one, and setting the flag to -3 if it is, seems like a reasonable way to prevent solvers from getting stuck at the same timepoint without producing an error.
Recommendations
- Please provide more details in the description about the problem and how the changes address it.
- Include information about how the changes were tested, including the testing methodology, test cases, and results.
Reviewed with AI Maintainer
Codecov Report
@@ Coverage Diff @@
## master #416 +/- ##
==========================================
- Coverage 81.59% 81.55% -0.05%
==========================================
Files 11 11
Lines 1429 1431 +2
==========================================
+ Hits 1166 1167 +1
- Misses 263 264 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Sundials solver is allowed to warn and continue if it returns a timestep with no change in t. If the solver doesn't recover, it will fail with eg SciMLBase.ReturnCode.MaxIters Reverts PR SciML#416, fixes SciML#420 For CVode and ARKode, it looks like this is intended behaviour: the Sundial solver emits a warning message, with an API call CVodeSetMaxHnilWarns ARKStepSetMaxHnilWarns to set the maximum number of warning messages printed, which is exposed to Julia as max_hnil_warns see Sundials driver code https://github.com/LLNL/sundials/blob/e8a3e67e3883bc316c48bc534ee08319a5e8c620/src/cvode/cvode.c#L1339-L1347 For IDA, there is no API like this so it's not so clear what should happen, however the behaviour prior to SciML#416 (restored by this PR) for the f_noconverge test added in test/common_interface/ida.jl is to return SciMLBase.ReturnCode.MaxIters which seems reasonable? (NB: @oscardssmith I can't reproduce the issue implied by the title of SciML#416 so perhaps what is missing here is the original failure case that demonstrates the issue? The call to solve in the f_noconverge test test/common_interface/ida.jl, returns SciMLBase.ReturnCode.MaxIters, it doesn't return with no error? Also SciML/SciMLBase.jl#458 shows IDA printing [IDAS ERROR] IDASolve At t = 0 and h = 1.49012e-18, the error test failed repeatedly or with |h| = hmin. which suggests that IDA did flag an error in that case ? )
This was found while trying to find a MWE for SciML/SciMLBase.jl#458