Skip to content
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

Split sampling.py into sampling.py and sampling_forward.py #6257

Merged
merged 6 commits into from
Nov 2, 2022

Conversation

michaelosthege
Copy link
Member

@michaelosthege michaelosthege commented Nov 1, 2022

Closes #6141

What is this PR about?
Splitting up the sampling.py module such that MCMC and prior/posterior predictive sampling are separated.

This also splits one of the longest-running test files.

I did not change any implementation - not even renamed a function.

For review, please check the __all__ settings, because they define what is the public API.

Checklist

Docs / Maintenance

  • Split sampling.py into sampling.py and sampling_forward.py

@ricardoV94
Copy link
Member

I would suggest sampling_forward or sampling_ancestral instead of sampling_predictive and to put draw in the same group (so just 2 modules, not 3). Predictive refers to the goal of sampling whereas forward/ancestral refers to the method of sampling.

@michaelosthege
Copy link
Member Author

@ricardoV94 we need three modules because sampling and sampling_predictive share some dependencies.

Tearing the tests apart might be the bigger problem here..

I like sampling_predictive because it's more intuitive for noobs what to expect to find in there.. But feel free to push the branch - I'm going biking for a few hours now 🚴‍♀️

@ricardoV94
Copy link
Member

ricardoV94 commented Nov 1, 2022

The predictive label is a bit misleading though.

You can equally easily use mcmc to do predictive sampling (e.g., if you want to incorporate transforms/potentials) or use sample_*_predictive to do posterior inference (e.g., if you have a closed form posterior)

@codecov
Copy link

codecov bot commented Nov 1, 2022

Codecov Report

Merging #6257 (49b826a) into main (ea721e4) will increase coverage by 0.03%.
The diff coverage is 95.76%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6257      +/-   ##
==========================================
+ Coverage   94.14%   94.17%   +0.03%     
==========================================
  Files         100      102       +2     
  Lines       21239    21472     +233     
==========================================
+ Hits        19996    20222     +226     
- Misses       1243     1250       +7     
Impacted Files Coverage Δ
pymc/sampling_forward.py 95.55% <95.55%> (ø)
pymc/parallel_sampling.py 87.32% <100.00%> (+0.29%) ⬆️
pymc/sampling_jax.py 97.18% <100.00%> (ø)
pymc/smc/kernels.py 97.37% <100.00%> (ø)
pymc/tests/distributions/test_mixture.py 99.35% <100.00%> (+<0.01%) ⬆️
pymc/tests/distributions/test_multivariate.py 99.44% <100.00%> (ø)
pymc/tests/distributions/test_timeseries.py 99.50% <100.00%> (+<0.01%) ⬆️
pymc/variational/opvi.py 87.01% <100.00%> (-0.03%) ⬇️
pymc/tests/stats/test_convergence.py 100.00% <0.00%> (ø)
pymc/stats/convergence.py 92.92% <0.00%> (+2.02%) ⬆️

@michaelosthege michaelosthege changed the title WIP: Split sampling into three modules Split sampling into three modules Nov 1, 2022
@ricardoV94
Copy link
Member

What about moving the few items left in sampling_utils to utils and getting rid of that third file?

@michaelosthege
Copy link
Member Author

What about moving the few items left in sampling_utils to utils and getting rid of that third file?

These itemw are used by both of the others - I think it's nice to keep sampling.py and sampling_forward.py fully independent.. After all these years, you know

@michaelosthege michaelosthege marked this pull request as ready for review November 2, 2022 06:35
@ricardoV94 ricardoV94 changed the title Split sampling into three modules Refactor sampling.py Nov 2, 2022
Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

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

Self-approval, someone else please review. Commits should be squashed

@ricardoV94 ricardoV94 requested a review from twiecki November 2, 2022 11:20
Copy link
Member Author

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

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

Diff looking good

@ricardoV94 ricardoV94 changed the title Refactor sampling.py Split sampling.py into sampling.py and sampling_forward.py Nov 2, 2022
@ricardoV94 ricardoV94 merged commit 23c4834 into main Nov 2, 2022
@ricardoV94 ricardoV94 deleted the split-sampling branch November 2, 2022 12:24
michaelosthege added a commit to michaelosthege/pymc that referenced this pull request Nov 5, 2022
This is a follow-up to pymc-devs#6257 where we split the `sampling.py` into two files.
michaelosthege added a commit to michaelosthege/pymc that referenced this pull request Nov 5, 2022
This is a follow-up to pymc-devs#6257 where we split the `sampling.py` into two files.
michaelosthege added a commit to michaelosthege/pymc that referenced this pull request Nov 5, 2022
This is a follow-up to pymc-devs#6257 where we split the `sampling.py` into two files.
michaelosthege added a commit to michaelosthege/pymc that referenced this pull request Nov 5, 2022
This is a follow-up to pymc-devs#6257 where we split the `sampling.py` into two files.
michaelosthege added a commit to michaelosthege/pymc that referenced this pull request Nov 5, 2022
This is a follow-up to pymc-devs#6257 where we split the `sampling.py` into two files.
michaelosthege added a commit to michaelosthege/pymc that referenced this pull request Nov 6, 2022
This is a follow-up to pymc-devs#6257 where we split the `sampling.py` into two files.
ricardoV94 pushed a commit that referenced this pull request Nov 7, 2022
This is a follow-up to #6257 where we split the `sampling.py` into two files.
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.

Split MCMC and Forward sampling functions in sampling.py
3 participants