diff --git a/CHANGELOG.md b/CHANGELOG.md index 90f19c15fe..7efb1a29c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,11 @@ See also our [versioning policy](https://amici.readthedocs.io/en/latest/versioni at import time instead of silently generating incorrect C++, since simulating such models is not yet supported (#3245). +* Fixed incorrect sensitivities for models with a single-species + conservation law -- a boundary-condition or `constant=true` species with + no counteracting rate rule -- whose total abundance depends on a free + parameter via an `InitialAssignment` (#3249). + * Fixed incorrect forward sensitivities for models with SBML events that have explicit priorities and can trigger simultaneously. Whenever a lower-priority event's assignment depended on a state that a @@ -57,6 +62,11 @@ See also our [versioning policy](https://amici.readthedocs.io/en/latest/versioni the sensitivity update used a stale, pre-cascade state instead of that already-updated state. The state trajectory itself was unaffected. +* Fixed incorrect adjoint sensitivities for models with SBML events that + are both state-triggered and state-updating. The trigger-time-sensitivity + terms were evaluated using the post-event state instead of the pre-event + state. The state trajectory was unaffected (#3257). + ### v1.1 (2026-09-03) **BREAKING CHANGES** diff --git a/models/model_calvetti_py/model_calvetti_py.h b/models/model_calvetti_py/model_calvetti_py.h index 238df11515..17b3190c55 100644 --- a/models/model_calvetti_py/model_calvetti_py.h +++ b/models/model_calvetti_py/model_calvetti_py.h @@ -541,7 +541,7 @@ class Model_model_calvetti_py : public amici::Model_DAE { * @return AMICI git commit hash */ std::string get_amici_commit() const override { - return "5d6d457fc1e217075c7074c8f844ac64b2a75084"; + return "f04344fe181ed0b63917c93672370d76e427fb22"; } bool has_quadratic_llh() const override { diff --git a/models/model_dirac_py/model_dirac_py.h b/models/model_dirac_py/model_dirac_py.h index 117968cf0f..091d0e3477 100644 --- a/models/model_dirac_py/model_dirac_py.h +++ b/models/model_dirac_py/model_dirac_py.h @@ -528,7 +528,7 @@ class Model_model_dirac_py : public amici::Model_ODE { * @return AMICI git commit hash */ std::string get_amici_commit() const override { - return "5d6d457fc1e217075c7074c8f844ac64b2a75084"; + return "f04344fe181ed0b63917c93672370d76e427fb22"; } bool has_quadratic_llh() const override { diff --git a/models/model_events_py/deltaxB.cpp b/models/model_events_py/deltaxB.cpp index 89ebc41e8e..91f309a304 100644 --- a/models/model_events_py/deltaxB.cpp +++ b/models/model_events_py/deltaxB.cpp @@ -7,9 +7,6 @@ namespace amici { namespace model_model_events_py { void deltaxB_model_events_py(realtype *deltaxB, const realtype t, const realtype *x, const realtype *p, const realtype *k, const realtype *h, const realtype *w, const realtype *dx, const int ie, const realtype *xdot, const realtype *xdot_old, const realtype *x_old, const realtype *xB, const realtype *tcl){ - const realtype x1_ = x[0]; - const realtype x2_ = x[1]; - const realtype x3_ = x[2]; const realtype p1_ = p[0]; const realtype p2_ = p[1]; const realtype p3_ = p[2]; @@ -21,18 +18,21 @@ void deltaxB_model_events_py(realtype *deltaxB, const realtype t, const realtype const realtype xdot_old0_ = xdot_old[0]; const realtype xdot_old1_ = xdot_old[1]; const realtype xdot_old2_ = xdot_old[2]; + const realtype x_old0_ = x_old[0]; + const realtype x_old1_ = x_old[1]; + const realtype x_old2_ = x_old[2]; const realtype xB0_ = xB[0]; const realtype xB1_ = xB[1]; const realtype xB2_ = xB[2]; switch(ie) { case 0: - deltaxB[1] = xB0_*(dx1dt_ - xdot_old0_)/(Heaviside_4_ + p2_*x1_*std::exp(-1.0/10.0*t) - p3_*x2_ + x3_ - 1) + xB1_*(dx2dt_ - xdot_old1_)/(Heaviside_4_ + p2_*x1_*std::exp(-1.0/10.0*t) - p3_*x2_ + x3_ - 1) + xB2_*(dx3dt_ - xdot_old2_)/(Heaviside_4_ + p2_*x1_*std::exp(-1.0/10.0*t) - p3_*x2_ + x3_ - 1); - deltaxB[2] = -xB0_*(dx1dt_ - xdot_old0_)/(Heaviside_4_ + p2_*x1_*std::exp(-1.0/10.0*t) - p3_*x2_ + x3_ - 1) - xB1_*(dx2dt_ - xdot_old1_)/(Heaviside_4_ + p2_*x1_*std::exp(-1.0/10.0*t) - p3_*x2_ + x3_ - 1) - xB2_*(dx3dt_ - xdot_old2_)/(Heaviside_4_ + p2_*x1_*std::exp(-1.0/10.0*t) - p3_*x2_ + x3_ - 1); + deltaxB[1] = xB0_*(dx1dt_ - xdot_old0_)/(Heaviside_4_ + p2_*x_old0_*std::exp(-1.0/10.0*t) - p3_*x_old1_ + x_old2_ - 1) + xB1_*(dx2dt_ - xdot_old1_)/(Heaviside_4_ + p2_*x_old0_*std::exp(-1.0/10.0*t) - p3_*x_old1_ + x_old2_ - 1) + xB2_*(dx3dt_ - xdot_old2_)/(Heaviside_4_ + p2_*x_old0_*std::exp(-1.0/10.0*t) - p3_*x_old1_ + x_old2_ - 1); + deltaxB[2] = -xB0_*(dx1dt_ - xdot_old0_)/(Heaviside_4_ + p2_*x_old0_*std::exp(-1.0/10.0*t) - p3_*x_old1_ + x_old2_ - 1) - xB1_*(dx2dt_ - xdot_old1_)/(Heaviside_4_ + p2_*x_old0_*std::exp(-1.0/10.0*t) - p3_*x_old1_ + x_old2_ - 1) - xB2_*(dx3dt_ - xdot_old2_)/(Heaviside_4_ + p2_*x_old0_*std::exp(-1.0/10.0*t) - p3_*x_old1_ + x_old2_ - 1); break; case 1: - deltaxB[0] = xB0_*(dx1dt_ - xdot_old0_)/(Heaviside_4_ - p1_*x1_*(1 - Heaviside_2_) + x3_ - 1) + xB1_*(dx2dt_ - xdot_old1_)/(Heaviside_4_ - p1_*x1_*(1 - Heaviside_2_) + x3_ - 1) + xB2_*(dx3dt_ - xdot_old2_)/(Heaviside_4_ - p1_*x1_*(1 - Heaviside_2_) + x3_ - 1); - deltaxB[2] = -xB0_*(dx1dt_ - xdot_old0_)/(Heaviside_4_ - p1_*x1_*(1 - Heaviside_2_) + x3_ - 1) - xB1_*(dx2dt_ - xdot_old1_)/(Heaviside_4_ - p1_*x1_*(1 - Heaviside_2_) + x3_ - 1) - xB2_*(dx3dt_ - xdot_old2_)/(Heaviside_4_ - p1_*x1_*(1 - Heaviside_2_) + x3_ - 1); + deltaxB[0] = xB0_*(dx1dt_ - xdot_old0_)/(Heaviside_4_ - p1_*x_old0_*(1 - Heaviside_2_) + x_old2_ - 1) + xB1_*(dx2dt_ - xdot_old1_)/(Heaviside_4_ - p1_*x_old0_*(1 - Heaviside_2_) + x_old2_ - 1) + xB2_*(dx3dt_ - xdot_old2_)/(Heaviside_4_ - p1_*x_old0_*(1 - Heaviside_2_) + x_old2_ - 1); + deltaxB[2] = -xB0_*(dx1dt_ - xdot_old0_)/(Heaviside_4_ - p1_*x_old0_*(1 - Heaviside_2_) + x_old2_ - 1) - xB1_*(dx2dt_ - xdot_old1_)/(Heaviside_4_ - p1_*x_old0_*(1 - Heaviside_2_) + x_old2_ - 1) - xB2_*(dx3dt_ - xdot_old2_)/(Heaviside_4_ - p1_*x_old0_*(1 - Heaviside_2_) + x_old2_ - 1); break; } } diff --git a/models/model_events_py/model_events_py.h b/models/model_events_py/model_events_py.h index 47a4c5a1dc..b726ba28ad 100644 --- a/models/model_events_py/model_events_py.h +++ b/models/model_events_py/model_events_py.h @@ -563,7 +563,7 @@ class Model_model_events_py : public amici::Model_ODE { * @return AMICI git commit hash */ std::string get_amici_commit() const override { - return "5d6d457fc1e217075c7074c8f844ac64b2a75084"; + return "f04344fe181ed0b63917c93672370d76e427fb22"; } bool has_quadratic_llh() const override { diff --git a/models/model_jakstat_adjoint_py/model_jakstat_adjoint_py.h b/models/model_jakstat_adjoint_py/model_jakstat_adjoint_py.h index 9c964b7b2d..ebd5fec55f 100644 --- a/models/model_jakstat_adjoint_py/model_jakstat_adjoint_py.h +++ b/models/model_jakstat_adjoint_py/model_jakstat_adjoint_py.h @@ -538,7 +538,7 @@ class Model_model_jakstat_adjoint_py : public amici::Model_ODE { * @return AMICI git commit hash */ std::string get_amici_commit() const override { - return "5d6d457fc1e217075c7074c8f844ac64b2a75084"; + return "f04344fe181ed0b63917c93672370d76e427fb22"; } bool has_quadratic_llh() const override { diff --git a/models/model_nested_events_py/deltaxB.cpp b/models/model_nested_events_py/deltaxB.cpp index 9b956b53ff..dbef568bef 100644 --- a/models/model_nested_events_py/deltaxB.cpp +++ b/models/model_nested_events_py/deltaxB.cpp @@ -7,20 +7,20 @@ namespace amici { namespace model_model_nested_events_py { void deltaxB_model_nested_events_py(realtype *deltaxB, const realtype t, const realtype *x, const realtype *p, const realtype *k, const realtype *h, const realtype *w, const realtype *dx, const int ie, const realtype *xdot, const realtype *xdot_old, const realtype *x_old, const realtype *xB, const realtype *tcl){ - const realtype Virus_ = x[0]; const realtype rho_V_ = p[3]; const realtype delta_V_ = p[4]; const realtype Heaviside_1_ = h[0]; const realtype dVirusdt_ = xdot[0]; const realtype xdot_old0_ = xdot_old[0]; + const realtype x_old0_ = x_old[0]; const realtype xB0_ = xB[0]; switch(ie) { case 0: - deltaxB[0] = xB0_*(dVirusdt_ - xdot_old0_)/(Heaviside_1_*Virus_*rho_V_ - Virus_*delta_V_); + deltaxB[0] = xB0_*(dVirusdt_ - xdot_old0_)/(Heaviside_1_*rho_V_*x_old0_ - delta_V_*x_old0_); break; case 1: - deltaxB[0] = -xB0_*(dVirusdt_ - xdot_old0_)/(-Heaviside_1_*Virus_*rho_V_ + Virus_*delta_V_); + deltaxB[0] = -xB0_*(dVirusdt_ - xdot_old0_)/(-Heaviside_1_*rho_V_*x_old0_ + delta_V_*x_old0_); break; } } diff --git a/models/model_nested_events_py/model_nested_events_py.h b/models/model_nested_events_py/model_nested_events_py.h index ddb07c59c6..434ec729ca 100644 --- a/models/model_nested_events_py/model_nested_events_py.h +++ b/models/model_nested_events_py/model_nested_events_py.h @@ -536,7 +536,7 @@ class Model_model_nested_events_py : public amici::Model_ODE { * @return AMICI git commit hash */ std::string get_amici_commit() const override { - return "5d6d457fc1e217075c7074c8f844ac64b2a75084"; + return "f04344fe181ed0b63917c93672370d76e427fb22"; } bool has_quadratic_llh() const override { diff --git a/models/model_neuron_py/deltaxB.cpp b/models/model_neuron_py/deltaxB.cpp index ce99069804..f5965c3736 100644 --- a/models/model_neuron_py/deltaxB.cpp +++ b/models/model_neuron_py/deltaxB.cpp @@ -7,19 +7,19 @@ namespace amici { namespace model_model_neuron_py { void deltaxB_model_neuron_py(realtype *deltaxB, const realtype t, const realtype *x, const realtype *p, const realtype *k, const realtype *h, const realtype *w, const realtype *dx, const int ie, const realtype *xdot, const realtype *xdot_old, const realtype *x_old, const realtype *xB, const realtype *tcl){ - const realtype v_ = x[0]; - const realtype u_ = x[1]; const realtype I0_ = k[1]; const realtype dvdt_ = xdot[0]; const realtype dudt_ = xdot[1]; const realtype xdot_old0_ = xdot_old[0]; const realtype xdot_old1_ = xdot_old[1]; + const realtype x_old0_ = x_old[0]; + const realtype x_old1_ = x_old[1]; const realtype xB0_ = xB[0]; const realtype xB1_ = xB[1]; switch(ie) { case 0: - deltaxB[0] = xB0_*(xdot_old0_/(I0_ - u_ + (1.0/25.0)*std::pow(v_, 2) + 5*v_ + 140) + (dvdt_ - xdot_old0_)/(I0_ - u_ + (1.0/25.0)*std::pow(v_, 2) + 5*v_ + 140) - 1) + xB1_*(dudt_ - xdot_old1_)/(I0_ - u_ + (1.0/25.0)*std::pow(v_, 2) + 5*v_ + 140); + deltaxB[0] = xB0_*(xdot_old0_/(I0_ + (1.0/25.0)*std::pow(x_old0_, 2) + 5*x_old0_ - x_old1_ + 140) + (dvdt_ - xdot_old0_)/(I0_ + (1.0/25.0)*std::pow(x_old0_, 2) + 5*x_old0_ - x_old1_ + 140) - 1) + xB1_*(dudt_ - xdot_old1_)/(I0_ + (1.0/25.0)*std::pow(x_old0_, 2) + 5*x_old0_ - x_old1_ + 140); break; } } diff --git a/models/model_neuron_py/model_neuron_py.h b/models/model_neuron_py/model_neuron_py.h index cb75000a47..daae269bc1 100644 --- a/models/model_neuron_py/model_neuron_py.h +++ b/models/model_neuron_py/model_neuron_py.h @@ -558,7 +558,7 @@ class Model_model_neuron_py : public amici::Model_ODE { * @return AMICI git commit hash */ std::string get_amici_commit() const override { - return "5d6d457fc1e217075c7074c8f844ac64b2a75084"; + return "f04344fe181ed0b63917c93672370d76e427fb22"; } bool has_quadratic_llh() const override { diff --git a/models/model_robertson_py/model_robertson_py.h b/models/model_robertson_py/model_robertson_py.h index 811add8586..7cbf76eb59 100644 --- a/models/model_robertson_py/model_robertson_py.h +++ b/models/model_robertson_py/model_robertson_py.h @@ -520,7 +520,7 @@ class Model_model_robertson_py : public amici::Model_DAE { * @return AMICI git commit hash */ std::string get_amici_commit() const override { - return "5d6d457fc1e217075c7074c8f844ac64b2a75084"; + return "f04344fe181ed0b63917c93672370d76e427fb22"; } bool has_quadratic_llh() const override { diff --git a/models/model_steadystate_py/model_steadystate_py.h b/models/model_steadystate_py/model_steadystate_py.h index 8009d38623..3b4e41331e 100644 --- a/models/model_steadystate_py/model_steadystate_py.h +++ b/models/model_steadystate_py/model_steadystate_py.h @@ -520,7 +520,7 @@ class Model_model_steadystate_py : public amici::Model_ODE { * @return AMICI git commit hash */ std::string get_amici_commit() const override { - return "5d6d457fc1e217075c7074c8f844ac64b2a75084"; + return "f04344fe181ed0b63917c93672370d76e427fb22"; } bool has_quadratic_llh() const override { diff --git a/python/sdist/amici/_symbolic/de_model.py b/python/sdist/amici/_symbolic/de_model.py index efcfa1a1f3..8433d35f40 100644 --- a/python/sdist/amici/_symbolic/de_model.py +++ b/python/sdist/amici/_symbolic/de_model.py @@ -27,6 +27,7 @@ ObservableTransformation, _default_simplify, amici_time_symbol, + smart_subs_dict, symbol_with_assumptions, toposort_symbols, unique_preserve_order, @@ -1444,6 +1445,20 @@ def _generate_sparse_symbol(self, name: str) -> None: self._sparsesyms[name] = symbol_list self._syms[name] = sparse_matrix + def _pre_event_eqs(self, name: str) -> list[sp.Matrix]: + """Re-express `dtaudx`/`dtaudp` purely in terms of the pre-event + state, for use inside `deltaxB`/`deltaqB` only. + + `w` is eliminated first (not available in the C++ functions), + then `x -> x_old` is substituted. + """ + w_to_expr = dict(zip(self.sym("w"), self.eq("w"))) + x_to_x_old = dict(zip(self.sym("x"), self.sym("x_old"))) + return [ + smart_subs_dict(smart_subs_dict(expr, w_to_expr), x_to_x_old) + for expr in self.eq(name) + ] + def _compute_equation(self, name: str) -> None: """ Computes the symbolic formula for a symbolic variable @@ -1852,18 +1867,20 @@ def _compute_equation(self, name: str) -> None: self._eqs[name] = event_eqs elif name == "deltaxB": + # express in terms of pre-event state + dtaudx_pre = self._pre_event_eqs("dtaudx") event_eqs = [] for ie, event in enumerate(self._events): # ==== 1st group of terms: Heaviside functions =========== tmp_eq = smart_multiply( self.sym("xdot") - self.sym("xdot_old"), - self.eq("dtaudx")[ie], + dtaudx_pre[ie], ) if event.updates_state: # ==== 2nd group of terms: Derivatives of Dirac deltas === # Part 2a: explicit time dependence of bolus function tmp_eq -= smart_multiply( - self.eq("ddeltaxdt")[ie], self.eq("dtaudx")[ie] + self.eq("ddeltaxdt")[ie], dtaudx_pre[ie] ) # Part 2b: implicit time dependence of bolus function tmp_eq -= smart_multiply( @@ -1872,7 +1889,7 @@ def _compute_equation(self, name: str) -> None: + self.eq("ddeltaxdx_old")[ie], self.sym("xdot_old"), ), - self.eq("dtaudx")[ie], + dtaudx_pre[ie], ) # ==== 3rd group of terms: Dirac deltas ================== tmp_eq += ( @@ -1883,18 +1900,20 @@ def _compute_equation(self, name: str) -> None: self._eqs[name] = event_eqs elif name == "deltaqB": + # express in terms of pre-event state + dtaudp_pre = self._pre_event_eqs("dtaudp") event_eqs = [] for ie, event in enumerate(self._events): # ==== 1st group of terms: Heaviside functions =========== tmp_eq = smart_multiply( self.sym("xdot") - self.sym("xdot_old"), - self.eq("dtaudp")[ie], + dtaudp_pre[ie], ) if event.updates_state: # ==== 2nd group of terms: Derivatives of Dirac deltas === # Part 2a: explicit time dependence of bolus function tmp_eq -= smart_multiply( - self.eq("ddeltaxdt")[ie], self.eq("dtaudp")[ie] + self.eq("ddeltaxdt")[ie], dtaudp_pre[ie] ) # Part 2b: implicit time dependence of bolus function tmp_eq -= smart_multiply( @@ -1903,7 +1922,7 @@ def _compute_equation(self, name: str) -> None: + self.eq("ddeltaxdx_old")[ie], self.sym("xdot_old"), ), - self.eq("dtaudp")[ie], + dtaudp_pre[ie], ) # ==== 3rd group of terms: Dirac deltas ================== tmp_eq += self.eq("ddeltaxdp")[ie] diff --git a/tests/sbml/testSBMLSuite.py b/tests/sbml/testSBMLSuite.py index c3c15d88ba..e2bb4ebe7b 100755 --- a/tests/sbml/testSBMLSuite.py +++ b/tests/sbml/testSBMLSuite.py @@ -141,35 +141,6 @@ def test_sbml_testsuite_case(test_id, compiled_case, result_path): write_result_file(simulated, test_id, result_path) -def _ast_has_piecewise(node: libsbml.ASTNode | None) -> bool: - """Recursively check whether a libsbml math AST contains a `piecewise` - function anywhere in its tree.""" - if node is None: - return False - if node.getType() == libsbml.AST_FUNCTION_PIECEWISE: - return True - return any( - _ast_has_piecewise(node.getChild(i)) - for i in range(node.getNumChildren()) - ) - - -def _model_has_event_jump_risk(sbml_model: libsbml.Model) -> bool: - """Whether this model has an event, or a piecewise formula.""" - if sbml_model.getNumEvents() > 0: - return True - for reaction in sbml_model.getListOfReactions(): - kinetic_law = reaction.getKineticLaw() - if kinetic_law is not None and _ast_has_piecewise( - kinetic_law.getMath() - ): - return True - return any( - _ast_has_piecewise(rule.getMath()) - for rule in sbml_model.getListOfRules() - ) - - # FIXME: Skip list - to be investigated further # test_id -> adjoint_only (whether forward is unaffected) _OTHER_KNOWN_SENSITIVITY_CHECK_ISSUES = { @@ -182,6 +153,18 @@ def _model_has_event_jump_risk(sbml_model: libsbml.Model) -> bool: "01104": True, "01107": True, "01148": True, + # Adjoint sensitivities & events. To revisit later. + "00348": True, + "00350": True, + "00354": True, + "00368": True, + "00373": True, + "00376": True, + "00379": True, + "00380": True, + "00396": True, + "00753": True, + "01106": True, } @@ -212,10 +195,6 @@ def _sensitivity_preflight_checks( pytest.skip( "Adjoint sensitivities for zero-state models are known to crash." ) - if uses_adjoint and _model_has_event_jump_risk(sbml_model): - pytest.skip( - "Adjoint sensitivities for (some) events are known to be wrong (https://github.com/AMICI-dev/AMICI/pull/3258)." - ) if test_id in _OTHER_KNOWN_SENSITIVITY_CHECK_ISSUES: adjoint_only = _OTHER_KNOWN_SENSITIVITY_CHECK_ISSUES[test_id] if uses_adjoint or not adjoint_only: