ENH: provide "inplace" argument to set_axis() #16994

toobaz · 2017-07-17T15:34:19Z

closes API: set_axis does not have inplace=False #14636
tests added / passed
passes git diff master -u -- "*.py" | flake8 --diff
whatsnew entry

This completes what was attempted in #14656 with tests and docstring.

In principle, set_axis could be mentioned somewhere in the docs (set_index is)... but I'm dubious because the API is horrible and inconsistent, in particular with Series, where it obliges the user to do s.set_axis(0, [...]). In other words, while I tended to agree with the suggestion that once .set_axis supports inplace=False, we wouldn't need any .relabel(), now that I see how it looks, I changed my mind (this is not the best place to discuss this, just explaining why I'm against "publicizing" set_axis too much).

Notice that in order to alleviate the problem, I changed the signature: axis and labels are now named parameters, so that one can do s.set_axis(labels=[...]). I admit the utility of this is debatable.

gfyoung · 2017-07-17T16:15:36Z

doc/source/whatsnew/v0.21.0.txt

 - :func:`Series.to_dict` and :func:`DataFrame.to_dict` now support an ``into`` keyword which allows you to specify the ``collections.Mapping`` subclass that you would like returned.  The default is ``dict``, which is backwards compatible. (:issue:`16122`)
 - :func:`RangeIndex.append` now returns a ``RangeIndex`` object when possible (:issue:`16212`)
 - :func:`Series.rename_axis` and :func:`DataFrame.rename_axis` with ``inplace=True`` now return ``None`` while renaming the axis inplace. (:issue:`15704`)
+- :func:`Series.set_axis` now supports the ``inplace=`` parameter. (:issue:`14656`)


Remove the "=" after "inplace"

gfyoung · 2017-07-17T16:17:18Z

pandas/core/generic.py

+        labels: list-like or Index
+            The values for the new index
+        inplace : boolean, default False
+            Whether to return a new %(klass)s. If True then value of copy is


But there is no copy parameter, so you can remove that second sentence.

"%(klass)s" --> "%(klass)s instance"

gfyoung · 2017-07-17T16:18:38Z

pandas/core/generic.py

+            Whether to return a new %(klass)s. If True then value of copy is
+            ignored.
+
+            WARNING: inplace=None currently falls back to to True, but


We actually would set inplace=False as the default explicitly, but this is fine.

Commentary for future reference

gfyoung · 2017-07-17T16:19:12Z

pandas/core/generic.py

+
+        Returns
+        -------
+        renamed : %(klass)s (new object) if inplace=False, None otherwise


Move the description (after the "klass") to a newline (and reword slightly so that it makes sense by itself)

gfyoung · 2017-07-17T16:19:40Z

pandas/core/generic.py

+        0  1   4
+        1  2   5
+        2  3   6
+        """


Add an example for "inplace=True"

gfyoung · 2017-07-17T16:21:02Z

pandas/tests/frame/test_alter_axes.py

+
+        # omitting the "axis=" parameter:
+        result = df.copy()
+        with tm.assert_produces_warning(warn):


Let's be more explicit about this warning value, as it is not part of the for loop.

gfyoung · 2017-07-17T16:21:21Z

pandas/tests/frame/test_alter_axes.py

+            result = df.set_axis(axis, list('abc'), inplace=False)
+            tm.assert_frame_equal(expected[axis], result)
+
+        # omitting the "axis=" parameter:


Remove the "=" and ":"

gfyoung · 2017-07-17T16:21:42Z

pandas/tests/frame/test_alter_axes.py

+            tm.assert_frame_equal(expected[axis], result)
+
+        # omitting the "axis=" parameter:
+        result = df.copy()


Nit: add newline beneath this (just for readability)

gfyoung · 2017-07-17T16:22:41Z

pandas/tests/series/test_alter_axes.py

+        tm.assert_series_equal(expected, result)
+
+        # omitting the "axis=" parameter:
+        result = s.copy()


Same comment as above

gfyoung · 2017-07-17T16:23:03Z

pandas/tests/series/test_alter_axes.py

+        result = s.set_axis(0, list('abcd'), inplace=False)
+        tm.assert_series_equal(expected, result)
+
+        # omitting the "axis=" parameter:


Same as comment as above

gfyoung · 2017-07-17T16:24:24Z

pandas/tests/series/test_alter_axes.py

+
+        # omitting the "axis=" parameter:
+        result = s.copy()
+        with tm.assert_produces_warning(warn):


Same comment as above

gfyoung · 2017-07-17T16:25:16Z

pandas/tests/frame/test_alter_axes.py

+                    result.set_axis(axis, list('abc'), **kwargs)
+                tm.assert_frame_equal(result, expected[axis])
+
+            # GH14636 (inplace=False)


Just reference the issue below the test definition line. This comment can be reduced to "inplace=False"

Are you sure? The scope of the test is broader (since set_axis() wasn't tested at all) than just the issue, which specifically refers to inplace=False.

But it's the issue that triggered the entire test, which is why I proposed that it be moved to the top of the function definition.

But it's the issue that triggered the entire test

Well, in fact I just decided to test something which wasn't tested.

But you are adding a new parameter to the signature based on the diff.

gfyoung · 2017-07-17T16:26:20Z

pandas/tests/series/test_alter_axes.py

+                    result.set_axis(0, list('abcd'), **kwargs)
+                tm.assert_series_equal(result, expected)
+
+        # GH14636 (inplace=False)


Same comment as above

gfyoung · 2017-07-17T16:27:23Z

pandas/tests/frame/test_alter_axes.py

+        expected['columns'] = expected[1]
+
+        for axis in expected:
+            # inplace=True


A sentence (on a newline) about why we issue FutureWarning when inplace=None would be good here.

gfyoung · 2017-07-17T16:27:59Z

pandas/tests/series/test_alter_axes.py

+        expected.index = list('abcd')
+
+        for axis in 0, 'index':
+            # inplace=True


Same comment as above

codecov · 2017-07-17T17:31:05Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #16994      +/-   ##
==========================================
- Coverage   90.99%   90.97%   -0.02%     
==========================================
  Files         161      161              
  Lines       49290    49299       +9     
==========================================
  Hits        44851    44851              
- Misses       4439     4448       +9

Flag	Coverage Δ
#multiple	`88.75% <100%> (ø)`	⬆️
#single	`40.19% <27.27%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/generic.py	`92.33% <100%> (+0.03%)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.76% <0%> (-0.1%)`	⬇️

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 0bd871f...02c370f. Read the comment docs.

codecov · 2017-07-17T17:31:13Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #16994      +/-   ##
==========================================
- Coverage   91.02%   91.01%   -0.02%     
==========================================
  Files         161      161              
  Lines       49351    49363      +12     
==========================================
+ Hits        44924    44927       +3     
- Misses       4427     4436       +9

Flag	Coverage Δ
#multiple	`88.78% <100%> (ø)`	⬆️
#single	`40.25% <15%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/groupby.py	`92.07% <100%> (ø)`	⬆️
pandas/core/generic.py	`92.05% <100%> (+0.05%)`	⬆️
pandas/core/reshape/pivot.py	`95.18% <100%> (ø)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.75% <0%> (-0.1%)`	⬇️

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 811bcbc...8fb9d0f. Read the comment docs.

jreback

so this looks fine. We could in principle switch the order of the parameters, e.g.

def set_axis(self, labels, axis, inplace=None)

to make this much more consistent (and Series could default axis=0).

I think this could done in a completely backward compat manner (as the axis must be a scalar and the labels must be a list).

jreback · 2017-07-17T23:46:02Z

pandas/core/generic.py

+        axis : int or string, default 0
+        labels: list-like or Index
+            The values for the new index
+        inplace : boolean, default False


default is None (currently), it is set to True if its None (and the warning happens)

jreback · 2017-07-17T23:46:22Z

pandas/tests/frame/test_alter_axes.py

        df = df.reset_index()
+
+    def test_set_axis(self):
+


can you add the issue number

jreback · 2017-07-17T23:47:14Z

pandas/tests/series/test_alter_axes.py

+    def test_set_axis(self):
+
+        s = Series(np.arange(4), index=[1, 3, 5, 7], dtype='int64')
+


issue number

toobaz · 2017-07-18T06:39:40Z

We could in principle switch the order of the parameters, e.g.

Aha, right! Would I emit a DeprecationWarning with the old signature?

jreback · 2017-07-18T10:38:22Z

Aha, right! Would I emit a DeprecationWarning with the old signature?

FutureWarning

pep8speaks · 2017-07-18T16:37:31Z

Hello @toobaz! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on July 23, 2017 at 20:58 Hours UTC

toobaz · 2017-07-18T16:41:06Z

Should I document the signature change in the "Other API Changes" section of the whatsnew (despite the old version still being supported)?

gfyoung · 2017-07-18T16:57:52Z

Should I document the signature change in the "Other API Changes" section of the whatsnew (despite the old version still being supported)?

Yes, you should. Users might pass in these arguments positionally, so this signature change would break their code unless they were informed.

toobaz · 2017-07-18T17:28:10Z

Users might pass in these arguments positionally, so this signature change would break their code unless they were informed.

We don't break anything (for now). This is precisely why I'm asking.

gfyoung · 2017-07-18T17:30:45Z

We don't break anything (for now). This is precisely why I'm asking.

True, but we still should let people know because there are many people who use the library, and we have no idea whose code we would break.

toobaz · 2017-07-18T17:35:21Z

True, but we still should let people know because there are many people who use the library, and we have no idea whose code we would break.

The number of users is not a measure of the quality of refactoring... the question is just "should we mention an API change despite it not breaking anything? Or should we mention it only in the future, when the compatibility mode is removed?"

You're welcome to suggest a sentence.

gfyoung · 2017-07-18T17:42:37Z

the question is just "should we mention an API change despite it not breaking anything?

You should mention it now because you changed the signature by switching the position of the arguments (that's all you need to mention that it's a signature change). That's going to break anyone's code whose function calls use positional arguments.

toobaz · 2017-07-18T17:49:02Z

you changed the signature by switching the position of the arguments

Maybe I see the misunderstanding: have you actually seen the code, the tests or the discussion above?

gfyoung · 2017-07-18T17:52:58Z

@toobaz : I have in fact, and my point still stands. You may have added deprecation warnings, but you should still mention the change as an API change because the new signature is the recommended way to pass in arguments.

toobaz · 2017-07-18T17:56:11Z

the new signature is the recommended way

Great, now we are on the same page. Will try to add a sentence.

jreback

small changes. If this is going to complement remove the need for .relabel. Then I would

add to api.rst
add docs on using this (with the new API), near set_index section in indexing
add sub-section in whatsnew where you show signature changing AND how it could/should be used, with ref to section above.

jreback · 2017-07-18T23:55:24Z

pandas/core/generic.py

-        """ public verson of axis assignment """
-        setattr(self, self._get_axis_name(axis), labels)
+    _shared_docs['set_axis'] = """Assign desired index to given axis
+


add a note that the signature changed in 0.21.0

(done: line 483)

jreback · 2017-07-18T23:56:04Z

pandas/core/generic.py

+
+    @Appender(_shared_docs['set_axis'] % dict(klass='NDFrame'))
+    def set_axis(self, labels, axis=0, inplace=None):
+        if isinstance(labels, (int, str)):


use is_scalar

jreback · 2017-07-19T00:00:07Z

pandas/tests/frame/test_alter_axes.py

+            # inplace=False
+            result = df.set_axis(list('abc'), axis=axis, inplace=False)
+            tm.assert_frame_equal(expected[axis], result)
+


can you add a test for invalid axis (e.g. 'foo', or 2)

test with axis ='index' and axis='columns' (should work)

can you add a test for invalid axis (e.g. 'foo', or 2)

(done: line 947)

jreback · 2017-07-19T00:00:21Z

pandas/tests/series/test_alter_axes.py

+        s = Series(np.arange(4), index=[1, 3, 5, 7], dtype='int64')
+
+        expected = s.copy()
+        expected.index = list('abcd')


test for invalid axis (1 or 'foo')

test with axis='index' (should work)

test for invalid axis (1 or 'foo')

(done: line 266)

toobaz · 2017-07-19T06:25:20Z

test with axis ='index' and axis='columns' (should work)

Already there (see lines 923-924)

toobaz · 2017-07-19T09:18:59Z

If this is going to complement remove the need for .relabel

Despite the new signature, I'm not 100% convinced. Can we wait for a bit more discussion? I will add there a summary of pros and cons and, once a decision is made, take care of the docs you mention.

closes pandas-dev#14636

toobaz · 2017-07-20T13:18:23Z

@jreback Some change requests still show up, but I should have solved them all in the last rebase.

(except the additional docs, for which I will wait #14829 and then provide another PR)

jreback

minor comment. pls add to api.rst as well where appropriate.

jreback · 2017-07-21T10:43:10Z

pandas/core/generic.py

+
+        Returns
+        -------
+        renamed : %(klass)s or None


this needs updating

Not sure what you mean... anyway, I just made the following line more explicit

jreback · 2017-07-21T10:44:06Z

pandas/core/generic.py

+    def set_axis(self, labels, axis=0, inplace=None):
+        if is_scalar(labels):
+            warnings.warn(
+                "set_axis now takes \"labels\" as first argument, and "


maybe just use r to avoid needing the quotes, or we simply use ' as the quote marker.

toobaz · 2017-07-23T20:59:51Z

minor comment. pls add to api.rst as well where appropriate.

Done for Series, was already present for DataFrame.

jreback · 2017-07-24T23:49:39Z

thanks!

gfyoung added Enhancement Indexing Related to indexing on series/frames, not to indexes themselves Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Jul 17, 2017

gfyoung reviewed Jul 17, 2017

View reviewed changes

pandas/tests/series/test_alter_axes.py

expected.index = list('abcd')

for axis in 0, 'index':

# inplace=True

Copy link

Member

gfyoung Jul 17, 2017

Choose a reason for hiding this comment

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

Same comment as above

toobaz force-pushed the set_axis_inplace branch from 02c370f to d2816ea Compare July 17, 2017 22:50

jreback reviewed Jul 17, 2017

View reviewed changes

toobaz force-pushed the set_axis_inplace branch from d2816ea to 06e03c7 Compare July 18, 2017 16:37

toobaz force-pushed the set_axis_inplace branch from 06e03c7 to 51912fb Compare July 18, 2017 16:44

toobaz force-pushed the set_axis_inplace branch from 51912fb to bfa7466 Compare July 18, 2017 18:10

jreback requested changes Jul 19, 2017

View reviewed changes

ENH: provide "inplace" argument to set_axis(), change signature

409f502

closes pandas-dev#14636

toobaz force-pushed the set_axis_inplace branch from bfa7466 to 95d6093 Compare July 19, 2017 09:28

jreback mentioned this pull request Jul 19, 2017

API: allow DataFrame.rename to take a list-like of colums #14829

Open

gfyoung approved these changes Jul 20, 2017

View reviewed changes

jreback requested changes Jul 21, 2017

View reviewed changes

jreback added this to the 0.21.0 milestone Jul 21, 2017

REF: adapt NDFrame.set_axis() calls to new signature

8fb9d0f

toobaz force-pushed the set_axis_inplace branch from 95d6093 to 8fb9d0f Compare July 23, 2017 20:58

jreback approved these changes Jul 24, 2017

View reviewed changes

jsexauer mentioned this pull request Jul 24, 2017

DEPR: Clean up list of deprecations from prior versions #6581

Closed

1 task

jreback closed this in 9e6bb42 Jul 24, 2017

toobaz deleted the set_axis_inplace branch July 25, 2017 04:41

WillAyd mentioned this pull request Jul 26, 2019

DEPR: NDFrame.set_axis inplace defaults to false #27525 #27600

Merged

jreback mentioned this pull request Jul 26, 2019

DEPR: deprecations log for removed issues #13777

Closed

+1   4
+2   5
+3   6
+                      """

		def test_set_axis(self):

		s = Series(np.arange(4), index=[1, 3, 5, 7], dtype='int64')

Uh oh!

ENH: provide "inplace" argument to set_axis() #16994

ENH: provide "inplace" argument to set_axis() #16994

Uh oh!

Conversation

toobaz commented Jul 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gfyoung Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gfyoung Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codecov bot commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jreback left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toobaz commented Jul 18, 2017

Uh oh!

gfyoung Jul 17, 2017 •

edited

Loading

gfyoung Jul 17, 2017 •

edited

Loading

codecov bot commented Jul 17, 2017 •

edited

Loading

codecov bot commented Jul 17, 2017 •

edited

Loading

pep8speaks commented Jul 18, 2017 •

edited

Loading