Skip to content
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

ENH Add missing base.is_clusterer() function #28936

Conversation

ChVeen
Copy link
Contributor

@ChVeen ChVeen commented May 2, 2024

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?

Copy link

github-actions bot commented May 2, 2024

✔️ Linting Passed

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

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

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.

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

@adrinjalali
Copy link
Member

Needs a changelog introducing the new function.

@ChVeen
Copy link
Contributor Author

ChVeen commented May 6, 2024

Needs a changelog introducing the new function.

Thanks for the hint. Added to changelog...

@ChVeen
Copy link
Contributor Author

ChVeen commented May 7, 2024

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
Member

Yes, the changelog would need to be moved.

@ChVeen
Copy link
Contributor Author

ChVeen commented May 8, 2024

Moved the changelog entry from version 1.5 to 1.6.

Copy link
Member

@glemaitre glemaitre left a comment

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.

doc/modules/classes.rst Outdated Show resolved Hide resolved
doc/api_reference.py Outdated Show resolved Hide resolved
Copy link
Member

@glemaitre glemaitre left a comment

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
Contributor

Do we need the versionadded thing when adding new API?

@Charlie-XIAO
Copy link
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
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants