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

ENH: Allow rename_axis to specify index and columns arguments #20046

Merged
merged 19 commits into from
Oct 29, 2018

Conversation

Dr-Irv
Copy link
Contributor

@Dr-Irv Dr-Irv commented Mar 7, 2018

"""Alter the name of the index or columns.
def rename_axis(self, mapper=None, **kwargs):
"""Alter the name of the index or name of index backing the
columns.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep this to be one line.

@@ -749,6 +749,20 @@ def swaplevel(self, i=-2, j=-1, axis=0):
# ----------------------------------------------------------------------
# Rename

# renamer function if passed a dict
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you just moved the function, but let's take this opportunity to write a brief docstring.

@codecov
Copy link

codecov bot commented Mar 9, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@0d6cb3c). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #20046   +/-   ##
=========================================
  Coverage          ?   92.18%           
=========================================
  Files             ?      161           
  Lines             ?    51184           
  Branches          ?        0           
=========================================
  Hits              ?    47185           
  Misses            ?     3999           
  Partials          ?        0
Flag Coverage Δ
#multiple 90.62% <100%> (?)
#single 42.23% <50%> (?)
Impacted Files Coverage Δ
pandas/core/generic.py 96.81% <100%> (ø)
pandas/core/common.py 98.41% <100%> (ø)

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 0d6cb3c...1263a47. Read the comment docs.

@jreback
Copy link
Contributor

jreback commented Mar 9, 2018

this implementation needs to follow the coding standards that are present in .rename, pls use this as a guide.

you have text saying that this has 2 calling conventions. This alone is making me -1 on this whole idea.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Mar 9, 2018

@jreback I'm not sure which coding standards that you are referring to in .rename. Can you be more specific? Note that for .rename_axis, the signature has to be different in order to preserve the deprecated behavior.

Secondly, with respect to the 2 calling conventions, that is true for .rename as well. I just copied that documentation pattern from there. Look at the comments from @jorisvandenbossche in #19978 where he suggested adding the index and columns keyword to rename_axis, which, IMHO, is a good idea.

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Mar 30, 2018

@jreback See above. Awaiting your feedback or maybe @jorisvandenbossche

@jreback
Copy link
Contributor

jreback commented Jul 8, 2018

@Dr-Irv if you can rebase can have a look

Copy link
Member

@gfyoung gfyoung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jreback
Copy link
Contributor

jreback commented Sep 25, 2018

@Dr-Irv can you rebase

@pep8speaks
Copy link

Hello @Dr-Irv! Thanks for updating the PR.

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Sep 26, 2018

@jreback Merged with latest master. Current failure in travis was due to a network error.

@jreback jreback added this to the 0.24.0 milestone Sep 26, 2018
@jreback
Copy link
Contributor

jreback commented Sep 26, 2018

@TomAugspurger @jorisvandenbossche if you'd have a look.

@@ -181,7 +181,8 @@ Other Enhancements
- :func:`read_html` copies cell data across ``colspan`` and ``rowspan``, and it treats all-``th`` table rows as headers if ``header`` kwarg is not given and there is no ``thead`` (:issue:`17054`)
- :meth:`Series.nlargest`, :meth:`Series.nsmallest`, :meth:`DataFrame.nlargest`, and :meth:`DataFrame.nsmallest` now accept the value ``"all"`` for the ``keep`` argument. This keeps all ties for the nth largest/smallest value (:issue:`16818`)
- :class:`IntervalIndex` has gained the :meth:`~IntervalIndex.set_closed` method to change the existing ``closed`` value (:issue:`21670`)
- :func:`~DataFrame.to_csv`, :func:`~Series.to_csv`, :func:`~DataFrame.to_json`, and :func:`~Series.to_json` now support ``compression='infer'`` to infer compression based on filename extension (:issue:`15008`).
- :func:`~DataFrame.to_csv` and :func:`~DataFrame.to_json` now support ``compression='infer'`` to infer compression based on filename (:issue:`15008`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually can you move this note to a sub-section and show the previous and new behavior. I now this is not a super common method, but its a fairly meaty change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jreback latest commit puts rename_axis() change in a subsection with an example. Also, I had messed up the merge of whatsnew in previous commit, so that is now fixed.

@jreback
Copy link
Contributor

jreback commented Oct 18, 2018

can you rebase

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Oct 19, 2018

@jreback merged in latest master. All green

@@ -1464,10 +1464,25 @@ for altering the ``Series.name`` attribute.

s.rename("scalar-name")

The Panel class has a related :meth:`~Panel.rename` class which can rename
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not needed, we are not advertising Panel any longer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I deleted that, but Panel appears a lot in that document!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i know, but no need to add at this point

def _get_rename_function(self, mapper):
"""
Returns a function that will map names/labels, dependent if mapper
is a dict, Series or just a function.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't need to be here (in the class). is it used in more than 1 place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is used in both rename() and rename_axis()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, but it doesn't need self right? can you move to pandas/core/common.py i think is ok for this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have moved it and removed leading underscore


Use either ``mapper`` and ``axis`` to
specify the axis to target with ``mapper``, or ``index``
and/or ``columns``.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a versionchanged

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Oct 24, 2018

@jreback Changes made, and all green.

Copy link
Contributor

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't looked at the tests yet, but a few comments first.

"""
Alter the name of the index or columns.
Alter the name of the index or name of index backing the columns.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this change, is it a typo?

Oh, I think I saying "alter the name of the Index object that is the columns"?

If so, I think that index needs to be capitalized. But I think the older version was fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will use "alter the name of the Index object that is the columns."

* ``(mapper, axis={'index', 'columns'}, ...)``

The first calling convention will only modify the names of
the index and/or the names of the index backing the columns.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same: index -> Index, or just remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to "names of the Index object that is the columns."

pandas/core/generic.py Show resolved Hide resolved
Returns a function that will map names/labels, dependent if mapper
is a dict, Series or just a function.
"""
if isinstance(mapper, (dict, ABCSeries)):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using compat.Mapping instead of dict.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

pandas/core/common.py Show resolved Hide resolved
@TomAugspurger
Copy link
Contributor

Could also maybe use a short docnote in advanced.rst.

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Oct 25, 2018

I've added a section on renaming to advanced.rst.

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Oct 25, 2018

@TomAugspurger I did the changes. See remark at #20046 (comment) . I didn't make a change there.

There was a failure in the CI due to a timeout on travis. Rather than resubmit, I figured you could review what's here and see if any other changes are needed.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. small comments. nice docs!


.. ipython:: python

df = pd.DataFrame({'x': [1,2,3,4,5,6], 'y': [10,20,30,40,50,60]},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you make this pep-y, iow spaces after commas (you can also line up the x and y columns on separate lines)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

doc/source/whatsnew/v0.24.0.txt Show resolved Hide resolved
@@ -454,3 +454,21 @@ def _pipe(obj, func, *args, **kwargs):
return func(*args, **kwargs)
else:
return func(obj, *args, **kwargs)


def get_rename_function(mapper):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you make this private (e.g. _get_rename_function)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

mi = MultiIndex.from_product([['a', 'b', 'c'], [1, 2]],
names=['ll', 'nn'])
df = DataFrame({'x': [i for i in range(len(mi))],
'y': [i * 10 for i in range(len(mi))]},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you sprinkle some comments to delineate the cases you are testing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Oct 26, 2018

@jreback @TomAugspurger all green

# and not a list or scalar, then call rename
msg = ("Using 'rename_axis' to alter labels is deprecated. "
"Use '.rename' instead")
warnings.warn(msg, FutureWarning, stacklevel=2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if you increase stack level by 1, you can revert your changes to the tests below. The stack level increased by 1 since you're using the decorator to fix the function signature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tip. It worked.

@@ -522,22 +522,72 @@ def test_rename_axis_inplace(self, float_frame):
def test_rename_axis_warns(self):
# https://github.com/pandas-dev/pandas/issues/17833
df = DataFrame({"A": [1, 2], "B": [1, 2]})
with tm.assert_produces_warning(FutureWarning) as w:
with tm.assert_produces_warning(FutureWarning,
check_stacklevel=False) as w:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So these check_stacklevel changes can hopefully all be reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Oct 27, 2018

@jreback @TomAugspurger Almost all Green, but the failure was due to something on Azure "before install". Let me know if there are more changes, and if there is some way to get that job to run again.

@jreback
Copy link
Contributor

jreback commented Oct 28, 2018

@Dr-Irv ok lgtm. can you rebase, lots of isort things going on. ping on green.

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Oct 29, 2018

@jreback merged with latest and all green

@TomAugspurger TomAugspurger merged commit a2e5994 into pandas-dev:master Oct 29, 2018
@TomAugspurger
Copy link
Contributor

Thanks!

thoo added a commit to thoo/pandas that referenced this pull request Oct 30, 2018
…y_tests

* repo_org/master: (52 commits)
  ENH: Allow rename_axis to specify index and columns arguments  (pandas-dev#20046)
  STY: proposed isort settings [ci skip] [skip ci] [ciskip] [skipci] (pandas-dev#23366)
  MAINT: Remove extraneous test.parquet file
  CLN: Follow-up comments to pandas-devgh-23392 (pandas-dev#23401)
  BUG GH23282 calling min on series of NaT returns NaT (pandas-dev#23289)
  unpin openpyxl (pandas-dev#23361)
  REF: collect ops dispatch functions in one place, try to de-duplicate SparseDataFrame methods (pandas-dev#23060)
  CLN: Remove pandas.tools module (pandas-dev#23376)
  CLN: Remove some dtype methods from API (pandas-dev#23390)
  CLN: Cleanup toplevel namespace shims (pandas-dev#23386)
  DOC: fixup whatsnew note for GH21394 (pandas-dev#23355)
  Fix import format at pandas/tests/extension directory (pandas-dev#23365)
  DOC: Remove Series.sortlevel from api.rst (pandas-dev#23395)
  API: Disallow dtypes w/o frequency when casting (pandas-dev#23392)
  BUG/TST/REF: Datetimelike Arithmetic Methods (pandas-dev#23215)
  STYLE: lint
  add np.nan* funcs to cython_table (pandas-dev#22109)
  Run Isort on tests/util single PR (pandas-dev#23347)
  BUG: Fix date_range overflow (pandas-dev#23345)
  Run Isort on tests/arrays single PR (pandas-dev#23346)
  ...
@Dr-Irv Dr-Irv deleted the issue19978 branch November 2, 2018 13:24
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API: Allow dictionary argument in rename_axis to change some names of MultiIndex
5 participants