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

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

wants to merge 4 commits into from

Conversation

Charlie-XIAO
Copy link
Contributor

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

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

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

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

@Charlie-XIAO
Copy link
Contributor Author

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