Support Unary Operations in Masked UDF #9409
Conversation
isVoid
added
feature request
Codecov Report
@@ Coverage Diff @@
## branch-21.12 #9409 +/- ##
================================================
+ Coverage 10.79% 11.05% +0.26%
================================================
Files 116 117 +1
Lines 18869 20250 +1381
================================================
+ Hits 2036 2239 +203
- Misses 16833 18011 +1178
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@isVoid @brandon-b-miller Thanks for tagging me on this! It's a really interesting part of the code. I have a few comments/questions.
Co-authored-by: Bradley Dice <[email protected]>
There was a problem hiding this comment.
This looks good on the whole - it looks like you've got to grips with extending UDF support with relative ease! There are a couple of comments on the diff on expanding the set of supported functions and tightening up the typing a bit, but I don't see anything major that needs changing.
| @@ -10,6 +11,26 @@ | |||
| operator.pow, | |||
| ] | |||
|
|
|||
| unary_ops = [ | |||
There was a problem hiding this comment.
All the unary math ops supported by the CUDA target can be found in Numba's cudamath.py, starting at this line: https://github.com/numba/numba/blob/master/numba/cuda/cudamath.py#L10 - it may be worth adding the complete set?
There was a problem hiding this comment.
I was able to get many unary ops in, except math.trunc. It's suggesting
NotImplementedError: No definition for lowering <built-in function trunc>(float64,) -> float64
I believe it should should've gotten trunc(float64)->int64, maybe something is not registered correctly?
There was a problem hiding this comment.
Besides math.log appears to be both a binary op and a unary op. Can we simply register math.log in binaryop as well as in unaryop to support both its usage?
Lastly, is there a place for all operator ops? Sorry for cramming all the questions in one place!
There was a problem hiding this comment.
For trunc, the lack of implementation in CUDA might be a bug in Numba - I'll check into it and get back to you.
For log with two arguments, this appears not to be supported by the CUDA target (probably not for any good technical reason) - if the CUDA target did support it, just registering log as both a unary and binary op would work (because when one typing fails Numba will carry on trying others until it finds a successful one).
For all the operators, there's https://github.com/numba/numba/blob/master/numba/cpython/numbers.py - you have to look for all the instances of lower_builtin in that file. It's using the exact same code as the CPU target, so it isn't duplicated in the CUDA target, but instead the CUDA target pulls it in by "magic"... I started trying to trace exactly why the typing is registered for the CUDA target but I ended up going through several layers and still didn't get to the bottom of it. However, the lowering is pulled in by a side effect of this import in the CUDA target context: https://github.com/numba/numba/blob/master/numba/cuda/target.py#L88
There was a problem hiding this comment.
Thanks for pointing out the operator locations. I was able to add invert. For abs, there's an error:
TypingError: No implementation of function Function(<built-in function abs>) found for signature:
E
E >>> abs(int32)
Which seems strange because the lowering for integer types are here: https://github.com/numba/numba/blob/2a792155c3dce43f86b9ff93802f12d39a3752dc/numba/cpython/numbers.py#L565
There was a problem hiding this comment.
Unless... it's not for cuda target?
|
rerun tests |
brandon-b-miller
added
5 - Ready to Merge
|
rerun tests |
|
@gpucibot merge |
|
thanks @isVoid ! 🚀 |
This PR adds support for several unary operations in masked udfs. Including trigonometry functions
sin,cos,tan; rounding functionsceilandfloor, sign functionsnegand logic functionsnot.closes #9405