Skip to content

Conversation

fonnesbeck
Copy link
Member

Closes #1529

@taku-y let me know if there was a rationale for having apparently redundant arguments (samples and size) before we merge this.

@kyleabeauchamp
Copy link
Contributor

The size argument is apparently used in the docs: https://pymc-devs.github.io/pymc3/notebooks/posterior_predictive.html

@springcoil
Copy link
Contributor

I think that doc needs changed then.

@aloctavodia
Copy link
Member

I think the rationale for the size argument is as follows. Suppose I fit a Gaussian distribution with 30 observed data points. I may want to use sample_ppc to get 100 posterior samples (samples=100) each one with the same size as the observed data points (size=30). For other models (like linear models) the size argument may look like redundant because each sample from sample_ppc will have the same size as the observed data points.

@twiecki
Copy link
Member

twiecki commented Dec 1, 2016

@aloctavodia yeah, that makes sense to me. I kinda felt like there was a reason for this but couldn't remember what.

@fonnesbeck
Copy link
Member Author

fonnesbeck commented Dec 1, 2016

If you are doing posterior predictive checks, you always want it the same shape as the observed data points; its the number of replicates that is what needs to be specified. I suppose you could pass a vector of indicators, or a random number to subsample from the original data, but for simplicity we should let users do that as post-processing. Having size and samples as is presently implemented is more confusing than helpful, I think.

@fonnesbeck
Copy link
Member Author

Is everyone cool with this PR, at least as far as a 3.0 release is concerned?

@aloctavodia
Copy link
Member

@fonnesbeck I agree with you. For ppc you always want it the same shape as the observed data points. The point is that in order to get that, you need to pass size for some models but not for others, so I also agree having samples and size arguments could be confusing.

@twiecki
Copy link
Member

twiecki commented Dec 2, 2016

I don't think we should merge this. It does not solve a problem, it just removes functionality that is necessary for some corner cases. I would thus just keep it as is until we refactor this code.

@fonnesbeck
Copy link
Member Author

That's fair enough. We should probably change the docstring then to make it clearer which should be used for routine checks.

@twiecki
Copy link
Member

twiecki commented Dec 5, 2016

I'm closing this then, we can change the doc-string in a separate PR.

@twiecki twiecki closed this Dec 5, 2016
@fonnesbeck fonnesbeck deleted the sample_ppc_size branch December 6, 2016 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants