Skip to content

Commit 252da79

Browse files
authored
Fix ReturnData::{preeq_wrms,posteq_wrms} with FSA and check_sensi_steadystate_conv_=True (#2071)
Documentation says, that ReturnData::{preeq_wrms,posteq_wrms} refers to the `weighted root-mean-square of the rhs when steadystate was reached`, independent of `Solver::check_sensi_steadystate_conv_`, but this wasn't what happened. Now it is. Closes #2066 Furthermore, this prevents unnecessary simulation if we are already in a steadystate when entering `SteadystateProblem::runSteadystateSimulation`.
1 parent ba70f54 commit 252da79

File tree

3 files changed

+38
-30
lines changed

3 files changed

+38
-30
lines changed

python/tests/test_preequilibration.py

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,14 @@
22

33
import itertools
44

5-
import amici
65
import numpy as np
76
import pytest
7+
from numpy.testing import assert_allclose
8+
9+
import amici
810
from test_pysb import get_data
911

12+
1013
@pytest.fixture
1114
def preeq_fixture(pysb_example_presimulation_module):
1215

@@ -115,11 +118,12 @@ def test_manual_preequilibration(preeq_fixture):
115118
assert rdata_sim.status == amici.AMICI_SUCCESS
116119

117120
for variable in ['x', 'sx']:
118-
assert np.isclose(
121+
assert_allclose(
119122
rdata_auto[variable],
120123
rdata_sim[variable],
121-
1e-6, 1e-6
122-
).all(), dict(pscale=pscale, plist=plist, variable=variable)
124+
atol=1e-6, rtol=1e-6,
125+
err_msg=str(dict(pscale=pscale, plist=plist, variable=variable))
126+
)
123127

124128

125129
def test_parameter_reordering(preeq_fixture):
@@ -172,11 +176,12 @@ def test_data_replicates(preeq_fixture):
172176
rdata_double = amici.runAmiciSimulation(model, solver, edata)
173177

174178
for variable in ['llh', 'sllh']:
175-
assert np.isclose(
176-
2*rdata_single[variable],
179+
assert_allclose(
180+
2 * rdata_single[variable],
177181
rdata_double[variable],
178-
1e-6, 1e-6
179-
).all(), dict(variable=variable, sensi_meth=sensi_meth)
182+
atol=1e-6, rtol=1e-6,
183+
err_msg=str(dict(variable=variable, sensi_meth=sensi_meth))
184+
)
180185

181186

182187
def test_parameter_in_expdata(preeq_fixture):

python/tests/test_pysb.py

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919

2020
from amici.gradient_check import check_derivatives
2121
from amici.testing import skip_on_valgrind, TemporaryDirectoryWinSafe
22-
22+
from numpy.testing import assert_allclose
2323

2424
@skip_on_valgrind
2525
def test_compare_to_sbml_import(pysb_example_presimulation_module,
@@ -82,10 +82,11 @@ def test_compare_to_sbml_import(pysb_example_presimulation_module,
8282
elif np.isnan(rdata_pysb[field]).all():
8383
assert np.isnan(rdata_sbml[field]).all(), field
8484
else:
85-
assert np.isclose(
85+
assert_allclose(
8686
rdata_sbml[field], rdata_pysb[field],
87-
atol=1e-6, rtol=1e-6
88-
).all(), field
87+
atol=1e-6, rtol=1e-6,
88+
err_msg=field
89+
)
8990

9091

9192
pysb_models = [

src/steadystateproblem.cpp

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -665,9 +665,6 @@ void SteadystateProblem::runSteadystateSimulation(
665665
if (backward)
666666
sensitivityFlag = SensitivityMethod::adjoint;
667667

668-
/* If run after Newton's method checks again if it converged */
669-
wrms_ = getWrms(model, sensitivityFlag);
670-
671668
int &sim_steps = backward ? numstepsB_ : numsteps_.at(1);
672669

673670
int convergence_check_frequency = 1;
@@ -676,10 +673,30 @@ void SteadystateProblem::runSteadystateSimulation(
676673
convergence_check_frequency = 25;
677674

678675
while (true) {
676+
if (sim_steps % convergence_check_frequency == 0) {
677+
// Check for convergence (already before simulation, since we might
678+
// start in steady state)
679+
wrms_ = getWrms(model, sensitivityFlag);
680+
if (wrms_ < conv_thresh) {
681+
if(check_sensi_conv_
682+
&& sensitivityFlag == SensitivityMethod::forward) {
683+
updateSensiSimulation(solver);
684+
// getWrms needs to be called before getWrmsFSA
685+
// such that the linear system is prepared for newton-type
686+
// convergence check
687+
if (getWrmsFSA(model) < conv_thresh)
688+
break; // converged
689+
} else {
690+
break; // converged
691+
}
692+
}
693+
}
694+
679695
/* check for maxsteps */
680696
if (sim_steps >= solver.getMaxSteps()) {
681697
throw IntegrationFailure(AMICI_TOO_MUCH_WORK, state_.t);
682698
}
699+
683700
/* increase counter */
684701
sim_steps++;
685702
/* One step of ODE integration
@@ -698,21 +715,6 @@ void SteadystateProblem::runSteadystateSimulation(
698715
xQ_);
699716
flagUpdatedState();
700717
}
701-
702-
/* Check for convergence */
703-
if (sim_steps % convergence_check_frequency == 0) {
704-
wrms_ = getWrms(model, sensitivityFlag);
705-
/* getWrms needs to be called before getWrmsFSA such that the linear
706-
system is prepared for newton type convergence check */
707-
if (wrms_ < conv_thresh && check_sensi_conv_ &&
708-
sensitivityFlag == SensitivityMethod::forward) {
709-
updateSensiSimulation(solver);
710-
wrms_ = getWrmsFSA(model);
711-
}
712-
}
713-
714-
if (wrms_ < conv_thresh)
715-
break; // converged
716718
}
717719

718720
// if check_sensi_conv_ is deactivated, we still have to update sensis

0 commit comments

Comments
 (0)