-
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
added option for weighted mean in ensemble #702
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #702 +/- ##
===========================================
+ Coverage 88.16% 89.53% +1.36%
===========================================
Files 79 97 +18
Lines 5257 6613 +1356
===========================================
+ Hits 4635 5921 +1286
- Misses 622 692 +70
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a test would be good
pypesto/ensemble/ensemble.py
Outdated
@@ -138,7 +140,8 @@ def condense_to_arrays(self): | |||
} | |||
|
|||
def compute_summary(self, | |||
percentiles_list: Sequence[int] = (5, 20, 80, 95) | |||
percentiles_list: Sequence[int] = (5, 20, 80, 95), | |||
weighting: bool = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weighting: bool = True | |
weigh: bool = True |
Option can be weigh
and values can be weights
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this should be False
by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is just a matter of taste I think. As LLH
is not included in a predictor by default, this will get turned to False
in line 168. The idea behind that for me was that if you include llh
in predictor for ensembles the default you probably would want is True
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in the default setting, weighting==True
but no weights are used? Might be confusing for users. Could call it something more descriptive, e.g. weights_if_llh_provided
, or make it False
by default?
pypesto/ensemble/ensemble.py
Outdated
# check if weightings shall be used or not depending on whether llh | ||
# is part of the prediciton output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# check if weightings shall be used or not depending on whether llh | |
# is part of the prediciton output | |
# Check whether LLH-based weights can be used. |
pypesto/ensemble/ensemble.py
Outdated
if AMICI_LLH not in self.prediction_results[0].conditions[0].output: | ||
weighting = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A user might explicitly want weights but not realize they're not used because of this. A warning might help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That warning would then be problematic if the weighting flag is True by default, because the ensemble prediction would issue a warning by default. But default settings should not be chosen such that they issue warnings, I think.
pypesto/ensemble/ensemble.py
Outdated
summary[STANDARD_DEVIATION] = np.average( | ||
(tmp_array.T-summary[MEAN].T).T**2, | ||
axis=-1, weights=weights) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yes, there needs to be an additional np.sqrt() 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then equivalent to np.average(np.abs(tmp_array - summary[MEAN]), axis=-1, weights=weights)
I guess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, at least that should not be the case mathematically...
pypesto/ensemble/ensemble.py
Outdated
if AMICI_LLH not in self.prediction_results[0].conditions[0].output: | ||
weighting = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That warning would then be problematic if the weighting flag is True by default, because the ensemble prediction would issue a warning by default. But default settings should not be chosen such that they issue warnings, I think.
pypesto/ensemble/ensemble.py
Outdated
output_list = [prediction.conditions[ic].output | ||
for prediction in self.prediction_results] | ||
else: | ||
output_list = [prediction.conditions[ic].output[AMICI_Y] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just saw below, that it seems you intend to change the type of prediction.conditions[ic].output
. However, this is a data member of the class PredictionConditionResult. I'd strongly argue that data members of classes should not switch type, if possible, and if inevitable: Only during major refactorings, as this will typically break backwards compatibility. Rather add a new data member for the weights...
pypesto/ensemble/ensemble.py
Outdated
output_list = [prediction.conditions[ic].output[AMICI_LLH] | ||
for prediction in self.prediction_results] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... I have the impression that this will be problematic. By default, a PredictionConditionResult has a member "output", which is supposed to be an ndarray. You're making a dict out of it, which might brake quite some dependencies...
I would rather suggest you add a new member "output_weight" to the PredictionConditionResult. This way, you can just use this new member here, and an additional field will not affect the functioning of old script, hence not destroy backwards compatibility.
…/pyPESTO into feature_ensemble_weights
…eature_ensemble_weights
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I am not really into that part of the code, I did only review code style (otherwise I would have to get an overview of the rest of the ensemble code first). So please let someone else review regarding functionality/structrue, but from style point of view: Looks fine to me :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
Maybe an example in a notebook?
compute_weighted_sigma=True) | ||
except TypeError: | ||
raise ValueError('Computing a summary failed.') | ||
n_conditions = len(self.prediction_results[0].conditions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can conditions differ between predictions? If so, can simply raise a NotImplementedError
if there are different conditions between the prediction results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they should not be able to, as the ensembles currently are created from the same amici_model which should generate the prediction the same no matter what the actual parameter values are.
Might be interesting way down the train, when we allow for different model structures to contribute to the same ensemble.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conditions are provided via amici.ExpData
(a single amici.Model
can simulate different sets of conditions).
if y_meas.shape != mean_traj.shape: | ||
raise ValueError('Shape of trajectory and shape ' | ||
'of measurements does not match.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this assumes equal number of timepoints -> same timepoints. However, given the pypesto.objective.AmiciObjective.set_custom_timepoints
method, maybe this can't be assumed. Could check timepoints explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure, but shouldn't the whole optimization fail if we set custom timepoints as we could not compute a likelihood anymore? 🤔 correct me if I am wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, often used for predictions rather than methods that require a likelihood.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in that case you wouldn't compute the chi^2 value either I'd say.
This for now is only a draft.
It assumes, that the post processor either returns only a np.ndarray or a dict/dict like object.
Perhaps want to add another default post processor, that would only need to receive
AMICI_outputs
do creat a return dict of things that should be returned (like 'x', 'y', 'llh' etc).Also need to add sigmas.
Currently the weighting is done in terms of llh of each condition separately. should I go with the llh for one parameter across all conditions?