Skip to content

FIX Update pairwise distance function argument names#26351

Merged
jjerphan merged 5 commits intoscikit-learn:mainfrom
Micky774:update_argkminclassmode_args
Jul 25, 2023
Merged

FIX Update pairwise distance function argument names#26351
jjerphan merged 5 commits intoscikit-learn:mainfrom
Micky774:update_argkminclassmode_args

Conversation

@Micky774
Copy link
Copy Markdown
Contributor

@Micky774 Micky774 commented May 10, 2023

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Updates arguments to conform to the names originally decided in #24076

Any other comments?

cc: @jjerphan @ogrisel

jjerphan
jjerphan previously approved these changes May 10, 2023
View reviewed changes
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.

I am +0 regarding this change, but if we agreed on those names at a point, let us just use them.

@Micky774 Micky774 added Quick Review For PRs that are quick to review Waiting for Second Reviewer First reviewer is done, need a second one! labels May 10, 2023
thomasjpfan
thomasjpfan reviewed May 23, 2023
View reviewed changes
sklearn/metrics/_pairwise_distances_reduction/_dispatcher.py Outdated
Comment on lines +492 to +493
class_membership,
unique_classes,
Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan May 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If I had the chance to rename these, I'll go with:

  • class_membership -> y_labels: Directly connects this parameter with y
  • unique_classes -> labels: This is consistent with the labels kwargs in the metrics module

Copy link
Copy Markdown
Contributor Author

@Micky774 Micky774 May 23, 2023

Choose a reason for hiding this comment

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

Great suggestion. I do much prefer those names. @jjerphan I wonder what you think regarding these.

Copy link
Copy Markdown
Member

@jjerphan jjerphan May 23, 2023

Choose a reason for hiding this comment

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

This indeed looks better (I trust you for coming up with the best, appropriate names).

Copy link
Copy Markdown
Member

@jjerphan jjerphan Jul 21, 2023

Choose a reason for hiding this comment

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

Small heads-up, @Micky774. Is there something missing for this PR? :)

@betatim betatim changed the title FIX Updated arguments FIX Update pairwise distance function argument names Jul 21, 2023
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jul 22, 2023 •
edited
Loading

✔️ Linting Passed

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

Generated for commit: 3a93bde. Link to the linter CI: here

thomasjpfan
thomasjpfan reviewed Jul 23, 2023
View reviewed changes
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.

CI failure looks real.

@jjerphan jjerphan dismissed their stale review July 23, 2023 19:27

Dismissing review due to failure and recent changes

@Micky774
Copy link
Copy Markdown
Contributor Author

Micky774 commented Jul 24, 2023

@jjerphan @thomasjpfan CI error fixed, new variable names adopted. Pinging for review whenever you have time :)

thomasjpfan
thomasjpfan approved these changes Jul 25, 2023
View reviewed changes
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.

I'm okay with the new names. LGTM

@jjerphan jjerphan merged commit 59048f9 into scikit-learn:main Jul 25, 2023
@Micky774 Micky774 deleted the update_argkminclassmode_args branch July 25, 2023 16:41
punndcoder28 pushed a commit to punndcoder28/scikit-learn that referenced this pull request Jul 29, 2023
@Micky774 @punndcoder28
FIX Update pairwise distance function argument names (scikit-learn#26351
55eb419 
)
@Micky774 Micky774 mentioned this pull request Aug 2, 2023
Merged
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
@Micky774 @REDVM
FIX Update pairwise distance function argument names (scikit-learn#26351
7fa995f 
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@jjerphan jjerphan jjerphan approved these changes

Assignees

No one assigned

Labels

module:metrics module:neighbors No Changelog Needed Quick Review For PRs that are quick to review Waiting for Second Reviewer First reviewer is done, need a second one!

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Micky774 @thomasjpfan @jjerphan