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

add deprecation warnings for old backends #3902

Merged

Conversation

michaelosthege
Copy link
Member

Some old backends are cluttering the codebase. We don't know of anybody still using them, so this PR adds deprecation warnings.

I'll add a line to the release notes just in case anybody ignores warnings..

Copy link
Member

@ColCarroll ColCarroll left a comment

Choose a reason for hiding this comment

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

I am, in general, super in favor of this!

My only reservation is that save_trace/load_trace is currently the only non-pickle way of reloading a trace to, for example, do ppc sampling. I'm sort of ok going back to suggesting pickle unless someone has a reasonable security reason not to.

RELEASE-NOTES.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 2, 2020

Codecov Report

Merging #3902 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3902      +/-   ##
==========================================
+ Coverage   83.41%   83.42%   +0.01%     
==========================================
  Files         103      103              
  Lines       14190    14203      +13     
==========================================
+ Hits        11836    11849      +13     
  Misses       2354     2354              
Impacted Files Coverage Δ
pymc3/backends/ndarray.py 92.63% <100.00%> (+0.15%) ⬆️
pymc3/backends/sqlite.py 93.51% <100.00%> (+0.10%) ⬆️
pymc3/backends/text.py 97.16% <100.00%> (+0.11%) ⬆️
pymc3/backends/tracetab.py 100.00% <100.00%> (ø)

Co-authored-by: Colin <[email protected]>
@michaelosthege
Copy link
Member Author

I am, in general, super in favor of this!
My only reservation is that save_trace/load_trace is currently the only non-pickle way of reloading a trace to, for example, do ppc sampling. I'm sort of ok going back to suggesting pickle unless someone has a reasonable security reason not to.

sample_posterior_predictive works with InferenceData since a few weeks ago.

@ColCarroll
Copy link
Member

😻 deprecate and merge away, then!

@twiecki twiecki merged commit 2d68025 into pymc-devs:master May 3, 2020
@michaelosthege michaelosthege deleted the backend-deprecation-warnings branch May 3, 2020 10:13
@rpgoldman
Copy link
Contributor

If we are going to deprecate save and load trace, shouldn't we go the further step of making sample() return an InferenceData object?

It seems like we shouldn't leave things in a state where we deprecate saving and loading MultiTrace objects, but still return them.

It also seems like we should revisit @aseyboldt 's idea of making PyMC3 variable creation support a dims argument (and possibly store coords on the Model object).

@michaelosthege
Copy link
Member Author

I'm all in for returning InferenceData. I still find the xarray indexing super confusing, but this is the way.

I'll do a PR tomorrow to add sample(..., return_inferencedata=False), with a FutureWarning, so we can change it to sample(..., return_inferencedata=True) in v4.0.0.
Then we'll see how much breaks (half the test suite I suppose 😅).

AlexAndorra pushed a commit that referenced this pull request May 4, 2020
* updated the Hogg notebook

* attempted to clarify the kwargs in sample() docstring describing how to pass kwargs to the steppers

I believe this fixes #3197

I also noted this need for more clarity in my updated notebook in this PR

`pymc3/docs/source/notebooks/GLM-robust-with-outlier-detection.ipynb`

* Remove deprecated stuff (#3906)

* remove file which is not used

* remove deprecated code

* repair tests and notebooks that used deprecated API

* mention #3906

Co-authored-by: Michael Osthege <[email protected]>

* add deprecation warnings for old backends (#3902)

* add deprecation warnings for old backends

* mention backend deprecation #3902

* fix typo

Co-authored-by: Colin <[email protected]>

Co-authored-by: Michael Osthege <[email protected]>
Co-authored-by: Colin <[email protected]>

* minor formatting to notebook and rework of docstring for sample function

notebook: dropped all headings one level lower to comply with TOC logic, and very minor language edits

sampling.py: clarifired language around single vs compoundstep

* updated the Hogg notebook

* attempted to clarify the kwargs in sample() docstring describing how to pass kwargs to the steppers

I believe this fixes #3197

I also noted this need for more clarity in my updated notebook in this PR

`pymc3/docs/source/notebooks/GLM-robust-with-outlier-detection.ipynb`

* minor formatting to notebook and rework of docstring for sample function

notebook: dropped all headings one level lower to comply with TOC logic, and very minor language edits

sampling.py: clarifired language around single vs compoundstep

* updates folowing AlexAndorra review:

upgrade to arviz=0.7
set prior params to slightly simpler (more justifiable) values, and testvals to simplier defaults
explanatory clarifications
formatting, typos,

* removed the note re step_kwargs, since this PR updates the appropriate docstring

* a cell had become markdown, silly. reset it to code and rerun

* minor code reformatting via black_nbconvert, final check and re-run

* rerun notebook purely as a lazy but safe way to trigger new CI
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.

4 participants