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

Row major Gram matrices #3639

Merged
merged 3 commits into from
Mar 31, 2021
Merged

Conversation

tfeher
Copy link
Contributor

@tfeher tfeher commented Mar 19, 2021

This PR add row major Gram matrices. These will be used in SVM kernels to allow flexibility in the input layout (#2198).

For the benchmarked cases, row major input is around 2.5% slower on average.

image

@tfeher tfeher requested a review from a team as a code owner March 19, 2021 10:59
@tfeher tfeher added 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change and removed CUDA/C++ labels Mar 19, 2021
@teju85
Copy link
Member

teju85 commented Mar 24, 2021

@tfeher So, your comparison above is only on the gram-matrix kernels and NOT the end-to-end SVM perf, right?

Copy link
Member

@teju85 teju85 left a comment

Choose a reason for hiding this comment

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

Changes LGTM.

@dantegd
Copy link
Member

dantegd commented Mar 24, 2021

@tfeher the only thing missing to merge are a couple of updates to copyright years:

Copyright headers incomplete in some of the files!

  cpp/bench/prims/gram_matrix.cu:2 Issue: Current year not included in the copyright header
  cpp/src_prims/matrix/grammatrix.cuh:2 Issue: Current year not included in the copyright header

@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 24, 2021
@tfeher
Copy link
Contributor Author

tfeher commented Mar 30, 2021

So, your comparison above is only on the gram-matrix kernels and NOT the end-to-end SVM perf, right?

Yes, the comparison above is only for the kernels. In this PR the row major kernels are not yet used by SVM.

@tfeher tfeher added 4 - Waiting on Reviewer Waiting for reviewer to review or respond and removed 4 - Waiting on Author Waiting for author to respond to review labels Mar 30, 2021
@tfeher
Copy link
Contributor Author

tfeher commented Mar 30, 2021

rerun tests

@codecov-io
Copy link

Codecov Report

Merging #3639 (526f14a) into branch-0.19 (14bd6c1) will increase coverage by 1.58%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.19    #3639      +/-   ##
===============================================
+ Coverage        80.72%   82.30%   +1.58%     
===============================================
  Files              228      227       -1     
  Lines            17629    17591      -38     
===============================================
+ Hits             14231    14479     +248     
+ Misses            3398     3112     -286     
Flag Coverage Δ
dask 46.42% <ø> (+1.45%) ⬆️
non-dask 74.27% <ø> (+1.33%) ⬆️

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

Impacted Files Coverage Δ
python/cuml/_thirdparty/sklearn/utils/_pprint.py 0.00% <0.00%> (-27.54%) ⬇️
python/cuml/linear_model/linear_regression.pyx 88.23% <0.00%> (-3.53%) ⬇️
python/cuml/cluster/dbscan.pyx 98.19% <0.00%> (-1.81%) ⬇️
...hirdparty/sklearn/preprocessing/_discretization.py 83.59% <0.00%> (-0.62%) ⬇️
python/cuml/solvers/qn.pyx 97.20% <0.00%> (-0.43%) ⬇️
python/cuml/dask/solvers/cd.py 100.00% <0.00%> (ø)
python/cuml/cluster/__init__.py 100.00% <0.00%> (ø)
python/cuml/common/doc_utils.py 100.00% <0.00%> (ø)
python/cuml/pipeline/__init__.py 100.00% <0.00%> (ø)
python/cuml/common/numba_utils.py 0.00% <0.00%> (ø)
... and 60 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 14bd6c1...526f14a. Read the comment docs.

@dantegd
Copy link
Member

dantegd commented Mar 31, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit b10f31f into rapidsai:branch-0.19 Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Waiting on Reviewer Waiting for reviewer to review or respond improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants