Fix group selection in `sample_posterior_predictive` when `predictions=True` is passed in kwargs by butterman0 · Pull Request #426 · pymc-devs/pymc-extras

butterman0 · 2025-02-17T13:46:48Z

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 · 2025-02-17T13:50:14Z

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 · 2025-02-18T10:38:10Z

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


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

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.

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?

Just checking you agree with setting predictions=True as default @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 · 2025-02-18T11:32:21Z

        return prior_predictive_samples

-    def sample_posterior_predictive(self, X_pred, extend_idata, combined, **kwargs):
+    def sample_posterior_predictive(self, X_pred, extend_idata, predictions, combined, **kwargs):


Provide default

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 · 2025-02-18T12:40:53Z


        posterior_predictive_samples = self.sample_posterior_predictive(
-            X_pred, extend_idata, combined=False, **kwargs
+            X_pred, extend_idata, predictions, combined=False, **kwargs


pass by keyword to be on the safe side

ricardoV94 · 2025-02-18T12:41:25Z

        X_pred = self._validate_data(X_pred)
        posterior_predictive_samples = self.sample_posterior_predictive(
-            X_pred, extend_idata, combined, **kwargs
+            X_pred, extend_idata, predictions, combined, **kwargs


pass by keyword argument

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?

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

butterman0 · 2025-03-06T16:08:34Z

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 · 2025-03-09T21:03:56Z

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?

butterman0 · 2025-04-25T07:40:24Z

pre-commit.ci autofix

butterman0 · 2025-04-25T07:43:47Z

Hi @ricardoV94, let me know if anything else.

ricardoV94 · 2025-05-20T14:19:53Z

@butterman0 sorry for the delay, running the tests, we can merge if they pass

butterman0 · 2025-05-24T15:27:05Z

pre-commit.ci autofix

butterman0 · 2025-05-24T15:32:05Z

Hi @ricardoV94, one main problem (incorrect argument) arose in the testing which was solved. There were a few more under the hood that arose when I added another test: to test the predict_posterior function.

The predict_posterior function calls _validate_data, which requires a) a 2D array and b) returns a numpy array. Therefore I changed _data_setter in the test setup to handle this.

I assume _validate_data should actually be called in predict as well (I have another pull request #452 open regarding this, which I will edit now), so I added it. Subsequently, I updated the predict calls throughout the tests such that they all pass 2D arrays to _validate_data via predict with data_setter handling it as before.

Let me know if anything is off - test are passing on my side.

… = True

for more information, see https://pre-commit.ci

butterman0 · 2025-08-04T14:16:03Z

Hi @ricardoV94, I'm not totally sure what the problem is as all the tests are passing on my side. I will have a look at the test output when it runs.

butterman0 · 2025-08-22T05:48:15Z

Hi @ricardoV94, what do you think of the updates?

I also am considering writing a python package for the biodiversity model I'm making using PyMC, and in particular model builder. There's a few design choices specifically regarding model builder I wanted to ask about to help make this. Where would be the best forum for that?

ricardoV94 reviewed Feb 18, 2025

View reviewed changes

ricardoV94 reviewed Mar 9, 2025

View reviewed changes

butterman0 force-pushed the enhance/allow_predictions_group branch from 7f84f03 to 7950f99 Compare May 19, 2025 12:20

ricardoV94 approved these changes May 20, 2025

View reviewed changes

ricardoV94 added the bug Something isn't working label May 20, 2025

butterman0 mentioned this pull request May 24, 2025

Minor fixes to modelbuilder class #452

Merged

butterman0 requested a review from ricardoV94 May 27, 2025 12:44

butterman0 and others added 12 commits August 4, 2025 15:49

Fix group selection for posterior predictive samples when predictions…

9bafb10

… = True

refactor: make predictions argument explicit

cb9d1ce

refactor: change default predictions to True

5902182

doc: update docstrings

8cdfaa2

docs: update

803630b

refactor: pass predictions by keyword

dc53c71

test: added test for predictions grouping

091c059

[pre-commit.ci] auto fixes from pre-commit.com hooks

ef3de24

for more information, see https://pre-commit.ci

add missing call to validate data

a42d27f

update predict calls to handle validate data and predictions group

eb9024b

Consolidate test with pytest paramterize

d7270b8

[pre-commit.ci] auto fixes from pre-commit.com hooks

776908f

for more information, see https://pre-commit.ci

butterman0 force-pushed the enhance/allow_predictions_group branch from f4d04c2 to 776908f Compare August 4, 2025 13:52

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

Conversation

butterman0 commented Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Uh oh!

butterman0 commented Feb 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

butterman0 Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ricardoV94 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

butterman0 Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

butterman0 commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ricardoV94 commented Mar 9, 2025

Uh oh!

ricardoV94 left a comment

Choose a reason for hiding this comment

Uh oh!

butterman0 commented Apr 25, 2025

Uh oh!

butterman0 commented Apr 25, 2025

Uh oh!

ricardoV94 commented May 20, 2025

Uh oh!

butterman0 commented May 24, 2025

Uh oh!

butterman0 commented May 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

butterman0 commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

butterman0 commented Aug 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

butterman0 commented Feb 17, 2025 •

edited

Loading

butterman0 Feb 18, 2025 •

edited

Loading

butterman0 Feb 18, 2025 •

edited

Loading

butterman0 commented Mar 6, 2025 •

edited

Loading

butterman0 commented May 24, 2025 •

edited

Loading

butterman0 commented Aug 4, 2025 •

edited

Loading