Skip to content

MAINT validate parameter in MDS#23650

Merged
jeremiedbb merged 6 commits intoscikit-learn:mainfrom
kianelbo:mds-validate
Jun 28, 2022
Merged

MAINT validate parameter in MDS#23650
jeremiedbb merged 6 commits intoscikit-learn:mainfrom
kianelbo:mds-validate

Conversation

@kianelbo
Copy link
Copy Markdown
Contributor

@kianelbo kianelbo commented Jun 16, 2022

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

Micky774 commented Jun 22, 2022

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 and others added 2 commits June 23, 2022 19:57
@kianelbo
Use _validate_params in MDS
65172f5
@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
Copy Markdown
Contributor

Micky774 commented Jun 23, 2022

@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
Copy Markdown
Member

glemaitre commented Jun 27, 2022

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
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

jeremiedbb
jeremiedbb approved these changes Jun 28, 2022
View reviewed changes
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 pushed a fix to the constraint of eps

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

jeremiedbb commented Jun 28, 2022

Thanks @kianelbo

@kianelbo kianelbo deleted the mds-validate branch June 28, 2022 13:34
ogrisel pushed a commit to ogrisel/scikit-learn that referenced this pull request Jul 11, 2022
@kianelbo @Micky774
@glemaitre @jeremiedbb @ogrisel
MAINT validate parameter in MDS (scikit-learn#23650)
7f12920 
Co-authored-by: Meekail Zain <34613774+Micky774@users.noreply.github.com>
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

@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