Skip to content
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

API: Deprecate renamae_axis and reindex_axis #17842

Merged
merged 9 commits into from Oct 11, 2017

Conversation

@TomAugspurger
Copy link
Contributor

commented Oct 10, 2017

Closes #17833

Some notes:

@TomAugspurger TomAugspurger added this to the 0.21.0 milestone Oct 10, 2017

@jsexauer jsexauer referenced this pull request Oct 10, 2017
0 of 89 tasks complete
@TomAugspurger

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2017

Hmm, looks like I'll have to give Panel.reindex a similar treatment as DataFrame.reindex in #17800

REF: Refactor axis style validator to generic
This sets us up to re-use it for Panel reindex
@jorisvandenbossche

This comment has been minimized.

Copy link
Member

commented Oct 10, 2017

Hmm, looks like I'll have to give Panel.reindex a similar treatment as DataFrame.reindex in #17800

I really wouldn't bother with that. Panel is deprecated, and IMO there is no need in making Panel's functions consistent.

@TomAugspurger

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2017

I really wouldn't bother with that. Panel is deprecated, and IMO there is no need in making Panel's functions consistent.

I wish I had that option :) It seems to be unavoidable though since some generic methods used reindex_axis on frame / panel. Changing those to reindex(foo, axis=axis) caused failures.

I have it working for Panel. Working on Panel4D now, hopefully not much longer.

@TomAugspurger

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2017

We're getting the refactoring of _validate_axis_style_args you and Jeff asked for though ;)

@jorisvandenbossche

This comment has been minimized.

Copy link
Member

commented Oct 10, 2017

Another option is to just make a _reindex_axis for now that has the actual implementation and is not deprecated, and use that where reindex_axis is used now internally, and we can get rid of it once Panel/Panel4D is gone.
Again, personally I wouldn't loose more time on it than necessary :-)

@jorisvandenbossche
Copy link
Member

left a comment

some doc comments

@@ -810,6 +810,8 @@ Deprecations
- ``.get_value`` and ``.set_value`` on ``Series``, ``DataFrame``, ``Panel``, ``SparseSeries``, and ``SparseDataFrame`` are deprecated in favor of using ``.iat[]`` or ``.at[]`` accessors (:issue:`15269`)
- Passing a non-existent column in ``.to_excel(..., columns=)`` is deprecated and will raise a ``KeyError`` in the future (:issue:`17295`)
- ``raise_on_error`` parameter to :func:`Series.where`, :func:`Series.mask`, :func:`DataFrame.where`, :func:`DataFrame.mask` is deprecated, in favor of ``errors=`` (:issue:`14968`)
- Using :meth:`DataFrame.rename_axis` and :meth:`Series.rename_axis` to alter index or column *labels* is now deprecated in favor of using ``.rename``. ``rename_axis`` may still be used to alter the name of the index or columns (:issue:`17833`).
- :meth:`~NDFrame.reindex_axis` has been deprecated in favor of :meth:`~NDFrame.reindex`. See :ref`here` <whatsnew_0210.enhancements.rename_reindex_axis> for more (:issue:`17833`).

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Oct 10, 2017

Member

'NDFrame' is not something that exists in the documentation. You can just use DataFrame (it will not be shown because of the ~, and the docstrings are the same for series/dataframe)

@@ -89,6 +89,7 @@ def _align_core(terms):
for axis, items in zip(range(ndim), axes):
ti = terms[i].value

# TODO: handle this for when reindex_axis is removed...
if hasattr(ti, 'reindex_axis'):

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Oct 10, 2017

Member

and using 'reindex' instead of 'reindex_axis' is not good enough?

the axis *labels* by passing a mapping or scalar. This behavior is
deprecated and will be removed in a future version. Use ``rename``
instead.
See Also
--------
pandas.NDFrame.rename

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Oct 10, 2017

Member

sidenote: this is useless (does not make a link). Unless you want to start with shared docstring/substitution, I would just explicitly add both for series/frame: pandas.Series.rename, pandas.DataFrame.rename

axes = self._validate_axis_style_args(
labels, 'labels', axes=[items, major_axis, minor_axis],
axis=axis, method_name='reindex')
if self.ndim >= 4:

This comment has been minimized.

Copy link
@TomAugspurger

TomAugspurger Oct 10, 2017

Author Contributor

I got a bit annoyed with PanelND :) I think this is acceptable until we remove it.

@@ -116,7 +116,9 @@ def test_pivot_table_dropna_categoricals(self):

result_false = df.pivot_table(index='B', columns='A', values='C',
dropna=False)
expected_columns = Series(['a', 'b', 'c', 'd'], name='A')
expected_columns = (

This comment has been minimized.

Copy link
@TomAugspurger

TomAugspurger Oct 10, 2017

Author Contributor

I wanted to flag this change in the tests. AFAICT, this is actually the desired behavior? Looking for issues about this now.

@jorisvandenbossche
Copy link
Member

left a comment

The support for Panel(nd) makes it much less readable .., but since you already did the hard work, it's ok for me :-)
Added some more comments.

axis = self._get_axis_name(axis)
if any(x is not None for x in axes):
msg = (
"Can't specify both 'axis' and {aliases}"

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Oct 10, 2017

Member

add ". " at the end of this string? (point + space)

@@ -4580,7 +4580,7 @@ def _get_sorted_data(self):

# this is sort of wasteful but...
sorted_axis = data.axes[self.axis].take(self.sort_idx)
sorted_data = data.reindex(sorted_axis, axis=self.axis)
sorted_data = data.reindex_axis(sorted_axis, axis=self.axis)

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Oct 10, 2017

Member

This won't cause a deprecation warning in our own code?

This comment has been minimized.

Copy link
@TomAugspurger

TomAugspurger Oct 10, 2017

Author Contributor

data here is a BlockManager (I incorrectly changed this in an earlier commit)


elif _all_not_none(arg, *axes):
msg = (
"Cannot specify all of '{arg_name}', {aliases}"

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Oct 10, 2017

Member

also missing ". "

"Can't specify both 'axis' and {aliases}"
"Specify either\n"
"\t.{method_name}({arg_name}, axis=axis), or\n"
"\t.{method_name}(index=index, columns=columns)" # TODO

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Oct 10, 2017

Member

does your 'todo' point to the fact that "index=index, columns=columns" is not generic for all NDFrame types ? (like Panels) If that is the case, I personally wouldn't care.

This comment has been minimized.

Copy link
@TomAugspurger

TomAugspurger Oct 11, 2017

Author Contributor

Yep, I'll happily ignore :)

expected_columns = Series(['a', 'b', 'c', 'd'], name='A')
expected_columns = (
Series(['a', 'b', 'c', 'd'], name='A').astype('category')
)

This comment has been minimized.

Copy link
@jorisvandenbossche

jorisvandenbossche Oct 10, 2017

Member

how is this change related?

This comment has been minimized.

Copy link
@TomAugspurger

TomAugspurger Oct 11, 2017

Author Contributor

Not sure :) The fact that it's only not a CategoricalIndex when in the columns and dropna=True seems like a bug on master.

# This is on master
In [7]: df.pivot_table(index="B", columns="A", values="C", dropna=False).columns
Out[7]: Index(['a', 'b', 'c', 'd'], dtype='object', name='A')

In [8]: df.pivot_table(index="A", columns="B", values="C", dropna=False).index
Out[8]: CategoricalIndex(['a', 'b', 'c', 'd'], categories=['a', 'b', 'c', 'd'], ordered=False, name='A', dtype='category')

I've added a whatsnew.

TomAugspurger added some commits Oct 10, 2017

@codecov

This comment has been minimized.

Copy link

commented Oct 11, 2017

Codecov Report

Merging #17842 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17842      +/-   ##
==========================================
- Coverage   91.22%   91.21%   -0.02%     
==========================================
  Files         163      163              
  Lines       50014    50033      +19     
==========================================
+ Hits        45627    45637      +10     
- Misses       4387     4396       +9
Flag Coverage Δ
#multiple 89.02% <98.38%> (ø) ⬆️
#single 40.26% <62.9%> (-0.04%) ⬇️
Impacted Files Coverage Δ
pandas/core/panel4d.py 100% <100%> (ø) ⬆️
pandas/core/panel.py 96.96% <100%> (+0.01%) ⬆️
pandas/core/series.py 94.99% <100%> (ø) ⬆️
pandas/plotting/_core.py 82.45% <100%> (ø) ⬆️
pandas/core/generic.py 92.2% <100%> (+0.11%) ⬆️
pandas/core/sparse/scipy_sparse.py 97.05% <100%> (ø) ⬆️
pandas/core/computation/align.py 97.89% <100%> (-0.05%) ⬇️
pandas/core/frame.py 97.75% <100%> (-0.12%) ⬇️
pandas/io/pytables.py 92.79% <100%> (ø) ⬆️
pandas/core/internals.py 94.38% <100%> (ø) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 727ea20...c780537. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented Oct 11, 2017

Codecov Report

Merging #17842 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17842      +/-   ##
==========================================
- Coverage   91.22%   91.21%   -0.02%     
==========================================
  Files         163      163              
  Lines       50014    50033      +19     
==========================================
+ Hits        45627    45637      +10     
- Misses       4387     4396       +9
Flag Coverage Δ
#multiple 89.02% <98.38%> (ø) ⬆️
#single 40.26% <62.9%> (-0.04%) ⬇️
Impacted Files Coverage Δ
pandas/core/frame.py 97.75% <100%> (-0.12%) ⬇️
pandas/io/pytables.py 92.79% <100%> (ø) ⬆️
pandas/core/indexing.py 93% <100%> (ø) ⬆️
pandas/core/internals.py 94.38% <100%> (ø) ⬆️
pandas/core/groupby.py 91.99% <100%> (ø) ⬆️
pandas/core/reshape/pivot.py 96.38% <100%> (ø) ⬆️
pandas/core/panel4d.py 100% <100%> (ø) ⬆️
pandas/core/series.py 94.99% <100%> (ø) ⬆️
pandas/core/panel.py 96.96% <100%> (+0.01%) ⬆️
pandas/core/computation/align.py 97.89% <100%> (-0.05%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 727ea20...c780537. Read the comment docs.

@TomAugspurger TomAugspurger merged commit 3544394 into pandas-dev:master Oct 11, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@TomAugspurger TomAugspurger deleted the TomAugspurger:depr-rename_axis branch Oct 11, 2017

@jreback

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2017

https://travis-ci.org/pandas-dev/pandas/jobs/287943551

@TomAugspurger some deprecation warnings still in test_panel I think. can you fix.

ghost pushed a commit to reef-technologies/pandas that referenced this pull request Oct 16, 2017

API: Deprecate renamae_axis and reindex_axis (pandas-dev#17842)
* API: Deprecate renamae_axis and reindex_axis

Closes pandas-dev#17833

* REF: Refactor axis style validator to generic

This sets us up to re-use it for Panel reindex

* fixup! API: Deprecate renamae_axis and reindex_axis

* fixup! API: Deprecate renamae_axis and reindex_axis

* fixup! API: Deprecate renamae_axis and reindex_axis

* fixup! API: Deprecate renamae_axis and reindex_axis

* Ugh

* fixup! API: Deprecate renamae_axis and reindex_axis

* Fixup

alanbato added a commit to alanbato/pandas that referenced this pull request Nov 10, 2017

API: Deprecate renamae_axis and reindex_axis (pandas-dev#17842)
* API: Deprecate renamae_axis and reindex_axis

Closes pandas-dev#17833

* REF: Refactor axis style validator to generic

This sets us up to re-use it for Panel reindex

* fixup! API: Deprecate renamae_axis and reindex_axis

* fixup! API: Deprecate renamae_axis and reindex_axis

* fixup! API: Deprecate renamae_axis and reindex_axis

* fixup! API: Deprecate renamae_axis and reindex_axis

* Ugh

* fixup! API: Deprecate renamae_axis and reindex_axis

* Fixup

No-Stream added a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017

API: Deprecate renamae_axis and reindex_axis (pandas-dev#17842)
* API: Deprecate renamae_axis and reindex_axis

Closes pandas-dev#17833

* REF: Refactor axis style validator to generic

This sets us up to re-use it for Panel reindex

* fixup! API: Deprecate renamae_axis and reindex_axis

* fixup! API: Deprecate renamae_axis and reindex_axis

* fixup! API: Deprecate renamae_axis and reindex_axis

* fixup! API: Deprecate renamae_axis and reindex_axis

* Ugh

* fixup! API: Deprecate renamae_axis and reindex_axis

* Fixup
@jreback jreback referenced this pull request Jun 29, 2019
141 of 141 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.