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

Simplify mcmc module #6269

Merged
merged 6 commits into from
Nov 7, 2022
Merged

Conversation

michaelosthege
Copy link
Member

@michaelosthege michaelosthege commented Nov 5, 2022

What is this PR about?

I'm refactoring mcmc.py (ex sampling.py) to prepare for extraction of population-sampling related code from mcmc.py.

Next to the simplification and moving of code, I also removed support for selecting which variables to include in the trace.
I have never seen this applied anywhere, but anybody feel free to object.
However, if we decide to not make this breaking change, we should still refactor the internal functions towards a clear distinction of trace vs. trace_vars.

Major / Breaking Changes

  • Removed support for selectively tracking variables via pm.sample(trace=[...]).

Bugfixes / New features

  • None

Docs / Maintenance

  • Moved internal functions in mcmc.py down into other modules.

@michaelosthege michaelosthege added maintenance major Include in major changes release notes section labels Nov 5, 2022
@michaelosthege michaelosthege self-assigned this Nov 5, 2022
@michaelosthege michaelosthege added this to the v4.4.0 milestone Nov 5, 2022
@codecov
Copy link

codecov bot commented Nov 5, 2022

Codecov Report

Merging #6269 (21e9824) into main (781d974) will decrease coverage by 11.92%.
The diff coverage is 69.49%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #6269       +/-   ##
===========================================
- Coverage   94.38%   82.46%   -11.93%     
===========================================
  Files         108      111        +3     
  Lines       23839    23820       -19     
===========================================
- Hits        22501    19643     -2858     
- Misses       1338     4177     +2839     
Impacted Files Coverage Δ
pymc/tests/backends/test_base.py 0.00% <0.00%> (ø)
pymc/backends/base.py 68.09% <14.28%> (-19.57%) ⬇️
pymc/sampling/population.py 71.42% <71.42%> (ø)
pymc/stats/convergence.py 92.72% <91.66%> (-0.21%) ⬇️
pymc/sampling/mcmc.py 92.29% <100.00%> (+2.25%) ⬆️
pymc/tests/sampling/test_mcmc.py 98.61% <100.00%> (-0.14%) ⬇️
pymc/tests/sampling/test_population.py 100.00% <100.00%> (ø)
pymc/tests/stats/test_convergence.py 100.00% <100.00%> (ø)
pymc/tests/step_methods/test_metropolis.py 100.00% <100.00%> (ø)
pymc/tests/backends/test_ndarray.py 0.00% <0.00%> (-100.00%) ⬇️
... and 20 more

@michaelosthege michaelosthege force-pushed the simplify-mcmc branch 3 times, most recently from a4e92cb to 217fa40 Compare November 5, 2022 21:03
@ricardoV94
Copy link
Member

Next to the simplification and moving of code, I also removed support for selecting which variables to include in the trace.

Is this similar to the "broken functionality" in this issue: #6116. If so we could also remove it there and close the issue

@michaelosthege
Copy link
Member Author

Is this similar to the "broken functionality" in this issue: #6116. If so we could also remove it there and close the issue

This might be a matter of incorrect type annotation: In pm.sample the trace was advertised as List[str] (among other types), but the test case actually fed a List[TensorVariable]. I don't know if List[str] even worked.

But yes, it sounds like a similar feature.

If we want to support this, I would suggest to reintroduce it with a separate kwarg trace_vars: Optional[Sequence[str]] (str because we'll only want to support named RVs..).

@michaelosthege michaelosthege marked this pull request as ready for review November 7, 2022 16:09
This is in preparation of decoupling population-sampling related code
from other functions in `mcmc.py`.
This uncouples several things:
* `_init_trace` is now independent of abstract step-methods
* `mcmc` is now unaware of `NDArray`
* code for population-sampling can now be extracted from `mcmc`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance major Include in major changes release notes section
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants