Add sllh computation back to `petab_objective.simulate_petab` #1548

dilpath · 2021-09-13T23:33:32Z

Was removed in #1498 due to erroneous implementation.

parameters are now scaled within simulate_petab
added FD checks, essentially copied from pypesto.objective.ObjectiveBase.check_grad{_multi_eps} [1]
added a test case: a PEtab problem based on the Lotka-Volterra example in yaml2sbml [2], extended to test more of the simulate_petab SLLH computation with
- multiple experimental conditions
- preequilibration

[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

codecov · 2021-09-13T23:43:42Z

Codecov Report

Merging #1548 (bc4c48c) into develop (255be19) will increase coverage by 1.16%.
The diff coverage is 95.83%.

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

Flag	Coverage Δ
petab	`54.90% <95.83%> (+1.16%)`	⬆️

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

Impacted Files	Coverage Δ
python/sdist/amici/petab_objective.py	`93.77% <93.47%> (+0.34%)`	⬆️
python/tests/test_petab_objective.py	`100.00% <100.00%> (ø)`

... and 3 files with indirect coverage changes

…; clean up

dweindl

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.

python/amici/petab_objective.py

yannikschaelte

if llh and sllh are correct (in particular the gradient check is happy), LGTM

python/amici/petab_objective.py

yannikschaelte · 2021-11-05T11:09:10Z

python/amici/petab_objective.py

@@ -150,6 +160,15 @@ def simulate_petab(

    # Compute total llh
    llh = sum(rdata['llh'] for rdata in rdatas)
+    # Compute total sllh


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.

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).

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

python/tests/test_petab_objective.py

python/amici/petab_objective.py

yannikschaelte · 2021-11-05T11:17:04Z

python/amici/petab_objective.py

+                # SLLH is (un)scaled here with methods that were written for
+                # parameters.
+                # First unscale w.r.t. model scale.
+                partial_parameter_sllh = unscale_parameter(


I thought we had somewhere also a function rescale(val, scale_in, scale_out), but nvm

dilpath · 2021-11-05T17:02:36Z

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.

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 <[email protected]> Co-authored-by: Daniel Weindl <[email protected]>

FFroehlich

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.

python/amici/petab_objective.py

Co-authored-by: Fabian Fröhlich <[email protected]>

dilpath · 2021-11-07T18:51:57Z

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.

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

dweindl

tbc

tests/benchmark-models/test_petab_benchmark.py

dweindl · 2023-05-08T10:26:03Z

tests/benchmark-models/test_petab_benchmark.py

+ATOL: float = 1e-3
+RTOL: float = 1e-2
+
+benchmark_path = Path(__file__).parent.parent.parent / "Benchmark-Models-PEtab" / "Benchmark-Models"


Could use the benchmark models package to simplify things, but fine as is.

true, should then also change upstream tests, keeping as is for now

tests/benchmark-models/test_petab_benchmark.py

python/tests/petab_test_problems/lotka_volterra/model/writer.py

python/tests/test_petab_objective.py

python/tests/petab_test_problems/lotka_volterra/model/convert_to_petab.sh

documentation/example_errors.ipynb

.github/workflows/test_benchmark_collection_models.yml

dweindl

👍

python/sdist/amici/petab_objective.py

Co-authored-by: Dilan Pathirana <[email protected]>

add sllh to simulate_petab

4dc559a

dilpath and others added 3 commits September 14, 2021 18:09

use ExpData for plist if available

2a7fb85

Merge branch 'develop' into fix_simulate_petab_sllh

6eb0ddc

Merge branch 'develop' into fix_simulate_petab_sllh

9f4613c

dweindl assigned dilpath Oct 17, 2021

dilpath and others added 8 commits October 25, 2021 23:29

use plist if set; breakpoint at error; preliminary test

5e666cb

add sensi test to preexisting benchmark model test

6b0be2b

remove outdated test and test cases

16d646b

add FD check for petab problems; add petab problem test case for SLLH…

a408330

…; clean up

Merge branch 'develop' into fix_simulate_petab_sllh

bb1412a

typo, RTOL

3f7c5ec

fix paths

2bcc164

docs

4e51f91

dilpath requested review from dweindl, FFroehlich and yannikschaelte November 5, 2021 09:12

dilpath marked this pull request as ready for review November 5, 2021 09:12

dweindl approved these changes Nov 5, 2021

View reviewed changes

python/amici/petab_objective.py Outdated Show resolved Hide resolved

python/amici/petab_objective.py Outdated Show resolved Hide resolved

yannikschaelte approved these changes Nov 5, 2021

View reviewed changes

dilpath and others added 2 commits November 6, 2021 21:07

Apply suggestions from code review

60445b5

Co-authored-by: Yannik Schälte <[email protected]> Co-authored-by: Daniel Weindl <[email protected]>

reviews: docstring;error handling;optional sllh aggregation

2cc7d66

FFroehlich reviewed Nov 7, 2021

View reviewed changes

dilpath and others added 3 commits November 7, 2021 11:49

Apply suggestions from code review

839f585

Co-authored-by: Fabian Fröhlich <[email protected]>

review: clean up

7dee733

Merge branch 'develop' into fix_simulate_petab_sllh

3d449e5

dilpath added 2 commits November 8, 2021 20:02

ignore parameters missing due to estimate=0

4bc1729

Merge branch 'fix_simulate_petab_sllh' of https://github.com/AMICI-de…

ced3201

…v/AMICI into fix_simulate_petab_sllh

FFroehlich added 3 commits May 6, 2023 20:42

Update test_benchmark_collection_models.yml

af569e3

fix paths

764f68c

Update test_petab_benchmark.py

eba5790

FFroehlich self-assigned this May 7, 2023

FFroehlich and others added 9 commits May 7, 2023 11:56

restrict test set

e2759a5

Update test_petab_benchmark.py

4dcb459

Update test_petab_benchmark.py

b841596

Update test_petab_benchmark.py

0d9e8dd

Update test_petab_benchmark.py

4849be4

use fiddy main

574130f

Merge branch 'develop' into fix_simulate_petab_sllh

4b16443

Update test_petab_benchmark.py

0956e24

Merge branch 'develop' into fix_simulate_petab_sllh

cb67463

FFroehlich marked this pull request as ready for review May 8, 2023 10:18

FFroehlich requested a review from a team as a code owner May 8, 2023 10:18

FFroehlich requested a review from dweindl May 8, 2023 10:19

FFroehlich added 2 commits May 8, 2023 11:28

oopsie

e462867

Merge branch 'fix_simulate_petab_sllh' of https://github.com/AMICI-de…

84f1065

…v/AMICI into fix_simulate_petab_sllh

dweindl reviewed May 8, 2023

View reviewed changes

Merge branch 'develop' into fix_simulate_petab_sllh

fa29356

dweindl approved these changes May 8, 2023

View reviewed changes

python/sdist/amici/petab_objective.py Outdated Show resolved Hide resolved

python/sdist/amici/petab_objective.py Outdated Show resolved Hide resolved

address code review

da6623b

dilpath commented May 8, 2023

View reviewed changes

python/sdist/amici/petab_objective.py Outdated Show resolved Hide resolved

python/sdist/amici/petab_objective.py Outdated Show resolved Hide resolved

dilpath and others added 4 commits May 8, 2023 18:59

remove custom grad check methods; adjust test

4354faa

Update python/sdist/amici/petab_objective.py

ff794f5

Co-authored-by: Dilan Pathirana <[email protected]>

Merge branch 'develop' into fix_simulate_petab_sllh

6586224

Merge branch 'develop' into fix_simulate_petab_sllh

bc4c48c

FFroehlich merged commit d2fb35e into develop May 9, 2023

FFroehlich deleted the fix_simulate_petab_sllh branch May 9, 2023 07:15

dweindl mentioned this pull request Aug 18, 2023

Gradient tests for benchmark collection #967

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sllh computation back to `petab_objective.simulate_petab` #1548

Add sllh computation back to `petab_objective.simulate_petab` #1548

dilpath commented Sep 13, 2021 •

edited

Loading

codecov bot commented Sep 13, 2021 •

edited

Loading

dweindl left a comment

yannikschaelte left a comment

yannikschaelte Nov 5, 2021

dilpath Nov 6, 2021

dweindl Nov 7, 2021

yannikschaelte Nov 5, 2021

dilpath commented Nov 5, 2021

FFroehlich left a comment

dilpath commented Nov 7, 2021

dweindl left a comment

dweindl May 8, 2023

FFroehlich May 8, 2023

dweindl left a comment

Add sllh computation back to petab_objective.simulate_petab #1548

Add sllh computation back to petab_objective.simulate_petab #1548

Conversation

dilpath commented Sep 13, 2021 • edited Loading

codecov bot commented Sep 13, 2021 • edited Loading

Codecov Report

dweindl left a comment

Choose a reason for hiding this comment

yannikschaelte left a comment

Choose a reason for hiding this comment

yannikschaelte Nov 5, 2021

Choose a reason for hiding this comment

dilpath Nov 6, 2021

Choose a reason for hiding this comment

dweindl Nov 7, 2021

Choose a reason for hiding this comment

yannikschaelte Nov 5, 2021

Choose a reason for hiding this comment

dilpath commented Nov 5, 2021

FFroehlich left a comment

Choose a reason for hiding this comment

dilpath commented Nov 7, 2021

dweindl left a comment

Choose a reason for hiding this comment

dweindl May 8, 2023

Choose a reason for hiding this comment

FFroehlich May 8, 2023

Choose a reason for hiding this comment

dweindl left a comment

Choose a reason for hiding this comment

Add sllh computation back to `petab_objective.simulate_petab` #1548

Add sllh computation back to `petab_objective.simulate_petab` #1548

dilpath commented Sep 13, 2021 •

edited

Loading

codecov bot commented Sep 13, 2021 •

edited

Loading