CLN/API: move plotting funcs to pandas.plotting #16005
Conversation
jorisvandenbossche
force-pushed
the
api-move-plotting
branch
from
8393e55
to
72ffe97
Compare
| return lines | ||
|
|
||
|
|
||
| def _maybe_resample(series, ax, kwargs): |
There was a problem hiding this comment.
side issue since this entire module is private, the functions themselves don't need to be (but can fix later)
| @@ -649,6 +649,7 @@ def pxd(name): | |||
| 'pandas.io.msgpack', | |||
| 'pandas._libs', | |||
| 'pandas.formats', | |||
| 'pandas.plotting', | |||
There was a problem hiding this comment.
did the test module change as well?
There was a problem hiding this comment.
the test module was already moved to pandas/tests/plotting before, so no layout changes there I think
| from pandas.tools.plotting import scatter_matrix, plot_params | ||
|
|
||
| # deprecate tools.plotting, plot_params and scatter_matrix on the top namespace | ||
| import pandas.tools.plotting |
There was a problem hiding this comment.
ideally this could actually be done in pandas.plotting.api, so main is not crowded like this
There was a problem hiding this comment.
I think this line is just to make sure pd.tools.plotting is tab-completable/directly usable (without doing from pandas.tools import plotting as a user) for back-compat, so not related to pandas.plotting
There was a problem hiding this comment.
yeah its probably fine. also need to move these in the test_api to the deprecated section. (also add on to the deprecated issue tracker)
There was a problem hiding this comment.
there are test for that in pandas/tests/plotting/test_deprecated.py (the top-evel deprecations of plot_params are in test_api.py as well)
added to #6581
Codecov Report
@@ Coverage Diff @@
## master #16005 +/- ##
==========================================
- Coverage 91.02% 90.99% -0.03%
==========================================
Files 145 154 +9
Lines 50391 50474 +83
==========================================
+ Hits 45870 45931 +61
- Misses 4521 4543 +22
Continue to review full report at Codecov.
|
jorisvandenbossche
force-pushed
the
api-move-plotting
branch
from
3d7663e
to
468313a
Compare
doc/source/whatsnew/v0.20.0.txt
Outdated
| @@ -21,6 +21,7 @@ Highlights include: | |||
| - Support for S3 handling now uses ``s3fs``, see :ref:`here <whatsnew_0200.api_breaking.s3>` | |||
| - Google BigQuery support now uses the ``pandas-gbq`` library, see :ref:`here <whatsnew_0200.api_breaking.gbq>` | |||
| - Switched the test framework to use `pytest <http://doc.pytest.org/en/latest>`__ (:issue:`13097`) | |||
| - The ``pandas.tools.plotting`` module has been deprecated, moved to ``pandas.plotting``. See :ref:`here <whatsnew_0200.api_breaking.plotting>` (:issue:`12548`) | |||
There was a problem hiding this comment.
I would put the issue number in the sub-section
doc/source/whatsnew/v0.20.0.txt
Outdated
| The ``pandas.tools.plotting`` module has been deprecated, in favor of the top level ``pandas.plotting`` module. All the public plotting functions are now available | ||
| from ``pandas.plotting``. | ||
|
|
||
| Further, the top-level ``pandas.scatter_matrix`` and ``pandas.plot_params`` are also deprecated. |
There was a problem hiding this comment.
are deprecated (also is redundant)
| from pandas.compat import lrange | ||
| import pandas.compat as compat | ||
| import pandas._libs.lib as lib | ||
| import pandas.core.common as com |
There was a problem hiding this comment.
side issue: these are pretty verbose imports, usually can just do from pandas import INdex, Series, date_range etc.
There was a problem hiding this comment.
indeed (but will leave that for other PR)
pandas/plotting/_converter.py
Outdated
| MUSEC_PER_DAY = 1e6 * SEC_PER_DAY | ||
|
|
||
|
|
||
| def _mpl_le_2_0_0(): |
There was a problem hiding this comment.
yep, probably left-over from rebase
pandas/tests/api/test_api.py
Outdated
| @@ -69,7 +69,7 @@ class TestPDApi(Base, tm.TestCase): | |||
| 'melt', 'notnull', 'offsets', | |||
| 'merge', 'merge_ordered', 'merge_asof', | |||
| 'period_range', | |||
| 'pivot', 'pivot_table', 'plot_params', 'qcut', | |||
| 'pivot', 'pivot_table', 'qcut', | |||
| TimeSeries_TimedeltaFormatter()) | ||
| else: | ||
| raise TypeError('index type not supported') | ||
|
|
There was a problem hiding this comment.
is this file staying for a reason?
There was a problem hiding this comment.
Not fully sure, I suppose the idea was back compat? (@sinhrks ?)
But this one only imports tsplot, I don't think we should regard that as public, so can just as well remove this file then.
|
@jorisvandenbossche I am fine with merging this before #15998 (or after). lmk. |
|
Fixed the comments (apart from import clean-up, can do later a PR) Would like to get this in before I have to rebase again (it was a rather painful rebase :-)) the tseries.plotting (also tseries.converter is kept as a shim) question, can maybe deal with that when moving/reorganizing the tseries module. |
merge as soon as you are ready.
sure (though I would deprecate those modules as you moved them). can do as a followup though. |
|
ready to merge? |
|
When it is green, yes |
|
thanks! |
|
and thanks @sinhrks for the original work! |
closes #12548, rebase of #13579