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

Cython wrapper for v-measure #4785

Merged
merged 5 commits into from
Jul 14, 2022
Merged

Cython wrapper for v-measure #4785

merged 5 commits into from
Jul 14, 2022

Conversation

shaswat-indian
Copy link
Contributor

This PR resolves #802 by adding python API for v_measure_score.

Also came across an issue while working on this.

@shaswat-indian shaswat-indian requested review from a team as code owners June 23, 2022 22:52
@github-actions github-actions bot added CUDA/C++ Cython / Python Cython or Python issue labels Jun 23, 2022
@beckernick beckernick added feature request New feature or request 3 - Ready for Review Ready for review by team non-breaking Non-breaking change labels Jun 28, 2022
@shaswat-indian
Copy link
Contributor Author

rerun tests

lowener
lowener approved these changes Jun 29, 2022
View reviewed changes
Copy link
Contributor

@lowener lowener left a comment

Choose a reason for hiding this comment

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

LGTM, can you add this metric to docs/source/api.rst

@codecov-commenter
Copy link

Codecov Report

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

@@               Coverage Diff               @@
##             branch-22.08    #4785   +/-   ##
===============================================
  Coverage                ?   77.62%           
===============================================
  Files                   ?      180           
  Lines                   ?    11384           
  Branches                ?        0           
===============================================
  Hits                    ?     8837           
  Misses                  ?     2547           
  Partials                ?        0
Flag Coverage Δ
dask 45.52% <0.00%> (?)
non-dask 67.26% <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 d78a805...6e4c80d. Read the comment docs.

wphicks
wphicks approved these changes Jul 11, 2022
View reviewed changes
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.

Had one question, but it doesn't impact approval. LGTM!

cpp/src_prims/metrics/v_measure.cuh Outdated Show resolved Hide resolved
@wphicks
Copy link
Contributor

wphicks commented Jul 11, 2022

@gpucibot merge

@caryr35 caryr35 added this to PR-WIP in v22.08 Release via automation Jul 11, 2022
@caryr35 caryr35 moved this from PR-WIP to PR-Needs review in v22.08 Release Jul 11, 2022
@shaswat-indian
Copy link
Contributor Author

rerun tests

lowener
lowener requested changes Jul 12, 2022
View reviewed changes
Copy link
Contributor

@lowener lowener left a comment

Choose a reason for hiding this comment

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

I just saw that v_measure was also present in raft. I think it would make more sense to wrap the raft version, and remove the C++ file of cuml to avoid code duplication.

@wphicks
Copy link
Contributor

wphicks commented Jul 12, 2022

I just saw that v_measure was also present in raft. I think it would make more sense to wrap the raft version, and remove the C++ file of cuml to avoid code duplication.

Agreed, but let's raise that as a separate issue, since the functionality was not yet removed from cuML and the wrappers have already been implemented. I suspect it will be straightforward to migrate based on the code in this PR.

@dantegd
Copy link
Member

dantegd commented Jul 12, 2022

rerun tests

1 similar comment
@dantegd
Copy link
Member

dantegd commented Jul 14, 2022

rerun tests

@rapids-bot rapids-bot bot merged commit 97941e3 into rapidsai:branch-22.08 Jul 14, 2022
v22.08 Release automation moved this from PR-Needs review to Done Jul 14, 2022
jakirkham pushed a commit to jakirkham/cuml that referenced this pull request Feb 27, 2023
@shaswat-indian
Cython wrapper for v-measure (rapidsai#4785)
6d58217 
This PR resolves rapidsai#802 by adding python API for `v_measure_score`.

Also came across an [issue](rapidsai#4784) while working on this.

Authors:
  - Shaswat Anand (https://github.com/shaswat-indian)

Approvers:
  - Micka (https://github.com/lowener)
  - William Hicks (https://github.com/wphicks)

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

@wphicks wphicks wphicks approved these changes

@lowener lowener lowener requested changes

Assignees
No one assigned
Labels
3 - Ready for Review Ready for review by team CUDA/C++ Cython / Python Cython or Python issue feature request New feature or request non-breaking Non-breaking change
Projects
No open projects
v22.08 Release
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Cython wrapper for v-measure
6 participants
@shaswat-indian @codecov-commenter @wphicks @dantegd @lowener @beckernick