1570 Allow setting tolerance for last timestep in SystemIntegrator manually#1574
1570 Allow setting tolerance for last timestep in SystemIntegrator manually#1574annawendler wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1574 +/- ##
==========================================
- Coverage 97.47% 97.46% -0.02%
==========================================
Files 190 190
Lines 15966 16037 +71
==========================================
+ Hits 15563 15630 +67
- Misses 403 407 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
annawendler
changed the title
reneSchm
left a comment
reneSchm
left a comment
There was a problem hiding this comment.
Nice addition, though I'd like some changes.
| FP& get_last_step_tolerance() | ||
| { | ||
| return m_integrator.get_last_step_tolerance(); | ||
| } |
There was a problem hiding this comment.
This mashes two design philosophies. Either make a read-only getter (i.e. const FP& or simply FP) and a setter, or a read-write getter (without a setter). For integral types, which we can treat FP as, a read-write should be enough, as the setter won't do much.
| /** | ||
| * @brief Change the tolerance for the last time step. | ||
| * @param last_step_tolerance The new last_step_tolerance. | ||
| */ | ||
| void set_last_step_tolerance(FP last_step_tolerance) | ||
| { | ||
| m_last_step_tolerance = last_step_tolerance; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Access the tolerance for the last time step. | ||
| * @return A reference to the last_step_tolerance. | ||
| * @{ | ||
| */ | ||
| FP& get_last_step_tolerance() | ||
| { | ||
| return m_last_step_tolerance; | ||
| } | ||
|
|
||
| const FP& get_last_step_tolerance() const | ||
| { | ||
| return m_last_step_tolerance; | ||
| } | ||
| /** @} */ |
| sim.set_last_step_tolerance(new_tol); | ||
| EXPECT_EQ(sim.get_last_step_tolerance(), new_tol); |
There was a problem hiding this comment.
A more meaningful test would be to reproduce the issue you want to solve here.
A simplified set up should work too, e.g. make the simulation take a 0.8 step until 1.0, once with a 0.25 tolerance, and once with the default. Such that the first one stops at 0.8, the second at 1.0.
| } | ||
| } | ||
|
|
||
| TEST(TestOdeIntegrator, integratorLastStepTolerance) |
There was a problem hiding this comment.
I think running essentially the same test twice has little benefit, right? The other test should cover this sufficiently
| FP t = t0; | ||
|
|
||
| for (size_t i = results.get_num_time_points() - 1; fabs((tmax - t) / (tmax - t0)) > eps; ++i) { | ||
| for (size_t i = results.get_num_time_points() - 1; fabs((tmax - t) / (tmax - t0)) > m_last_step_tolerance; |
There was a problem hiding this comment.
We have (tmax - t) / (tmax - t0) <= 1 and (tmax - t) / (tmax - t0) --> 0.
So we should add a requirement that m_last_step_tolerance > FP{0.0}. I'd suggest an assert here as well as documentation on the getter.
There was a problem hiding this comment.
That is, for m_last_step_tolerance <= 0 we get an infinite loop, for m_last_step_tolerance >= 1, we simply don't do anything. So the valid range is (0, 1), though >=1 is less problematic.
2 participants
Changes and Information
Merge Request - Guideline Checklist
Please check our git workflow. Use the draft feature if the Pull Request is not yet ready to review.
Checks by code author
Checks by code reviewer(s)
Closes #1570