Skip to content
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

State sensitivities computation at steady state #2080

Open
plakrisenko opened this issue May 10, 2023 · 3 comments
Open

State sensitivities computation at steady state #2080

plakrisenko opened this issue May 10, 2023 · 3 comments
Labels
enhancement new Newly created

Comments

@plakrisenko
Copy link
Member

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 three SteadyStateSensitivityModes 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:/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?

@plakrisenko plakrisenko added enhancement new Newly created labels May 10, 2023
@FFroehlich
Copy link
Member

FFroehlich commented May 12, 2023

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

bool turnOffNewton = solver.getNewtonMaxSteps() == 0 || (
and
bool SteadystateProblem::getSensitivityFlag(
(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.

@dweindl
Copy link
Member

dweindl commented May 12, 2023

I made some attempts at refactoring, but ultimately gave up

i think that was that: #1485
maybe something to be learned from the discussion back then...

@plakrisenko
Copy link
Member Author

In the current implementation (i) and (ii) can't be controlled independently by the user, but have to be set via SteadyStateSensitivityMode and SensitivityMethodPreequilibration

But SteadyStateSensitivityMode and SensitivityMethodPreequilibration don't control (i) at all?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new Newly created
Projects
None yet
Development

No branches or pull requests

3 participants