-
Notifications
You must be signed in to change notification settings - Fork 47
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
Decouple pypesto.ensemble.ensemble.Ensemble from pypesto.predict.amici_predictor.AmiciPredictor #1294
Comments
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 pyPESTO/pypesto/result/predict.py Lines 113 to 115 in 82b7d5d
pyPESTO/pypesto/result/predict.py Lines 28 to 34 in 82b7d5d
Can we / do we want to generalize that? If not, I would suggest to rename those objects to something like |
Currently,
pypesto.ensemble.ensemble.Ensemble.predict
is annotated to take apredictor: Callable
. However, it assumes this to be anAmiciPredictor
(or at least to have anamici_objective: AmiciObjective
attribute), .e.g., here:pyPESTO/pypesto/ensemble/ensemble.py
Line 855 in e389257
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 aPredictionResult
, or anEnsemblePrediction
.I'd propose:
pypesto.ensemble.ensemble.Ensemble._map_parameters_by_objective
to thepredictor
AmiciPredictor
dependencyAmiciPredictor.__call__
will then need the parameter IDs along with the parameter values. Breaking change.predictor: Callable
topredictor: Callable[[Sequence], PredictionResult]
(and respecting that)pypesto.ensemble.ensemble.Ensemble.predict
altogether, and rather offer that same functionality viaWhateverPredictor.predict(ensemble: pypesto.ensemble.ensemble.Ensemble, ...)
. This seems more appropriate, since different predictors might want to offer different arguments.Predictor
base class that implements the respective functionality that's currently included inpypesto.ensemble.ensemble.Ensemble.predict
.The text was updated successfully, but these errors were encountered: