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

fix equilibraton status discrepancy #1991

Merged
merged 3 commits into from
Feb 14, 2023
Merged

fix equilibraton status discrepancy #1991

merged 3 commits into from
Feb 14, 2023

Conversation

plakrisenko
Copy link
Member

Turn off Newton's method if newton_maxsteps is set to 0, so that in that case the resulting status will be [0 1 0] ([Did not run; Successful run; Did not run]) if numerical integration was successful. Before the status was [-2 1 0] ([Error: The method did not converge to a steady state within the maximum number of steps (Newton's method or simulation). ; Successful run; Did not run]).

Overall Newton's method is turned off if newton_maxsteps is set to 0 or if 'integrationOnly' approach is chosen for sensitivity computation in combination with forward sensitivities approach. The latter is necessary as numerical integration of the model ODEs and corresponding forward sensitivities ODEs is coupled. If 'integrationOnly' approach is chosen for sensitivity computation it is enforced that steady state is computed only be numerical integration as well.

@plakrisenko plakrisenko requested a review from a team as a code owner February 14, 2023 14:03
Copy link
Member

@dweindl dweindl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

src/steadystateproblem.cpp Outdated Show resolved Hide resolved
Co-authored-by: Daniel Weindl <dweindl@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Feb 14, 2023

Codecov Report

Merging #1991 (7558029) into develop (529320f) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1991      +/-   ##
===========================================
- Coverage    76.06%   76.02%   -0.05%     
===========================================
  Files           76       76              
  Lines        13002    13003       +1     
===========================================
- Hits          9890     9885       -5     
- Misses        3112     3118       +6     
Flag Coverage Δ
cpp 73.12% <100.00%> (-0.07%) ⬇️
petab 60.05% <ø> (ø)
python 68.89% <ø> (ø)
sbmlsuite ?

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

Impacted Files Coverage Δ
src/steadystateproblem.cpp 83.29% <100.00%> (-1.55%) ⬇️
src/spline.cpp 65.59% <0.00%> (-6.46%) ⬇️
src/solver.cpp 76.62% <0.00%> (-0.25%) ⬇️
src/exception.cpp 74.28% <0.00%> (ø)
src/solver_cvodes.cpp 70.06% <0.00%> (ø)
src/sundials_matrix_wrapper.cpp 81.02% <0.00%> (+0.81%) ⬆️
src/amici.cpp 76.00% <0.00%> (+4.00%) ⬆️

forward sensitivities ODEs is coupled. If 'integrationOnly' approach is
chosen for sensitivity computation it is enforced that steady state is
computed only by numerical integration as well. */
bool turnOffNewton = solver.getNewtonMaxSteps() == 0 || (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need ((it == -1 && solver.getSensitivityMethodPreequilibration() == SensitivityMethod::forward) || solver.getSensitivityMethod() == SensitivityMethod::forward)) anyways?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latter is necessary as numerical integration of the model ODEs and corresponding forward sensitivities ODEs is coupled. If 'integrationOnly' approach is chosen for sensitivity computation it is enforced that steady state is computed only be numerical integration as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns off Newton's method if SteadyStateSensitivityMode::integrationOnly is set together with SensitivityMethod::forward

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, makes sense, thanks :)

@sonarcloud
Copy link

sonarcloud bot commented Feb 14, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@plakrisenko plakrisenko merged commit beca593 into develop Feb 14, 2023
@dweindl dweindl deleted the fix_eq_status branch February 19, 2023 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants