Skip to content

ENH Add missing base.is_clusterer() function#28936

Merged
Charlie-XIAO merged 16 commits into
scikit-learn:mainfrom
ChVeen:ENH-Add-missing-base.is_clusterer()-function
May 22, 2024
Merged

ENH Add missing base.is_clusterer() function#28936
Charlie-XIAO merged 16 commits into
scikit-learn:mainfrom
ChVeen:ENH-Add-missing-base.is_clusterer()-function

Conversation

@ChVeen

@ChVeen ChVeen commented May 2, 2024

Copy link
Copy Markdown
Contributor

Reference Issues/PRs

Fixes #28960

What does this implement/fix? Explain your changes.

This PR proposes to add the missing base.is_clusterer() function analogously to base.is_classifier().
There is a user demand for this as can be seen in discussion #28904.
The missing unit test for base.is_regressor() is added as well.

Any other comments?

@github-actions

github-actions Bot commented May 2, 2024

Copy link
Copy Markdown

✔️ Linting Passed

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

Generated for commit: 1a5df50. Link to the linter CI: here

@adrinjalali adrinjalali left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since this is not introducing clusterer as a new thing, and it already exists, I'm okay with it.

@adrinjalali

Copy link
Copy Markdown
Member

Needs a changelog introducing the new function.

@ChVeen

ChVeen commented May 6, 2024

Copy link
Copy Markdown
Contributor Author

Needs a changelog introducing the new function.

Thanks for the hint. Added to changelog...

@ChVeen

ChVeen commented May 7, 2024

Copy link
Copy Markdown
Contributor Author

BTW: I added this new function to the changelog of version 1.5.
If this PR is about to be merged after releasing 1.5, I've to shift the changelog entry to the new version 1.6.

@adrinjalali

Copy link
Copy Markdown
Member

Yes, the changelog would need to be moved.

@ChVeen

ChVeen commented May 8, 2024

Copy link
Copy Markdown
Contributor Author

Moved the changelog entry from version 1.5 to 1.6.

@glemaitre glemaitre left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should also add the function into the API reference in the file doc/modules/classes.rst under the function of the module sklearn.base.

Comment thread sklearn/tests/test_base.py Outdated
Comment thread doc/modules/classes.rst Outdated
Comment thread doc/api_reference.py

@glemaitre glemaitre left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks @ChVeen

@Charlie-XIAO

Copy link
Copy Markdown
Contributor

Do we need the versionadded thing when adding new API?

Comment thread sklearn/base.py
@Charlie-XIAO

Copy link
Copy Markdown
Contributor

LGTM, I'm enabling auto-merge. Thanks @ChVeen!

@Charlie-XIAO Charlie-XIAO enabled auto-merge (squash) May 22, 2024 09:59
@Charlie-XIAO Charlie-XIAO merged commit e16a6dd into scikit-learn:main May 22, 2024
@jeremiedbb jeremiedbb mentioned this pull request Jul 2, 2024
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Base function to check if the model is a clusterer (analogous to base.is_classifier() and base.is_regressor())?

4 participants