API: Restore implicit converter registration #18307
Conversation
|
I added the "special" section in the whatsnew for this and the changes we're making to NaN/empty sum. |
doc/source/whatsnew/v0.21.1.txt
Outdated
|
|
||
| This caused problems for some users, so we're temporarily reverting that change; | ||
| pandas will again register the converters on import. Using the converters | ||
| without explicitly registering the formatters will cause a ``FutureWarning``: |
There was a problem hiding this comment.
I would not use 'the formatters' as it actually points to the same as 'converters' but with another word, just making it confusing. I would either just leave it out or use 'them' to avoid repeating converters
There was a problem hiding this comment.
Meant to use formatters converters everywhere. Fixing.
doc/source/whatsnew/v0.21.1.txt
Outdated
| >>> from pandas.tseries import converter | ||
| >>> converter.register() | ||
|
|
||
| As the error message says, you'll need to register the converts if you intend to |
There was a problem hiding this comment.
converts -> converters
doc/source/whatsnew/v0.21.1.txt
Outdated
|
|
||
| As the error message says, you'll need to register the converts if you intend to | ||
| use them with matplotlib plotting functions. Pandas plotting functions, such as | ||
| ``Series.plot``, will register them for you. (:issue:`18301`) |
There was a problem hiding this comment.
maybe a bit more explicit like: "When using pandas plotting functions, such as Series.plot, the converters will be registered for you and you do not need to do this explicitly"
| @@ -2352,7 +2354,6 @@ def boxplot_frame_groupby(grouped, subplots=True, column=None, fontsize=None, | |||
| >>> grouped = df.unstack(level='lvl1').groupby(level=0, axis=1) | |||
| >>> boxplot_frame_groupby(grouped, subplots=False) | |||
| """ | |||
| _setup() | |||
There was a problem hiding this comment.
this one doesn't need to be replaced with _converter.WARN = False ?
pandas/tests/plotting/test_warns.py
Outdated
| plt = pytest.importorskip('matplotlib.pyplot') | ||
|
|
||
|
|
||
| def test_warns(): |
There was a problem hiding this comment.
Is this duplicate with the other test file?
There was a problem hiding this comment.
Yes, forgot to remove it from my first commit.
|
What are people's thoughts on moving the |
Codecov Report
@@ Coverage Diff @@
## master #18307 +/- ##
==========================================
- Coverage 91.39% 91.38% -0.02%
==========================================
Files 164 164
Lines 49854 49869 +15
==========================================
+ Hits 45566 45571 +5
- Misses 4288 4298 +10
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #18307 +/- ##
==========================================
+ Coverage 91.35% 91.43% +0.08%
==========================================
Files 164 157 -7
Lines 49802 51424 +1622
==========================================
+ Hits 45496 47019 +1523
- Misses 4306 4405 +99
Continue to review full report at Codecov.
|
|
Should we also add some language on how to avoid this warning without registering the converters by default? So if you in the future don't want to manually register them, but rely on the matplotlib ones (eg when working with datetime.datetime) ? |
|
I guess a pandas option? And we check that before ever calling register? |
|
Though, future-proofing a bit, should that be spread across several options, one per type? We currently register for In the ideal future, we would I think just do Timestamp and Period, and matplotlib would have datetime64, I don't know who wants to own python dates / times / datetimes. cc @tacaswell if any of this interests you. |
|
Added a rough implementation of the option in 16c33f1. In [1]: import pandas as pd
In [2]: import matplotlib.pyplot as plt
In [3]: fig, ax = plt.subplots()
In [4]: s = pd.Series(range(12), index=pd.date_range('2017', periods=12))
In [5]: pd.options.plotting.matplotlib.register_formatters = False
In [6]: ax.plot(s)
Out[6]: [<matplotlib.lines.Line2D at 0x1131ce588>] |
if its not too much trouble. I would like to at least deprecate |
|
Thanks ❤️ . |
doc/source/whatsnew/v0.21.1.txt
Outdated
|
|
||
| As the error message says, you'll need to register the converters if you intend | ||
| to use them with matplotlib plotting functions. Pandas plotting functions, such | ||
| as ``Series.plot``, will register them for you calling ``converter.register()`` |
There was a problem hiding this comment.
"for you calling" -> "for you, and calling"
doc/source/whatsnew/v0.21.1.txt
Outdated
| as ``Series.plot``, will register them for you calling ``converter.register()`` | ||
| first is not necessary. | ||
|
|
||
| Finally, control the formatters, we've added a new option: |
|
Some other things related to the option:
|
| converter.register() | ||
| with ctx: | ||
| with tm.assert_produces_warning(None) as w: | ||
| ax.plot(s.index, s.values) |
There was a problem hiding this comment.
If you did set the option to False, it should not warn?
There was a problem hiding this comment.
Sorry, forget, that is what is being tested :-) (and they have been registered manually afterwards)
Should we test what happens if you do not register manually after you set the option?
There was a problem hiding this comment.
You mean set the option to True or False? I think my latest commit does that.
After setting it to False, I think we should not warn even if they don't register manually (which is what my tests checks hopefully)
Switched to formatters everywhere.
Correct. To be more specific, they're always registered when pandas is imported. The option toggles whether or not they're in the In [8]: pd.options.plotting.matplotlib.register_formatters = False
In [9]: s.plot()Will fall back to whatever matplotlib does with datetimes / periods (potentially an exception, e.g. for periods).
Hmm, not sure about that. My registry is empty upon import: In [1]: import matplotlib.units
In [2]: matplotlib.units.registry
Out[2]: {}Though maybe I'm misunderstanding. The intent is return the registry to the state before we mucked with it.
Yeah, thoughts on |
|
The unit registry gets filled on import. If you import |
|
Ah thanks. In [1]: import matplotlib.units
In [2]: import matplotlib.pyplot as plt
In [3]: matplotlib.units.registry
Out[3]:
{str: <matplotlib.category.StrCategoryConverter at 0x10beb7f60>,
numpy.str_: <matplotlib.category.StrCategoryConverter at 0x10beb7f28>,
bytes: <matplotlib.category.StrCategoryConverter at 0x10beb7ef0>,
numpy.bytes_: <matplotlib.category.StrCategoryConverter at 0x10beb7f98>,
datetime.date: <matplotlib.dates.DateConverter at 0x10bf2c208>,
datetime.datetime: <matplotlib.dates.DateConverter at 0x10bf2c240>}
In [4]: import pandas as pd
In [5]: matplotlib.units.registry
Out[5]:
{str: <matplotlib.category.StrCategoryConverter at 0x10beb7f60>,
numpy.str_: <matplotlib.category.StrCategoryConverter at 0x10beb7f28>,
bytes: <matplotlib.category.StrCategoryConverter at 0x10beb7ef0>,
numpy.bytes_: <matplotlib.category.StrCategoryConverter at 0x10beb7f98>,
datetime.date: <pandas.plotting._converter.DatetimeConverter at 0x10bf2c240>,
datetime.datetime: <pandas.plotting._converter.DatetimeConverter at 0x10eaa3828>,
pandas._libs.tslib.Timestamp: <pandas.plotting._converter.DatetimeConverter at 0x10eaa3780>,
pandas._libs.period.Period: <pandas.plotting._converter.PeriodConverter at 0x10eaa37b8>,
datetime.time: <pandas.plotting._converter.TimeConverter at 0x10bf2c208>,
numpy.datetime64: <pandas.plotting._converter.DatetimeConverter at 0x10eaac0f0>}
In [6]: pd.options.plotting.matplotlib.register_formatters = False
In [7]: matplotlib.units.registry
Out[7]:
{str: <matplotlib.category.StrCategoryConverter at 0x10beb7f60>,
numpy.str_: <matplotlib.category.StrCategoryConverter at 0x10beb7f28>,
bytes: <matplotlib.category.StrCategoryConverter at 0x10beb7ef0>,
numpy.bytes_: <matplotlib.category.StrCategoryConverter at 0x10beb7f98>}So not great... We could of course maintain our own global state when we overwrite them... Though we'll have to make sure |
It has always been conceptually |
|
Another issue for down the road matplotlib/matplotlib#9779 doesn't handle
Though hopefully this can be handled on the matplotlib side by getting the import pandas as pd
import matplotlib.pyplot as plt
from matplotlib import units
x = pd.date_range("2017", periods=120, freq='H')
y = range(120)
z = x.tz_localize("US/Central")
%matplotlib inline
plt.plot(x, y); |
|
Yes, I think that should be easily solvable in matplotlib. I was still planning to run all the different reported examples in pandas issues related to this with matplotlib master to see if it would solve those cases. |
|
For MPL 2.2+, do we want to overwrite their converters with our own? I think no, but I'll have to check the differences. |
|
To summarize, I think in the long term we want those options:
Before 0.21.0, 'True' was the default, and in the long term we want 'auto' to become the default. But as a transition period, I think we want a hybrid of True/'auto', which would be 'auto' + deprecation warning for implicitly used converters (i.e. register our converters but with warn=True). And indeed there I would only do this warning for those types that are in the meantime not supported by matplotlib itself, and thus not overwrite the ones matplotlib in the meantime added. But, my proposal it to now do a complete revert for 0.21.1, and start the transition period only in 0.22.0 (or the first release when matplotlib 2.2 is out). |
|
Sorry for the delay on this. I'm finally done with traveling and conferences for a bit. This PR now
So, after MPL 2.2, we should just need to change the default in |
- actually switch the default to not warn - We do overwrite matplotlib's formatters
doc/source/whatsnew/v0.21.1.txt
Outdated
| for regular ``matplotlib.pyplot`` plotting methods, so we're temporarily | ||
| reverting that change; pandas will again register the converters on import. | ||
| Pandas plotting functions, such as ``Series.plot``, will continue to register | ||
| them for you. |
There was a problem hiding this comment.
This sentence now feels a bit out of place, as the previous one already said that they are registered again on import.
doc/source/whatsnew/v0.21.1.txt
Outdated
| .. code-block:: python | ||
|
|
||
| >>> from pandas.plotting import register_matplotlib_converters | ||
| >>> register_matplotlib_converters() |
There was a problem hiding this comment.
I am not sure if I would show that here. Because people who glance over it might think they need to add that to their code, which is not the case.
pandas/tseries/converter.py
Outdated
| def register(): | ||
| from pandas.plotting._converter import register as register_ | ||
| msg = ("'pandas.tseries.converter.register' has been moved and renamed to " | ||
| "'pandas.plotting.register_converters'. ") |
There was a problem hiding this comment.
register_converters -> register_matplotlib_converters()
|
@tacaswell can you have a look |
I thought about that, but there are test-order issues with that. Do you know a clean way of running a single test in it's own process? I could use subprocess, but that seems fragile. |
Not directly, googling said pytest does not have a way to specify this, but found this: https://stackoverflow.com/questions/45462374/mark-test-to-be-run-in-independent-process (not sure how robust it is) |
|
Added ee7b457 with a test using subprocess. I worry that's fragile to PATH issues, but let's see. |
|
@TomAugspurger Let's merge this? |
* API: Restore implicit converter registration * Remove matplotlib from blacklist * fixup! Remove matplotlib from blacklist * Add option for toggling formatters * Remove move * Handle no matplotlib * Cleanup * Test no register * Restore original state * Added deregister * Doc, naming * Naming * Added deprecation * PEP8 * Fix typos * Rename it all * Missed one * Check version * No warnings by default * Update release notes * Test fixup - actually switch the default to not warn - We do overwrite matplotlib's formatters * Doc update * Fix deprecation message * Test added by default
|
I opened #18720 as a follow-up issue about the 'auto' behaviour (only modifying matplotlibs units registry in pandas' plotting context) |
* API: Restore implicit converter registration * Remove matplotlib from blacklist * fixup! Remove matplotlib from blacklist * Add option for toggling formatters * Remove move * Handle no matplotlib * Cleanup * Test no register * Restore original state * Added deregister * Doc, naming * Naming * Added deprecation * PEP8 * Fix typos * Rename it all * Missed one * Check version * No warnings by default * Update release notes * Test fixup - actually switch the default to not warn - We do overwrite matplotlib's formatters * Doc update * Fix deprecation message * Test added by default
Closes #18301
TODO: