Enable Newton solver with simulation #1075
Conversation
…en steady state was found by simulation
Codecov Report
@@ Coverage Diff @@
## develop #1075 +/- ##
===========================================
- Coverage 72.55% 72.24% -0.32%
===========================================
Files 59 59
Lines 9180 9216 +36
===========================================
- Hits 6661 6658 -3
- Misses 2519 2558 +39
Continue to review full report at Codecov.
|
|
Okay, it is 100% reasonable, that the failing test does indeed fail. I'm just confused about the fact that it worked beforehand... 🤔 😕 I will add a jupyter notebook as documentation of the steadystate solver within this PR, which explains what the steadystate solver should do... (in my opinion). Happy to excahnge opinions on that... |
Thats part of the public API of CVODES/IDAS, so I don't see any reason why we shouldn't expose this functionality in AMICI. The implementation absolutely fine from my point of view. |
True, we never actually check whether |
…djoints with postequilibration
|
Please have a look at my changes and if tests pass and everything looks fine, feel free to merge! |
| # add rdatas | ||
| rdatas[sensi_meth] = amici.runAmiciSimulation(model, solver, edata) | ||
| assert rdatas[sensi_meth]['status'] == amici.AMICI_SUCCESS | ||
|
|
||
| for variable in ['llh', 'sllh']: |
There was a problem hiding this comment.
I didn't add it in my commits, but checked it rather by hand:
In an ideal world, also sx0 and sx_ss are checked for here...
| # add rdatas | ||
| rdatas[sensi_meth] = amici.runAmiciSimulation(model, solver, edata) | ||
| assert rdatas[sensi_meth]['status'] == amici.AMICI_SUCCESS | ||
|
|
||
| for variable in ['llh', 'sllh']: |
There was a problem hiding this comment.
| for variable in ['llh', 'sllh']: | |
| for variable in ['llh', 'sllh', 'sx_ss', 'sx0']: |
|
|
||
| rdatas = {} | ||
| for sensi_meth in [amici.SensitivityMethod.forward, | ||
| amici.SensitivityMethod.adjoint]: |
There was a problem hiding this comment.
That's just a question, not necessary, but:
Do we also want to not only check FSA and ASA, but also FD?
| solver.setSensitivityMethod(sensi_meth) | ||
| solver.setNewtonMaxSteps(0) | ||
| solver.SteadyStateSensitivityMethod = \ | ||
| amici.SteadyStateSensitivityMode.simulationFSA |
There was a problem hiding this comment.
Add another loop for Newton solver as well?
Or are we overtesting then?
|
SonarCloud Quality Gate failed.
|
This is a suggestion how to fix an existing problem in the steadystate solver (happy about alternative suggestions how to handle it):
Currently, if a model has the timepoint
infspecified and forward sensis and Newton's method fails due to a poor initial guess, the steadystate solver will integrate a long time with full forward sensis.However, this is not ideal (or even completely prohibitive) some applications:
If the Jacobian is invertible, it would be fine to simulate without sensis and then just solve the linear system of equations. This can be way faster...
One point I'm not happy about in this solution: I had to implement a function in the solver code to switch off forward sensi computation. As this one is to be used in steadystateproblem, it must be part of the public API of solver. However, this function touches stuff in the solver object, that should imho not be touchable by a public API...
Hence, I would be happy about suggestions how to solve this better...