Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport PR #38737 on branch 1.2.x (BUG/REG: RollingGroupby MultiIndex levels dropped) #38784

Conversation

meeseeksmachine
Copy link
Contributor

Backport PR #38737: BUG/REG: RollingGroupby MultiIndex levels dropped

@lumberbot-app lumberbot-app bot added this to the 1.2.1 milestone Dec 29, 2020
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

Personally, I think we should first discuss this further before merging into 1.2.x, see #38737 (comment)

@jreback
Copy link
Contributor

jreback commented Dec 29, 2020

Personally, I think we should first discuss this further before merging into 1.2.x, see #38737 (comment)

ok. I actually think we should change this. I am also ok doing it for 1.2.x, but conversatively it might be better in 1.3

@jreback jreback added Groupby Window rolling, ewma, expanding labels Dec 29, 2020
@jorisvandenbossche
Copy link
Member

To be clear I am fine with doing this for 1.2.x, if we know this is the behaviour we actually want. But so we should probably first discuss, and based on that can still merge into 1.2.x.

@mroeschke
Copy link
Member

Started a new discussion in #38787.

Regardless, isn't it okay to still fix this perceived regression in 1.2.x and then possibly have a formal API behavior change in 1.3.0?

@jorisvandenbossche
Copy link
Member

So we changed behaviour between 1.0.5 and 1.1.5, which is now also in 1.2.0 (for certain cases). So it's actually already a regression in 1.1.5, I think, and not only 1.2.0?

Assume that we decide that we actually want to keep the behaviour from 1.1.5/1.2.0 long-term, then I don't think we should revert back to the 1.0.5 behaviour in 1.2.1, to then change again in 1.3.0 to the behaviour of 1.1.5/1.2.0 (but then we could rather say: "OK, we accidentally changed behaviour in 1.1.5, but we actually think it is better, so let's now leave it as is").

That's a big if on what behaviour we want long term, and it might well be that we also long term want to change back to the 1.0.5 behavior. In which case this PR should certainly be merged for 1.2.x. But I would still have the discussion first to avoid the potential back and forth change in that example scenario above.

@simonjayhawkins
Copy link
Member

@jorisvandenbossche not merging a backport is not ideal as causes the releases notes to be out of sync and the potential for merge conflicts preventing auto backports

I think we should consider one of the following as a preference to leaving this in limbo

  1. merging the backport, and can then maybe revert before release if necessary
  2. revert the PR to master, reopen, and only merge when ready
  3. close this PR, move the release notes on master, and then do a manual backport when ready.

@simonjayhawkins
Copy link
Member

added the blocker tag to #38787. so I think the best option is now to backport and revert if necessary.

will merge this shortly is no objections.

@jreback
Copy link
Contributor

jreback commented Dec 31, 2020

yep let's do it

@simonjayhawkins simonjayhawkins merged commit c70668c into pandas-dev:1.2.x Dec 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Groupby Window rolling, ewma, expanding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants