Skip to content

DOC Improve pairwise_distances docstring #31176

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.

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

Merged
merged 5 commits into from
Apr 16, 2025

Conversation

lucyleeow
Copy link
Member

@lucyleeow lucyleeow commented Apr 11, 2025

Reference Issues/PRs

  • more explicitly specifies what the function does, and clarifies things (e.g., it can take one or two arrays)
  • makes pairwise_distances docstring to be consistent with pairwise_kernels (which was updated in DOC Improve pairwise_kernel docstring #31103)

What does this implement/fix? Explain your changes.

Any other comments?

Also noticed that we use the term "vector array" in the docstring description, but later in the parameter description, we use the term "feature array" - should we be more consistent? Maybe some users will not quickly realise these mean the same thing.

cc @jeremiedbb @thomasjpfan who reviewed the previous RP

Copy link

github-actions bot commented Apr 11, 2025

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: fbe1b43. Link to the linter CI: here

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. Just a couple of nitpicks

@@ -2285,26 +2285,31 @@ def pairwise_distances(
):
"""Compute the distance matrix from a vector array X and optional Y.

This method takes either a vector array or a distance matrix, and returns
This method takes one or two vector arrays or a distance matrix, and returns
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a function.

Suggested change
This method takes one or two vector arrays or a distance matrix, and returns
This function takes one or two vector arrays or a distance matrix, and returns

let's update pairwise kernels as well

@lucyleeow
Copy link
Member Author

Thanks @jeremiedbb ! Changes made.

Any thoughts on:

Also noticed that we use the term "vector array" in the docstring description, but later in the parameter description, we use the term "feature array" - should we be more consistent? Maybe some users will not quickly realise these mean the same thing.

@jeremiedbb
Copy link
Member

I find feature array more explicit.

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 let you decide if you want to uniformize vector/feature array :)

@lucyleeow
Copy link
Member Author

Thanks @jeremiedbb ! I've amended to "feature array" - there was one use case of "vector array" in euclidean_distances which I have also changed.

@jeremiedbb
Copy link
Member

Thanks

@jeremiedbb jeremiedbb enabled auto-merge (squash) April 16, 2025 08:20
@jeremiedbb jeremiedbb merged commit 853b34d into scikit-learn:main Apr 16, 2025
34 checks passed
@lucyleeow lucyleeow deleted the doc_pwdist branch April 16, 2025 09:24
lucyleeow added a commit to EmilyXinyi/scikit-learn that referenced this pull request Apr 23, 2025
Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants