-
-
Notifications
You must be signed in to change notification settings - Fork 366
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
Fixes issue with Numba CUDA target altering max/min semantics. #934
Fixes issue with Numba CUDA target altering max/min semantics. #934
Conversation
This is entirely untested and relies on numba/numba#5941, open question about what to do for version Numba 0.50. which has neither the old atomic min/max behaviour nor the new nanmin/nanmax functions. |
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.
Thanks! I haven't tested it, but hopefully @jonmmease and/or @philippjfr can.
def cuda_atomic_nanmax(ary, idx, val) | ||
return cuda.atomic.max(ary, idx, val) | ||
else: | ||
# WHAT DO YOU WANT TO DO HERE? |
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.
# WHAT DO YOU WANT TO DO HERE? | |
raise ImportError("Datashader's CUDA support requires numba!=0.50.0") |
cuda_atomic_max(data, (i, j), lower) | ||
cuda_atomic_min(data, (i, j), upper) |
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.
cuda_atomic_max(data, (i, j), lower) | |
cuda_atomic_min(data, (i, j), upper) | |
cuda_atomic_nanmax(data, (i, j), lower) | |
cuda_atomic_nanmin(data, (i, j), upper) |
Thanks for the review @jbednar, once 0.51 is out I'll revisit (or someone else can, I have no particular attachment to the patch, feel free to push to my branch :)) with view to test this and make it actually work opposed to it just being a guess! |
If anyone's up for testing this, I've fixed the picky little errors at #940. (See stuartarchibald#1.) Note however that #940 has one more change that might be important. @philippjfr asked why we couldn't just import Also, if you're going to manually test, best to just test Python 3.7 for now. There are currently unrelated problems with Dask that make the test suite fail on 2.7 and 3.6. |
xref numba/numba#6020 which is @gmarkall carrying on with some extra details on top of the original PR on the Numba side. |
@dHannasch I tested this with numba 0.51(https://github.com/numba/numba/releases/tag/0.51.0rc1), The version check seems to work as well. Just a couple of fixes required |
I'd like to go ahead and merge and then fix things up in a separate PR. Any objections? |
@philippjfr that works |
As title.
Closes #932