Skip to content

MAINT Parameters validation for sklearn.metrics.pairwise.nan_euclidean_distances #26067

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
Closed
Charlie-XIAO wants to merge 4 commits into from
Closed

MAINT Parameters validation for sklearn.metrics.pairwise.nan_euclidean_distances #26067

Charlie-XIAO wants to merge 4 commits into from

Conversation

@Charlie-XIAO
Copy link
Contributor

@Charlie-XIAO Charlie-XIAO commented Apr 3, 2023

Reference Issues/PRs

Towards #24862.

What does this implement/fix? Explain your changes.

Automatic parameters validation for sklearn.metrics.pairwise.nan_euclidean_distances

@glemaitre glemaitre added No Changelog Needed Validation related to input validation labels Apr 4, 2023
glemaitre
glemaitre reviewed Apr 4, 2023
View reviewed changes
glemaitre
glemaitre reviewed Apr 4, 2023
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.

The failure with the KNNImputer makes me think that the docstring here is not up-to-date. If we accept a wider range of values as in KNNImputer, we should probably write a couple of unit tests for the nan_euclidean_distances to be sure that they work.

@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Apr 4, 2023

I checked the tests of KNNImputer, it only tested with -1 and np.nan, as we have in the tests for nan_euclidean_distances. I have updated the docstring correspondingly, but for the reason above, I haven't yet made changes to the tests.

@jeremiedbb
Copy link
Member

jeremiedbb commented Apr 4, 2023

The "missing_values" constraint is not appropriate for this function because it allows more than numerical values.
We discussed it with @glemaitre and the solution is to extend the constraint to be more flexible. I'm going to open a PR and then we'll be able to come back to this.

@jeremiedbb jeremiedbb mentioned this pull request Apr 4, 2023
Merged
@jeremiedbb
Copy link
Member

jeremiedbb commented Apr 4, 2023

I included these changes in #26085 to showcase the need for the extended constraint. Thanks @Charlie-XIAO

@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Apr 4, 2023

Sure, thank you very much!

@Charlie-XIAO Charlie-XIAO deleted the param-val-nan_euclidean_distances branch September 23, 2023 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@glemaitre glemaitre glemaitre left review comments

Assignees

No one assigned

Labels

module:metrics No Changelog Needed Validation related to input validation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Charlie-XIAO @jeremiedbb @glemaitre