ICB-DCM / pyPESTO

python Parameter EStimation TOolbox
https://pypesto.readthedocs.io
BSD 3-Clause "New" or "Revised" License
207 stars 45 forks source link

Decouple pypesto.ensemble.ensemble.Ensemble from pypesto.predict.amici_predictor.AmiciPredictor #1294

Open dweindl opened 6 months ago

dweindl commented 6 months ago

Currently, pypesto.ensemble.ensemble.Ensemble.predict is annotated to take a predictor: Callable. However, it assumes this to be an AmiciPredictor (or at least to have an amici_objective: AmiciObjective attribute), .e.g., here: https://github.com/ICB-DCM/pyPESTO/blob/e389257973b063d7e1219abd4b5570f3277ecaa9/pypesto/ensemble/ensemble.py#L855

I don't think this is ideal. This makes it unnecessarily cumbersome to create a custom predictor, which should just be some callable that takes a parameter vector and parameter IDs and produces a PredictionResult, or an EnsemblePrediction.

I'd propose:

dweindl commented 3 months ago

I think, before working on this, we'll need to discuss the desired ensemble functionality in pypesto.

The current ensemble prediction setup is 100% tailored to AMICI

https://github.com/ICB-DCM/pyPESTO/blob/82b7d5da1273b9d442a634e5aabbda9685279d77/pypesto/result/predict.py#L113-L115

https://github.com/ICB-DCM/pyPESTO/blob/82b7d5da1273b9d442a634e5aabbda9685279d77/pypesto/result/predict.py#L28-L34

Can we / do we want to generalize that? If not, I would suggest to rename those objects to something like AmiciPredictionResult/AmiciPredictionConditionResult to make it very clear that they are only intended to work with AmiciObjective, AmiciPredictor, ...