Skip to content

Refactor sample_posterior_predictive_w for V4 #5800

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

Closed

Conversation

zaxtax
Copy link
Contributor

@zaxtax zaxtax commented May 24, 2022

This tries to address #4807

Right now, it passes all unit tests, but doesn't use the fast sampling procedure of sample_posterior_predictive

The code is also not feature-complete.

@ricardoV94
Copy link
Member

fast posterior_predictive_sampling no longer exists (everything is fast now). Also some tweaks will be needed following #5787 which should be merged soon!

@codecov
Copy link

codecov bot commented May 24, 2022

Codecov Report

Merging #5800 (6683916) into main (1e7d91f) will increase coverage by 0.43%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5800      +/-   ##
==========================================
+ Coverage   88.99%   89.43%   +0.43%     
==========================================
  Files          74       74              
  Lines       13809    13807       -2     
==========================================
+ Hits        12290    12348      +58     
+ Misses       1519     1459      -60     
Impacted Files Coverage Δ
pymc/sampling.py 89.68% <100.00%> (+7.16%) ⬆️
pymc/parallel_sampling.py 86.46% <0.00%> (-1.00%) ⬇️
pymc/backends/base.py 86.44% <0.00%> (-0.74%) ⬇️

@zaxtax
Copy link
Contributor Author

zaxtax commented May 26, 2022

Let me make those tweaks and push tonight.

@@ -1958,13 +1958,14 @@ def sample_posterior_predictive_w(
# TODO sample_posterior_predictive_w is currently only work for model with
# one observed.
# XXX: This needs to be refactored
# ppc[var.name].append(draw_values([var], point=param, size=size[idx])[0])
raise NotImplementedError()
ppcl[var.name].append(draw([var])[0])
Copy link
Member

Choose a reason for hiding this comment

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

This won't be conditioned on the posterior samples.

You will have to create a custom function similarly to what sample_posterior_predictive does. See recent changes in #5759

Copy link
Member

@ricardoV94 ricardoV94 May 26, 2022

Choose a reason for hiding this comment

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

It might also be useful to compare what changed between V3 and V4 in sample_posterior_predictive because some of the things in sample_posterior_predictive_w might no longer be needed / make sense

@zaxtax zaxtax force-pushed the refactor-sample_posterior_predictive_w branch from 5bb0f63 to f1d0f91 Compare May 31, 2022 22:43
@zaxtax zaxtax force-pushed the refactor-sample_posterior_predictive_w branch from f1d0f91 to 6683916 Compare May 31, 2022 22:45
@ricardoV94 ricardoV94 changed the title WIP: Refactor sample_posterior_predictive_w for V4 Refactor sample_posterior_predictive_w for V4 Jun 14, 2022
@ricardoV94 ricardoV94 marked this pull request as draft June 14, 2022 11:33
@zaxtax
Copy link
Contributor Author

zaxtax commented Oct 29, 2022

Closing in favour of #6254

@zaxtax zaxtax closed this Oct 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants