Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions cpp/memilio/compartments/simulation_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,31 @@ class SimulationBase
}
/** @} */

/**
* @brief Change the tolerance for the last time step.
* @param last_step_tolerance The new last_step_tolerance.
*/
void set_last_step_tolerance(FP new_tol)
{
m_integrator.set_last_step_tolerance(new_tol);
}

/**
* @brief Access the tolerance for the last time step.
* @return A reference to the last_step_tolerance.
* @{
*/
FP& get_last_step_tolerance()
{
return m_integrator.get_last_step_tolerance();
}
Comment on lines +171 to +174

Copy link
Copy Markdown
Member

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 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.


const FP& get_last_step_tolerance() const
{
return m_integrator.get_last_step_tolerance();
}
/** @} */

protected:
/**
* @brief Run the simulation up to a given time.
Expand Down
39 changes: 33 additions & 6 deletions cpp/memilio/math/integrator.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ class IntegratorCore
{
}

virtual ~IntegratorCore() {};
virtual ~IntegratorCore(){};

virtual std::unique_ptr<IntegratorCore<FP, Integrands...>> clone() const = 0;

Expand Down Expand Up @@ -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);

Expand All @@ -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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@reneSchm reneSchm Jun 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.
Expand All @@ -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,
Expand All @@ -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 {
Expand Down Expand Up @@ -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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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();
};

/**
Expand Down
12 changes: 12 additions & 0 deletions cpp/tests/test_compartments_simulation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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
Expand Down
14 changes: 14 additions & 0 deletions cpp/tests/test_numericalIntegration.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,20 @@ TEST(TestOdeIntegrator, integratorForcesLastStepSize)
}
}

TEST(TestOdeIntegrator, integratorLastStepTolerance)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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;
Expand Down
Loading