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

Remove support for continuation of traces #5019

Merged
merged 2 commits into from
Sep 24, 2021

Conversation

michaelosthege
Copy link
Member

@michaelosthege michaelosthege commented Sep 23, 2021

Choosing initial values for MCMC chains gets a lot easier if we stop supporting to start from existing traces.

It will also make it easier to swap backends in the future, when the public API no longer accepts MultiTrace objects.

There are more good reasons why continuation of traces is bad:

  • Properties like sampling time no longer apply
  • Model could have changed inbetween
  • Tuning will be different in the first and second part of the final result

Starting from existing posterior samples can still be achieved via the pm.sample(start=...) kwarg.

Concatenation of sampling results should be done in a postprocessing step.

Depending on what your PR does, here are a few things you might want to address in the description:

  • what are the (breaking) changes that this PR makes? 👉 Removes support for a little known feature that's not a good practice to begin with.
  • important background, or details about the implementation 👉 Docstrings and type hints were edited. Errors are raised in _choose_backend which is always involved.
  • are the changes—especially new features—covered by tests and docstrings? 👉 Yes.
  • linting/style checks have been run
  • consider adding/updating relevant example notebooks
  • right before it's ready to merge, mention the PR in the RELEASE-NOTES.md Will add this in the release notes draft document manually.

@michaelosthege michaelosthege added this to the vNext (4.0.0) milestone Sep 23, 2021
@codecov
Copy link

codecov bot commented Sep 23, 2021

Codecov Report

Merging #5019 (b2b21c6) into main (53e572c) will decrease coverage by 2.11%.
The diff coverage is 90.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5019      +/-   ##
==========================================
- Coverage   77.24%   75.13%   -2.12%     
==========================================
  Files          85       85              
  Lines       14004    14004              
==========================================
- Hits        10818    10522     -296     
- Misses       3186     3482     +296     
Impacted Files Coverage Δ
pymc3/sampling.py 87.01% <90.00%> (+0.12%) ⬆️
pymc3/step_methods/mlda.py 11.85% <0.00%> (-74.49%) ⬇️
pymc3/tests/models.py 82.01% <0.00%> (-5.76%) ⬇️

@twiecki twiecki merged commit 6a722da into pymc-devs:main Sep 24, 2021
@michaelosthege michaelosthege deleted the no-trace-continuation branch September 24, 2021 11:03
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.

2 participants