-
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
Add sllh computation back to petab_objective.simulate_petab
#1548
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #1548 +/- ##
===========================================
+ Coverage 53.73% 54.90% +1.16%
===========================================
Files 29 30 +1
Lines 4548 4617 +69
===========================================
+ Hits 2444 2535 +91
+ Misses 2104 2082 -22
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.
LGTM, but please wait for others' feedback.
Why was the extra test model required? Would keep things simpler if we could use benchmark collection model that's already used elsewhere.
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.
if llh and sllh are correct (in particular the gradient check is happy), LGTM
python/amici/petab_objective.py
Outdated
@@ -150,6 +160,15 @@ def simulate_petab( | |||
|
|||
# Compute total llh | |||
llh = sum(rdata['llh'] for rdata in rdatas) | |||
# Compute total sllh |
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.
I guess this was not the case before either, but maybe one could specify whether or not to calculate sensis? In the latter case, the simulation in runAmiciSimulations would be cheaper, and this mapping here not be performed.
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.
The optional solver
argument is expected to be provided and already setup to compute sensitivities, if sensitivities are desired. Could default to first order forward? Either makes sense to me (no default at least ensures the user defines how sensitivities should be computed). Before review, it defaulted to None
, and was handled in aggregate_sllh
. Now defaults to None
here, and aggregate_sllh
is only called if sensitivities were computed (so, an error is now raised there, as per your other suggestion).
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.
The optional solver argument is expected to be provided and already setup to compute sensitivities, if sensitivities are desired.
Yes!
python/amici/petab_objective.py
Outdated
# SLLH is (un)scaled here with methods that were written for | ||
# parameters. | ||
# First unscale w.r.t. model scale. | ||
partial_parameter_sllh = unscale_parameter( |
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.
I thought we had somewhere also a function rescale(val, scale_in, scale_out), but nvm
The extra test model is because a suitable benchmark problem may not exist. I tested a few with pyPESTO and some errors were always high. I could look for more (I may have also been using an optimized vector). The extra test model might still be a good idea, since many/all PEtab features could be added to it that do not all exist in a single benchmark problem simultaneously. |
Co-authored-by: Yannik Schälte <31767307+yannikschaelte@users.noreply.github.com> Co-authored-by: Daniel Weindl <dweindl@users.noreply.github.com>
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.
Not sure whether the implementation here is correct. Might also explain why errors are high for petab test models. I agree with Daniel that we should ideally get this working on all of the examples from the benchmark collection as well.
Co-authored-by: Fabian Fröhlich <fabian_froehlich@hms.harvard.edu>
Sure, then I'll compare the gradients for all benchmark problems between AMICI and pyPESTO. Might take a week or so due to other commitments. |
…v/AMICI into fix_simulate_petab_sllh
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.
tbc
ATOL: float = 1e-3 | ||
RTOL: float = 1e-2 | ||
|
||
benchmark_path = Path(__file__).parent.parent.parent / "Benchmark-Models-PEtab" / "Benchmark-Models" |
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.
Could use the benchmark models package to simplify things, but fine as is.
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.
true, should then also change upstream tests, keeping as is for now
python/tests/petab_test_problems/lotka_volterra/model/writer.py
Outdated
Show resolved
Hide resolved
python/tests/petab_test_problems/lotka_volterra/model/convert_to_petab.sh
Show resolved
Hide resolved
python/tests/petab_test_problems/lotka_volterra/model/convert_to_petab.sh
Outdated
Show resolved
Hide resolved
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.
👍
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
Was removed in #1498 due to erroneous implementation.
simulate_petab
pypesto.objective.ObjectiveBase.check_grad{_multi_eps}
[1]yaml2sbml
[2], extended to test more of thesimulate_petab
SLLH computation with[1] https://github.com/ICB-DCM/pyPESTO/blob/32a5daf888dd6c49fa3ca3b8b39e055b6f15ca9f/pypesto/objective/base.py#L397-L620
[2] https://github.com/yaml2sbml-dev/yaml2sbml/tree/773f4c563d90689cd974da8c114b6c7eaf316802/doc/examples/Lotka_Volterra/Lotka_Volterra_python