FIX Fix inconsistent naming convention for algorithm selection of HDBSCAN

[Shreesha3112]

Reference Issues/PRs

Fixes #26732

What does this implement/fix? Explain your changes.

• Fixes inconsistent naming convention for algorithm selection in HDBSCAN for "kd_tree" and "ball_tree"
• Maintains backward compatibility for "kdtree" and "balltree" and will be removed in 1.6?

current naming conventions

Other estimators(K-NN, DBSCAN, etc.) : algorithms : {"kd_tree", "bal_tree"}

HDBSCAN: algorithm : {"kdtree", "balltree"}

updated naming convention for HDBSCAN

algorithm : {"kd_tree", "ball_tree"}

[github-actions]

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: ddd14f7. Link to the linter CI: here}

[Shreesha3112]

Patch code coverage failed with partial hit for this line 800 due to missing else block at elif == "ball_tree". Probably not added since parameter validation is completed by validate_parameter_constraints.

[ogrisel]

Thanks for the PR!

Since scikit-learn 1.3.0 has been released with those names, our policy would mandate a deprecation cycle for the inconsistent names:

• https://scikit-learn.org/dev/developers/contributing.html#maintaining-backwards-compatibility

Though since HDBSCAN was just introduced in that release we could maybe do an exception here? Personally I feel we should follow our backward compat policy.

Any opinion @glemaitre and others?

[glemaitre]

I should have pushed for the change before the release :)

When speaking about this issue with @jeremiedbb, we thought to put it as a fix and include it in the next bug fixes release for which HDSCAN would have existed for about 1 month and where there is little chance to break production code.

However, I don't want that we introduce bad practices.

[Shreesha3112]

I can make sure the kd_tree and ball_tree in algorithm parameter value are backward compatible if required. I have found a similar use case that I can use as a reference, which is for depreciating None value for penalty parameter in LogisticRegression which was made in release 1.2

[Micky774]

So, for backstory purposes, the reason that the algorithm names here are inconsistent is because originally they were going to also encode the MST algorithm in their names when we had both prims and boruvka, so something of the sort {prims, boruvka}_{kd, ball}tree just for the sake of readability.

Still, I agree that after trimming boruvka (even if only temporarily) there's no reason for this inconsistency. It totally slipped past me🤦

I'd personally be okay considering it a backport fix, since most likely very few people are working with this new scikit-learn implementation of HDBSCAN in any serious or critical capacity. I think I prefer it, only because there's a lot of work to be done on HDBSCAN in the next few releases, and adding that deprecation is going to make following work potentially more complicated (e.g. if we reintroduce boruvka and decide to encode it in the very same keyword).

[thomasjpfan]

However, I don't want that we introduce bad practices.

Although this estimator is new, I prefer deprecating the parameter. The default is "auto", so it should not impact many users.

[Micky774]

Rather than filterwarnings everywhere, we can go ahead and use the new algorithm names. We need only test that the deprecation works as expected in one place and move on to future behavior everywhere else.

[Micky774]

LGTM, failing CI is unrelated. Thanks!

[thomasjpfan]

LGTM

[Micky774]

Thank you for work @Shreesha3112 😄