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.

Sign up for GitHub

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

Jump to bottom

[REVIEW] Expose sparse distances via semiring to Python API #3516

Merged
merged 29 commits into from
Apr 1, 2021
Merged

[REVIEW] Expose sparse distances via semiring to Python API #3516

merged 29 commits into from
Apr 1, 2021

Conversation

lowener
Copy link
Contributor

@lowener lowener commented Feb 18, 2021

Closes #3478.

@lowener lowener changed the title Expose sparse distances via semiring to Python API [WIP] Expose sparse distances via semiring to Python API Feb 18, 2021
@github-actions github-actions bot added CUDA/C++ Cython / Python Cython or Python issue labels Feb 18, 2021
@lowener lowener marked this pull request as ready for review March 5, 2021 17:38
@lowener lowener requested review from a team as code owners March 5, 2021 17:38
@lowener lowener changed the title [WIP] Expose sparse distances via semiring to Python API Expose sparse distances via semiring to Python API Mar 5, 2021
@lowener lowener requested a review from cjnolet March 15, 2021 17:43
@lowener lowener changed the title Expose sparse distances via semiring to Python API [REVIEW] Expose sparse distances via semiring to Python API Mar 15, 2021
cjnolet
cjnolet requested changes Mar 16, 2021
View reviewed changes
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

Looks good so far. Most of my comments are minor things. Looking forward to having this in 0.19.

cpp/src_prims/sparse/distance/l2_distance.cuh Outdated Show resolved Hide resolved
python/cuml/metrics/__init__.py Outdated Show resolved Hide resolved
python/cuml/metrics/pairwise_distances.pyx Show resolved Hide resolved
python/cuml/test/test_metrics.py Outdated Show resolved Hide resolved
python/cuml/test/test_metrics.py Outdated Show resolved Hide resolved
python/cuml/test/test_metrics.py Show resolved Hide resolved
python/cuml/metrics/pairwise_distances.pyx Outdated Show resolved Hide resolved
python/cuml/metrics/pairwise_distances.pyx Outdated Show resolved Hide resolved
@lowener lowener requested a review from a team as a code owner March 30, 2021 18:56
@github-actions github-actions bot added the CMake label Mar 30, 2021
@github-actions github-actions bot removed the CMake label Mar 30, 2021
@lowener lowener requested a review from cjnolet March 30, 2021 21:31
cjnolet
cjnolet approved these changes Apr 1, 2021
View reviewed changes
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

Changes look good so I'll go ahead and approve now so we can get this in quickly. Left a comment about a small nitpick but we can do that in a follow-in if you prefer.

@github-actions github-actions bot added the CMake label Apr 1, 2021
@cjnolet cjnolet added feature request New feature or request non-breaking Non-breaking change labels Apr 1, 2021
@JohnZed
Copy link
Contributor

JohnZed commented Apr 1, 2021

@gpucibot merge

@codecov-io
Copy link

Codecov Report

Merging #3516 (4d1c1c9) into branch-0.19 (fd9ec89) will increase coverage by 1.92%.
The diff coverage is 82.14%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.19    #3516      +/-   ##
===============================================
+ Coverage        80.70%   82.63%   +1.92%     
===============================================
  Files              227      227              
  Lines            17615    17718     +103     
===============================================
+ Hits             14217    14641     +424     
+ Misses            3398     3077     -321
Flag Coverage Δ
dask 46.43% <0.89%> (+1.43%) ⬆️
non-dask 74.70% <82.14%> (+1.78%) ⬆️

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

Impacted Files Coverage Δ
...cuml/_thirdparty/sklearn/preprocessing/__init__.py 100.00% <ø> (ø)
...l/_thirdparty/sklearn/preprocessing/_imputation.py 63.85% <ø> (+1.05%) ⬆️
...cuml/_thirdparty/sklearn/utils/skl_dependencies.py 80.00% <ø> (+26.07%) ⬆️
python/cuml/cluster/__init__.py 100.00% <ø> (ø)
python/cuml/cluster/agglomerative.pyx 96.47% <ø> (ø)
python/cuml/cluster/dbscan.pyx 98.23% <ø> (-1.77%) ⬇️
python/cuml/cluster/kmeans.pyx 92.03% <ø> (+0.07%) ⬆️
python/cuml/common/array_sparse.py 96.29% <ø> (+1.95%) ⬆️
python/cuml/common/doc_utils.py 100.00% <ø> (ø)
python/cuml/common/memory_utils.py 79.26% <ø> (+0.15%) ⬆️
... and 115 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 668aea1...4d1c1c9. Read the comment docs.

@rapids-bot rapids-bot bot merged commit 4a946e0 into rapidsai:branch-0.19 Apr 1, 2021
@lowener lowener deleted the 019-expose-spmv branch April 1, 2021 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjnolet cjnolet cjnolet approved these changes

Assignees
No one assigned
Labels
CMake Cython / Python Cython or Python issue feature request New feature or request non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Expose sparse distances via semiring to Python API
4 participants
@lowener @JohnZed @codecov-io @cjnolet