Skip to content

DOC Update paired_manhattan_distances and make it pass numpydoc validation#23900

Merged
thomasjpfan merged 4 commits intoscikit-learn:mainfrom
philipp-jung:documentation_paired_manhattan_distances
Jul 16, 2022
Merged

DOC Update paired_manhattan_distances and make it pass numpydoc validation#23900
thomasjpfan merged 4 commits intoscikit-learn:mainfrom
philipp-jung:documentation_paired_manhattan_distances

Conversation

@philipp-jung
Copy link
Copy Markdown
Contributor

Reference Issues/PRs

What does this implement/fix? Explain your changes.

I added additional documentation to paired_manhattan_distances to make
it pass numpydoc validation as described in issue #21350.

Any other comments?

While doing this, I created a small example. I wrote the documentation
bearing in mind the docstrings of manhattan_distances and
paired_distances. #europython22

I added additional documentation to `paired_manhattan_distances` to make
it pass numpydoc validation as described in issue #21350.

While doing this, I created a small example. I wrote the documentation
bearing in mind the docstrings of `manhattan_distances` and
`paired_distances`.
@Micky774
Copy link
Copy Markdown
Contributor

Hey there @philipp-jung, thanks for the PR! Per #21350,

Include the function name in the title of the pull request. For example: "DOC Ensures that config_context passes numpydoc validation".

could you change your title to something a bit more descriptive?

@philipp-jung philipp-jung changed the title Addresses #21350 DOC Update paired_manhattan_distances and make it pass numpydoc validation Jul 14, 2022
@philipp-jung
Copy link
Copy Markdown
Contributor Author

I missed that instruction, thank you for pointing it out @Micky774 :-)

Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@philipp-jung philipp-jung requested a review from thomasjpfan July 15, 2022 11:45
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Copy link
Copy Markdown
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

LGTM.

Thank you, @philipp-jung. 🍀

"""Compute the paired L1 distances between X and Y.

Distances are calculated between (X[0], Y[0]), (X[1], Y[1]), ...,
(X[n_samples], Y[n_samples]).
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I am off by 1 here -- if shape(X) == shape(Y) == (n_samples, n_features) == (2, 2), then distances are calculated for (X[0], Y[0]), (X[1], Y[1]). Do you agree @jjerphan?

Suggested change
(X[n_samples], Y[n_samples]).
(X[n_samples - 1], Y[n_samples - 1]).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's right.

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Thank you for the updates! LGTM

@thomasjpfan thomasjpfan merged commit 912a717 into scikit-learn:main Jul 16, 2022
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Aug 4, 2022
…idation (scikit-learn#23900)

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
glemaitre pushed a commit that referenced this pull request Aug 5, 2022
…idation (#23900)

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
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.

4 participants