-
-
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
Remove several functions and objects from PyMC root namespace #6973
base: main
Are you sure you want to change the base?
Conversation
9fd14d6
to
ce10976
Compare
Depending on how semver-y people feel, this might be best targeted for PyMC v6. |
ce10976
to
e7e8a54
Compare
I think that will be a strong argument not to go on with these changes |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #6973 +/- ##
===========================================
- Coverage 88.65% 40.55% -48.10%
===========================================
Files 100 100
Lines 16896 16868 -28
===========================================
- Hits 14979 6841 -8138
- Misses 1917 10027 +8110
|
I'll put in a vote against this -- there are enough pymc scripts and libraries out there that don't follow library updates very closely, but would like to continue to benefit from better sampling routines by having liberal version specifications. Probably the right way would be to use some deprecations library that issued a warning for ~a year, and provided a copy/paste upgrade path. I'm not sure the benefits (tidier namespace? are there others?) outweigh the work and downsides. |
def alias_deprecation(func, alias: str): | ||
original = func.__name__ | ||
|
||
@functools.wraps(func) | ||
def wrapped(*args, **kwargs): | ||
raise FutureWarning( | ||
f"The function `{alias}` from PyMC was an alias for `{original}` from ArviZ. " | ||
"It was removed in PyMC 4.0. " | ||
f"Switch to `pymc.{original}` or `arviz.{original}`." | ||
) | ||
|
||
return wrapped | ||
|
||
|
||
# Aliases of ArviZ functions | ||
autocorrplot = alias_deprecation(az.plot_autocorr, alias="autocorrplot") | ||
forestplot = alias_deprecation(az.plot_forest, alias="forestplot") | ||
kdeplot = alias_deprecation(az.plot_kde, alias="kdeplot") | ||
energyplot = alias_deprecation(az.plot_energy, alias="energyplot") | ||
densityplot = alias_deprecation(az.plot_density, alias="densityplot") | ||
pairplot = alias_deprecation(az.plot_pair, alias="pairplot") | ||
traceplot = alias_deprecation(az.plot_trace, alias="traceplot") | ||
compareplot = alias_deprecation(az.plot_compare, alias="compareplot") |
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.
It looks to me like this has already been issuing deprecation warnings. Was this working and warning against using everything slated to be removed since v4?
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.
That was just for utilities whose names have changed. Those could be safely removed by now yes
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.
Ah, then I agree with @ColCarroll that we should definitely deprecate all the other stuff well in advance of removing it.
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.
Also why remove the deprecation warning? In case you want to clear up the namespace then you could refactor it into something like https://peps.python.org/pep-0562/
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 removed this deprecation warning because I removed the objects that were being deprecated as well
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.
These one specifically were deprecated since v4, seems safe no?
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.
Aren't there still lots of people who use PyMC3 because of the name recognition and all the SEO? Seems pretty low-effort to provide explicit instructions for them.
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 silly. This will not be the thing that people switching from v3 to v5 will find challenging
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.
Ya, I agree it's silly in the case of these ArviZ warnings, but you could do something similar to provide a transition period for the rest of the stuff. Proof-of-concept: ricardoV94#4
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 agree, I am going to try and do that!
I'm in favor. Cruft accumulates in any code base and it requires consistent effort to stem the tide, which this attempts. I don't think many people are using these functions and those can easily adjust their imports. In a version or two we'll all have forgotten about it, just like with aesara and pytensor. |
Closes #6761
Also
The following script prints 174 after and 272 before this PR:
The following entries were removed:
📚 Documentation preview 📚: https://pymc--6973.org.readthedocs.build/en/6973/