Issue to track the votes for PDEP-8: Inplace methods in pandas
Pull request with the PDEP discussion: https://github.com/pandas-dev/pandas/pull/51466
Rendered PDEP for easy reading: https://github.com/pandas-dev/pandas/blob/2654fe9ba91dd44ed66f0c246d4ed0c5dde7e391/web/pandas/pdeps/0008-inplace-methods-in-pandas.md (or a website preview at https://jorisvandenbossche.github.io/pandas-website-preview/pdeps/0008-inplace-methods-in-pandas.html)
Cast your vote in a comment below. * +1: approve. * 0: abstain. * Reason: A one sentence reason is required. * -1: disapprove * Reason: A one sentence reason is required. - A disapprove vote requires prior participation in the linked issues/PRs.
Voting will close in 15 days, i.e., on December 29, 2023.
@pandas-dev/pandas-core
Comment From: jorisvandenbossche
And starting with my own +1 (so I am not forgotten in the count ;))
Comment From: phofl
+1 :)
Comment From: rhshadrach
+1
Comment From: mroeschke
+1
Comment From: WillAyd
+1
Comment From: jreback
group 2 inplace returning self when did this change?
i was ok with the proposal of leaving inplace in group2 (even though we should just kill it entirely ) until i saw this
Comment From: jorisvandenbossche
group 2 inplace returning self, when did this change?
It was mentioned as an "open question" from the beginning (with one of the options to return self), but updated to just propose that option of returning self more recently. I summarized the changes I pushed to the proposal on the PR: https://github.com/pandas-dev/pandas/pull/51466#issuecomment-1782982457 mentioning the open questions and that I was planning to update the text about returning self, and then https://github.com/pandas-dev/pandas/pull/51466#issuecomment-1808158093 stating that I had updated the text that way. Further, it was also mentioned in the summary of the PDEP I sent to the pandas-dev mailing a month ago.
Let's leave any further discussion on the PR itself -> https://github.com/pandas-dev/pandas/pull/51466
Comment From: Dr-Irv
+1
Comment From: jbrockmendel
+1
Comment From: attack68
+1
Comment From: lukemanley
+1
Comment From: fangchenli
+1
Comment From: lithomas1
+1
Comment From: jreback
-1 i'll repeat what i stated on the PR (which has had no response)
returning self on an inplace method is a terrible idea which has happened before - see #1893 - it took years to reverse this
Having these return self also now adds a HUGE amount of complexity to the api. you have the standard inplace methods, such as insert and .loc which return None, now you are adding a different case for these inplace methods. this also differs from the standard library where inplace methods return None.
tbh I would either:
remove all inplace everywhere; the supposed performance benefefits of having 4 methods with inplace is dubious when compared to the added complexity
if you insist on the above, then reanme these to ffill_inplace and so on (to avoid the typing issue); don't love this, these still should return None
Comment From: twoertwein
+1
But I think type annotations should not be the motivation for returning self for inplace=True: yes, it would simplify the type annotations, but keyword-only+overloads can handle this complexity. (off-topic: if the goal is to make pandas typing friendlier, avoiding @inherit_names would be more important)
Comment From: simonjayhawkins
using Quorum definition from #53576 of the 11 voting members and the voting period of 15 days then under the not yet ratified voting process then PDEP-8 should have been accepted.
I'm not suggesting for 1 minute that we dismiss @jreback concerns just to fit in with the draft process. But the purpose of the PDEP process is to facilitate the decision process and avoid discussions going stale.
Once the voting period ends, any voter may tally the votes in a comment, using the format: w-x-y-z, where w stands for the total of approving, x of abstaining, z of disapproving votes cast, and z of number of voting members who did not respond to the VOTE issue. The tally of the votes will state if a quorum has been reached or not.
12 approve (technically 1 approval outside voting window) 0 abstain 1 disapprove
can I suggest that we close this vote. potentially make some changes to the proposal that are acceptable to @jreback and then perhaps start the vote again.
Until we have all agreed on the voting process then we need to follow the governance in existence.
Comment From: jreback
I actually just saw @simonjayhawkins comments. This PDEP should not be rejected just because I put a -1 out here. That is the entire point of this process. I'll re-iterate what I was trying to achieve here.
I am fully +1 on removing inplace everywhere if possible
I don't really love the fact that we have inplace on a small (4) subset of methods but could live with that.
I don't think the return value should change on those methods, again If the group thinks this is fine, then ok.
So I would propose to either: - push the PDEP thru as is - cleave the 4 methods that are slightly controvertial and execute on the rest of the PDEP.
cc @jorisvandenbossche
Comment From: simonjayhawkins
This PDEP should not be rejected just because I put a -1 out here.
agree.
my thinking was that the PDEP is supposed to be a consensus seeking process and so if we go to the vote and there are objections, these objections should have been discussed thoroughly and the outcome basically agreed in the PDEP discussion. This is my understanding of why -1 votes should have a one-liner with the reason and have participated in the discussion so that there is no surprises.
from the PDEP... "Our workflow was created to support and enable a consensus seeking process"
IMHO it is the process that has failed here.
- cleave the 4 methods that are slightly controvertial and execute on the rest of the PDEP.
My comments were to reinvigorate a discussion/vote that had gone stale so that the PDEP could be accepted. I was expecting that this outcome was the probable solution to getting the bulk of the PDEP accepted as the "offending paragraph" could immediately be included in a PDEP revision in a follow up.
Comment From: jorisvandenbossche
First, apologies for the awfully slow response here. Both in not timely closing the vote (which might have avoided some of the discussion above), as not yet responding to the actual concerns around the vote and not moving forward this PDEP.
While actively working on the 2.3 / 3.0 releases the last weeks, I was thinking it would still be good to at least start with the deprecation in 3.0 (so people might still stop using inplace while updating for CoW).
[@simonjayhawkins] using Quorum definition from #53576 of the 11 voting members and the voting period of 15 days then under the not yet ratified voting process then PDEP-8 should have been accepted.
My proposal would be to still follow those rules and mark the PDEP as accepted, see below.
if we go to the vote and there are objections, these objections should have been discussed thoroughly and the outcome basically agreed in the PDEP discussion.
The objection (i.e returning self for the few cases that keep the keyword) had already been discussed before the vote as well (I explicitly mentioned this update at https://github.com/pandas-dev/pandas/pull/51466#issuecomment-1782982457, which triggered some discussion about it in the comments below that), and the topic (pros/cons) is included in the PDEP (see the 'Return the calling object (self) also when using inplace=True?" section in the PDEP text).
In the end, this is a (subjective) trade-off between the pros/cons, and then the goal of the PDEP process is to make that explicit, acknowledge the discussion and describe that in the PDEP text, if there is no full consensus.
(Jeff also already had mentioned that objection earlier in the PDEP discussion, so it was not a "surprise")
[@jreback] This PDEP should not be rejected just because I put a -1 out here. That is the entire point of this process. .... So I would propose to either:
- push the PDEP thru as is
- cleave the 4 methods that are slightly controvertial and execute on the rest of the PDEP.
My preference would be to go for the first option.
The second option (leaving out those 4 methods that would keep an inplace keyword) essentially means keeping the status quo for those 4 methods, i.e. having an inplace keyword but not starting to return self (the PDEP also proposes to let those methods keep the keyword, so the only difference with fully executing the PDEP is the return value). Given that this was a clear part of the proposal to return self (it's mentioned in the abstract) when we voted on it, I would prefer to just follow the existing vote.
In summary, I would propose to mark the current PDEP as accepted. Concerns in doing that?
Comment From: WillAyd
No concerns from my end. Dissents are valid and noted, but if we reached the votes required by the PDEP process then let's move forward
Comment From: rhshadrach
In summary, I would propose to mark the current PDEP as accepted.
+1
Comment From: jbrockmendel
Closable?
Comment From: mroeschke
I think we just need a PR to amend PDEP-8 as accepted to close this voting issue