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

Bring back stats and plotting aliases #4536

Merged
merged 5 commits into from
Mar 14, 2021

Conversation

michaelosthege
Copy link
Member

As discussed in #4528 this PR brings back the old pm.plotting and pm.stats submodules.

However they now have deprecation warnings that instruct users to switch to the original function names, either via pm.* or arviz directly.

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? It un-breaks!
  • important background, or details about the implementation All objects except __* ones are stolen from the arviz.stats or arviz.plots submodules. This is to make pm.plots and pm.stats as identical as possible, so users can seamlessly switch to the original ArviZ submodules.
  • are the changes—especially new features—covered by tests and docstrings? They are covered in ArviZ.
  • linting/style checks have been run
  • consider adding/updating relevant example notebooks No.
  • right before it's ready to merge, mention the PR in the RELEASE-NOTES.md

densityplot = map_args(az.plot_density)
pairplot = map_args(az.plot_pair)

# Use compact traceplot by default
Copy link
Member Author

Choose a reason for hiding this comment

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

ArviZ switched their default in v0.11.0 and pinned that as a minimum.

"""
import functools
import sys
import warnings

import arviz as az


def map_args(func):
swaps = [("varnames", "var_names")]
Copy link
Member Author

Choose a reason for hiding this comment

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

These should have been around for long enough - I replaced the soft with a hard warning.

r2_score = map_args(az.r2_score)
rhat = map_args(az.rhat)
summary = map_args(az.summary)
waic = map_args(az.waic)
Copy link
Member Author

Choose a reason for hiding this comment

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

All the names are identical - therefore they can be imported without wrapping.

@michaelosthege
Copy link
Member Author

michaelosthege commented Mar 13, 2021

As you can see there are two commits. The first is the revert of the one that removed the wrappers a while back.

In the second commit I straightened and simplified the API and implementation.

(We'll probably want to squash-merge.)

- :func:`pymc3.bfmi <arviz:arviz.bfmi>`
- :func:`pymc3.compare <arviz:arviz.compare>`
- :func:`pymc3.ess <arviz:arviz.ess>`
- :data:`pymc3.geweke <arviz:arviz.geweke>`
Copy link
Member

Choose a reason for hiding this comment

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

we deleted geweke in ArviZ, it had basically been broken for a while (worked only on 1d arrays) and nobody had complained. This is why there are questions on discourse about it being missing, but so far nobody has complained about it being missing, only about the import error it causes with old pymc3 versions.

- :func:`pymc3.compare <arviz:arviz.compare>`
- :func:`pymc3.ess <arviz:arviz.ess>`
- :data:`pymc3.geweke <arviz:arviz.geweke>`
- :func:`pymc3.hpd <arviz:arviz.hpd>`
Copy link
Member

Choose a reason for hiding this comment

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

I understand wanting to keep the original pm.hpd name, but az.hpd no longer exists either, it is now az.hdi. This one is used and had a deprecation warning for several arviz releases indicating users to switch from hpd to hdi.

Note that currently the code does not do any hpd-hdi aliasing on pymc3 side though.

@twiecki
Copy link
Member

twiecki commented Mar 14, 2021

I like it, but I'm not a fan of the deprecation warning. For me the ideal path forward is:

  • v3 will keep these wrappers indefinitely.
  • v4 will also have wrappers but even more light-weight as they are just forwards to the arviz functions.

If we agree on this, we don't need deprecation warnings here, except for maybe that the names will change from traceplot to plot_trace.

@michaelosthege
Copy link
Member Author

Terminology:

  • wrapper: something that is wrapped with map_args; where the name of the ArviZ function is written in the PyMC3 codebase
  • forwards: the stuff that was imported via for example for attr in dir(az.stats):

v3 will keep these wrappers indefinitely.
We can do that - the DeprecationWarning does not say when they will go away. In reality they go away with v4.

If we agree on this, we don't need deprecation warnings here, except for maybe that the names will change from traceplot to plot_trace.

As soon as the original ArviZ function name is used, there is no DeprecationWarning.
All the warnings are just about the name changes!

@twiecki
Copy link
Member

twiecki commented Mar 14, 2021

Ah, perfect! So calling pm.traceplot says to call pm.plot_trace, and when I call pm.plot_trace() there's no warning?

@michaelosthege
Copy link
Member Author

Ah, perfect! So calling pm.traceplot says to call pm.plot_trace, and when I call pm.plot_trace() there's no warning?

Exactly:
https://github.com/pymc-devs/pymc3/pull/4536/files#diff-7551c50e746859405a4695f62a2732aa5b98cbdf10a23439c57d1f5ff6c2a935R40-R44

@twiecki
Copy link
Member

twiecki commented Mar 14, 2021

Awesome, same page.

RELEASE-NOTES.md Outdated Show resolved Hide resolved
pymc3/stats/__init__.py Outdated Show resolved Hide resolved
docs/source/api/plots.rst Outdated Show resolved Hide resolved
docs/source/api/stats.rst Outdated Show resolved Hide resolved
michaelosthege and others added 2 commits March 14, 2021 15:53
Co-authored-by: Oriol Abril-Pla <[email protected]>
Co-authored-by: Oriol Abril-Pla <[email protected]>
@michaelosthege
Copy link
Member Author

For some reason I couldn't use "Add suggestion to batch".
If y'all are fine with it let's just sqash-merge.

@michaelosthege michaelosthege merged commit 3079983 into pymc-devs:v3 Mar 14, 2021
@michaelosthege michaelosthege deleted the fix-v3-4528 branch March 14, 2021 16:24
michaelosthege added a commit to michaelosthege/pymc that referenced this pull request Mar 16, 2021
Also see pymc-devs#4536 where the wrappers were brought back for v3.

Closes pymc-devs#4528
michaelosthege added a commit to michaelosthege/pymc that referenced this pull request Mar 16, 2021
Also see pymc-devs#4536 where the wrappers were brought back for v3.

Closes pymc-devs#4528
michaelosthege added a commit to michaelosthege/pymc that referenced this pull request Mar 16, 2021
Also see pymc-devs#4536 where the wrappers were brought back for v3.

Closes pymc-devs#4528
michaelosthege added a commit that referenced this pull request Mar 17, 2021
Also see #4536 where the wrappers were brought back for v3.

Closes #4528
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.

3 participants