Skip to content

MAINT validate parameters in Birch #23593

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 6 commits into from
Jun 28, 2022
Merged

MAINT validate parameters in Birch #23593

merged 6 commits into from
Jun 28, 2022

Conversation

kianelbo
Copy link
Contributor

Reference Issues/PRs

towards #23462

What does this implement/fix? Explain your changes.

Added _parameter_constraints for Birch and removed the existing individual param checks.

@kianelbo kianelbo mentioned this pull request Jun 11, 2022
Closed
@kianelbo kianelbo force-pushed the birch-validation branch 2 times, most recently from 3249cd8 to 7a44078 Compare June 12, 2022 15:59
@kianelbo kianelbo changed the title Use _validate_params in Birch [MRG] Use _validate_params in Birch Jun 12, 2022
@jeremiedbb jeremiedbb added the Validation related to input validation label Jun 13, 2022
@glemaitre glemaitre changed the title [MRG] Use _validate_params in Birch MAINT Use _validate_params in Birch estimator Jun 13, 2022
@jeremiedbb jeremiedbb mentioned this pull request Jun 14, 2022
Merged
@glemaitre glemaitre self-requested a review June 27, 2022 08:17
@glemaitre glemaitre changed the title MAINT Use _validate_params in Birch estimator MAINT validate parameters in Birch Jun 27, 2022
glemaitre
glemaitre approved these changes Jun 27, 2022
View reviewed changes
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. I think that we should let the test that checks for passing a non-ClusterMixin estimator since it is not covered by the common test specifically.

jeremiedbb
jeremiedbb approved these changes Jun 28, 2022
View reviewed changes
Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM. I just updated the docstring to reflect the fact that None is valid for n_clusters

@jeremiedbb jeremiedbb merged commit 24c2448 into scikit-learn:main Jun 28, 2022
@jeremiedbb
Copy link
Member

Thanks @kianelbo

@kianelbo kianelbo deleted the birch-validation branch June 28, 2022 13:25
ogrisel pushed a commit to ogrisel/scikit-learn that referenced this pull request Jul 11, 2022
@kianelbo @glemaitre
@jeremiedbb @ogrisel
MAINT validate parameters in Birch (scikit-learn#23593)
de15ae6 
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: jeremiedbb <jeremiedbb@yahoo.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasjpfan thomasjpfan thomasjpfan left review comments

@glemaitre glemaitre glemaitre approved these changes

@jeremiedbb jeremiedbb jeremiedbb approved these changes

Assignees
No one assigned
Labels
module:cluster No Changelog Needed Validation related to input validation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kianelbo @jeremiedbb @thomasjpfan @glemaitre