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

REF: only fallback to masked op for object dtype #40728

Merged

Conversation

jorisvandenbossche
Copy link
Member

@jorisvandenbossche jorisvandenbossche commented Apr 1, 2021

While investigating #40727, I noticed that we are often trying _masked_arith_op, while it will end up giving the exact same error (for example when using ints, which don't have NaNs).
From what I understand, I think that, in theory, we only need to fallback to a masked version of the op when having object dtype (I might be wrong here, though). So this PR tries that.

For numeric dtypes, all operations should simply work with NaNs?
For object dtype, we can have NaN or None, which doesn't necessarily work out of the box, so here we need to fallback to a masked operation.
ExtensionArrays don't end up here, and datetimelike dtypes get wrapped into EAs before getting here.

@jorisvandenbossche jorisvandenbossche added Numeric Operations Arithmetic, Comparison, and Logical operations Refactor Internal refactoring of code labels Apr 1, 2021
local_dict={"a_value": a_value, "b_value": b_value},
casting="safe",
)
except TypeError:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catching the error here, instead of higher op in _na_arithmetic_op, ensures that we try again with the plain standard op instead of directly try again with _masked_arith_op

@jbrockmendel
Copy link
Member

jbrockmendel commented Apr 1, 2021

just ran all the tests asserting that we only get here with object dtype, and found that the failures were floordiv/rfloordiv (handled by #40727) and operator.pow (note: not the same object as pow) with integer dtype. does this handle the latter case?

@jorisvandenbossche
Copy link
Member Author

See my inline comment at https://github.com/pandas-dev/pandas/pull/40728/files#r605632277, that's handling the integer pow case (by catching that error within expressions.py, so it falls back to _evaluate_standard (the plain op) within expressions.py, and not to _masked_arith_op in array_ops.py)

@jorisvandenbossche
Copy link
Member Author

Now, there are still some failures in test_expressions.py that I need to look at (but since those are testing directly expressions.py, it might also be that I need to update tests)

@jbrockmendel
Copy link
Member

sounds good. also looks like with this we can get rid of the last usage of maybe_upcast_putmask

@jorisvandenbossche
Copy link
Member Author

It was actually a bug in my change in the reversing of the operands

@jreback jreback added this to the 1.3 milestone Apr 2, 2021
@jreback jreback merged commit ae8849c into pandas-dev:master Apr 2, 2021
@jreback
Copy link
Contributor

jreback commented Apr 2, 2021

thanks @jorisvandenbossche

@jorisvandenbossche jorisvandenbossche deleted the ops-refactor-masked-fallback branch April 2, 2021 15:12
vladu pushed a commit to vladu/pandas that referenced this pull request Apr 5, 2021
JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request Jul 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Numeric Operations Arithmetic, Comparison, and Logical operations Refactor Internal refactoring of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants