[REVIEW] HDBSCAN - #3821
Conversation
tfeher
left a comment
tfeher
left a comment
There was a problem hiding this comment.
Thanks @cjnolet and @divyegala for the updates!
Please add a link to the API documentation. Apart from that I have only a few minor comments left, after that we shall be good to go.
tfeher
left a comment
tfeher
left a comment
There was a problem hiding this comment.
Thanks for the updates, the PR looks good to me!
dantegd
left a comment
dantegd
left a comment
There was a problem hiding this comment.
Everything looks good from me too, just requested adding an item to the follow ups issue (if it’s not already there). Once hdbscan python package is in integration we should be good to merge.
| dtype=dtype, | ||
| memptr=mem_ptr)).to_output('numpy') | ||
|
|
||
| def _construct_condensed_tree_attribute(self, ptr, dtype="int32"): |
There was a problem hiding this comment.
Can you add the item of trying this as a property, I think it can be a follow up nice to have but not indispensable (and need to think about the getattr…)
dantegd
left a comment
dantegd
left a comment
There was a problem hiding this comment.
Totally missed the conda change, which goes hand in hand with adding hdbscan to the rapids-build meta package
@dantegd, absolutely I'd like to use a lazy-loaded property here. I initially tried this as a lazy-populated property (using |
|
Added item to our follow-on task to revisit the property / lazy-loaded members for plotting objects: #3879 (comment) |
|
PR to add hdbscan package to |
cjnolet
added
5 - Ready to Merge
|
Removing |
|
rerun tests |
1 similar comment
|
rerun tests |
|
@gpucibot merge |
|
Going to try again in hopes the changes to the integration repo just weren't picked up in time. |
|
rerun tests |
1 similar comment
|
rerun tests |
Codecov Report
@@ Coverage Diff @@
## branch-21.06 #3821 +/- ##
===============================================
Coverage ? 85.13%
===============================================
Files ? 230
Lines ? 18138
Branches ? 0
===============================================
Hits ? 15441
Misses ? 2697
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
We had originally tried to ease the review process by separating the Python and CUDA/C++ layer code for HDBSCAN (see NVIDIA#3773), and for the most part we have been separating the commits, however this made it very cumbersome to keep cherry-picking commits backwards so we abandoned NVIDIA#3773 and continued working on this PR. Authors: - Corey J. Nolet (https://github.com/cjnolet) - Divye Gala (https://github.com/divyegala) Approvers: - Tamas Bela Feher (https://github.com/tfeher) - Dante Gama Dessavre (https://github.com/dantegd) URL: NVIDIA#3821
6 participants
We had originally tried to ease the review process by separating the Python and CUDA/C++ layer code for HDBSCAN (see #3773), and for the most part we have been separating the commits, however this made it very cumbersome to keep cherry-picking commits backwards so we abandoned #3773 and continued working on this PR.