REGR: fix case all-NaN/numeric object column in groupby

[jorisvandenbossche]

Fixes #39329

This basically reverts #35841, although the code itself has changed quite a bit since then, so it's simpler here as the original PR² (not dealing with blocks anymore)

The original code comment that was removed in #35841 "We've split an object block!" was correct, as it can actually happen in those example cases.

[jreback]

will have a look but let's leave for 1.2.3

[jreback]

instead of this can you now just define as_array properly?

[jreback]

of simply consolidate first?

[jorisvandenbossche]

It's already consolidated above, but if you have multiple dtypes, that doesn't help

[jorisvandenbossche]

To be clear, as the diff seems to be larger (because I remove the assert above and moved the comment), but basically the only code addition is this:

            if len(mgr.blocks) != 1:
                return mgr.as_array()

to handle the case of multiple blocks that need to be converted into an array (while now this raised an AssertionError because of assert len(mgr.blocks) == 1

[jorisvandenbossche]

This is a quite straightforward revert of #35841, a clean-up PR in which was said "we need to identify a test case in which this doesnt work". And so I have added a few tests that actually do result in multiple bocks in that code path.
Also, you can only end up in the new code for cases that would otherwise have led to an AssertionError, so I think this is safe to include for 1.2.2

[jreback]

we had this discussion before
about late PRs

appreciate them but it's too late
and 1.2.3 is appropriate

[jorisvandenbossche]

@simonjayhawkins is not releasing today, so there is still a bit of time. Other PRs have been merged today as well. If it's not ready by tomorrow, then fine that it's for 1.2.3, but until then this can be assumed for 1.2.2 IMO (not as a blocker, but as "include if merged by releasing").

[jorisvandenbossche]

cc @jbrockmendel the tests added here are example cases of how you can end up with multiple blocks in this code path. Basically if you have multiple object dtype columns (a single block originally), but some column is entirely numeric, after the aggregation we infer the numeric dtype, and so you can end up with columns of different dtypes (and thus multiple blocks).

[jbrockmendel]

does the consolidation status of df matter here? does it matter that df is not constructed directly?

[jorisvandenbossche]

It doesn't matter in this case. The dataframe that gets resampled (after the first groupby) seems to be consolidated anyway (I assume that creating the sub-dataframe to resample might incur consolidation?). But parametrized the test to cover both cases to be sure.

[jbrockmendel]

but some column is entirely numeric

lets make sure to add this to the running list fo value-dependent behaviors

[jorisvandenbossche]

Since multiple people reviewed this, I think this can be merged?

(and to repeat: this has no impact whatsoever on code that already worked before)

[simonjayhawkins]

Thanks @jorisvandenbossche

[jreback]

thanks @jorisvandenbossche

[jreback]

@meeseeksdev backport 1.2.x

[jreback]

cc @simonjayhawkins @jorisvandenbossche for manual backport

[jorisvandenbossche]

but some column is entirely numeric

lets make sure to add this to the running list fo value-dependent behaviors

@jbrockmendel I think this is more a case of "we liberally try to infer object dtype" (although of course those are strictly also a kind of value dependent behaviour, but for object dtype we have plenty of that)

[simonjayhawkins]

@jorisvandenbossche the conflict is

<<<<<<< HEAD
            assert len(result._mgr.blocks) == 1

            # unwrap DataFrame to get array
            result = result._mgr.blocks[0].values
            return result
=======
            mgr = result._mgr
            assert isinstance(mgr, BlockManager)

            # unwrap DataFrame to get array
            if len(mgr.blocks) != 1:
                # We've split an object block! Everything we've assumed
                # about a single block input returning a single block output
                # is a lie. See eg GH-39329
                return mgr.as_array()
            else:
                result = mgr.blocks[0].values
                return result
>>>>>>> e58a193408... REGR: fix case all-NaN/numeric object column in groupby  (#39655)

are you happy to accept the incoming changes here. will also include changes from #36010, specifically the assert isinstance(mgr, BlockManager)

[jorisvandenbossche]

@simonjayhawkins I don't directly see the difference with the diff of this PR, the above seems exactly what I changed, so accepting the incoming changes in that conflict should be fine

[simonjayhawkins]

I don't directly see the difference with the diff of this PR

should be more visible on #39677, on 1.2.x we don't have the mgr = result._mgr, but I think this is worth having to avoid the duplication, but also comes with an extra assert that was not on 1.2.x