series.rename doesn't return 'None' when inplace=True

[giuliobeseghi]

Code Sample, a copy-pastable example if possible

import pandas as pd
series = pd.Series(range(10))
should_be_none = series.rename('new_name', inplace=True)
print(should_be_none)

Problem description

When a method is called with the inplace argument, the method should:

• transform the series inplace
• return None

In this case, the method does:

• transform the series inplace
• returns the series transformed (not a copy)

Expected Output

None

Output of pd.show_versions()

[paste the output of pd.show_versions() here below this line]

INSTALLED VERSIONS

commit : None
python : 3.7.4.final.0
python-bits : 64
OS : Windows
OS-release : 10
machine : AMD64
processor : Intel64 Family 6 Model 60 Stepping 3, GenuineIntel
byteorder : little
LC_ALL : None
LANG : None
LOCALE : None.None

pandas : 0.25.1
numpy : 1.16.5
pytz : 2019.3
dateutil : 2.8.0
pip : 19.2.3
setuptools : 41.4.0
Cython : 0.29.13
pytest : 5.2.1
hypothesis : None
sphinx : 2.2.0
blosc : None
feather : None
xlsxwriter : 1.2.1
lxml.etree : 4.4.1
html5lib : 1.0.1
pymysql : None
psycopg2 : None
jinja2 : 2.10.3
IPython : 7.8.0
pandas_datareader: None
bs4 : 4.8.0
bottleneck : 1.2.1
fastparquet : None
gcsfs : None
lxml.etree : 4.4.1
matplotlib : 3.1.1
numexpr : 2.7.0
odfpy : None
openpyxl : 3.0.0
pandas_gbq : None
pyarrow : None
pytables : None
s3fs : None
scipy : 1.3.1
sqlalchemy : 1.3.9
tables : 3.5.2
xarray : None
xlrd : 1.2.0
xlwt : 1.3.0
xlsxwriter : 1.2.1

[pv8473h12]

Hi, is it alright if I work on this?

[giuliobeseghi]

Yes of course. I didn't start working on it because I don't know how inplace is actually implemented. If you know it, it should be relatively easy :)

[pv8473h12]

I agree that the current implementation is inconsistent. That being said, this could be a breaking change for some people. How should we address this?

[TomAugspurger]

I don’t think we want to change this.

…

On Oct 11, 2019, at 16:24, pv8493013j ***@***.***> wrote:  I agree that the current implementation is inconsistent. That being said, this could be a breaking change for some people. How should we address this? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[giuliobeseghi]

I'm not sure if I agree with this. I think it's quite safe to change the implementation. In fact, I just see two options:

1. The programmer has realized that inplacedoesn't work as expected. Therefore, instead of calling series.rename('new_name', inplace=True), I assume that the programmer has changed the expression to series = series.rename('new_name'). A fix won't have any impact here.
2. The programmer hasn't realized that inplace doesn't work as expected. Therefore, I assume that the programmer still uses series.rename('new_name'). A fix won't have any impact here either.

I'd agree with calling it a 'breaking change' just in a hypothetic case 3:
3. The programmer has decided to rename the series with series = series.rename('new_name', inplace=True)
But I see no reasons why one should choose this - and it could be harmful, because it would be wrong if he repeated the same with another series = series.method(inplace=True)

The reasons why I proposed this change are:

1. The method returns an unexpected output. I had to make some tests to make sure that the series was correctly modified inplace, because I knew there was a bug.
2. I thought this behavior is inconsistent, and could mislead some pandas beginners to understand the concept of inplace.

Why do you think it could be a breaking change for anyone?

[TomAugspurger]

We have an issue about deprecating inplace: #16529

I’d prefer not to make changes (breaking or deprecations) while that’s pending.

[simonjayhawkins]

@TomAugspurger noting your comments about not wanting to make changes, but since I think this could be considered a bug, it maybe worth getting this fixed for 1.0

[simonjayhawkins]

... without deprecation cycle

[jreback]

hmm this is quite inconsistent with the return value of an inplace operation elsewhere - so i would consider this a bug

we r not doing an inplace deprecation for 1.0 so i think fixing this return value is worthwhile

since this has been in place quite a while a deprecation cycle is warranted - would be happy to take that for 1.0

[jreback]

on my previous comment; actually this would be very hard to deprecate as it’s a return value (meaning it would always show the deprecation warning if inplace=True)

sp choice between doing. nothing until we deprecate inplace or do a breaking chance and just change the return value to None?

i am +0 on breaking this now - so not super strong

[TomAugspurger]

I just don't think the benefit of this change (consistency, anything else?) warrants breaking API.

[giuliobeseghi]

I basically raised this topic because:

1. I got a series printed at some point while calling the method in place;
2. I thought "gosh, this is a bug, the series hasn't been changed in place and a new object was returned!"
3. I finally realised that "hey, the series has been changed in place, but also a new object has been returned, so my code is fine."

So yes, this is technically a bug but the method does what it should do (plus it does an extra thing that it shouldn't do). The only benefit I see is that if we changed it we wouldn't have developers wondering if the method has worked as they expect (so they could save a little time).

But I'm +1 to wait for the inplace deprecation.

[TomAugspurger]

Pushing off 1.0.