PDEP-8: Inplace methods in pandas
State: closed

cc @pandas-dev/pandas-core @pandas-dev/pandas-triage

Closes https://github.com/pandas-dev/pandas/issues/16529


Labels:
name: Copy / view semantics
name: inplace
description: Relating to inplace parameter or equivalent
name: PDEP
description: pandas enhancement proposal

Comments:
I would say this proposal has a fatal flaw. What's to stop inplace / copy keywords being added back going forwards? 

Further the notion of an inplace method returning self is just very confusing in itself.

Thus i would be -1 on a partial change as this PDEP proposes.

That said I would be +1 on removing all uses of inplace (to the extent a copy keyword is needed for CoW compat is needed that is unavoidable)

Reasons are similar for what you are proposing. 
- performance is not meaningful with CoW
- syntax with inplace is not great
Adding back inplace in other methods than suggested in the PDEP is useless, because with CoW all these methods already return views if possible, so no benefit of adding inplace/copy back in.

The main issue with methods in group 2 is that they will __always__ create copies under CoW because modifying the underlying array inplace violates CoW principles when returning a new object.

```
df = DataFrame({"a": [1.5, np.nan, 2.5]})
df2 = df.fillna(100.5)
```

If we modify the array without copying, we would always change ``df`` and ``df2``.  With ``inplace`` this could avoid copying, but yeah I agree that the syntax with inplace is terrible.
I'm also wondering if we should add an inplace keyword to EA methods as well.

Currently ``fillna`` is the only EA method that would benefit from this, but interpolate (https://github.com/pandas-dev/pandas/issues/50950) would benefit from this too, if it was added to the EA interface.
If Copy-on-Write is not made the default in the future what are the implications for this PDEP?
> Personally I think this PDEP is too big, and a bit difficult to read and discuss.

maybe worth trying to avoid repetition to reduce the word count. For instance, where there are bullet points, be more concise where the detail is expanded on later.
> > Personally I think this PDEP is too big, and a bit difficult to read and discuss.
> 
> maybe worth trying to avoid repetition to reduce the word count. For instance, where there are bullet points, be more concise where the detail is expanded on later.

could the Open Questions section be completely removed and maybe open issues instead?
> Personally I think this PDEP is too big, and a bit difficult to read and discuss.

@datapythonista If possible to be more specific, are there specific sections you find difficult to read that we would improve, or something in the general flow / structure of the document that is confusing? 

Yes, the text is quite long, but that's also because we wanted to big thorough and complete and clear (but for sure we can improve on that based on feedback). 
Personally I think we can certainly _try_ to discuss this as a single PDEP, many of the aspects are very much related.

---

If we want to reduce the size, I would personally remove most of the discussion of the `copy` keyword (apart from a brief mention how it interacts with inplace, and in the rejected ideas section), and focus on only the `inplace` keyword. In the end, `copy=False` does not do anything inplace, it's just another way to avoid a copy (one of the reasons to use `inplace=True` as well). But the fact that the `copy` keyword essentially becomes obsolete is entirely due to Copy-on-Write (PDEP-7). In fact, we already implemented that in main when CoW is enabled.
> @datapythonista If possible to be more specific, are there specific sections you find difficult to read that we would improve, or something in the general flow / structure of the document that is confusing?

From past discussions, I think it should be easy to reach consensus to remove `inplace` from where it doesn't make sense. I think a PDEP dedicated to that should be much simpler and focussed, and we can get that part out of the question. And I personally see it as independent from how inplace should behave in the places where it makes sense.

Mixing all together with the different categories of functions and methods is what I had in mind when I said it's a bit difficult to read (or in my mind, more difficult than two separate PDEPs).

The part that I found confusing is the last part of `Proposed changes and reasoning`. It's not clear if the proposal is to use `df = df.replace(..., inplace='True')`. Also, I should get more into the details of CoW probably, but without a very good understanding about that, it's not easy to understand the reasoning why having `inplace` is better than just replacing the data (or making the copy as you call it).

In any case, excellent work with the content of the PDEP. To be clear, my comments are only related to possible ways to make this PDEP easier to understand and discuss. The analysis is great, and I think this PDEP is very valuable. We've been discussing about this since before pandas 1.0... :)
> [@simonjayhawkins] If Copy-on-Write is not made the default in the future what are the implications for this PDEP?

That's a good question. The current proposal assumes that CoW will be made the default. If it turns out that we have to reconsider CoW the coming months based on user feedback (with the option in 2.0), then we will also have to reconsider this proposal. 
(but I think by the time we would add deprecation warnings for this PDEP, we should already have some more experience with CoW to have a better idea how that is going)



> > [@simonjayhawkins] If Copy-on-Write is not made the default in the future what are the implications for this PDEP?
> 
> That's a good question. The current proposal assumes that CoW will be made the default. If it turns out that we have to reconsider CoW the coming months based on user feedback (with the option in 2.0), then we will also have to reconsider this proposal. (but I think by the time we would add deprecation warnings for this PDEP, we should already have some more experience with CoW to have a better idea how that is going)

In that case, I feel like it'd be better to open a new PDEP. Maybe we should mark this one as "conditionally accepted" then (if accepted), to reflect that.
> Maybe we should mark this one as "conditionally accepted" then (if accepted), to reflect that.

sgtm. keeps the discussion simpler if we make some assumptions.
This seems to be in good-enough shape to accept provided that copy-on-write becomes the default, as we expect it to do.

My two cents: I have a passing familiarity with copy-on-write, and it was enough to understand this PDEP proposal. I think it's worded clearly enough, although it does take reading the examples from all the group to understand the difference between pandas objects mutation and the underlying data mutation.
I also agree that this is heavily dependent on CoW becoming the default. otherwise this does not make much sense the way it is written, so conditionally accepted sounds like a good solution till we agree that CoW becomes the default.

Splitting the PDEP isn't really straightforward. Splitting would mean that we'd remove the group where we want to keep the ``inplace`` keyword. Thus, discussing if we want to get rid of inplace everywhere or not would be out of scope here which is the biggest point of discussion imo. The more efficient behavior will become the default with CoW, so there isn't really a use case to keep the keywords anyway. If we keep group 1 out of here and accept this PDEP, then we would more or less accept the proposal as is.
@Dr-Irv answering https://github.com/pandas-dev/pandas/pull/51466#pullrequestreview-1335936509

> I would still prefer to remove the `inplace` keyword entirely. But I understand that for the methods in Group 2, there's a performance issue if it is not available.
> 
> How about for those methods, we replace the `inplace` keyword with something else, such as `mutate=True`. Advantages of this are: ...

I see the appeal of making a "clean slate" and use a different name, but I think there are also disadvantages to that: 
1) for the people using the inplace keyword in case of group 2 methods, that's perfectly valid code and that would stay valid with this proposal, so changing the keyword name will require everyone in this case to update their code just for the sake of using a different name. 
2) the history of the keyword aside, I personally can't directly think of a better name than "inplace". For _that_ subset of cases, IMO it nicely describes what it does and matches how the term is used for eg inplace operators on arrays
> I see the appeal of making a "clean slate" and use a different name, but I think there are also disadvantages to that:
> 
> 1. for the people using the inplace keyword in case of group 2 methods, that's perfectly valid code and that would stay valid with this proposal, so changing the keyword name will require everyone in this case to update their code just for the sake of using a different name.

And it helps discourage the usage, because we'd go through a deprecation cycle, and forces people to think about whether/why they were using `inplace` at all.

> 2. the history of the keyword aside, I personally can't directly think of a better name than "inplace". For _that_ subset of cases, IMO it nicely describes what it does and matches how the term is used for eg inplace operators on arrays

Or we switch to what @WillAyd suggested, along the lines of `copy_pandas_object` and  `copy_underlying_data` .  So for group 2, `inplace=True` becomes `copy_underlying_data=False`, and for group 3, we pick the appropriate keyword based on the actual method.




> and for group 3, we pick the appropriate keyword based on the actual method.

Patrick mentioned it above as well (https://github.com/pandas-dev/pandas/pull/51466#discussion_r1140654705), but we don't need any keyword for those methods. 
Or at least, that's what we propose in this PDEP (see the *"Methods in groups 3 and 4 will lose their `copy` and `inplace` keywords. .."* paragraph in the "Proposed changes and reasoning" section). If someone wants to argue to keep a keyword for those methods as well, then you need to discuss the idea itself and not just the name of the keyword ;)
> > I see the appeal of making a "clean slate" and use a different name, but I think there are also disadvantages to that:
> > 
> > 1. for the people using the inplace keyword in case of group 2 methods, that's perfectly valid code and that would stay valid with this proposal, so changing the keyword name will require everyone in this case to update their code just for the sake of using a different name.
> 
> And it helps discourage the usage, because we'd go through a deprecation cycle, and forces people to think about whether/why they were using `inplace` at all.
> 
> > 2. the history of the keyword aside, I personally can't directly think of a better name than "inplace". For _that_ subset of cases, IMO it nicely describes what it does and matches how the term is used for eg inplace operators on arrays
> 
> Or we switch to what @WillAyd suggested, along the lines of `copy_pandas_object` and `copy_underlying_data` . So for group 2, `inplace=True` becomes `copy_underlying_data=False`, and for group 3, we pick the appropriate keyword based on the actual method.

IMO you're too close to the issue and are thinking too much like a direct pandas contributor. 99.99% of users don't care about the internals more than a "oh that's cool <archive thought/>" sort of thing.

Correspondingly, it's totally besides the point what is happening when you call `inplace`. What's relevant is "next time the object is accessed the changes were applied". Getting rid of it or renaming it will just generate endless questions into the future and more migration and update headaches. 

**Strongly oppose** this PDEP. 
Could you elaborate which part you are opposing?

> I'm also wondering if we should add an inplace keyword to EA methods as well.

> Currently fillna is the only EA method that would benefit from this, but interpolate (https://github.com/pandas-dev/pandas/issues/50950) would benefit from this too, if it was added to the EA interface.

Came here with this in mind.  I'd like to move the relevant logic out of the blocks and into the EAs, but for that need either copy or inplace on the EA method(s).  In either case, we'd need to figure out what to do in cases like SparseArray where it can't be done inplace or ArrowEA where setting is not zero-copy.
I pushed some updates to the PDEP related to earlier feedback points on the text (so not actual changes to the core proposal, but trying to explain it clearer):

- Simplified the text by focusing on the the `inplace` keyword, and removing some content about the `copy` keyword (eg removed those methods from the tables), given that most of the discussion items are related to `inplace`, and the `copy` keyword no longer being useful is essentially just a consequence of the CoW change.
- Tried to explain the "values-inplace" vs "object-inplace" difference better (eg related to https://github.com/pandas-dev/pandas/pull/51466#discussion_r1140324175)

(with the two commits combined, the text actually didn't get shorter ... but hopefully clearer!)
> [@datapythonista ] The part that I found confusing is the last part of `Proposed changes and reasoning`. It's not clear if the proposal is to use `df = df.replace(..., inplace='True')`. Also, I should get more into the details of CoW probably, but without a very good understanding about that, it's not easy to understand the reasoning why having `inplace` is better than just replacing the data (or making the copy as you call it).

In the last commit, I did an attempt to explain this better, how CoW affects that `df.replace(..)` can never happen inplace (it would violate CoW rules), and therefore leaving a use case for an `inplace=True` keyword. Feedback welcome!
@pandas-dev/pandas-core I would like to try to move this towards a final discussion round. As mentioned above (https://github.com/pandas-dev/pandas/pull/51466#issuecomment-1671603650), we already updated the text to focus on the `inplace` keyword, hopefully making the PDEP a bit clearer and simpler.


Re-reading the discussion up to now, I think one of the main points brought up (by Irv and Will) is about the `inplace` keyword for those cases where we propose to keep it. Why not remove it entirely? And when keeping a keyword, why not use a different keyword name? (eg `mutate`) The reason to keep it is covered by a section in the PDEP, and to summarize the reasons for our preference to keep the name `inplace`:

1) For those methods that would keep the keyword, the effect of it stays the same (except for the return value, see below, but that typically shouldn't have compatibility issues). So deprecating `inplace` here as well, and asking users to rename it to something else, feels as unnecessary code churn just for sake of renaming.
2) In the future, this group of methods will also be clearly "different", because they are the only ones that have an `inplace` keyword. 
3) Although there is of course a history to the name, the `inplace` keyword name as such still seems the most descriptive name for what it actually does in the future (we _only_ need a keyword for group 2 methods that can work values-inplace, so we don't need different keyword names for the different behaviours in the different groups)

There might be an argument that this code churn can be positive because people would be triggered to re-evaluate whether they actually need to keep the keyword (also for the method calls where it could stay), but since we are removing the keyword in more than half of the cases, I think people that use `inplace` will generally see the warning anyway, and will already be forced to think about this.


---

Further, there are currently two topics mentioned as "open questions" in the PDEP. I think for both of those we have a clear proposed answer nowadays, which I summarize below (but still have to update the PDEP text to make that clearer):

- **With `inplace=True`, should we silently copy or raise an error if the data has references?** (and requires to trigger a copy with CoW)
  -> Here we would propose to certainly not raise an error, as that would defeat the purpose and ease of use of the keyword (it's difficult to know in advance if this will happen or not). For now, we would propose to simply copy silently if needed (thus, `inplace=True` would mean "try inplace whenever possible", and not guarantee it is actually done inplace), and we can still add a warning about it later if there is demand for that.
- **Return the calling object (`self`) also when using `inplace=True`?** 
  -> Here, we propose to indeed return `self`, enabling the usage of the keyword in methods chains (and simplifying type annotations). We assume this will not give much problems in practice (you generally don't assert the current return value of `None`?)


---

Finally, the timeline as written in the current PDEP is a bit outdated, and we probably cannot realistically remove `inplace` on a short notice in 3.0. So a new timeline could look like:

- Start with adding a DeprecationWarning in the next release (2.2 if accepted before that)
- Somewhere during 3.x (eg in 3.1), change the DeprecationWarning to a FutureWarning
- Remove the keyword in 4.0


We still have to update the PDEP text to clarify the choice for those open questions, and to update the timeline.
Follow-up on my previous comment:

- Updated the text around the "open questions" as proposed above (methods with `inplace=True` will still silently copy if needed for CoW, and will return `self`, so there is a return value also for `inplace=True`)
- Updated the timeline as proposed above (I only added that we would already change the behaviour for methods that keep `inplace=True` in 3.0)
- Updated the abstract to reflect better the latest PDEP version
@jorisvandenbossche I have three comments on your proposal
1) the current effort to simplify and improve the consistency of the pandas API is fantastic. This proposal goes into the right direction IMHO
2) with `inplace=True` silently falling back to a copy is an issue when you manipulate large data. I would largely prefer an exception (e.g., `InplaceError`) when I develop my code as it will give me the opportunity to explore alternatives that will not have a performance issue (with a copy) and make a real in-place modification. 
3) returning `self` with `inplace=True` goes against the Python convention. In my opinion, it is hard to justify and hard to teach. In case you want to chain methods, I would largely prefer a `chain=True` keyword, that will change the behavior by returning `self`. 
> 2\. with `inplace=True` silently falling back to a copy is an issue when you manipulate large data. I would largely prefer an exception

If it were always clear whether an operation can be done inplace before hand, I wouldn't have too much opposition. However, I don't believe that's the case. I'd be in favor of a warning if we implement #55385.

> 3\. returning `self` with `inplace=True` goes against the Python convention.

Where is this convention written down?
> If it were always clear whether an operation can be done inplace before hand, I wouldn't have too much opposition. However, I don't believe that's the case. I'd be in favor of a warning if we implement https://github.com/pandas-dev/pandas/issues/55385.

I wasn't aware of #55385. An opt-in performance warning is by far the best option. However, if #55385 is not implemented, raising in case of a copy with `inplace=True` will be an improvement. Indeed, you are right that it was never clear wether an operation was done inplace, and my understanding is that CoW is the attempt to solve two issues: make the behavior consistent, make the performance predictable. Having an `inplace=True` that silently fall back to a copy will be a regression in that respect. 

>>      3. returning self with inplace=True goes against the Python convention.

> Where is this convention written down?

This behavior is explicitly described in the official documentation. See for instance https://docs.python.org/3/faq/design.html#why-doesn-t-list-sort-return-the-sorted-list
Also, I am not aware of any counter example in the builtins and stdlib.
> > Where is this convention written down?
> 
> This behavior is explicitly described in the official documentation. See for instance [docs.python.org/3/faq/design.html#why-doesn-t-list-sort-return-the-sorted-list](https://docs.python.org/3/faq/design.html#why-doesn-t-list-sort-return-the-sorted-list)

While I agree this is an argument for not returning an instance of `self`, I find calling this a "convention" far too strong. In the case of pandas, it seems clear to me the benefits of returning the instance - namely promoting method chaining - outweighs this drawback.

Thanks for the feedback, @arnaudlegout!

> 2\. with `inplace=True` silently falling back to a copy is an issue when you manipulate large data. I would largely prefer an exception (e.g., `InplaceError`) when I develop my code

I was also going to suggest whether a warning would be sufficient for this use case. 
Personally I think an error would be too annoying. Especially when developing, you might be testing code interactively, and when doing that you will easily trigger a copy (eg because there is a reference from checking some values interactively, like the IPython example that is included in the PDEP). Getting those exceptions while testing things interactively will become annoying IMO.

> While I agree this is an argument for not returning an instance of self, I find calling this a "convention" far too strong. In the case of pandas, it seems clear to me the benefits of returning the instance - namely promoting method chaining - outweighs this drawback.

My proposition was not to prevent method chaining, but to make it explicit (when `inplace=True`) with a method argument (e.g., `chain=True`). The behavior you propose does not exist in Python builtins and does not exist in numpy. Making it implicit might be a difficulty for new comers. As in-place modification is quite rare in the pandas API, I believe adding a method argument to make the chaining explicit is not an overkill. I understand that a drawback is that if you chain methods and forget the `chain=True` argument then you will introduce a bug. This drawback might be too important for the benefit.

Another option is to explain in the documentation of each method with an `inplace` arguement that `inplace=True` makes the change in place and the method will return self. May be this is enough. At least, I want to make clear that as the proposed behavior could be surprising there must be a clear documentation of the effect of `inplace=True`.
> Another option is to explain in the documentation of each method with an `inplace` arguement that `inplace=True` makes the change in place and the method will return self. May be this is enough. At least, I want to make clear that as the proposed behavior could be surprising there must be a clear documentation of the effect of `inplace=True`.

Yes, if we do this, we will certainly have to document this very well (personally, given this is not the default behaviour and so most users won't run into this, I think I am personally fine with the atypical behaviour of returning self)