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

Support sparse input features in QN solvers and Logistic Regression #3827

Merged
merged 20 commits into from
Jun 1, 2021
Merged

Support sparse input features in QN solvers and Logistic Regression #3827

merged 20 commits into from
Jun 1, 2021

Conversation

achirkin
Copy link
Contributor

@achirkin achirkin commented May 5, 2021 •
edited

Allow input features tensor X to be sparse for the functions of a QN solver (logistic regression).

@github-actions github-actions bot added CUDA/C++ Cython / Python Cython or Python issue labels May 5, 2021
@achirkin achirkin added 2 - In Progress Currenty a work in progress improvement Improvement / enhancement to an existing function non-breaking Non-breaking change and removed CUDA/C++ Cython / Python Cython or Python issue labels May 5, 2021
@achirkin achirkin requested a review from tfeher May 5, 2021 14:46
@cjnolet cjnolet self-requested a review May 5, 2021 20:55
@github-actions github-actions bot added CUDA/C++ Cython / Python Cython or Python issue labels May 6, 2021
@achirkin achirkin mentioned this pull request May 6, 2021
Merged
@github-actions github-actions bot added the CMake label May 6, 2021
rapids-bot bot pushed a commit to rapidsai/raft that referenced this pull request May 6, 2021
@achirkin
Add a wrapper for cusparseCreateDnMat (#220)
d1b02ca 
Add a wrapper required by rapidsai/cuml#3827

Authors:
  - Artem M. Chirkin (https://github.com/achirkin)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)

URL: #220
tfeher
tfeher requested changes May 11, 2021
View reviewed changes
Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Thanks @achirkin for this PR! The internals look good, I have some small comments. I see that predict / decision function are not yet updated. I guess it make sense to converge on the common sparse / dense interface before implementing those.

cpp/include/cuml/linear_model/glm.hpp Outdated Show resolved Hide resolved
cpp/src/glm/qn/simple_mat/base.hpp Outdated Show resolved Hide resolved
cpp/src/glm/qn/glm_base.cuh Outdated Show resolved Hide resolved
cpp/src/glm/qn/glm_base.cuh Outdated Show resolved Hide resolved
cpp/src/glm/qn/simple_mat/dense.hpp Outdated Show resolved Hide resolved
cpp/src/glm/qn/simple_mat/sparse.hpp Outdated Show resolved Hide resolved
cpp/src/glm/qn/simple_mat/sparse.hpp Outdated Show resolved Hide resolved
cpp/src/glm/qn/qn.cuh Outdated Show resolved Hide resolved
python/cuml/common/array_sparse.py Outdated Show resolved Hide resolved
python/cuml/common/array_sparse.py Outdated Show resolved Hide resolved
@dantegd dantegd added this to PR-WIP in v21.06 Release via automation May 14, 2021
@achirkin achirkin changed the title Support sparse input features in 'fit' for QN solvers Support sparse input features in QN solvers May 19, 2021
@achirkin achirkin marked this pull request as ready for review May 19, 2021 17:09
@cjnolet cjnolet added the 4 - Waiting on Author Waiting for author to respond to review label May 21, 2021
@achirkin achirkin added 3 - Ready for Review Ready for review by team and removed 4 - Waiting on Author Waiting for author to respond to review labels May 21, 2021
@achirkin achirkin requested a review from cjnolet May 21, 2021 10:22
@achirkin
Copy link
Contributor Author

achirkin commented May 21, 2021 •
edited

Ugh, it seems I've hit the same issue as the one workarounded in #3679 . I've added the same "skip" statement for now, till we decide what to do with it.

Interestingly, the test succeeds on my setup:

# nvcc --version
nvcc: NVIDIA (R) Cuda compiler driver
Copyright (c) 2005-2021 NVIDIA Corporation
Built on Sun_Feb_14_21:12:58_PST_2021
Cuda compilation tools, release 11.2, V11.2.152
Build cuda_11.2.r11.2/compiler.29618528_0

# conda list
# Name                    Version                   Build  Channel
cudatoolkit               11.2.72              h2bc3f7f_0    nvidia
librmm                    0.20.0a210517   cuda11.2_gbe6aca7_32    rapidsai-nightly
rmm                       0.20.0a210517   cuda_11.2_py38_gbe6aca7_32    rapidsai-nightly
...

cuda-memcheck also did not reveal any problems.

@dantegd dantegd added 4 - Waiting on Reviewer Waiting for reviewer to review or respond and removed 3 - Ready for Review Ready for review by team labels May 22, 2021
tfeher
tfeher approved these changes May 25, 2021
View reviewed changes
Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Thanks @achirkin for fixing the issues, the PR looks good to me!

dantegd
dantegd requested changes May 26, 2021
View reviewed changes
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 one quick comment about not needing to update the RAFT PIN for merging

cpp/cmake/Dependencies.cmake Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the CMake label May 26, 2021
@achirkin achirkin requested a review from dantegd May 26, 2021 05:35
@cjnolet cjnolet mentioned this pull request May 26, 2021
Open
@cjnolet
Copy link
Member

cjnolet commented May 26, 2021

rerun tests

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.06@95efa25). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.06    #3827   +/-   ##
===============================================
  Coverage                ?   85.17%           
===============================================
  Files                   ?      228           
  Lines                   ?    17892           
  Branches                ?        0           
===============================================
  Hits                    ?    15239           
  Misses                  ?     2653           
  Partials                ?        0
Flag Coverage Δ
dask 47.91% <0.00%> (?)
non-dask 77.42% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out 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 95efa25...a3e9ca7. Read the comment docs.

cjnolet
cjnolet approved these changes Jun 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.

LGTM

v21.06 Release automation moved this from PR-Needs review to PR-Reviewer approved Jun 1, 2021
@dantegd dantegd changed the title Support sparse input features in QN solvers Support sparse input features in QN solvers and Logistic Regression Jun 1, 2021
@cjnolet
Copy link
Member

cjnolet commented Jun 1, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 64fb1f4 into rapidsai:branch-21.06 Jun 1, 2021
v21.06 Release automation moved this from PR-Reviewer approved to Done Jun 1, 2021
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
@achirkin
Support sparse input features in QN solvers and Logistic Regression (r…
552769a 
…apidsai#3827)

Allow input features tensor `X` to be sparse for the functions of a QN solver (logistic regression).

Authors:
  - Artem M. Chirkin (https://github.com/achirkin)

Approvers:
  - Tamas Bela Feher (https://github.com/tfeher)
  - Corey J. Nolet (https://github.com/cjnolet)
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: rapidsai#3827
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dantegd dantegd dantegd approved these changes

@cjnolet cjnolet cjnolet approved these changes

@tfeher tfeher tfeher approved these changes

Assignees
No one assigned
Labels
4 - Waiting on Reviewer Waiting for reviewer to review or respond CUDA/C++ Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
No open projects
v21.06 Release
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@achirkin @cjnolet @codecov-commenter @dantegd @tfeher