DOC: Improve "inplace" param in Dataframe.rename() docs #24037

nmusolino · 2018-12-01T17:39:36Z

closes Unclear documentation of inplace parameter for pandas.DataFrame.rename. #23076
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pep8speaks · 2018-12-01T17:39:42Z

Hello @nmusolino! Thanks for submitting the PR.

There are no PEP8 issues in the file pandas/core/frame.py !
There are no PEP8 issues in the file pandas/tests/frame/test_alter_axes.py !

nmusolino · 2018-12-01T17:52:27Z

As described in #23076, the description of "inplace" in the current documentation is "backwards".

Arguably, there should be a wider cleanup to use a consistent phrasing for inplace across pandas, but the scope of this PR is just fixing the "backwards" parameter documentation in this method.

Here is some prior art for how "inplace" is documented in DataFrame methods with a similar purpose:

DataFrame.drop()

    inplace : bool, default False
        If True, do operation inplace and return None.

DataFrame.reset_index()

    inplace : boolean, default False
        Modify the DataFrame in place (do not create a new object)

DataFrame.rename_axis(). This is probably from a shared NDFrame docstring.

    inplace : boolean, default False
         Modifies the object directly, instead of creating a new Series or DataFrame.

codecov · 2018-12-01T18:25:11Z

Codecov Report

Merging #24037 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #24037   +/-   ##
=======================================
  Coverage   42.46%   42.46%           
=======================================
  Files         161      161           
  Lines       51557    51557           
=======================================
  Hits        21892    21892           
  Misses      29665    29665

Flag	Coverage Δ
#single	`42.46% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/frame.py	`38.7% <ø> (ø)`	⬆️

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 5b0610b...f273ef0. Read the comment docs.

codecov · 2018-12-01T18:25:11Z

Codecov Report

Merging #24037 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #24037   +/-   ##
=======================================
  Coverage   42.46%   42.46%           
=======================================
  Files         161      161           
  Lines       51557    51557           
=======================================
  Hits        21892    21892           
  Misses      29665    29665

Flag	Coverage Δ
#single	`42.46% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/frame.py	`38.7% <ø> (ø)`	⬆️

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 5b0610b...f273ef0. Read the comment docs.

gfyoung · 2018-12-02T10:12:52Z

Arguably, there should be a wider cleanup to use a consistent phrasing for inplace across pandas

That sounds like a plan. Maybe we could do it in this PR even.

nmusolino · 2018-12-02T17:31:50Z

I'm willing to work on that in a future PR. My suggestion is to get this PR in right now, in order to fix the mistaken docstring and close the associated issue (#23076).

The problem with the current DataFrame.rename() docstring, just to be explicit, is this: in general, when a parameter <X> has a description "whether to <blank>", the implication is that when the parameter X is true, the method will "<blank>".

The current docstring does not follow that convention, and in fact inverts that convention.

As for the wider change, I think that will take more discussion:

How should the parameter description be phrased?
Should the "Returns" section of each method include a note about returning None when inplace is True (where applicable)?
Should the parameter description mention that internal data structures (e.g. block manager or index) might be copied even when inplace is True?

Or even, should that effort be made in light of #16529: API/DEPR: Deprecate inplace parameter? I just saw that deprecation proposal now.

jreback · 2018-12-02T17:50:24Z

cc @datapythonista

datapythonista

+1 on standardizing the descriptions for inplace. But the proposed description is no exactly correct.

datapythonista · 2018-12-02T17:58:26Z

pandas/core/frame.py

-            Whether to return a new DataFrame. If True then value of copy is
-            ignored.
+            If True, perform the operation in-place, modifying this DataFrame,
+            and return None.


I don't think this is technically correct. The name is misleading, but using inplace doesn't necessarily mean that we're modifying the DataFrame, as we can be returning a copy.

I think the previous description is more accurate, except that it sounds like inplace=True will return a new DataFrame.

Sorry, I don't understand your first paragraph. Can you give an example of when an operation with inplace=True doesn't modify "self", i.e. the data frame on which the method is called?

When inplace is True, the method should modify the data frame, and no second frame should be created. When inplace is False, the method should return a second data frame, and self should be unmodified.

If you have a DataFrame named df, using the memory in the address 1000-2000, and you do df.operation(inplace=True), df will be modified, but the data can be now in the memory address 3000-4000.

So, technically speaking, the operation is not in place, as the result has been copied to a different location (even if the reference df is reassigned to the new memory).

That's why I don't like your description, as for a user who doesn't care about the memory is perfectly fine and correct. But if you think about the memory, you description is not really correct.

@nmusolino there are about 2 inplace operations that actually operate inplace. all others, simply do an internal copy, do the op, then assign back to the top level data. inplace has never been a real operation in pandas, nor will it ever be. This is why we are deprecating it. You can certainly give a longer explanation, but discouraging usage would be good here.

@jreback are we ready to deprecate all inplace parameters? A quick grep shows around 60 methods with an inplace parameter, so it's not so much work to start raising warnings (and update the descriptions) for all them.

Or do we prefer to wait?

@datapythonista :

See #16529 - I think we were planning to wait until 1.0, though can always push up.

Thanks for pointing out the issue @gfyoung, I'll continue the conversation there.

@datapythonista, @jreback , I understand that these operations may involve copies. I wrote earlier that "internal data structures (e.g. block manager or index) might be copied even when inplace is True."

I think the distinction you are trying to draw is that a rename or fillna operation does not necessarily overwrite individual elements in place; is that right? As a simple example, I seem to recall that Index objects are generally immutable, so any operation that changes the index would necessarily replace the index.

Avoiding that implication is a reasonable point. To be clear, however, the methods are modifying the dataframe.

How about this description?

inplace : boolean, default False If true, rename index labels in this DataFrame and return None, without creating a new DataFrame.

Or a simpler, more generic version:

inplace : boolean, default False If true, modify this DataFrame and return None; does not create a new DataFrame.

datapythonista · 2018-12-03T11:58:40Z

Closing in favor of #24063, where the inplace argument will be deprecated, and all descriptions updated.

nmusolino added 2 commits December 1, 2018 12:37

DOC: Improve "inplace" param in Dataframe.rename() docs

38f2a80

TST: Assert result of in-place rename is None

f273ef0

nmusolino mentioned this pull request Dec 1, 2018

Unclear documentation of inplace parameter for pandas.DataFrame.rename. #23076

Closed

gfyoung added Docs DataFrame DataFrame data structure labels Dec 2, 2018

jreback requested a review from datapythonista December 2, 2018 17:50

datapythonista requested changes Dec 2, 2018

View reviewed changes

datapythonista closed this Dec 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOC: Improve "inplace" param in Dataframe.rename() docs #24037

DOC: Improve "inplace" param in Dataframe.rename() docs #24037

nmusolino commented Dec 1, 2018 •

edited

Loading

pep8speaks commented Dec 1, 2018

nmusolino commented Dec 1, 2018 •

edited

Loading

codecov bot commented Dec 1, 2018

codecov bot commented Dec 1, 2018 •

edited

Loading

gfyoung commented Dec 2, 2018

nmusolino commented Dec 2, 2018 •

edited

Loading

jreback commented Dec 2, 2018

datapythonista left a comment

datapythonista Dec 2, 2018

nmusolino Dec 2, 2018

datapythonista Dec 2, 2018

jreback Dec 2, 2018

datapythonista Dec 3, 2018 •

edited

Loading

gfyoung Dec 3, 2018

datapythonista Dec 3, 2018

nmusolino Dec 3, 2018 •

edited

Loading

datapythonista commented Dec 3, 2018

DOC: Improve "inplace" param in Dataframe.rename() docs #24037

DOC: Improve "inplace" param in Dataframe.rename() docs #24037

Conversation

nmusolino commented Dec 1, 2018 • edited Loading

pep8speaks commented Dec 1, 2018

nmusolino commented Dec 1, 2018 • edited Loading

codecov bot commented Dec 1, 2018

Codecov Report

codecov bot commented Dec 1, 2018 • edited Loading

Codecov Report

gfyoung commented Dec 2, 2018

nmusolino commented Dec 2, 2018 • edited Loading

jreback commented Dec 2, 2018

datapythonista left a comment

Choose a reason for hiding this comment

datapythonista Dec 2, 2018

Choose a reason for hiding this comment

nmusolino Dec 2, 2018

Choose a reason for hiding this comment

datapythonista Dec 2, 2018

Choose a reason for hiding this comment

jreback Dec 2, 2018

Choose a reason for hiding this comment

datapythonista Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

gfyoung Dec 3, 2018

Choose a reason for hiding this comment

datapythonista Dec 3, 2018

Choose a reason for hiding this comment

nmusolino Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

datapythonista commented Dec 3, 2018

nmusolino commented Dec 1, 2018 •

edited

Loading

nmusolino commented Dec 1, 2018 •

edited

Loading

codecov bot commented Dec 1, 2018 •

edited

Loading

nmusolino commented Dec 2, 2018 •

edited

Loading

datapythonista Dec 3, 2018 •

edited

Loading

nmusolino Dec 3, 2018 •

edited

Loading