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

[REVIEW] Add cython hinge_loss #3409

Merged
merged 30 commits into from
Feb 22, 2021

Conversation

Nanthini10
Copy link
Contributor

@Nanthini10 Nanthini10 commented Jan 25, 2021

Implements hinge_loss with tests. API matches and code is adapted from sklearn https://scikit-learn.org/stable/modules/generated/sklearn.metrics.hinge_loss.html

NOTE: We're not using the C++ hinge implementation in order to comply with sklearn API. The C++ implementation is used in the intermediate step in SGD and accept different set of parameters than what is used to calculate hinge_loss.

@Nanthini10 Nanthini10 requested a review from a team as a code owner January 25, 2021 20:13
@github-actions github-actions bot added the Cython / Python Cython or Python issue label Jan 25, 2021
@Nanthini10 Nanthini10 added 3 - Ready for Review Ready for review by team Cython / Python Cython or Python issue feature request New feature or request and removed Cython / Python Cython or Python issue labels Jan 25, 2021
@Nanthini10 Nanthini10 added the non-breaking Non-breaking change label Jan 25, 2021
@Nanthini10 Nanthini10 changed the title [FEA] Add cython hinge_loss [REVIEW] Add cython hinge_loss Jan 25, 2021
@wphicks
Copy link
Contributor

wphicks commented Jan 28, 2021

Looks like this was hit by the IVFPQ error on CUDA 11 and network errors on 10.1 and 10.2. Going to rerun.

@wphicks
Copy link
Contributor

wphicks commented Jan 28, 2021

rerun tests

@wphicks
Copy link
Contributor

wphicks commented Jan 29, 2021

@Nanthini10 Could you merge branch-0.18 into your branch? We just got the xfail on IVFPQ in, and I think that's the only thing keeping your PR from passing. The other failures were timeouts on the clone.

Copy link
Contributor

@wphicks wphicks left a comment

Choose a reason for hiding this comment

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

One small tweak for a redundant check, and one question (which I'm honestly not sure the answer to), but otherwise this looks great!

I know you mentioned it to me offline, but would you mind also adding a comment (either on the PR or in the code) that explains why we're not using the hinge_loss implemented in the CUDA/CPP layer? I figure having that there may help avoid confusion down the line about why this doesn't match the pattern of other metrics.

python/cuml/metrics/hinge_loss.pyx Outdated Show resolved Hide resolved
python/cuml/metrics/hinge_loss.pyx Outdated Show resolved Hide resolved
@wphicks wphicks added 4 - Waiting on Author Waiting for author to respond to review and removed 3 - Ready for Review Ready for review by team labels Feb 4, 2021
@Nanthini10 Nanthini10 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 Feb 10, 2021
Copy link
Contributor

@mdemoret-nv mdemoret-nv left a comment

Choose a reason for hiding this comment

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

Everything looks good to me. Well done.

@Nanthini10 Nanthini10 added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 4 - Waiting on Reviewer Waiting for reviewer to review or respond labels Feb 17, 2021
@beckernick
Copy link
Member

Should this be targeting 0.19?

@Nanthini10
Copy link
Contributor Author

Should this be targeting 0.19?

Yes, this is for 0.19 @beckernick

@mdemoret-nv mdemoret-nv changed the base branch from branch-0.18 to branch-0.19 February 19, 2021 20:39
@mdemoret-nv
Copy link
Contributor

@Nanthini10 I changed the base to branch-0.19. Tests are running.

@codecov-io
Copy link

codecov-io commented Feb 19, 2021

Codecov Report

Merging #3409 (57ea041) into branch-0.19 (39c7262) will increase coverage by 7.57%.
The diff coverage is 75.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.19    #3409      +/-   ##
===============================================
+ Coverage        71.77%   79.35%   +7.57%     
===============================================
  Files              212      226      +14     
  Lines            17075    18007     +932     
===============================================
+ Hits             12256    14289    +2033     
+ Misses            4819     3718    -1101     
Flag Coverage Δ
dask 44.01% <21.95%> (?)
non-dask 71.66% <73.10%> (?)

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

Impacted Files Coverage Δ
...on/cuml/_thirdparty/sklearn/preprocessing/_data.py 63.61% <ø> (+0.07%) ⬆️
...l/_thirdparty/sklearn/preprocessing/_imputation.py 62.40% <ø> (-0.11%) ⬇️
python/cuml/experimental/explainer/common.py 88.05% <42.85%> (-4.01%) ⬇️
python/cuml/neighbors/nearest_neighbors.pyx 92.43% <50.00%> (-0.29%) ⬇️
python/cuml/common/import_utils.py 59.43% <66.66%> (+3.43%) ⬆️
python/cuml/experimental/explainer/base.pyx 67.06% <67.06%> (ø)
python/cuml/metrics/hinge_loss.pyx 73.33% <73.33%> (ø)
python/cuml/dask/common/utils.py 43.68% <83.33%> (+16.13%) ⬆️
python/cuml/experimental/explainer/kernel_shap.pyx 97.75% <100.00%> (+0.48%) ⬆️
...n/cuml/experimental/explainer/permutation_shap.pyx 98.82% <100.00%> (+0.84%) ⬆️
... and 74 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 befdf98...57ea041. Read the comment docs.

@JohnZed
Copy link
Contributor

JohnZed commented Feb 22, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 981acb7 into rapidsai:branch-0.19 Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge Cython / Python Cython or Python issue feature request New feature or request non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants