-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Pymc3 Plot & Diagnostics & Arviz Dependency #4397
Pymc3 Plot & Diagnostics & Arviz Dependency #4397
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Note to self: Is |
Codecov Report
@@ Coverage Diff @@
## master #4397 +/- ##
========================================
Coverage 88.17% 88.17%
========================================
Files 88 87 -1
Lines 14563 14338 -225
========================================
- Hits 12841 12643 -198
+ Misses 1722 1695 -27
|
Thanks @CloudChaoszero! I think we should make sure to also change the |
875f4b9
to
721af7c
Compare
Sounds great. I'll proceed with that in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good so far, I only have a minor suggestion about the api page. I'm looking forward to have the example notebooks updated.
721af7c
to
2f788a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @CloudChaoszero ,
I think this PR should move forward and get merged before the 3.11.0
release. We should keep just the plot_posterior_predictive_glm
(and its tests) around because there is no replacement for it.
I think it's fair to remove the other plotting and stats functions, because we advertised ArviZ for a long time and even though they did not have deprecation warnings, their replacement is just a minor inconvenience for the users. 3.11
will bring much more severe breaking changes than this.
2f788a6
to
c588520
Compare
Thanks for the feedback y'all! 😄 Alright, so I implemented the following, based on reviews: 3.11This PR is against Documentation@OriolAbril Keeping the plot_posterior_predictive_glm Function@michaelosthege I went ahead and added a warning to warnings.warn(
"The `plot_posterior_predictive_glm` function will migrate to Arviz in a future release. "
"\nKeep up to date with `ArviZ <https://arviz-devs.github.io/arviz/>`_ for future updates.",
DeprecationWarning,
) @AlexAndorra Question: Are there any other Arviz diagnostics dependencies I should consider, outside of the plots topic? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @CloudChaoszero, starting to look good!
I added a few comments and suggestions below to make it ready to merge 😉
@CloudChaoszero please try to bundle the commits. Every commit that is pushed causes the entire test pipeline to run, resulting in a lot of unnecessary compute. When committing suggested changes from Github use the "add suggestion to batch" and when committing locally, try to push all commits at the same time - unless you want to trigger the CI for some specific reason. Many currently running CI jobs can probably be cancelled.. |
b5045c8
to
c90a6f3
Compare
@michaelosthege Whoops, pardon! 😅 Thanks for the additional details. @AlexAndorra Updated the PR. I am (slowly) also working on pymc-example PR to update docs, regarding this change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice - barring a couple of nitpicks, this looks good to me
pymc3/plots/__init__.py
Outdated
@@ -1,4 +1,4 @@ | |||
# Copyright 2020 The PyMC Developers | |||
# Copyright 2021 The PyMC Developers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this needs to change (and if it does, I think the change needs doing for the whole codebase?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, let's not change that here then
pymc3/plots/__init__.py
Outdated
"exploratory analysis of Bayesian models." See https://arviz-devs.github.io/arviz/ | ||
for details on plots. | ||
Plots are delegated to the `ArviZ <https://arviz-devs.github.io/arviz/>`_ library, a general purpose library for | ||
"exploratory analysis of Bayesian models." For more details, see https://arviz-devs.github.io/arviz/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"exploratory analysis of Bayesian models." For more details, see https://arviz-devs.github.io/arviz/ | |
"exploratory analysis of Bayesian models". For more details, see https://arviz-devs.github.io/arviz/ . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would actually remove the "
in exploratory analysis of Bayesian models.
pymc3/plots/__init__.py
Outdated
Plots are delegated to the `ArviZ <https://arviz-devs.github.io/arviz/>`_ library, a general purpose library for | ||
"exploratory analysis of Bayesian models." For more details, see https://arviz-devs.github.io/arviz/ | ||
|
||
Only the `plot_posterior_predictive_glm` is kept in the PyMC code base for now, but it'll will move to ArviZ once the latter adds features for regression plot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the `plot_posterior_predictive_glm` is kept in the PyMC code base for now, but it'll will move to ArviZ once the latter adds features for regression plot. | |
Only `plot_posterior_predictive_glm` is kept in the PyMC code base for now, but it will move to ArviZ once the latter adds features for regression plots. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick update @CloudChaoszero ! Still a few typos to iron out
docs/source/api/plots.rst
Outdated
- :func:`pymc3.densityplot <arviz:arviz.plot_density>` | ||
- :func:`pymc3.pairplot <arviz:arviz.plot_pair>` | ||
.. warning:: | ||
The `plot_posterior_predictive_glm` function will removed in a future PyMC3 release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The `plot_posterior_predictive_glm` function will removed in a future PyMC3 release. | |
The `plot_posterior_predictive_glm` function will be removed in a future PyMC3 release. |
pymc3/plots/__init__.py
Outdated
@@ -1,4 +1,4 @@ | |||
# Copyright 2020 The PyMC Developers | |||
# Copyright 2021 The PyMC Developers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, let's not change that here then
pymc3/plots/__init__.py
Outdated
"exploratory analysis of Bayesian models." See https://arviz-devs.github.io/arviz/ | ||
for details on plots. | ||
Plots are delegated to the `ArviZ <https://arviz-devs.github.io/arviz/>`_ library, a general purpose library for | ||
"exploratory analysis of Bayesian models." For more details, see https://arviz-devs.github.io/arviz/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would actually remove the "
in exploratory analysis of Bayesian models.
docs/source/api/plots.rst
Outdated
- :func:`pymc3.kdeplot <arviz:arviz.plot_kde>` | ||
- :func:`pymc3.densityplot <arviz:arviz.plot_density>` | ||
- :func:`pymc3.pairplot <arviz:arviz.plot_pair>` | ||
.. warning:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not use an sphinx warning here but inside its api docs.
"""Plot posterior predictive of a linear model.
Parameters
----------
trace : arviz.InferenceData or MultiTrace
Output of pm.sample()
eval : array_like
Array over which to evaluate lm
lm : function, default: linear function
Function mapping parameters at different points
to their respective outputs.
input: point, sample
output: estimated value
samples : int, default=30
How many posterior samples to draw.
kwargs : mapping, optional
Additional keyword arguments are passed to ``matplotlib.pyplot.plot()``.
Warnings
--------
The `plot_posterior_predictive_glm` function will removed in a future PyMC3 release.
"""
something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great @OriolAbril I added it. However, the Warning did not show in my local version. 🤷
Sounds great @AlexAndorra. I went ahead and also updated the plots docs to have the warning be withing the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take the warning in the api docs is working with the current docstring? The two images without the warning are the same and don't match the docstring text. Other than that everything looks great
🔥 Remove arviz plots * Remove directly imported arviz plots used in pymc3 plots 🔥 Remove all plots from PyMC3 Plots module 🔥 Remove PyMC3 plots references in Docs 🎨 Mention Plotting & Diagnostics in API page and remove plots reference in __init__.py ⏪ Revert posterior_plot function, test, and docs 🎨 Add deprecation warning to posterior_plot function 🎨 Add context on plot import and import back into __init__.py ✏️ Add warning and details of posterior_plot added Update docs/source/api/plots.rst Co-authored-by: Alexandre ANDORRA <[email protected]> Update docs/source/api/plots.rst Co-authored-by: Alexandre ANDORRA <[email protected]> Update pymc3/plots/__init__.py Co-authored-by: Alexandre ANDORRA <[email protected]> Update pymc3/plots/__init__.py Co-authored-by: Alexandre ANDORRA <[email protected]> ✏️ Update docs to add stats.rst details ✏️ Minor docs notation for posterioplot function(s) 📝 Add breakline before docstring title
a4c8f5b
to
8076612
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @CloudChaoszero ! Found one last typo. And does the warning display in the plot docs now?
Done! And @AlexAndorra I believe not.
|
Yeah but we definitely need this warning in the docs as well. And I don't see why it would not work for you locally but work when we recompile the new docs. What do you think @OriolAbril ? |
@CloudChaoszero did you do |
@MarcoGorelli Hmm, maybe not. But yeah, that looks good to me! 👏 thanks for verifying. CC: @AlexAndorra |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aaaah, perfect then, thanks for chiming in with some doc magic @MarcoGorelli 🧙♂️
This is good to merge now 🥳 Thanks @CloudChaoszero !
Description
This PR starts the (Breakchange) removal of PyMC3 Plots and Diagnostics process referred in #4371, and relying on referred functionality from Arviz. See an example from the quote below
The Implementation Process
Add Deprecation warning to one remaining (plot) function
Add a
DeprecateWarning
to functionplot_posterior_predictive_glm
V43.11 implementationNotes
what are the (breaking) changes that this PR makes?
important background, or details about the implementation
are the changes—especially new features—covered by tests and docstrings?
consider adding/updating relevant example notebooks
right before it's ready to merge, mention the PR in the RELEASE-NOTES.md