-
Notifications
You must be signed in to change notification settings - Fork 302
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
Fea hungarian expose precision #1673
Fea hungarian expose precision #1673
Conversation
rerun tests |
Codecov Report
@@ Coverage Diff @@
## branch-21.08 #1673 +/- ##
==============================================
Coverage ? 0.22%
==============================================
Files ? 80
Lines ? 3542
Branches ? 0
==============================================
Hits ? 8
Misses ? 3534
Partials ? 0 Continue to review full report at Codecov.
|
rerun tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some minor comments about variable naming, but besides theses complaints, looks good to me.
vertex_t num_rows, | ||
vertex_t num_columns, | ||
vertex_t *assignment, | ||
weight_t precision); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, we used epsilon in raft and we're using precision here, I got they are two different functions, but both epsilon and precision are used to evaluate equality.... so better be consistent?
weight_t const *costs, | ||
vertex_t num_rows, | ||
vertex_t num_columns, | ||
vertex_t *assignment, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assignment"s"? (as this is an array of values and we're using cost"s", without "s", one may think cost"s" is an array and assignment is a single value).
weight_t{0}, | ||
thrust::maximum<weight_t>()); | ||
|
||
rmm::device_uvector<weight_t> tmp_cost(n * n, handle.get_stream_view()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use _v for tmp_row_assignment_v and tmp_col_assignment_v but no _v for tmp_cost? IIRC, you use _v for vectors and better be consistent at least within a code block.
@gpucibot merge |
Completes the work of exposing the precision parameter to the caller in python as requested by #1645 Fixes a few PR comments from PR #1673 that were deferred as that PR required merging to fix the build. Authors: - Chuck Hastings (https://github.com/ChuckHastings) Approvers: - Seunghwa Kang (https://github.com/seunghwak) - Brad Rees (https://github.com/BradReesWork) URL: #1674
Closes #1645
Closes #1646
Expose the precision parameter (epsilon in the Date/Nagi implementation) of the Hungarian algorithm to be controllable by the user. Add support for rectangular matrices.
Will be enabled for CI after rapidsai/raft#275 is merged.