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

Ensemble viz #565

Merged
merged 35 commits into from
Feb 8, 2021
Merged

Ensemble viz #565

merged 35 commits into from
Feb 8, 2021

Conversation

paulstapor
Copy link
Contributor

This PR should complete very most of the ensemble functionality, including nice plotting and some more convenience things, such as storing ensemble_predictions directly to h5 files

@codecov-io
Copy link

codecov-io commented Feb 4, 2021

Codecov Report

Merging #565 (976b817) into develop (ddcfd6b) will decrease coverage by 55.53%.
The diff coverage is 19.31%.

Impacted file tree graph

@@             Coverage Diff              @@
##           develop     #565       +/-   ##
============================================
- Coverage    89.81%   34.27%   -55.54%     
============================================
  Files           75       79        +4     
  Lines         4878     5117      +239     
============================================
- Hits          4381     1754     -2627     
- Misses         497     3363     +2866     
Impacted Files Coverage Δ
pypesto/petab/importer.py 0.00% <0.00%> (-85.81%) ⬇️
pypesto/prediction/amici_predictor.py 13.88% <ø> (-83.34%) ⬇️
pypesto/visualize/ensemble.py 10.16% <0.00%> (-89.84%) ⬇️
pypesto/ensemble/ensemble.py 10.89% <7.14%> (-70.28%) ⬇️
pypesto/prediction/prediction.py 21.59% <7.14%> (-72.32%) ⬇️
pypesto/ensemble/covariance_analysis.py 18.75% <18.75%> (ø)
pypesto/visualize/dimension_reduction.py 18.91% <18.91%> (ø)
pypesto/ensemble/utils.py 21.05% <21.05%> (ø)
pypesto/ensemble/dimension_reduction.py 38.88% <38.88%> (ø)
pypesto/ensemble/__init__.py 100.00% <100.00%> (ø)
... and 66 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 ddcfd6b...976b817. Read the comment docs.

@paulstapor
Copy link
Contributor Author

paulstapor commented Feb 5, 2021

Shall I just also add an engine and create a task to be executed for predictions? I just found out that multi-processing within the amici objective doesn't work without an engine...

@yannikschaelte
Copy link
Member

What is "ICB objective"? If you want to multi-process the execution, a first thing would be to check whether you can formalize this as an "EnsembleTask" derived from https://github.com/ICB-DCM/pyPESTO/blob/master/pypesto/engine/task.py, similar to for optimization and profiling. If that does not work (e.g. because you need more dynamic scheduling or interprocess communication), one would need to write the full Engine anew.

@paulstapor
Copy link
Contributor Author

paulstapor commented Feb 5, 2021

What is "ICB objective"? If you want to multi-process the execution, a first thing would be to check whether you can formalize this as an "EnsembleTask" derived from https://github.com/ICB-DCM/pyPESTO/blob/master/pypesto/engine/task.py, similar to for optimization and profiling. If that does not work (e.g. because you need more dynamic scheduling or interprocess communication), one would need to write the full Engine anew.

amici objecive... Too much distraction due to icb (com health) orga retreat... 😂
I looked at the optimization task... I think that should be redoable for predictions or ensembles

@yannikschaelte
Copy link
Member

What is "ICB objective"? If you want to multi-process the execution, a first thing would be to check whether you can formalize this as an "EnsembleTask" derived from https://github.com/ICB-DCM/pyPESTO/blob/master/pypesto/engine/task.py, similar to for optimization and profiling. If that does not work (e.g. because you need more dynamic scheduling or interprocess communication), one would need to write the full Engine anew.

amici objecive... Too much distraction due to icb (com health) orga retreat...
I looked at the optimization task... I think that should be redoable for predictions or ensembles

Nice! ICB objective is a classical example of failed multitasking 😆

@paulstapor
Copy link
Contributor Author

So, what's the consense? Adding an engine to predictions + ensembles? Or leave that t @dilpath in #527 `:D

@yannikschaelte
Copy link
Member

No objections to anything from my side ;)

@dilpath
Copy link
Member

dilpath commented Feb 5, 2021

So, what's the consense? Adding an engine to predictions + ensembles? Or leave that t @dilpath in #527 `:D

👍 Can work on this now

@paulstapor
Copy link
Contributor Author

So, what's the consense? Adding an engine to predictions + ensembles? Or leave that t @dilpath in #527 `:D

+1 Can work on this now

No super big rush there... Just found out that mutlithreading within AmiciObjective also works well for the AmiciPredictor...
I'm more interested in getting this here into develop :)

Copy link
Member

@dilpath dilpath left a comment

Choose a reason for hiding this comment

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

Will review the rest in a little bit, sorry for delays

pypesto/ensemble/covariance_analysis.py Outdated Show resolved Hide resolved
pypesto/ensemble/covariance_analysis.py Outdated Show resolved Hide resolved
pypesto/ensemble/covariance_analysis.py Outdated Show resolved Hide resolved
pypesto/ensemble/covariance_analysis.py Outdated Show resolved Hide resolved
pypesto/ensemble/covariance_analysis.py Outdated Show resolved Hide resolved
pypesto/ensemble/covariance_analysis.py Outdated Show resolved Hide resolved
pypesto/ensemble/covariance_analysis.py Outdated Show resolved Hide resolved
pypesto/ensemble/covariance_analysis.py Outdated Show resolved Hide resolved
pypesto/ensemble/covariance_analysis.py Outdated Show resolved Hide resolved
pypesto/ensemble/covariance_analysis.py Outdated Show resolved Hide resolved
Copy link
Member

@dilpath dilpath left a comment

Choose a reason for hiding this comment

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

👍

There are no tests that assert that the new methods work as intended, only tests that ensure that errors are not produced during a standard workflow.

pypesto/ensemble/ensemble.py Outdated Show resolved Hide resolved
pypesto/ensemble/ensemble.py Outdated Show resolved Hide resolved
pypesto/ensemble/ensemble.py Outdated Show resolved Hide resolved
pypesto/ensemble/ensemble.py Outdated Show resolved Hide resolved
pypesto/ensemble/ensemble.py Outdated Show resolved Hide resolved
f.create_group(base + f'{LOWER_BOUND}s')
for i_cond, lower_bounds in enumerate(ensemble_prediction.lower_bound):
condition_id = \
ensemble_prediction.prediction_results[0].condition_ids[i_cond]
Copy link
Member

Choose a reason for hiding this comment

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

If ensemble_prediction.prediction_results[i].condition_ids is equal for all i then maybe the condition_ids could be stored independently of ensemble_prediction.prediction_results to avoid duplication, e.g. ensemble_prediction.condition_ids?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In principle yes, but we store a multitude of PredictionResults here. As PredictionResult also must work as a class of its own, it makes sense to also store the conditionIds in there. One could now either reduce what's stored, therefore introduce a new class... or storing something redundant... What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

For the purposes of e.g. plotting by condition or observable, it would be nice to automatically ensure that all PredictionResults share the same condition IDs, and the prediction for the same condition between different PredictionResults contains the same observable IDs. And also to provide methods to easily get all data by condition or observable, from an EnsemblePrediction. Maybe a wrapper for SampleEnsemblePrediction? Anyway, I can do this, let me know what you think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would actually suggest to open 3 issues:

  • wishes and stuff we can use the ensemble functionality for (e.g., what you just wrote)
  • wishes and stuff we can use the prediction functionality for (e.g., prediction profiles)
  • wishes and stuff we can use the dimesnion reduction functionality for (e.g., analyzing optimization runs from a multi-start visually)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But yes: Asserting these things are equal would be good. Currently, they must be equal, just by the way the predict functions creates things...

pypesto/prediction/prediction.py Outdated Show resolved Hide resolved
pypesto/visualize/dimension_reduction.py Outdated Show resolved Hide resolved
pypesto/visualize/dimension_reduction.py Outdated Show resolved Hide resolved
pypesto/visualize/dimension_reduction.py Outdated Show resolved Hide resolved
paulstapor and others added 14 commits February 8, 2021 13:40
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
@paulstapor paulstapor merged commit e9a5d5e into develop Feb 8, 2021
@paulstapor paulstapor deleted the ensemble_viz branch February 8, 2021 20:00
@yannikschaelte yannikschaelte mentioned this pull request Mar 17, 2021
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.

4 participants