-
Notifications
You must be signed in to change notification settings - Fork 24
1570 Allow setting tolerance for last timestep in SystemIntegrator manually #1574
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,7 +60,7 @@ class IntegratorCore | |
| { | ||
| } | ||
|
|
||
| virtual ~IntegratorCore() {}; | ||
| virtual ~IntegratorCore(){}; | ||
|
|
||
| virtual std::unique_ptr<IntegratorCore<FP, Integrands...>> clone() const = 0; | ||
|
|
||
|
|
@@ -188,8 +188,7 @@ class SystemIntegrator | |
| using std::fabs; | ||
| using std::max; | ||
| using std::min; | ||
| const FP t0 = results.get_last_time(); | ||
| const FP eps = Limits<FP>::zero_tolerance(); | ||
| const FP t0 = results.get_last_time(); | ||
| assert(tmax > t0); | ||
| assert(dt > 0); | ||
|
|
||
|
|
@@ -205,7 +204,8 @@ class SystemIntegrator | |
| FP dt_min_restore = m_core->get_dt_min(); // used to restore dt_min, if it was decreased to reach tmax | ||
| 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; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| ++i) { | ||
| // We don't make time steps too small as the error estimator of an adaptive integrator | ||
| //may not be able to handle it. this is very conservative and maybe unnecessary, | ||
| //but also unlikely to happen. may need to be reevaluated. | ||
|
|
@@ -226,7 +226,7 @@ class SystemIntegrator | |
| results.get_last_time() = t; | ||
|
|
||
| // if dt has been changed by step, register the current m_core as adaptive. | ||
| m_is_adaptive |= !floating_point_equal<FP>(dt, dt_copy, eps); | ||
| m_is_adaptive |= !floating_point_equal<FP>(dt, dt_copy, Limits<FP>::zero_tolerance()); | ||
| } | ||
| m_core->get_dt_min() = dt_min_restore; // restore dt_min | ||
| // if dt was decreased to reach tmax in the last time iteration, | ||
|
|
@@ -237,7 +237,7 @@ class SystemIntegrator | |
| if (!step_okay) { | ||
| log_warning("Adaptive step sizing failed. Forcing an integration step of size dt_min."); | ||
| } | ||
| else if (fabs((tmax - t) / (tmax - t0)) > eps) { | ||
| else if (fabs((tmax - t) / (tmax - t0)) > m_last_step_tolerance) { | ||
| log_warning("Last time step too small. Could not reach tmax exactly."); | ||
| } | ||
| else { | ||
|
|
@@ -271,10 +271,37 @@ class SystemIntegrator | |
| { | ||
| return *m_core; | ||
| } | ||
| /** @} */ | ||
|
|
||
| /** | ||
| * @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; | ||
| } | ||
| /** @} */ | ||
|
Comment on lines
+276
to
+299
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above |
||
|
|
||
| private: | ||
| std::unique_ptr<Core> m_core; | ||
| bool m_is_adaptive; | ||
| FP m_last_step_tolerance = Limits<FP>::zero_tolerance(); | ||
| }; | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -90,6 +90,18 @@ TEST(TestCompartmentSimulation, copy_simulation) | |
| EXPECT_EQ(sim.get_integrator_core().get_dt_max(), sim_copy_assign.get_integrator_core().get_dt_max()); | ||
| } | ||
|
|
||
| TEST(TestCompartmentSimulation, last_step_tolerance) | ||
| { | ||
| auto sim = mio::Simulation<double, MockModel>(MockModel(), 0.0); | ||
|
|
||
| // Check default. | ||
| EXPECT_EQ(sim.get_last_step_tolerance(), mio::Limits<double>::zero_tolerance()); | ||
| // Check setter of tolerance. | ||
| auto new_tol = 1e-2; | ||
| sim.set_last_step_tolerance(new_tol); | ||
| EXPECT_EQ(sim.get_last_step_tolerance(), new_tol); | ||
|
Comment on lines
+101
to
+102
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
|
|
||
| struct MockSimulateSim { // looks just enough like a simulation for the simulate functions not to notice | ||
|
|
||
| // this "model" converts to and from int implicitly, exposing its value after calling chech_constraints | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -406,6 +406,20 @@ TEST(TestOdeIntegrator, integratorForcesLastStepSize) | |
| } | ||
| } | ||
|
|
||
| TEST(TestOdeIntegrator, integratorLastStepTolerance) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think running essentially the same test twice has little benefit, right? The other test should cover this sufficiently |
||
| { | ||
| using testing::_; | ||
| auto mock_core = std::make_unique<testing::StrictMock<MockIntegratorCore>>(); | ||
|
|
||
| auto integrator = mio::OdeIntegrator<double>(std::move(mock_core)); | ||
| // Check default. | ||
| EXPECT_EQ(integrator.get_last_step_tolerance(), mio::Limits<double>::zero_tolerance()); | ||
| // Check setter of tolerance. | ||
| auto new_tol = 1e-2; | ||
| integrator.set_last_step_tolerance(new_tol); | ||
| EXPECT_EQ(integrator.get_last_step_tolerance(), new_tol); | ||
| } | ||
|
|
||
| TEST(TestStochasticIntegrator, EulerMaruyamaIntegratorCore) | ||
| { | ||
| using X = Eigen::Vector2d; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mashes two design philosophies. Either make a read-only getter (i.e.
const FP&or simplyFP) 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.