Skip to content

MAINT Deprecate matching as metric #26264

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
Merged
merged 10 commits into from
May 19, 2023
Merged

MAINT Deprecate matching as metric #26264

merged 10 commits into from
May 19, 2023

Conversation

magnusbarata
Copy link
Contributor

Reference Issues/PRs

Fixes #25532.

What does this implement/fix? Explain your changes.

Deprecate matching as a metric to be consistent with scipy.spatial.distance.
Deprecation is done similar to #25417.

jjerphan
jjerphan requested changes Apr 25, 2023
View reviewed changes
Copy link
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.

Thank you, @magnusbarata for this PR!

Can you add deprecation notes for this metric on relevant user documentation similar to #25417?

To be explicit, I am thinking of this kind of note:

scikit-learn/sklearn/cluster/_optics.py

Lines 94 to 95 in f0d6a9c

.. note::
`'kulsinski'` is deprecated from SciPy 1.9 and will removed in SciPy 1.11.

@magnusbarata
Copy link
Contributor Author

@jjerphan Thankyou for your review!

I have 2 questions:

  1. scikit-learn/sklearn/metrics/pairwise.py

    Line 653 in f0d6a9c

    - from scipy.spatial.distance: ['braycurtis', 'canberra', 'chebyshev',

    Is it okay if I add the note here, although matching is not even listed in this list of metrics?

  2. https://github.com/scipy/scipy/blob/c1ed5ece8ffbf05356a22a8106affcd11bd3aee0/scipy/spatial/distance.py#L1831
    Looking at this line in Scipy, it seems they still allowed matching as a synonym for hamming. If that the case, instead of completely removing matching, should I proceed with only removing matching as a boolean metric?

@jjerphan
Copy link
Member

jjerphan commented May 1, 2023

Hi, then I think we should remove hamming as well.

magnusbarata
magnusbarata commented May 2, 2023
View reviewed changes
Copy link
Contributor Author

@magnusbarata magnusbarata left a comment

Choose a reason for hiding this comment

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

https://github.com/scipy/scipy/blob/c1ed5ece8ffbf05356a22a8106affcd11bd3aee0/scipy/spatial/distance.py#L1831
Looking at this line in Scipy, it seems they still allowed matching as a synonym for hamming. If that the case, instead of completely removing matching, should I proceed with only removing matching as a boolean metric?

I mean something like this.

@magnusbarata
Copy link
Contributor Author

Hi, then I think we should remove hamming as well.

Are you sure we should remove hamming? Because it is still a valid metric in Scipy.
If we should remove hamming as well, is it okay to add those changes in this PR or should I make another PR?

@jjerphan
Copy link
Member

jjerphan commented May 4, 2023

To me, we must be inline with SciPy's deprecation cycles. Hence if SciPy hasn't deprecated "hamming" as an identifier, we mustn't as well.

@magnusbarata
Copy link
Contributor Author

To me, we must be inline with SciPy's deprecation cycles. Hence if SciPy hasn't deprecated "hamming" as an identifier, we mustn't as well.

I see. Thank you for clarifying that. SciPy hasn't deprecated hamming as an identifier, so I will keep it as is.

Can you add deprecation notes for this metric on relevant user documentation similar to #25417?

I have added the deprecation notes.

@magnusbarata magnusbarata requested a review from jjerphan May 8, 2023 15:27
@jjerphan jjerphan changed the title [MRG] Deprecate matching as metric MAINT Deprecate matching as metric May 8, 2023
jjerphan
jjerphan approved these changes May 8, 2023
View reviewed changes
Copy link
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, @magnusbarata.

@magnusbarata @jjerphan
Apply code review suggestions
462d96a 
Change wordings for documentation

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
@magnusbarata
Copy link
Contributor Author

@jjerphan Thank you for your approval, I have applied your suggestions as well. 👍

@jjerphan jjerphan added Quick Review For PRs that are quick to review Waiting for Second Reviewer First reviewer is done, need a second one! labels May 9, 2023
thomasjpfan
thomasjpfan reviewed May 16, 2023
View reviewed changes
Copy link
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 @magnusbarata !

magnusbarata and others added 2 commits May 17, 2023 22:53
@magnusbarata
Copy link
Contributor Author

Hi @thomasjpfan, thank you for your review. I have added your suggestion.

thomasjpfan
thomasjpfan approved these changes May 19, 2023
View reviewed changes
Copy link
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.

LGTM

@thomasjpfan thomasjpfan merged commit 86541f2 into scikit-learn:main May 19, 2023
@magnusbarata magnusbarata deleted the deprecate_matching_as_metric branch May 19, 2023 05:19
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
@magnusbarata @jjerphan
@thomasjpfan @REDVM
MAINT Deprecate matching as metric (scikit-learn#26264)
f924460 
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
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
cython module:metrics module:neighbors 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.

pairwise_distances is inconsistent with scipy.spatial.distance when using metric="matching"
3 participants
@magnusbarata @jjerphan @thomasjpfan