8000 ENH Add `HDBSCAN` as a new estimator in `sklearn.cluster` by Micky774 · Pull Request #26385 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

ENH Add HDBSCAN as a new estimator in sklearn.cluster #26385

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
Merged
merged 24 commits into from
May 31, 2023
Merged

ENH Add HDBSCAN as a new estimator in sklearn.cluster #26385

merged 24 commits into from
May 31, 2023

Conversation

Micky774
Copy link
Contributor
@Micky774 Micky774 commented May 16, 2023 •
edited
Loading

Reference Issues/PRs

Towards #24686

What does this implement/fix? Explain your changes.

Each change has been separately reviewed (see #24686 for details).

Edit: due to git shenanigans, some changes needed to be made within this PR. The novel changes included w/ this PR are:

  1. Replaced cnp.*_t typing with *_t from _typedefs.pxd
  2. Replaced *.shape[0] pattern with len(*) for ndarray objects
  3. Trimmed unused variables (thanks to Cython linting pre-commit)

Any other comments?

cc: @thomasjpfan @jjerphan @glemaitre

Micky774 and others added 11 commits May 16, 2023 12:35
@Micky774 @thomasjpfan
@jjerphan @glemaitre
Include HDBSCAN as a sub-module for sklearn.cluster (scikit-learn…
bd04bda 
…#22616)

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@Micky774 @thomasjpfan @jjerphan
CLN Cleaned cluster/_hdbscan/_linkage.pyx (scikit-learn#24857)
488fb1f 
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
@Micky774 @jjerphan
DOC Adds HDBSCAN.dbscan_clustering section to plot_hdbscan.py (sc…
df7cf3a 
…ikit-learn#25538)

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
@Micky774 @thomasjpfan @jjerphan
ENH Extends outlier encoding scheme to HDBSCAN.dbscan_clustering (s…
800c2a6 
…cikit-learn#24698)

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
@Micky774 @jjerphan
@thomasjpfan @glemaitre
CLN Cleaned cluster/_hdbscan/_reachability.pyx (scikit-learn#24701)
29c53b7 
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@Micky774
CLN Update cluster/_hdbscan/_tree.pyx style and syntax (scikit-lear…
70ae320 
…n#25768)
@Micky774 @jjerphan
CLN Cleaned TreeUnionFind in _hdbscan/_tree.pyx (scikit-learn#25827)
a6e061e 
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
@Micky774 @jjerphan
ENH _hdbscan HIERARCHY data type introduction (scikit-learn#25826)
d013d38 
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
@Micky774 @thomasjpfan
CLN BFS Style Improvement (scikit-learn#26096)
792833a 
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@Micky774
CLN _hdbscan/_tree.pyx algorithms refactor (scikit-learn#26011)
c02c455
@Micky774
CLN HDBSCAN _tree.pyx::do_labelling refactor (scikit-learn#26101)
607dea8
@jjerphan
Copy link
Member
jjerphan commented May 16, 2023 •
edited
Loading

Since all PRs target the branch of this PR have been reviewed, merged, and are referenced in the issue, I thinking making the CI pass is sufficient to accept this PR.

What do you think?

@Micky774 Micky774 marked this pull request as draft May 16, 2023 20:59
@Micky774
Copy link
Contributor Author

I am going to mark it as a draft for now to resolve CI issues (it seems some changes weren't carried over due to git shenanigans) but will reopen once those are addressed.

@Micky774
Copy link
Contributor Author
Micky774 commented May 21, 2023 •
edited
Loading

Should be good for review now @jjerphan @thomasjpfan!

Since all PRs target the branch of this PR have been reviewed, merged, and are referenced in the issue, I thinking making the CI pass is sufficient to accept this PR.

There are some not-yet-reviewed changes. For your convenience, here are the novel changes (all of which are included in these five commits)

These changes are comprised of:

  1. Deletion of submodule-level setup.py files corresponding to old build strategy
  2. Update to setup.py for building _hdbscan submodule
  3. Sphinx doc appeasement in clustering.rst, plot_hdbscan.py, and hdbscan.py
  4. Updated sklearn/cluster/_hierarchical_fast.pyx::UnionFind to avoid redeclaration of attributes and use new typings
  5. Updated shape semantics in Cython files (len for lists/sets, .shape[i] for memoryviews, PyArray_SHAPE for ndarray objects)

@Micky774 Micky774 marked this pull request as ready for review May 21, 2023 00:41
jjerphan
jjerphan approved these changes May 22, 2023
View reviewed changes
Copy link
Member
@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

LGTM.

Thank you for bringing HDBSCAN in scikit-learn, @Micky774!

Here are just minor comments.

@jjerphan
Copy link
Member

I think it is reasonable to wait for a third reviewer to approve this PR before merging it.

@Micky774
Copy link
Contributor Author

cc: @scikit-learn/core-devs in case anyone would like to donate a third review :)

adrinjalali
adrinjalali reviewed May 23, 2023
View reviewed changes
Copy link
Member
@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

The copyright here: https://github.com/scikit-learn-contrib/hdbscan/blob/master/LICENSE isn't really BSD, and doesn't allow to just use it, to me it seems we need a copy of that file here? Unless that copyright is changed or we somehow get a written permission from all contributors to that package.

I left a few comments just to make a few things more readable, otherwise I'm happy to merge.

@Micky774
Copy link
Contributor Author

@adrinjalali Should be ready for a second-pass :)

adrinjalali
adrinjalali approved these changes May 31, 2023
View reviewed changes
Copy link
Member
@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

PRETTY KEWL!!!

@adrinjalali adrinjalali merged commit 415b11a into scikit-learn:main May 31, 2023
@jjerphan
Copy link
Member

Thank you for working on integrating HDBSCAN in scikit-learn, @Micky774! 🎉

@Micky774 Micky774 deleted the hdbscan_sync branch May 31, 2023 14:05
@Micky774
Copy link
Contributor Author

Thank you to everyone for all your help with this! This was a massive collaboration and I greatly appreciate your persistence and patience with review and API discussions <3

@lmcinnes Wanted to ping you as well! Thanks again to you and your team for the fantastic original implementation.

The first major hurdle is done, now to slowly reintroduce some functionality (e.g. boruvka), optimize, and extend support (e.g. float32 data).

@Micky774 Micky774 mentioned this pull request May 31, 2023
Closed
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
@Micky774 @thomasjpfan
@jjerphan @glemaitre @REDVM
FEAT Add HDBSCAN as a new estimator in sklearn.cluster (scikit-le…
a447987 
…arn#26385)

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jjerphan jjerphan jjerphan approved these changes

@adrinjalali adrinjalali adrinjalali approved these changes

@thomasjpfan thomasjpfan thomasjpfan approved these changes

Assignees
No one assigned
Labels
cython module:cluster module:utils
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Micky774 @jjerphan @adrinjalali @thomasjpfan
0