PLOT: Split matplotlib specific code from pandas plotting #26414
Conversation
datapythonista
added
Refactor
|
Thanks, will take a look later. In case it's useful now, I started TomAugspurger@b07aba2, which adds a config option for plotting, but never got to submitting a PR. |
|
Totally on-board with the concept. What other backends do you expect (either implemented here or downstream)? My understanding is many of the other graphics libraries wrap matplotlib themselves. |
|
I know of two libraries that have reimplemented the pd.DataFrame.plot API
|
|
I think Phillip started hvplot with the discussions of this issue, but as we didn't finish this work, it ended monkeypatching pandas. The goal here is that projects like the ones Tom mentioned, and others like https://github.com/PatrikHlobil/Pandas-Bokeh can be implemented simpler and in a standard way, so we make the life of devs and users easier. Personally, I'd at some point split the matplotlib backend from pandas and have it as a third party package. But that's for the long term, and unrelated to this PR. |
|
For what I could see in the code, the API that pandas plotting backends need to provide are:
The implementation in this PR moves all the Any comment appreciated. |
|
This is great - thanks for working on it! |
Codecov Report
@@ Coverage Diff @@
## master #26414 +/- ##
===========================================
- Coverage 91.69% 41.74% -49.95%
===========================================
Files 174 178 +4
Lines 50739 50739
===========================================
- Hits 46524 21181 -25343
- Misses 4215 29558 +25343
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #26414 +/- ##
=========================================
- Coverage 91.88% 91.79% -0.1%
=========================================
Files 174 179 +5
Lines 50699 50763 +64
=========================================
+ Hits 46587 46600 +13
- Misses 4112 4163 +51
Continue to review full report at Codecov.
|
| @@ -0,0 +1,8 @@ | |||
| from .boxplot import BoxPlot, boxplot_frame, boxplot_frame_groupby | |||
There was a problem hiding this comment.
Rename this module to pandas/plotting/_matplotlib
pandas/plotting/matplotlib/main.py
Outdated
| @@ -0,0 +1,1374 @@ | |||
| import re | |||
There was a problem hiding this comment.
Maybe "core" instead of "main"? I typically associate main with __main__ / running from the command line.
|
I am too 💤 right now to have much sensible comments on the code. If you are interested in pulling this whole thing out and putting it in If there is interest in more aggressive re-thinking of the API DistrictDataLabs/yellowbrick#826 , matplotlib/matplotlib#14058, and https://paper.dropbox.com/doc/Matplotlib-4.0--AdQMb0ugRPWeqq3U4dEAHuSDAQ-WTYwd0NQaSHTjtLUZwkNx may be of interest. |
…lready moved) into the matplotlib specific and the pandas plotting stuff
|
Hello @datapythonista! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-06-08 22:28:02 UTC |
|
100% agree, there is no functional change, and no public API change in this PR. Just moving all the code specific to matplotlib from Ideally would like to avoid touching any tests to guarantee that no unintentional functional change happens. In practice I need to update the import paths of some tests, since they import private functions that have been moved. Still work in progress, the change is much bigger than what I originally thought, but will provide more details on what the PR involves when it's ready to be merged. |
|
thanks @datapythonista yeah also happy to do this in pieces if its easier. |
…blic plotting objects)
|
@datapythonista are you able to reproduce the CI failures like https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=12231 locally? If not, I can try taking a look. |
|
No, I'm taking a look, but very confused on what's going on, even the mypy error. The CI was green before merging master, which confuses me even more. If you can take a look too, that would be great, let me know if you find something. |
|
@datapythonista haven't had the chance to look too deep but interesting that |
|
thanks @WillAyd, I also saw that, and removing the Appender does fix the problem, but I think there is a root error, Appender is being used eveywhere... :\ |
|
Yea not sure why it's appearing now. Probably just need to annotate Appender for proper inference. I don't think it's critical for this PR so if that's the only hangup feel free to add |
|
I tried annotating |
|
No problem. As mentioned I think OK to mark as type: ignore for now so it doesn't hold this up. I've opened #26629 to address separately |
|
No idea what's going on here. The 3 errors seem unrelated. One of them is a failure on the qt backend of matplotlib because can't find the environment variable The other error is caused by a plot losing the tick information in a plot. None of them reproduce locally with the dependencies of the build (one is the mac build, may be someone with a mac can try to reproduce). The mypy error seems to be nonsense too. |
|
I tried on a Mac, and wasn't able to reproduce the error (will try in an exact replica of that env later). I will note that the warning shows up under pytest under your branch, whereas it didn't before. I'm not sure why that would be. |
|
I fixed the errors, not sure why they were caused, but importing matplotlib to see if it was installed before registering the converters was causing the problems (including the mypy one which is weird). CI is green, and it's generating the warning reported by @TomAugspurger like in master. Should be ready to be merged. |
|
merge master one more time, ping on green. |
|
|
||
| def _make_plot(self): | ||
| if self.subplots: | ||
| from pandas.core.series import Series |
There was a problem hiding this comment.
later on I suppose could move all imports to the top as this is now a delayed import generally
|
@datapythonista after merge if you can create any needed followup issues, and issues for splitting tests and moving imports |
|
@jreback this is green. Will open PRs for the immediate follow up things, and create issues for the rest once merged. |
|
thanks @datapythonista very nice! |
git diff upstream/master -u -- "*.py" | flake8 --diffIn order to be able to change pandas plotting backend, I think we need 3 main things:
pandas.set_option('plotting.backend', 'pandas.matplotlib'))This PR is splitting most of the matplotlib backend code into a new
_matplotlib.pyfile. Some matplotlib code still lives inplotting/_core.py, but would be nice to have early feedback on whether everybody is happy with this approach before moving forward. CC: @jreback @TomAugspurger