Refactor groupby helper from tempita to fused types

[noamher]

Refactoring groupby_helper to use fused types instead of tempita for functions such as group_add.

@jbrockmendel if this is what you meant in the previous PR I'll do the refactoring for the rest of the functions in groupby_helper.pxi.in.

[jreback]

we likely have this type elsewhere

[jbrockmendel]

cython has a builtin floating fused type for float+double, better to re-use that as long as we're OK with declaring that float==float32_t and double==float64_t

[noamher]

As you said, floating is a fused type for float + double.
From what I saw in the cython code here and here , float is np.float32_t and double is np.float64_t.

So the fused type I created, floating, can be replaced with cython's floating.
It is possible that sometime in the future cython will use different definitions for np.float32_t and np.float64_t. If that is a concern, we can stick with defining our own floating fused type.

[noamher]

@jbrockmendel @WillAyd @jreback I made a change that the group_add function is not looked up by type (e.g. grou_add_float64) but rather directly by name.

It seems that after the change, when group_add is used with a ndarray[int64], the ndarray is first cast into ndarray[float64], then passed into group_add[float64], then cast back into ndarray[int64].

This is different then before the change (which passed the tests), because the cast back into ndarray[int64] did not occur so the result was an ndarray[float64].

See here.

What do you think is the best solution? keep the group_add_{type} = group_add_floating statements?.

[jbrockmendel]

[...] ndarray is first cast into ndarray[float64] [...]

This is definitely not desirable. Any guesses why this might be happening?

What do you think is the best solution? keep the group_add_{type} = group_add_floating statements?

The best case would be to just have group_add and when we call it from outside cython, cython will figure out what its supposed to do (correctly, without doing an unnecessary casting). In my experience, doing this lets us get rid of a bunch of ugly non-cython code.

Next best would be to keep group_add_dtype functions. In these cases, I wouldn't suggest group_add_{type} = group_add_floating but more specifically group_add_{type} = group_add_floating[{type}]. And if feasible, I'd prefer group_add[{type}] over group_add_floating[{type}].

Is this helpful?

[WillAyd]

@noamher generally we want to try and simplify the calls we are making from the groupby module back to the Cython layer, so I'd be a little hesitant to add more logic therein.

Might be missing the point but can you provide a specific example of the issue you are trying to solve? Certain types simply not resolving to the function correctly?

[noamher]

Just to be clear, the specialization works. I preferred to use the actual types rather than string thouh but couldn't make it work.

I guess the cyhton layer can do the cast into float and back into int.
I'll try to understand what happens to the nans when the output is cast back into floats.

[jbrockmendel]

I preferred to use the actual types rather than string thouh but couldn't make it work.

Yah I've found this tricky too. Don't sweat it.

I've looked into keeping the name group_add without the group_add_{type} members (i.e. changing things at the caller side).

Agreed with @WillAyd, definitely don't want to add more complexity on the python side. Removing the group_add_{type}s would be nice, but if we have to do that in a follow-up that's OK too.

[noamher]

Used cython.floating instead of creating a fused type, is that okay?

[noamher]

@jreback green now.

[codecov]

Codecov Report

Merging #24954 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #24954      +/-   ##
==========================================
+ Coverage   92.37%   92.37%   +<.01%     
==========================================
  Files         166      166              
  Lines       52408    52408              
==========================================
+ Hits        48411    48412       +1     
+ Misses       3997     3996       -1

Flag	Coverage Δ
#multiple	90.79% <ø> (ø)	⬆️
#single	42.86% <ø> (-0.01%)	⬇️

Impacted Files	Coverage Δ
pandas/core/groupby/ops.py	96.72% <ø> (ø)	⬆️
pandas/util/testing.py	88.13% <0%> (+0.09%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3c9d6e...937e44c. Read the comment docs.

[codecov]

Codecov Report

Merging #24954 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #24954      +/-   ##
==========================================
+ Coverage   92.38%   92.38%   +<.01%     
==========================================
  Files         166      166              
  Lines       52398    52398              
==========================================
+ Hits        48406    48407       +1     
+ Misses       3992     3991       -1

Flag	Coverage Δ
#multiple	90.8% <ø> (ø)	⬆️
#single	42.88% <ø> (-0.01%)	⬇️

Impacted Files	Coverage Δ
pandas/core/groupby/ops.py	96.72% <ø> (ø)	⬆️
pandas/util/testing.py	88.13% <0%> (+0.09%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95f8dca...fedab7f. Read the comment docs.

[WillAyd]

So do we even need this object fallback loop?

[noamher]

not for group_add and everything worked fine until now.
still it seemed like a bug.

[jbrockmendel]

group_add_float32 = group_add_floating[float32_t]
group_add_float64 = group_add_floating[float64_t]

or even better, track down the places where these are used and call the non-specialized one directly.

Probably also shorten the name to just group_add

[noamher]

After shortening the name to group_add, it's not required to define group_add_float32 and group_add_float64. The function is found by its name ("group_add").

[jreback]

seems ok, ping when green.

[WillAyd]

Do we still need these? Thought from a previous comment that they weren't required

Side note - I see you've been force pushing changes. If you merge master you can push without forcing, which typically does a better job of maintaining comment history

[noamher]

Yes, they are still needed (to avoid changing the code calling this function). atleast until some logic to handle int64_t differently is added to the _group_add function.

I force push to keep only 1 commit (avoid the bloat).
I can add commits and squash before merging, do you think that's better?

[WillAyd]

Unrelated to this PR but I see there are some functions at the end of the template file which have been converted to fused types but still remain in the template file. Could make sense to move them here as well (separate PR)

[noamher]

I'll take a look at that (for next PR). thanks.

[noamher]

@WillAyd which functions did you find that are unnecessary? I couldn't find any.

[jreback]

small comment, ping on green.

[jreback]

just call this group_add

[noamher]

@jreback

I can't. Because then it will be found and used instead of the typed version.
Which will cause problems for a rather odd case where int64 is the type and the function is supposed to not be found at first, then found after the ndarray is cast into float64.

[noamher]

Any name but that will work.

[jreback]

I can't. Because then it will be found and used instead of the typed version.
Which will cause problems for a rather odd case where int64 is the type and the function is supposed to not be found at first, then found after the ndarray is cast into float64.

not sure I understand we do this for all other functions.

[noamher]

@jreback my plan is to do this over a few pr to all functions in groupy_helper.pxi.in.

I can move some other function in this PR but I think it may be easier to do it in another pr.
Alternatively I can close this PR and concentrate some more changes into a new PR.

[noamher]

I can't. Because then it will be found and used instead of the typed version.
Which will cause problems for a rather odd case where int64 is the type and the function is supposed to not be found at first, then found after the ndarray is cast into float64.

not sure I understand we do this for all other functions.

@jreback I already have the next PR ready for the rest of the functions (group_prod, group_var, group_mean, group_ohlc).
I had to do the same thing (e.g. creating the group_prod_float64) for them as well for the same reason.

Description of the issue:

The issue is that the calling mechanism, first looks up by funcname, then by funcname_{{type}}.
The code the calls the calling mechanism has a fallback so that if an group_add is looked up with type int64, then the fallback is to cast into the input ndarray into float64, then lookup group_add with type float64.

So group_add is eventually called with an ndarray of type float64 and returns a result which is an ndarray of float64. Now, before returning the result, the fallback code casts the result back into int64. Then returns the result.

Implications:

Creating a group_add function that will be looked-up directly (not using type) means that when the lookup type is int64, the method will still be found (without the type in the name anyway). Calling it will succeed because of an implicit cast from int64 to float64.
Because the fallback code was never needed, there will be no cast back into int64.
This will fail tests and change the behavior of whatever's calling it in the first place.

To avoid this implications, group_add_{{type}} are defined the the function group_add is renamed into something that will fail the first lookup (e.g. _group_add).

I know that's not the prettiest solution but changing the calling mechanism is something I really tried to avoid because it seems a bit sensitive code to change without breaking anything.

[noamher]

I'll take a look at that (for next PR). thanks.

[noamher]

@WillAyd which functions did you find that are unnecessary? I couldn't find any.

[noamher]

Yes, they are still needed (to avoid changing the code calling this function). atleast until some logic to handle int64_t differently is added to the _group_add function.

I force push to keep only 1 commit (avoid the bloat).
I can add commits and squash before merging, do you think that's better?

[jreback]

@noamher my concern here is the naming consistency. IIRC @jbrockmendel did things like this a while back. Is this pattern consistent with other fused types are callled in groupby & algos?

[noamher]

@jreback
It will be consistent when I make the changes to the other functions.

Currently (as if this PR), only group_add exist, because the other functions (e.g. group_mean) only exist in their specialized form (e.g. group_mean_float64) which is created by tempita.

What other functions should group_add be consistent with?

[jreback]

@noamher ok thanks. if you have followups to keep consistentcy would be good.