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

Faster GLM preprocessing by fusing kernels #4549

Merged
merged 8 commits into from
Feb 10, 2022

Conversation

achirkin
Copy link
Contributor

@achirkin achirkin commented Feb 2, 2022

Fuse fit_intercept and normalize kernels when both are enabled. This change reduces the preprocess/postprocess runtime almost by half when the data is normalized (which is false by default though).
Furthermore, it changes the behavior of the "normalize" switch from dividing by the column-wise L2 norm to dividing by the column-wise standard deviation.

@achirkin achirkin requested review from a team as code owners February 2, 2022 09:21
@achirkin achirkin added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change and removed CMake CUDA/C++ labels Feb 2, 2022
@achirkin achirkin marked this pull request as draft February 2, 2022 12:21
@github-actions github-actions bot added CMake CUDA/C++ Cython / Python Cython or Python issue labels Feb 4, 2022
@github-actions github-actions bot removed the CMake label Feb 5, 2022
@achirkin achirkin marked this pull request as ready for review February 5, 2022 08:25
@achirkin achirkin requested a review from a team as a code owner February 5, 2022 08:25
@achirkin achirkin added the 3 - Ready for Review Ready for review by team label Feb 5, 2022
@achirkin
Copy link
Contributor Author

achirkin commented Feb 8, 2022

rerun tests

@cjnolet
Copy link
Member

cjnolet commented Feb 8, 2022

rerun tests

@codecov-commenter
Copy link

Codecov Report

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

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-22.04    #4549   +/-   ##
===============================================
  Coverage                ?   85.73%           
===============================================
  Files                   ?      239           
  Lines                   ?    19585           
  Branches                ?        0           
===============================================
  Hits                    ?    16792           
  Misses                  ?     2793           
  Partials                ?        0           
Flag Coverage Δ
dask 46.18% <0.00%> (?)
non-dask 78.73% <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 9921c61...6f056df. Read the comment docs.

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.

A couple very minor things. PR looks great overall.

cpp/test/sg/cd_test.cu Show resolved Hide resolved
cpp/test/sg/ridge.cu Outdated Show resolved Hide resolved
python/cuml/linear_model/elastic_net.pyx Show resolved Hide resolved
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.

I meant to request changes.

@achirkin
Copy link
Contributor Author

rerun tests

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!

@cjnolet
Copy link
Member

cjnolet commented Feb 10, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 7ab0f4c into rapidsai:branch-22.04 Feb 10, 2022
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
Fuse fit_intercept and normalize kernels when both are enabled. This change reduces the preprocess/postprocess runtime almost by half when the data is normalized (which is false by default though).
Furthermore, it changes the behavior of the "normalize" switch from dividing by the column-wise L2 norm to dividing by the column-wise standard deviation.

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

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

URL: rapidsai#4549
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team CUDA/C++ Cython / Python Cython or Python issue 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

3 participants