-
Notifications
You must be signed in to change notification settings - Fork 30
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 get_model_settings for initial states + sensitivities #1751
Conversation
Initial states and initial state sensitivities should only be included if custom values have been set. Otherwise, `set_model_settings(model, get_model_settings(model))` will permanently set the given values, preventing evaluation of `Model::fx0` / `Model::fsx0`. Fixes ICB-DCM/pyPESTO#837
Codecov Report
@@ Coverage Diff @@
## develop #1751 +/- ##
===========================================
+ Coverage 77.76% 77.77% +0.01%
===========================================
Files 74 74
Lines 12012 12018 +6
===========================================
+ Hits 9341 9347 +6
Misses 2671 2671
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Thanks a lot!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, do we want to add a test for this?
…ies in case of parameter scaling
Yes. Helped to discover another 2 old issues and 1 newly introduced issue... |
SonarCloud Quality Gate failed. |
Fixes a couple of issues in
amici.{get,set}_model_settings
Initial states and initial state sensitivities should only be included if custom values have been set. Otherwise,
set_model_settings(model, get_model_settings(model))
will permanently set the given values, preventing evaluation ofModel::fx0
/Model::fsx0
.Fixes
MultiProcessEngine
results in incorrect ensemble prediction trajectories in the sampling diagnostics notebook ICB-DCM/pyPESTO#837If a model had custom initial state sensitivities set, those would not have been set correctly by
set_model_settings(model, get_model_settings(model))
, becausesetParameter{List,Scale}
was called aftersetInitialStateSensitivities
and cleared initial state sensitivities. (Even if they would have been set, they would have been wrong in case of scaled parameters, due to usingsetInitialStateSensitivities
instead ofsetUnscaledInitialStateSensitivities
).