-
Notifications
You must be signed in to change notification settings - Fork 90
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
feat: complex reducers #3149
feat: complex reducers #3149
Conversation
The tests don't work in this directory, at least right now: it failed in the
Is src/awkward/_connect/cuda/cuda_kernels/awkward_reduce_prod_complex.cu not checked in? |
Oh. I accidentally turned the tests on without pushing the prod complex kernel. I'll add the tests we discussed tomorrow along with the kernel. I have another completed kernel yet to push. This PR will be finalized by Friday. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ManasviGoyal - looks good! I would suggest to change xfail
to skip
to avoid the long listings due to the assertions like:
> raise AssertionError(f"CuPyKernel not found: {index!r}")
E AssertionError: CuPyKernel not found: ('awkward_reduce_argmin', <class 'numpy.int64'>, <class 'numpy.float64'>, <class 'numpy.int64'>)
all the other tests pass!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ManasviGoyal - I don't think I saw this error yesterday.
@jpivarski - I think, we need to be able to deal with NaN
s or Inf
s in a way that it is communicated to a user. Perhaps, via an overflow_flag
or similar. The test highlights the difference between CPU
and GPU
where the first returns a NaN
the latter returns 0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ManasviGoyal - Thanks! It looks good, all tests pass. Please, merge it when you are ready. Thanks!
@ianna I agree it would be good to retest the |
@ianna For resolving the conflict - I had to fix the tests generation for combinations so we would want to keep that change and not the one in this PR. |
revert merge change
@ManasviGoyal - we skip the following tests - is it expected? Thanks!
|
This is expected and since all the tests pass I'm going to merge the PR. Well done @ManasviGoyal ! |
also added: