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

Fixing compatibility issue with CUDA array interface #3594

Merged
merged 2 commits into from
Mar 11, 2021

Conversation

lowener
Copy link
Contributor

@lowener lowener commented Mar 9, 2021

Closes #3576.

This PR is a fix for CUDA arrays that don't have the strides property or that set this property to None.
Since it seems to be compliant with CUDA array interface v2 we should be able to support it.

@lowener lowener requested a review from a team as a code owner March 9, 2021 16:48
@github-actions github-actions bot added the Cython / Python Cython or Python issue label Mar 9, 2021
@lowener lowener added 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 9, 2021
Copy link
Member

@dantegd dantegd left a comment

Choose a reason for hiding this comment

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

Just a comment about the comment, otherwise looks great, thanks @lowener !

Comment on lines 313 to 314
# __cuda_array_interface__ v2 requires the strides to be omitted
# (either not set or set to None) for C-contiguous arrays.
Copy link
Member

Choose a reason for hiding this comment

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

I would just change the wording of the comment, since the spec doesn't require strides to be omitted for C contiguous arrays, but if the strides are not given or none, then the array is C-contiguous. So, a C-contiguous array can also have its strides set (correctly, implying its contiguity of course)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I fixed it

@dantegd dantegd added 4 - Waiting on Author Waiting for author to respond to review and removed 3 - Ready for Review Ready for review by team labels Mar 9, 2021
@lowener lowener requested a review from dantegd March 11, 2021 13:58
@codecov-io
Copy link

Codecov Report

Merging #3594 (991f9c0) into branch-0.19 (fd9ec89) will increase coverage by 0.11%.
The diff coverage is 50.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.19    #3594      +/-   ##
===============================================
+ Coverage        80.70%   80.82%   +0.11%     
===============================================
  Files              227      227              
  Lines            17615    17739     +124     
===============================================
+ Hits             14217    14338     +121     
- Misses            3398     3401       +3     
Flag Coverage Δ
dask 45.29% <0.00%> (+0.30%) ⬆️
non-dask 73.09% <50.00%> (+0.17%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
python/cuml/common/memory_utils.py 79.26% <50.00%> (+0.15%) ⬆️
python/cuml/cluster/kmeans.pyx 91.95% <0.00%> (ø)
python/cuml/dask/solvers/cd.py 100.00% <0.00%> (ø)
python/cuml/common/numba_utils.py 0.00% <0.00%> (ø)
python/cuml/internals/global_settings.py 100.00% <0.00%> (ø)
python/cuml/dask/preprocessing/encoders.py 100.00% <0.00%> (ø)
python/cuml/metrics/_ranking.py 98.57% <0.00%> (+0.02%) ⬆️
python/cuml/preprocessing/encoders.py 95.10% <0.00%> (+0.02%) ⬆️
python/cuml/common/array_descriptor.py 98.24% <0.00%> (+0.03%) ⬆️
python/cuml/common/array.py 96.99% <0.00%> (+0.04%) ⬆️
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd220fc...991f9c0. Read the comment docs.

@dantegd
Copy link
Member

dantegd commented Mar 11, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit b54296c into rapidsai:branch-0.19 Mar 11, 2021
@lowener lowener deleted the 019-contiguous-array branch March 14, 2021 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Waiting on Author Waiting for author to respond to review Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
3 participants