simplify steadystate logic #1485

FFroehlich · 2021-04-03T14:50:10Z

No description provided.

codecov · 2021-04-03T15:02:29Z

Codecov Report

Merging #1485 (99d0cfb) into develop (3d61ac6) will increase coverage by 0.12%.
The diff coverage is 97.95%.

❗ Current head 99d0cfb differs from pull request most recent head 2c4c380. Consider uploading reports for the commit 2c4c380 to get more accurate results

@@             Coverage Diff             @@
##           develop    #1485      +/-   ##
===========================================
+ Coverage    79.43%   79.55%   +0.12%     
===========================================
  Files           66       66              
  Lines        10370    10374       +4     
===========================================
+ Hits          8237     8253      +16     
+ Misses        2133     2121      -12

Flag	Coverage Δ
cpp	`75.94% <97.95%> (+0.18%)`	⬆️
petab	`67.85% <ø> (ø)`
python	`68.15% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
python/amici/numpy.py	`82.08% <ø> (ø)`
src/steadystateproblem.cpp	`88.08% <97.91%> (+2.05%)`	⬆️
include/amici/steadystateproblem.h	`100.00% <100.00%> (ø)`
src/newton_solver.cpp	`76.75% <0.00%> (-2.17%)`	⬇️
src/solver_cvodes.cpp	`70.41% <0.00%> (+0.20%)`	⬆️
src/spline.cpp	`71.57% <0.00%> (+6.31%)`	⬆️
src/exception.cpp	`91.30% <0.00%> (+8.69%)`	⬆️

paulstapor · 2021-04-06T08:00:11Z

That one was on my to-do-list anyway... Would have been good to discuss this prior to you putting effort in it...
The logic here isn't simple, but that's not (let's say, maybe not only) because it's not perfectly programmed, but rather because the problem as such is intricate. You probably saw it from the fact that you want to simplify the code, but actually added more lines than you removed...

So, give me some background please, as the PR has no description:

What is precisely you aim of the code? What does the current code not do? What should the new code do (better)?
(How) Does this relate to Clean-up steady-state solver code #1073 ?

FFroehlich · 2021-04-06T15:02:00Z

That one was on my to-do-list anyway... Would have been good to discuss this prior to you putting effort in it...
The logic here isn't simple, but that's not (let's say, maybe not only) because it's not perfectly programmed, but rather because the problem as such is intricate. You probably saw it from the fact that you want to simplify the code, but actually added more lines than you removed...

The big issue is that there was a single function that served 3 different purposes (check whether simulation needs to be stored, whether simulation needs to be run with sensitivities, and whether sensitivities are to be computed using the implicit function theorem). This is in violation of the single-responsibility principle (https://en.wikipedia.org/wiki/Single-responsibility_principle), giving rise to code execution that is so complicated that eve you got confused about it #1461 .

So, give me some background please, as the PR has no description:

What is precisely you aim of the code? What does the current code not do? What should the new code do (better)?

the simulationFSA sensitivity mode is intended to be used when the Jacobian is singular. In that case, sensitivity computation via the implicit function theorem should never be used.
Having a mode where you use forward sensitivities only when simulation is run is not practically relevant, at least I cannot tell any situations when the user would want to do that. Primarily, since simulation has to be run until convergence of sensitivities. Moreover, it is questionable whether forward sensitivities will be accurate when newtons method was used prior to simulation, since the sensitivity initialization are not updated by newtons method. While you may get lucky and they may converge eventually, but whether this will work in multistable systems is unclear.
You cannot run adjoint sensitivity analysis and then use the same solver for postequilibration using sensitivity analysis. I added a check that detects that situation and raises an issue
Many of the test that were supposed to test combinations of forward and adjoint methods simply used newtons method to compute sensitivity and hence didn't really test what they were supposed to test. I added checks for preeq and posteq status flags that verifies that the right method were run.
Convergence of steadystate sensitivities were based on norm of x_, not sx_, which honestly just doesn't make too much sense.

(How) Does this relate to Clean-up steady-state solver code #1073 ?

It partially addresses the item Add flag "simulationOnly" to steadyStateSensitivityMethod, which does not even try to launch the Newton solver.

paulstapor

That's somewhat orthogonal to what I have planned to do with the steady state solver, therefore I'm not completely happy about it
If I'm not mistaken, the SteadyStateContext flag isn't used any more by now. In this case, please remove it also from include/amici/defines.h

paulstapor · 2021-04-07T11:28:07Z

src/steadystateproblem.cpp

+
+ if (solver.getSensitivityMethodPreequilibration()
+ == SensitivityMethod::adjoint &&
+ model.getSteadyStateSensitivityMode() ==
+ SteadyStateSensitivityMode::simulationFSA)
+ throw AmiException("Preequilibration using adjoint sensitivities "
+ "is not compatible with simulationFSA "
+ "sensitivity mode.");


I understand that setting SteadyStateSensitivityMode::simulationFSA and allowing to use adjoint preequilibration is counter-intuitive. However, it would make much sense to allow the user to switch off the Newton solver and using adjoint preequilibration... Would be happy if there was a way to encode this...

I understand that setting SteadyStateSensitivityMode::simulationFSA and allowing to use adjoint preequilibration is counter-intuitive. However, it would make much sense to allow the user to switch off the Newton solver and using adjoint preequilibration... Would be happy if there was a way to encode this...

How about setting the allowed newton steps to 0?

Ideally, there would be the options newtonOnly, simulationFSA, and simulationASA and something that first tries Newton and uses simulation only if Newton fails... But no need to implement this now.

Something else: I'd advocate to move the steadyStateSensitivitiyMode from model to solver. The value may be model dpeendent, but it encodes the behavior of the solver. Therefore, I'd be happy if we could move this at some point. What's your opinion on this?

Ideally, there would be the options newtonOnly, simulationFSA, and simulationASA and something that first tries Newton and uses simulation only if Newton fails... But no need to implement this now.

I think most of these settings can already be achieved with existing options.

Something else: I'd advocate to move the steadyStateSensitivitiyMode from model to solver. The value may be model dpeendent, but it encodes the behavior of the solver. Therefore, I'd be happy if we could move this at some point. What's your opinion on this?

Yes, I agree, I found it confusing that this is in model, not solver.

paulstapor · 2021-04-07T11:37:13Z

src/steadystateproblem.cpp

+ if (solver->getSensitivityOrder() >= SensitivityOrder::first &&
+ model->getSteadyStateSensitivityMode() ==
+ SteadyStateSensitivityMode::simulationFSA &&
+ solver->getSensitivityMethod() != SensitivityMethod::forward) {


Again not quite getting why you want to exclude this, but okay if you feel like it's really necessary...
Anyhow: All these quantities should have been set prior to the creation of the SteadyStateProblem object and could be caught in the constructor (together with the additional check you implemented there), ideally by using a method like check_steady_state_solver_options or similar...

This check may have become obsolete after adding the respective checks to the constructor.

paulstapor · 2021-04-07T11:41:51Z

src/steadystateproblem.cpp

@@ -462,7 +472,7 @@ bool SteadystateProblem::checkConvergence(const Solver *solver,
 sx_ = solver->getStateSensitivity(t_);
 model->fsxdot(t_, x_, dx_, ip, sx_[ip], dx_, xdot_);
 wrms_ = getWrmsNorm(
- x_, xdot_, solver->getAbsoluteToleranceSteadyStateSensi(),
+ sx_[ip], xdot_, solver->getAbsoluteToleranceSteadyStateSensi(),


include/amici/steadystateproblem.h

FFroehlich · 2021-04-07T14:17:29Z

That's somewhat orthogonal to what I have planned to do with the steady state solver, therefore I'm not completely happy about it

If you plan stuff for a year or so and never follow through, you kinda have to live with the possibility that someone starts working on it beforehand.

If I'm not mistaken, the SteadyStateContext flag isn't used any more by now. In this case, please remove it also from include/amici/defines.h

👍

paulstapor · 2021-04-07T17:50:03Z

That's somewhat orthogonal to what I have planned to do with the steady state solver, therefore I'm not completely happy about it

If you plan stuff for a year or so and never follow through, you kinda have to live with the possibility that someone starts working on it beforehand.

No problem with somebody else working on it at all. But I created a milestone and organized issues for this for the precise reason that one can discuss the way how its implemented prior to it being done...

…-dev/AMICI into simplify_steadystate_logic

FFroehlich · 2021-04-11T20:35:46Z

That's somewhat orthogonal to what I have planned to do with the steady state solver, therefore I'm not completely happy about it

@paulstapor I added a mixed mode which restores the possibility to use combinations of FSA with newton methods

sonarcloud · 2021-04-11T21:17:02Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.4% Duplication

FFroehlich · 2021-04-12T00:17:43Z

okay at this point I simply give up and consider the simulationFSA functionality broken.

AMICI/python/tests/test_preequilibration.py

Line 307 in 4e671a5

for setting1, setting2 in itertools.product(settings, settings):

never ran because once a generator is used, its empty.

simplify logic

a568ef6

FFroehlich requested a review from paulstapor April 3, 2021 14:50

FFroehlich added 4 commits April 3, 2021 15:30

FIX ALL THE BUGS

5569759

refactor tests

b8b3157

fixup

47e35a4

fixup

7f11d45

paulstapor approved these changes Apr 7, 2021

View reviewed changes

Fabian Fröhlich and others added 11 commits April 9, 2021 18:14

Merge branch 'develop' into simplify_steadystate_logic

78f8f90

update docstring

8688cc9

remove check

1cb00dc

remove enum

9777808

Merge branch 'develop' into simplify_steadystate_logic

75caed2

mark more functions const

e1b6817

Merge branch 'simplify_steadystate_logic' of https:/AMICI…

bc35887

…-dev/AMICI into simplify_steadystate_logic

fix code smell

d5352be

fixup

0530ebe

fix status check

fee18b9

add mixed mode and extend tests

99d0cfb

FFroehlich requested a review from paulstapor April 11, 2021 20:33

attempted to fix, gave up

2c4c380

FFroehlich closed this Apr 12, 2021

dweindl deleted the simplify_steadystate_logic branch April 8, 2022 21:15

dweindl mentioned this pull request May 12, 2023

State sensitivities computation at steady state #2080

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simplify steadystate logic #1485

simplify steadystate logic #1485

FFroehlich commented Apr 3, 2021

codecov bot commented Apr 3, 2021 •

edited

Loading

paulstapor commented Apr 6, 2021

FFroehlich commented Apr 6, 2021 •

edited

Loading

paulstapor left a comment

paulstapor Apr 7, 2021

FFroehlich Apr 7, 2021

paulstapor Apr 7, 2021

FFroehlich Apr 7, 2021

paulstapor Apr 7, 2021

FFroehlich Apr 7, 2021

paulstapor Apr 7, 2021

FFroehlich commented Apr 7, 2021

paulstapor commented Apr 7, 2021

FFroehlich commented Apr 11, 2021

sonarcloud bot commented Apr 11, 2021

FFroehlich commented Apr 12, 2021

simplify steadystate logic #1485

simplify steadystate logic #1485

Conversation

FFroehlich commented Apr 3, 2021

codecov bot commented Apr 3, 2021 • edited Loading

Codecov Report

paulstapor commented Apr 6, 2021

FFroehlich commented Apr 6, 2021 • edited Loading

paulstapor left a comment

Choose a reason for hiding this comment

paulstapor Apr 7, 2021

Choose a reason for hiding this comment

FFroehlich Apr 7, 2021

Choose a reason for hiding this comment

paulstapor Apr 7, 2021

Choose a reason for hiding this comment

FFroehlich Apr 7, 2021

Choose a reason for hiding this comment

paulstapor Apr 7, 2021

Choose a reason for hiding this comment

FFroehlich Apr 7, 2021

Choose a reason for hiding this comment

paulstapor Apr 7, 2021

Choose a reason for hiding this comment

FFroehlich commented Apr 7, 2021

paulstapor commented Apr 7, 2021

FFroehlich commented Apr 11, 2021

sonarcloud bot commented Apr 11, 2021

FFroehlich commented Apr 12, 2021

codecov bot commented Apr 3, 2021 •

edited

Loading

FFroehlich commented Apr 6, 2021 •

edited

Loading