BUG: incorrect EA casting in groubpy.agg

[jbrockmendel]

• closes #xxxx
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

The existing code is implicitly assuming that _from_sequence is strict, so that maybe_cast_to_extension_array will only return an EA when correct. Since that assumption is untrue, the current code will return incorrect results, as in the test this adds.

If we just removed L729-L741 in groupby.ops, we would have 12ish test failures. A few of those would be for float64 result failing to cast back to Float64, and the rest would be for ndarray[Decimal objects] failing to cast back to DecimalArray. Until _from_sequence is reliable, I would rather remove these few lines and return correct-but-suboptimally-casted results than have these kludges.

cc @jorisvandenbossche

[jreback]

why are you doing this here, rather than inside maybe_cast_results itself?

[jbrockmendel]

Because I'm trying to get rid of maybe_cast_result altogether. It uses maybe_cast_to_extension_array whereas we should be casting yoda-style (do or do not)

[jbrockmendel]

(as mentioned in the OP, id actually rather remove this chunk of code entirely)

[jreback]

sure but I'd rather NOT do it in groupby at all (any casting like this), and instead push it to dtypes/cast.py this seems like going backwards.

[jbrockmendel]

so you're good with just ripping this out?

[jbrockmendel]

and instead push it to dtypes/cast.py this seems like going backwards.

depends on how we think of dtypes.cast. I think of it as "low-level helper functions related to casting" NOT "anything related to casting". I don't want to put groupby-specific casting code in there. (i also dont like having DataFrame.reset_index code in there)

[jreback]

and instead push it to dtypes/cast.py this seems like going backwards.

depends on how we think of dtypes.cast. I think of it as "low-level helper functions related to casting" NOT "anything related to casting". I don't want to put groupby-specific casting code in there. (i also dont like having DataFrame.reset_index code in there)

i don't think virtually any casting code should be in groupby / frame. but we have to put it somewhere (and of course ideally there isn't any special casing on the class of the parent container).

so i think its the lesser of evils to keep it all together in dtypes/cast.py potentially allowing for re-use / refactoring.

[jbrockmendel]

updated to remove the incorrect casting without special-casing pandas-internal EAs

[jorisvandenbossche]

@jbrockmendel Can you show an example of what currently gives a wrong output?
(looking at the diff it seems something with Period? )

[jreback]

ok code / tests look ok, can you add a whatsnew note in the EA section (or groupby)

[jorisvandenbossche]

Needs further discussion first

[jbrockmendel]

Can you show an example of what currently gives a wrong output?
(looking at the diff it seems something with Period? )

dti = pd.date_range("2012-01-01", periods=4)
pi = dti.to_period("D")

df = DataFrame({"a": [0, 0, 1, 1], "b": pi})
gb = df.groupby("a")

>>> gb["b"].agg(lambda x: x.iloc[0].year)
a
0    2012-01-01
1    2012-01-01
Name: b, dtype: period[D]

[jorisvandenbossche]

I would personally focus on fixing the actual cause (stricter way to cast back), instead of doing this "fix" (because it's fixing period, but at a cost of causing regressions for other dtypes)

[jbrockmendel]

(because it's fixing period, but at a cost of causing regressions for other dtypes)

how much of your opinion is based on IntegerArray/FloatingArray/BooleanArray? We could special-case those similar to what we do in _ea_wrap_cython_operation.

For general EAs (and DecimalArray) I think "sometimes giving completely incorrect results" is a much bigger problem than "sometimes giving correct results in a sub-optimal container"

[jorisvandenbossche]

how much of your opinion is based on IntegerArray/FloatingArray/BooleanArray?

Yes, quite a bit ;). Also based on geopandas, but our _from_sequence is already pretty strict there anyway, so we don't really have this problem.

Now, I don't think this is a critical bug fix for 1.2, so assuming it's for 1.3 (I think the rc will be cut any time now), that gives us more time to fix this, in which case I would prefer to focus on getting #38315 done.
(if we do that, will move your period test there to ensure that is fixed)

We could also still merge this, and revert most of the test changes in #38315, but not sure what buys us that (apart from a bigger diff in #38315)

[jbrockmendel]

Yes, quite a bit ;).

I'd be OK with re-implementing part of an earlier commit that special-cases those dtypes. That would fix the Period-like bug and only change the Decimal-like cases, which are relatively straightforward conceptually. Would you be OK with that?

[jorisvandenbossche]

Is this the correct expected result? When subtracting Periods, you get offset objects, not Timedelta objects?

[jbrockmendel]

good point. probably not great that tm.assert_series_equal can't tell the difference either

[jorisvandenbossche]

That would fix the Period-like bug and only change the Decimal-like cases, which are relatively straightforward conceptually. Would you be OK with that?

I still don't fully understand what would buy us that, since #38315 is also fixing the Period-like bug, but without regressing on the decimal case. So I would just need to revert most of this PR in #38315 (except the period test case, which I can add there), making the diff there larger.
Your input on the different questions I laid out in #38315 is very welcome, to ensure we can move it forward.

[jbrockmendel]

since #38315 is also fixing the Period-like bug, but without regressing on the decimal case

#38315 is very ambitious and potentially addressing multiple intertwined issues. I'm not ready to consider it a "solved problem".

[jorisvandenbossche]

I'm not ready to consider it a "solved problem".

But this PR is also not "solving the problem" ..
It's IMO rather complicating the situation by incorrecting part of the tests. I am fine if the PR would just focus on fixing the Period case though, then it already fixes this specific case and adds a test, making #38315 actually easier instead of more complex.

[jorisvandenbossche]

Inside maybe_cast_result, we already have a special case check for not casting back when having datetime or categorical data. You could add periods to that check, and then this line can be left as is, while still fixing the period issue.
Then we can add the period test, without needing to change the decimal tests.

[jbrockmendel]

Inside maybe_cast_result, we already have a special case check for not casting back when having datetime or categorical data.

That's actually what I find most objectionable about maybe_cast_result. It is a kludge that special-cases pandas-internal EAs.

[jorisvandenbossche]

But as I said below, it would at least be a consistent "kludge"... ;)
Not calling the method here that was exactly written to be called here, it not making it any less kludgy IMO.

So I think it would be good to add the period test case here, and then focus on #38315 to properly fix it.

[jbrockmendel]

not making it any less kludgy

This is the only remaining usage of that kludge, so getting rid of it would certainly make it less kludgy.

and then focus on #38315 to properly fix it.

It's going to be a while before 1.3, so I'm fine sticking a pin in this to see if a better solution is implemented between now and then. If it doesn't, we should do this for 1.3.

[jorisvandenbossche]

I think updating maybe_cast_result to special case periods as well is actually a nice short term solution to get this fix in (we already do that for other dtypes as well, so while it is not the nicest thing to do (but one of the goals of #38315 will be to clean that up), it's at least consistent ..).