Skip to content

Refactor internal sampling functions #6444

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

Merged

Conversation

michaelosthege
Copy link
Member

@michaelosthege michaelosthege commented Jan 8, 2023

This is a refactor to simplify signatures of internal sampling functions.

The goal is to avoid returning/yielding BaseTrace or MultiTrace objects, making it easier to refactor the trace backend.

Checklist

Major / Breaking Changes

  • The pm.iter_sample function was removed.

Maintenance

  • Signatures of internal sampling functions were changed.
  • Refactoring of internal sampling code to make it more maintainable.

@michaelosthege michaelosthege added the trace-backend Traces and ArviZ stuff label Jan 8, 2023
@codecov
Copy link

codecov bot commented Jan 8, 2023

Codecov Report

Merging #6444 (2ab5264) into main (434333f) will decrease coverage by 44.21%.
The diff coverage is 62.50%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #6444       +/-   ##
===========================================
- Coverage   85.92%   41.70%   -44.22%     
===========================================
  Files         148      148               
  Lines       27691    27668       -23     
===========================================
- Hits        23794    11540    -12254     
- Misses       3897    16128    +12231     
Impacted Files Coverage Δ
pymc/tests/sampling/test_mcmc.py 98.60% <ø> (-0.02%) ⬇️
pymc/util.py 70.86% <ø> (-9.14%) ⬇️
pymc/sampling/population.py 20.39% <27.77%> (-51.83%) ⬇️
pymc/stats/log_likelihood.py 23.40% <66.66%> (-76.60%) ⬇️
pymc/sampling/mcmc.py 88.97% <84.61%> (-3.25%) ⬇️
pymc/step_methods/compound.py 100.00% <100.00%> (ø)
pymc/sampling_jax.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/gp/test_gp.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/gp/test_mean.py 0.00% <0.00%> (-100.00%) ⬇️
pymc/tests/gp/test_util.py 0.00% <0.00%> (-100.00%) ⬇️
... and 105 more

@michaelosthege michaelosthege force-pushed the refactor-sampling-functions branch from 356718a to d39b685 Compare January 9, 2023 00:12
@michaelosthege michaelosthege marked this pull request as ready for review January 9, 2023 08:18
@michaelosthege michaelosthege force-pushed the refactor-sampling-functions branch from 52d88e2 to da79e78 Compare January 9, 2023 10:29
@michaelosthege michaelosthege force-pushed the refactor-sampling-functions branch from da79e78 to af6a52c Compare January 9, 2023 11:17
sample_args = {
"draws": draws,
"step": step,
"start": initial_points,
"trace": trace,
"traces": traces,
Copy link
Member

Choose a reason for hiding this comment

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

Is potential breakage worth changing the arg to plural?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a signature change regardless: Optional[BaseTrace]Sequence[BaseTrace] for multiple functions that this dict is fed to.

All of these are internal API though, so breakage should not be a problem

@michaelosthege michaelosthege force-pushed the refactor-sampling-functions branch from ff5a6e9 to 2ab5264 Compare January 10, 2023 17:49
@michaelosthege michaelosthege merged commit 8547d6d into pymc-devs:main Jan 10, 2023
@michaelosthege michaelosthege deleted the refactor-sampling-functions branch January 10, 2023 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
trace-backend Traces and ArviZ stuff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants