Skip to content

MAINT validate parameters in Birch#23593

Merged
jeremiedbb merged 6 commits intoscikit-learn:mainfrom
kianelbo:birch-validation
Jun 28, 2022
Merged

MAINT validate parameters in Birch#23593
jeremiedbb merged 6 commits intoscikit-learn:mainfrom
kianelbo:birch-validation

Conversation

@kianelbo
Copy link
Copy Markdown
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 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
@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
Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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
Copy Markdown
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
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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants