Open plakrisenko opened 1 year ago
I agree, it's all a big giant mess.
There are two things that we need to control (i) how the steady state is computed (integration/newton) and (ii) how the sensitivities are computed (integration/newton). As far as I can tell, only newton steadystate + integration sensi does not work (not sure about this for ASA), but everything else should be viable. In the current implementation (i) and (ii) can't be controlled independently by the user, but have to be set via SteadyStateSensitivityMode
and SensitivityMethodPreequilibration
, which adds an additional layer of complexity between pre- and post-equilibration. This leads to very complex checks in https://github.com/AMICI-dev/AMICI/blob/81d32e785af4a70e72d458196d709e1d18563556/src/steadystateproblem.cpp#L112 and https://github.com/AMICI-dev/AMICI/blob/81d32e785af4a70e72d458196d709e1d18563556/src/steadystateproblem.cpp#L401 (which, to make things worse, does different things depending on which context it is called in).
I believe the reason you think the implementation is inconsistent is because in one case you are looking at controlling (i) vs (ii), but I think the issue with the code is really more fundamental. Any effort to simplify this code is very welcome, I made some attempts at refactoring, but ultimately gave up as Paul still had intentions to fix this himself.
I made some attempts at refactoring, but ultimately gave up
i think that was that: https://github.com/AMICI-dev/AMICI/pull/1485 maybe something to be learned from the discussion back then...
In the current implementation (i) and (ii) can't be controlled independently by the user, but have to be set via
SteadyStateSensitivityMode
andSensitivityMethodPreequilibration
But SteadyStateSensitivityMode
and SensitivityMethodPreequilibration
don't control (i) at all?
There are two approaches available in AMICI for state sensitivities computation at steady state: numerical integration and solving a linear system (
computeNewtonSensis
). There are now also threeSteadyStateSensitivityMode
s available:newtonOnly
,integrationOnly
,integrateIfNewtonFails
.I guess, originally
SteadyStateSensitivityMode
was only meant for forward sensitivity analysis. In this pull-request #1727 its usage was extended to adjoint sensitivity analysis.However,
integrateIfNewtonFails
is not quite compatible with FSA at the moment:One could maybe move
computeNewtonSensis
to here https://github.com/AMICI-dev/AMICI/blob/master/src/steadystateproblem.cpp#L121 (and after the second Newton's method attempt). Then the order will be similar to ASA case.Or should modes for sensitivities computation at steady state be separated for FSA and ASA, is it too confusing now?