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

[BUG] Floor division behavior of integer by series with zeroes differs from Pandas #7389

Closed
brandon-b-miller opened this issue Feb 16, 2021 · 12 comments · Fixed by #12074
Closed
Assignees
Labels
bug Something isn't working Python Affects Python cuDF API.

Comments

@brandon-b-miller
Copy link
Contributor

Describe the bug
Dividing an int by a series or dataframe with zeroes in it yields a different result from Pandas nullable dtypes.

Steps/Code to reproduce bug

import pandas as pd
1 // pd.Series([0], dtype='Int64') # Nullable
0    0
dtype: Int64

vs

>>> 1 // cudf.Series([0])
0    9223372036854775807

Expected behavior
I'm not sure which result is preferred here. We could cast to float but that'd require a scan.

Environment overview (please complete the following information)

  • Environment location: Bare Metal
  • Method of cuDF install: Source
@brandon-b-miller brandon-b-miller added bug Something isn't working Needs Triage Need team to review and classify labels Feb 16, 2021
@jrhemstad
Copy link
Contributor

So Pandas expects division by zero to yield zero?

@brandon-b-miller
Copy link
Contributor Author

Not sure if it's a bug, something that just hasn't been implemented yet, or otherwise. Raised pandas-dev/pandas#39847 to ask. In our case maybe we want to raise.

@kkraus14 kkraus14 added Python Affects Python cuDF API. and removed Needs Triage Need team to review and classify labels Feb 16, 2021
@github-actions
Copy link

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

@vyasr
Copy link
Contributor

vyasr commented Jul 22, 2022

Based on discussion in #7492, we initially decided that the appropriate solution would be to match pandas by setting division by zero entries to zero in the output, but in subsequent discussions we decided to take no action. @brandon-b-miller at this point do you think it's worth revisiting this and doing the post facto 0 filling, or should we just close this issue?

@brandon-b-miller
Copy link
Contributor Author

brandon-b-miller commented Jul 25, 2022

As long as we're doing things like INT_POW could there be precedent to add this at the libcudf level?

@jrhemstad
Copy link
Contributor

As long as we're doing things like INT_POW could there be precedent to add this at the libcudf level?

INT_POW is different. It's a distinct algorithm for doing the "pow" operation.

"INT_DIVISION_BY_0_YIELDS_0" is far more niche.

If this behavior is desired, it should be a pre/post-processing step.

@vyasr
Copy link
Contributor

vyasr commented Jul 26, 2022

I agree, I'd still want to do this as a post-processing step. I think we'd probably be OK to eat the perf hit though.

@brandon-b-miller
Copy link
Contributor Author

So we're OK with doing a filter and I guess a scatter every time we do floor division by a cuDF series?

@vyasr
Copy link
Contributor

vyasr commented Aug 1, 2022

Yeah I think so. This feels like a very typical case where cuDF Python should pay a performance cost for matching pandas behavior exactly. No need to overspecialize libcudf.

@vyasr
Copy link
Contributor

vyasr commented Aug 2, 2022

This issue is pretty similar to #5938 in spirit, although it'll require a pretty different type of fix.

@mroeschke
Copy link
Contributor

mroeschke commented Aug 23, 2022

This appears to be a moving target on the pandas side

>>> import pandas as pd
>>> pd.__version__
'1.4.3'
>>> 1 // pd.Series([0], dtype='Int64')
0    0
dtype: Int64

vs

In [1]: pd.__version__
Out[1]: '1.6.0.dev0'  # essentially 1.5

In [2]: 1 // pd.Series([0], dtype='Int64')
Out[2]:
0    inf
dtype: Float64

FWIW I do find the 0 result from pandas a bit odd. And given discussion on the pandas side related to this (pandas-dev/pandas#30188 and pandas-dev/pandas#32265) maybe in this case it may make sense for cuDF to return a more sensible result (e.g. not 0)?

@wence-
Copy link
Contributor

wence- commented Nov 8, 2022

At least as of #12074, cudf will match pandas 1.5 AFAICT in these cases.

rapids-bot bot pushed a commit that referenced this issue Nov 16, 2022
The type normalisation applied before heading into libcudf previously had slightly unexpected consequences for large int64 values. If not providing a `cudf.Scalar`, a bare `int64` scalar would be cast to `uint64` and then normal numpy type promotion would unify to `float64`. This is lossy, since int64 to float64 is neither surjective nor injective.

To avoid this, try very hard to maintain the dtype of the object coming in, and match pandas behaviour by applying numpy type promotion rules via `numpy.result_type`. 

- Closes #5938
- Closes #7389
- Closes #12072
- Closes #12092

Authors:
  - Lawrence Mitchell (https://github.com/wence-)

Approvers:
  - GALI PREM SAGAR (https://github.com/galipremsagar)

URL: #12074
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Python Affects Python cuDF API.
Projects
None yet
6 participants