Fix group selection in sample_posterior_predictive when predictions=True is passed in kwargs

[butterman0]

Summary

Fixes hard-coded group selection in sample_posterior_predictive which unnecessarily restricts usage of predict functions. Previously, if predictions=True (ideally set in pm.sample_posterior_predictive when predicting out-of-sample) is passed as a kwarg to the predict functions, the inference data was extracted from posterior_predictive group which is incorrect when predictions = True.

Changes

Selects appropriate group depending if predictions is passed.

[butterman0]

I'm sorry I haven't opened an issue first - I thought it was such a minor change that it wasn't necessary. This is also my first contribution so not 100% on the process!

[ricardoV94]

Would it be better to make predictions an explicit kwarg (with the same default as PyMC) and use that directly?

[butterman0]

Yes I initially had that! Although I wasn't sure what was best practice. It is nice to make it explicit, but it means passing predictions=False as explicit args through the predict method and then to sample_posterior_predictive which is used in other methods - although this shouldn't be a problem if keeping the same default as PyMC as you suggest.

[butterman0]

In fact, the class method sample_posterior_predictive is called twice, on both occasions it is for prediction: class methods predict and predict_posterior.

I think I would argue that in this case we would like the default to be predictions=True (as opposed to the pymc pm.sample_posterior_predictive default). The default would be set in the predict and predict_posterior methods.

I say this because when False, the posterior_predictive group in the idata object is overridden - meaning we would have to run fit or sample_model again if we wanted to do posterior predictive checks?

[butterman0]

Just checking you agree with setting predictions=True as default @ricardoV94 ?

[ricardoV94]

Yeah makes sense in the predict oriented methods

[ricardoV94]

The predictions argument should be mentioned in the docstrings now that it is explicit

[ricardoV94]

Provide default

[butterman0]

The other arguments do not have defaults. The sample_posterior_predictive is only called through the predict functions, which do have defaults.

Would you be able to explain why we would want predictions to have a default, when the other arguments do not?

[ricardoV94]

pass by keyword to be on the safe side

[ricardoV94]

pass by keyword argument

[butterman0]

I was aiming to keep it in the same format as current implementation. i.e. x_pred, extend_idata and combined do not use keyword arguments..

Similar question to the one above - should these all be changed to use keyword arguments? Why would we treat predictions differently?

[butterman0]

Hi @ricardoV94, let me know what you think and I can adjust.

[butterman0]

Hi @ricardoV94, I've made the requested changes.

Two commits updating the doc strings.

Most recent commit passing by keyword argument and setting default as requested.

I'm still a little unclear as to why we would treat the predictions argument differently to the other arguments in the sample_posterior_predictive method (i.e. combined and extend_idata). Similarly, the same question as to why we would pass by keyword when calling the method when we don't with the other variables.

Harry

[ricardoV94]

We should always pass by keyword argument, but since you didn't write the previous code I didn't ask you to change those lines

[ricardoV94]

Can you add a test that confirms this is working now?