Predictions #544

paulstapor · 2020-12-21T19:33:11Z

This is a concept for predictions within pyPESTO. I pushed this branch for a PR now, although 2 things are still missing:

I want to reuse the existing conversion reaction model in the unit tests (and modify it using libsbml inplace) rather than pushing a slightly altered model into the conversion reaction folder. I will remove the current extended conversion reaction model then, but I added to have the tests running-
I will add a wrapper for PEtab models, which uses the current framework, but makes things easier for the user, if they wirk with PEtab.

Yet, it's just before christmas and I wanted to have things pushed as I think most corner stones are set and we may want to discuss whether the overall concept is okay.

Why this whole concept of predictions? It's for four reasons:

I want and need something more flexible than just the AMICI output with the list of rdatas.
I added some functionality, which allows to use predictions also for large models and large datasets. For this purpose, the prediction class is just a very thin wrapper around some ndarrays.
This will be integrated into the concepts of Ensembles/Ensemble Predictions, which is next on my list and also particularly suited for large-scale models (which I also need somewhat urgently)
We will need something like predictions, if we ever want to implement prediction uncertainty in pyPESTO, which I think is very reasonable to have.

…ci objective

codecov-io · 2021-01-07T16:29:09Z

Codecov Report

Merging #544 (c70a47d) into develop (71722e5) will decrease coverage by 6.35%.
The diff coverage is 29.83%.

@@             Coverage Diff             @@
##           develop     #544      +/-   ##
===========================================
- Coverage    90.43%   84.07%   -6.36%     
===========================================
  Files           65       69       +4     
  Lines         4225     4403     +178     
===========================================
- Hits          3821     3702     -119     
- Misses         404      701     +297

Impacted Files	Coverage Δ
pypesto/prediction/amici_predictor.py	`14.08% <14.08%> (ø)`
pypesto/prediction/prediction.py	`22.50% <22.50%> (ø)`
pypesto/petab/importer.py	`76.64% <42.85%> (-4.28%)`	⬇️
pypesto/objective/amici.py	`89.75% <100.00%> (-1.10%)`	⬇️
pypesto/objective/base.py	`87.71% <100.00%> (ø)`
pypesto/prediction/__init__.py	`100.00% <100.00%> (ø)`
pypesto/prediction/constants.py	`100.00% <100.00%> (ø)`
pypesto/petab/pysb_importer.py	`0.00% <0.00%> (-100.00%)`	⬇️
pypesto/startpoint/util.py	`50.00% <0.00%> (-35.00%)`	⬇️
pypesto/optimize/optimizer.py	`57.14% <0.00%> (-33.70%)`	⬇️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71722e5...c70a47d. Read the comment docs.

dilpath

Looks good 👍 Making states available to the user would be useful for state uncertainties.

The notebook fails during optimization.

doc/example/conversion_reaction/conditions.tsv

pypesto/objective/amici.py

pypesto/objective/base.py

pypesto/objective/amici_prediction.py

dilpath · 2021-01-07T18:49:41Z

pypesto/objective/amici_prediction.py

+                # loop over parameters
+                for i_par in range(output_sensi[i_out].shape[1]):
+                    # create filename for this condition and parameter
+                    tmp_filename = os.path.join(output_path, output_file_dummy +


Could use the Path object here too.

pypesto/objective/amici_prediction.py

test/petab/test_amici_prediction.py

Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>

…ions

Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>

FFroehlich · 2021-01-08T15:49:02Z

pypesto/prediction/amici_predictor.py

+        outputs_sensi = amici_sy
+        timepoints = amici_t
+        if self.post_processor is not None:
+            outputs = self.post_processor(outputs)


Suggested change

outputs = self.post_processor(outputs)

outputs = self.post_processor(amici_y)

No change in logic, but better consistency with code below.

One important aspect here though is that this code may have issues if the user-provided post processing routines apply their changes in-place. May want to add documentation about this or add safeguards in the code.

Will change that a bit anyway, as this part of the code isn't really what I was aiming for. But yes, this is an important suggestion, will do so

pypesto/prediction/amici_predictor.py

FFroehlich · 2021-01-08T15:54:03Z

pypesto/prediction/amici_predictor.py

+            n_simulations = 1
+        else:
+            # simulate only a subset of conditions
+            n_simulations = 1 + int(len(self.amici_objective.edatas) /


Would feel more comfortable with an explicit call to ceil here.

FFroehlich · 2021-01-08T15:58:29Z

pypesto/prediction/amici_predictor.py

+            of the simulations. Default are the timepoints of the amici model.
+            This method takes a list of ndarrays (as returned in the field
+            ['t'] of amici ReturnData objects) as input.
+        max_num_conditions:


I am not a fan of the name, since its not really a max. What about condition_chunk_size?

chunk_size is the word I was looking for and didn't find... :D 👍

FFroehlich · 2021-01-08T16:02:44Z

test/petab/test_amici_prediction.py

+def conversion_reaction_model():
+    # read in sbml file
+    model_name = 'conversion_reaction'
+    sbml_file = f'../../doc/example/{model_name}/model_{model_name}.xml'


please use os.path.dirname and os.path.join

…l in post processing

dweindl · 2021-01-11T15:57:02Z

pypesto/__init__.py

@@ -21,6 +21,10 @@
    Objective,
    NegLogPriors,
    ObjectiveBase)
+from .prediction import (


Please add pypesto.prediction to API documentation (update sphinx main file).

dweindl · 2021-01-11T15:57:53Z

test/petab/test_amici_prediction.py

+
+
+def test_petab_prediction():
+    yaml_file = '../../doc/example/conversion_reaction/' \


Please use relative path based on __file__ to make tests run from different working directories.

paulstapor · 2021-01-12T08:32:29Z

All Comments should be addressed now. In particular, the changes I did now were:

refactoring the routine which actually calls amici, such that the actual call is more encapsulated to make sure that memory is freed
now states, observables and timepoints can be used in all postprocessing routines (passed via a list of dicts): This will allow an extension by an objective function on the prediction, and hence prediction/validation profiles to be implemented easily
using pathlib and nicer path handling
separating prediction results (also those routines which write the result to csv's or an h5) from the actual predictor
using the conversion reaction example sbml as it is, as modifying it causes trouble in too many other scripts
some cleanup in documentation and code

paulstapor · 2021-01-12T11:25:53Z

Fine to merge, or any further comments, remarks?

dweindl · 2021-01-12T11:47:15Z

Didn't get through everything, but two reviewers are enough for me. GTG.

dilpath

Looks good, fine for me, just a few suggestions but they're not so important I think.

dilpath · 2021-01-12T08:42:52Z

pypesto/prediction/amici_predictor.py


 from .constants import (MODE_FUN, OBSERVABLE_IDS, TIMEPOINTS, OUTPUT,
-                        OUTPUT_SENSI, TIME, CSV, H5, T, Y, SY, RDATAS)
+                        OUTPUT_SENSI, CSV, H5, T, X, SX, Y, SY, RDATAS)


In general, I think these constants should be unabbreviated and as unambiguous as possible (as in PEtab). e.g. OUTPUT_SENSI -> OUTPUT_SENSITIVITY, and X -> STATE. In particular, STATE instead of X might be important here since x is used for parameters elsewhere in pyPESTO, so if a constant is added at some point to represent parameters, it might also be PARAMETER instead of X too.

Regarding OUTPUT_SENSITIVITY and others, should some of these constants be imported from AMICI? For now, this is fine as is for me.

Another alternative, could be renamed e.g. X to AMICI_X or AMICI_STATE

dilpath · 2021-01-12T08:47:03Z

pypesto/prediction/amici_predictor.py

+        amici_outputs:
+            List of Dicts with the fields 't', 'x', 'sx', 'y', and 'sy' as
+            returned by an amici simulation


I guess this can be removed now

dilpath · 2021-01-12T08:50:08Z

pypesto/prediction/amici_predictor.py

+        timepoints = [outputs['t'] for outputs in amici_outputs]
+        # add outputs and sensitivities if requested
+        if 0 in sensi_orders:
+            outputs = [outputs['y'] for outputs in amici_outputs]


Suggested change

outputs = [outputs['y'] for outputs in amici_outputs]

outputs = [amici_output['y'] for amici_output in amici_outputs]

A little confusing without understanding the scope, since outputs would be on both sides of the assignment operator.

Also could rename elsewhere, where outputs is used to refer to elements of amici_outputs and not the outputs variable.

dilpath · 2021-01-12T09:49:48Z

pypesto/prediction/amici_predictor.py

-                result.to_csv(tmp_filename, sep='\t')
+        # postprocess
+        if self.post_processor is not None:
+            outputs = self.post_processor(amici_outputs)


outputs is overwritten here. If intended, then the above code that previously assigned a value to outputs (and similarly outputs_sensi) could be moved to be elif of these conditions, as e.g.

# add outputs and sensitivities if requested and post-processing was skipped if self.post_processor is not None: ... elif 0 in sensi_orders: outputs = [amici_output['y'] for amici_output in amici_outputs] if self.post_processor_sensi is not None: ... elif 1 in sensi_orders: outputs_sensi = [amici_output['sy'] for amici_output in amici_outputs]

Could have the post-processors be a dictionary such that any output can be processed, e.g.

AMICI_TIME = 't' AMICI_STATE = 'x' AMICI_OBSERVABLE = 'y' # user input post_processors = { AMICI_OBSERVABLE: my_custom_post_processor_callable, 'sy': my_custom_post_processor_sensi_callable, } # default processor default_post_processors = { AMICI_TIME: lambda t: t, AMICI_STATE: lambda x: x, AMICI_OBSERVABLE: lambda y: y, 'sy': lambda sy: sy, } output_types = amici_outputs[0].keys() # actual processing outputs = {} for output_type in output_types: outputs[output_type] = None if output_type in post_processors: outputs[output_type] = post_processors[output_type](amici_outputs) # deepcopy amici_outputs? elif output_type in default_post_processors: outputs[output_type] = default_post_processors[output_type](amici_outputs) # deepcopy amici_outputs?

pypesto/prediction/prediction.py

Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>

pypesto/prediction/amici_predictor.py

pypesto/petab/importer.py

pypesto/prediction/amici_predictor.py

FFroehlich · 2021-01-12T15:00:10Z

pypesto/prediction/amici_predictor.py

+        # return dependent on sensitivity order
+        return results
+
+    def _get_outputs(self, x, sensi_orders, mode) -> Tuple[List, List, List]:


Co-authored-by: Fabian Fröhlich <fabian@schaluck.com>

paulstapor added 9 commits December 14, 2020 14:53

adding a simple prediction class for amici models

13c7749

fix prediction implementation

dbe4f9d

add functionality for saving into file

8df23f0

encapsulate call to amici objective to enable slicing of calls to ami…

fdac30d

…ci objective

allow overloading of objective base

e3b4f32

document

36c3e8e

enable prediction test

857673c

introduce prediction results class

c9f1aab

add conversion reaction model

e6bc146

paulstapor requested review from FFroehlich, yannikschaelte, jvanhoefer and MerktSimon December 21, 2020 19:34

using existing conversion reaction model for unit testing

360d2a6

paulstapor requested a review from dilpath January 7, 2021 15:19

add thin wrapper around PEtab models

34cace3

dilpath approved these changes Jan 7, 2021

View reviewed changes

paulstapor and others added 12 commits January 8, 2021 09:50

add test for PEtab wrapper of predictions

8c6cc1f

rerun notebook petab_import with an example of predction functionality

9680bf0

Update doc/example/conversion_reaction/conditions.tsv

823d348

Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>

Update pypesto/objective/amici.py

2796b70

Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>

Update pypesto/objective/amici.py

edc6972

Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>

start adressding code review

232ddfc

Merge branch 'predictions' of github.com:ICB-DCM/pyPESTO into predict…

6d27cbd

…ions

Update pypesto/objective/amici_prediction.py

07e0774

Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>

Update pypesto/objective/base.py

ee756bb

Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>

Update pypesto/objective/amici_prediction.py

c704b2e

Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>

Update pypesto/objective/amici_prediction.py

ebc7ff6

Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>

Update pypesto/objective/amici_prediction.py

d5640ed

Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>

FFroehlich approved these changes Jan 8, 2021

View reviewed changes

paulstapor removed request for yannikschaelte and MerktSimon January 8, 2021 16:06

paulstapor added 4 commits January 11, 2021 14:05

change usage of paths and cleanup from code review

7ebd6e7

beautify paths in test

31c75c3

refactor call to Amici in prediction, allow also states of amici mode…

2c9e5e0

…l in post processing

reset old notebook

d8479c1

dweindl reviewed Jan 11, 2021

View reviewed changes

paulstapor added 4 commits January 11, 2021 17:03

fix tests 1

7c878ae

add prediction api to doc

242bd04

fix tests 2

d0f8c08

fix 3

3aa901e

paulstapor added 2 commits January 12, 2021 09:51

fix list vs set error

bd12f36

remove blank

9014467

dilpath approved these changes Jan 12, 2021

View reviewed changes

paulstapor and others added 2 commits January 12, 2021 13:45

Update pypesto/prediction/prediction.py

b6c35bc

Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>

Update pypesto/prediction/prediction.py

715a7c3

Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>

paulstapor commented Jan 12, 2021

View reviewed changes

pypesto/prediction/amici_predictor.py Outdated Show resolved Hide resolved

Update pypesto/prediction/amici_predictor.py

1734935

FFroehlich approved these changes Jan 12, 2021

View reviewed changes

paulstapor and others added 4 commits January 12, 2021 16:43

Update pypesto/petab/importer.py

0b5be01

Co-authored-by: Fabian Fröhlich <fabian@schaluck.com>

address comments second round

4eda3c4

add some safeguards in the default output

6af2131

add some safeguards in the default output

c70a47d

paulstapor merged commit 24f3ce3 into develop Jan 13, 2021

paulstapor deleted the predictions branch January 13, 2021 09:26

yannikschaelte mentioned this pull request Jan 19, 2021

v 0.2.3 #552

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Predictions #544

Predictions #544

paulstapor commented Dec 21, 2020

codecov-io commented Jan 7, 2021 •

edited

Loading

dilpath left a comment

dilpath Jan 7, 2021

FFroehlich Jan 8, 2021

FFroehlich Jan 8, 2021

paulstapor Jan 8, 2021 •

edited

Loading

FFroehlich Jan 8, 2021

FFroehlich Jan 8, 2021

paulstapor Jan 8, 2021

FFroehlich Jan 8, 2021

dweindl Jan 11, 2021

dweindl Jan 11, 2021

paulstapor commented Jan 12, 2021

paulstapor commented Jan 12, 2021

dweindl commented Jan 12, 2021

dilpath left a comment

dilpath Jan 12, 2021

dilpath Jan 12, 2021

dilpath Jan 12, 2021

FFroehlich Jan 12, 2021

dilpath Jan 12, 2021

dilpath Jan 12, 2021

dilpath Jan 12, 2021

dilpath Jan 12, 2021

dilpath Jan 12, 2021

FFroehlich Jan 12, 2021

	outputs = self.post_processor(outputs)
	outputs = self.post_processor(amici_y)



		def test_petab_prediction():
		yaml_file = '../../doc/example/conversion_reaction/' \

	outputs = [outputs['y'] for outputs in amici_outputs]
	outputs = [amici_output['y'] for amici_output in amici_outputs]

Predictions #544

Predictions #544

Conversation

paulstapor commented Dec 21, 2020

codecov-io commented Jan 7, 2021 • edited Loading

Codecov Report

dilpath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulstapor Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulstapor commented Jan 12, 2021

paulstapor commented Jan 12, 2021

dweindl commented Jan 12, 2021

dilpath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 7, 2021 •

edited

Loading

paulstapor Jan 8, 2021 •

edited

Loading