-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
NumPy ufunc implementation on Expr causes potential performance pitfall #15873
Comments
@alexander-beedie I'm not sure what the best way to go is here - do you have an idea on how to solve this issue? |
Hmm... Best way to start is probably to do a survey over the ufuncs and see how many are missing native polars equivalents? I suspect the number isn't too unmanageable, in which case we might want to add some of the more reasonable missing ones as native expressions. That would then give us a decent basis for disabling expression-based ufuncs (I don't really have a sense of how widely used they are) 🤔 We could add identification of the low-hanging fruit --such as |
I don't think it'd be an elementwise
It dispatches to numpy's addition instead of polars but it's still vectorized. I don't think there's a big performance hit there Consider
and just for good measure
|
For this use case I think you only want to take back the symbolic operators rather than worrying about all available np ufuncs. I think that can be done with the following additions to
There ought to be more checks for robustness but the idea is that if @stinodego I think the reason you said it was elementwise was because of this flag polars/py-polars/polars/expr/expr.py Line 333 in c95e41f
but that flag is just so the optimizer knows it's safe to run in streaming. It doesn't signal to run the operation one element at a time. Am I guessing right? Anyway, let me know what you guys think. |
The problem
Consider the following code example:
The first expression works as expected: it enters into
Expr.__add__
, converts the numpy float to a literal expression, and combines the two into a native Polars expression.The second expression is problematic: it enters into the
__add__
method of the numpy object, then calls the__array_ufunc__
implementation of the Expr object. This results in an elementwisemap_batches
call, which is much slower!Potential solutions
If we remove the ufunc implementation on Expr completely, the problem is solved as the code will now go through
Expr.__radd__
rather than the ufunc code.If we want to keep the ufunc code, we must detect inputs that can be converted to native Polars expressions. There are quite a few valid ufuncs though:
https://numpy.org/doc/stable/reference/ufuncs.html#available-ufuncs
The text was updated successfully, but these errors were encountered: