Skip to content

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

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.

Sign up for GitHub

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

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

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
34839ad
Fix group selection for posterior predictive samples when predictions…
butterman0 Feb 17, 2025
7cdd900
refactor: make predictions argument explicit
butterman0 Feb 18, 2025
a0501fe
refactor: change default predictions to True
butterman0 Feb 18, 2025
079e131
doc: update docstrings
butterman0 Feb 18, 2025
fb6b1e9
docs: update
butterman0 Mar 6, 2025
ce1b2d5
refactor: pass predictions by keyword
butterman0 Mar 6, 2025
4ea5fbc
test: added test for predictions grouping
butterman0 Apr 9, 2025
7f84f03
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Apr 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion pymc_extras/model_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,8 +650,11 @@ def sample_posterior_predictive(self, X_pred, extend_idata, combined, **kwargs):
if extend_idata:
self.idata.extend(post_pred, join="right")

# Determine the correct group dynamically
group_name = "predictions" if kwargs.get("predictions", False) else "posterior_predictive"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

@butterman0 butterman0 Feb 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah makes sense in the predict oriented methods


posterior_predictive_samples = az.extract(
post_pred, "posterior_predictive", combined=combined
post_pred, group_name, combined=combined
)

return posterior_predictive_samples
Expand Down