REGR: revert behaviour change for concat with empty/all-NaN data
State: closed

Closes https://github.com/pandas-dev/pandas/issues/45637 
Closes https://github.com/pandas-dev/pandas/issues/47284 



Initially I tried it with a more selective revert of only the changes in  #43577 and #43507, but then I ran into some other failures in existing tests. So in the end tried it with reverting all subsequent clean-up PRs as well. I assume quite some of those changes could be re-applied after this, but for now just ensuring the tests are passing.

Labels:
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves
name: Missing-data
description: np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode

Comments:
The change we agreed upon reverting was for empty-object-dtype, not all-NA
> The change we agreed upon reverting was for empty-object-dtype, not all-NA

does #47284 fall under the all-NA case? (you have proposed an alternative fix for that one)
> The change we agreed upon reverting was for empty-object-dtype, not all-NA

In my head, the discussion has always been about both. While the reported issue itself was with an example using an empty dataframe, the discussion in https://github.com/pandas-dev/pandas/issues/45637 has been about both cases (quoting a question from you: "would you only ignore the empty/all-NaN cases when they are object/float64, respectively?", on which I answered yes). EDIT: https://github.com/pandas-dev/pandas/issues/46922 was also closed as a duplicate of https://github.com/pandas-dev/pandas/issues/45637, and was about the all-NA case. 
Unfortunately we don't have notes from the meeting in March where we discussed this, but at the time Simon concluded on the issue "We are agreed that we want to revert https://github.com/pandas-dev/pandas/pull/43507 for 1.4.x" (https://github.com/pandas-dev/pandas/issues/45637#issuecomment-1073263409). And the PR was about both empty and all-NA.

Both the empty and all-NA case are causing regressions, and both can be done with a deprecation instead if we want.
> does https://github.com/pandas-dev/pandas/issues/47284 fall under the all-NA case? (you have proposed an alternative fix for that one)

i believe so, yes
> Unfortunately we don't have notes from the meeting in March where we discussed this

We specifically discussed object-empty.  Performance considerations in avoiding object dtype were the main reason we agreed on allowing this value-dependent behavior.
> > does #47284 fall under the all-NA case? (you have proposed an alternative fix for that one)
> 
> i believe so, yes

so slightly off-topic for the PR here but the discussion in https://github.com/pandas-dev/pandas/issues/45637#issuecomment-1063180503 there was the suggestion that only the all-NA case with float64 dtype would be special cased which would not catch the pd.NA series with object dtype.
Thanks @jorisvandenbossche lgtm except failing [Data Manager](https://github.com/pandas-dev/pandas/runs/6938046231?check_suite_focus=true#logs) test and missing release note (probably needs more than a one-liner and leave out any mention of future behavior until agreed).

> Initially I tried it with a more selective revert of only the changes in #43577 and #43507, but then I ran into some other failures in existing tests. So in the end tried it with reverting all subsequent clean-up PRs as well. I assume quite some of those changes could be re-applied after this, but for now just ensuring the tests are passing.

I also tried to be selective in an attempt to do this https://github.com/pandas-dev/pandas/issues/45637#issuecomment-1073263409 and found it not so straightforward so agree that this is probably the best way of going about this.


> We specifically discussed object-empty. 

@jbrockmendel then your recollection of it is different than mine. In any case, I think the discussion in https://github.com/pandas-dev/pandas/issues/45637 in days leading up to the meeting was about both.

> Performance considerations in avoiding object dtype were the main reason we agreed on allowing this value-dependent behavior.

Also the all-NaN case can result in object dtype (for non-numerical dtypes).

> the discussion in https://github.com/pandas-dev/pandas/issues/45637#issuecomment-1063180503 there was the suggestion that only the all-NA case with float64 dtype would be special cased which would not catch the pd.NA series with object dtype.

@simonjayhawkins if you are talking about the third paragraph in that comment, then I think it is the other way around: I mentioned _eventually_ only ignoring object-dtype all-NA. But with emphasis on "eventually": that's what I think might be a good idea in the future (a next major release) if we have better defaults for empty data that no longer uses float64 dtype for this.

@jorisvandenbossche regardless of our differing recollections about what we _had_ consensus on, it is clear that _now_ we only have consensus on the empty-object case.

The other thing we agreed on was looking to achieve consistency between DataFrame/Series/Index/EA behavior, which this actively moves away from.

Note also: https://github.com/pandas-dev/pandas/blob/main/doc/source/whatsnew/v1.4.0.rst#ignoring-dtypes-in-concat-with-empty-or-all-na-columns

Please do not self-merge.
> Note also: https://github.com/pandas-dev/pandas/blob/main/doc/source/whatsnew/v1.4.0.rst#ignoring-dtypes-in-concat-with-empty-or-all-na-columns

in the past we have added an update to the previous release note (but only for the current released docs) now we have the version switcher for the docs, people maybe viewing older docs more.
> now we have the version switcher for the docs, people maybe viewing older docs more.

I think that should be fine, since we only show a single (the latest) version per release branch in the dropdown. So once 1.4.3 is released, the "1.4" docs would show the docs of that version, and the versions for 1.4.0-1.4.2 are not accessible from the dropdown.
> @jorisvandenbossche regardless of our differing recollections about what we _had_ consensus on, it is clear that _now_ we only have consensus on the empty-object case.

Both are behavior changes and not strictly bug fixes so as it's a grey area. I think in cases where it is not clearly a bug fix and there is disagreement we should not make the changes without deprecation.

So if we can agree that the status quo should have taken priority, rather than needing consensus, we need to get back to the old behavior and therefore have to revert both at this time?

> Both are behavior changes and not strictly bug fixes so as it's a grey area. I think in cases where it is not clearly a bug fix and there is disagreement we should not make the changes without deprecation.

@jreback and I agreed at the time to consider it a bugfix.  im not wild about the precedent of second-guessing that, especially after the release.

That said, this isn't a hill worth dying on.  Go ahead and we can re-fix it for 2.0.
> @jreback and I agreed at the time to consider it a bugfix. im not wild about the precedent of second-guessing that, especially after the release.

I understand but i'm sure I've seen @jorisvandenbossche mention somewhere that the behavior was intentional. So that implies to me it was not a bug.


AFAICT it was "intentional" in that it was needed to make DataFrame.append work bc DataFrame.append used to do a .reindex that was made unnecessary.  But again, not a hill.
This passed locally when resolving the conflicts. (another merge of main since then). So should be good to merge if green(ish) (typing failures are on other PRs)
The other failing tests were failing when #47377 was merged. so to make sure we don't miss anything will merge main once #47462 is merged.
Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

1. Checkout backport branch and update it.

```
git checkout 1.4.x
git pull
```

2. Cherry pick the first parent branch of the this PR on top of the older branch:
```
git cherry-pick -x -m1 d43d6e27d1af0b07df495e113e9818fc2f1909b1
```

3. You will likely have some merge/cherry-pick conflict here, fix them and commit:

```
git commit -am 'Backport PR #47372: REGR: revert behaviour change for concat with empty/all-NaN data'
```

4. Push to a named branch:

```
git push YOURFORK 1.4.x:auto-backport-of-pr-47372-on-1.4.x
```

5. Create a PR against branch 1.4.x, I would have named this PR:

> "Backport PR #47372 on branch 1.4.x (REGR: revert behaviour change for concat with empty/all-NaN data)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the `Still Needs Manual Backport` label once the PR gets merged.

If these instructions are inaccurate, feel free to [suggest an improvement](https://github.com/MeeseeksBox/MeeseeksDev).
                
Thanks @jorisvandenbossche