Skip to content

MAINT validate parameter in MDS #23650

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 parameter in MDS #23650

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 MDS and removed the existing individual param checks.

@kianelbo kianelbo changed the title Use _validate_params in MDS [MRG] Use _validate_params in MDS Jun 16, 2022
@jeremiedbb jeremiedbb added No Changelog Needed Validation related to input validation labels Jun 21, 2022
@Micky774
Copy link
Contributor

Looks like there was a failure on the CI side of things (I don't think due to this PR). Perhaps merge main and push changes to prompt a new round of CI checks

Micky774
Micky774 reviewed Jun 23, 2022
View reviewed changes
@kianelbo @Micky774
Use the new 'verbose' constraint for MDS params validations
6b4db07 
Co-authored-by: Meekail Zain <34613774+Micky774@users.noreply.github.com>
@Micky774
Copy link
Contributor

@jeremiedbb In this case since MDS is essentially a light wrapper around the smacof function, would it be appropriate to include validation of that function via decorator in this PR, or would you prefer these PRs to stay focused on only the estimator API?

@glemaitre
Copy link
Member

would you prefer these PRs to stay focused on only the estimator API?

I think this is better to stay focused on the class for the moment.

@glemaitre glemaitre self-requested a review June 27, 2022 09:55
@glemaitre glemaitre changed the title [MRG] Use _validate_params in MDS MAINT validate parameter in MDS 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

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 pushed a fix to the constraint of eps

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

Thanks @kianelbo

@kianelbo kianelbo deleted the mds-validate branch June 28, 2022 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Micky774 Micky774 Micky774 left review comments

@glemaitre glemaitre glemaitre approved these changes

@jeremiedbb jeremiedbb jeremiedbb approved these changes

Assignees
No one assigned
Labels
module:manifold 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 @Micky774 @glemaitre @jeremiedbb