Skip to content

MAINT: Remove np.in1d and np.trapz usages #27140

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 5 commits into from
Sep 6, 2023
Merged

MAINT: Remove np.in1d and np.trapz usages #27140

merged 5 commits into from
Sep 6, 2023

Conversation

mtsokol
Copy link
Contributor

@mtsokol mtsokol commented Aug 23, 2023

Hi!
This PR changes np.in1d calls to np.isin as np.in1d is being made private in numpy/numpy#24445.

In three cases I wasn't sure if arrays passed to it are surely 1d (so higher dimensional), therefore I called .ravel() to fully reproduce in1d behavior.
If any of these lines actually operate on 1d arrays, then isin and in1d can be used interchangeably.

Also np.trapz is removed in favor of scipy.interpolate.trapezoid (same implementation).

@github-actions
Copy link

github-actions bot commented Aug 23, 2023 •
edited
Loading

✔️ Linting Passed

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

Generated for commit: 1888f65. Link to the linter CI: here

@mtsokol mtsokol force-pushed the remove-in1d-usages branch from ade66be to 451dbea Compare August 23, 2023 10:01
@mtsokol
Copy link
Contributor Author

mtsokol commented Aug 30, 2023 •
edited
Loading

Hi! As numpy/numpy#24445 will be merged soon, would there be a chance for a review?

rgommers
rgommers reviewed Aug 30, 2023
View reviewed changes
Copy link
Contributor

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks @mtsokol. I'd recommend a tweak to the trapezoid imports. Other than that it looks good to me.

sklearn/metrics/_plot/tests/test_precision_recall_display.py Outdated
@@ -2,6 +2,7 @@

import numpy as np
import pytest
from scipy.integrate import trapz as trapezoid
Copy link
Contributor

Choose a reason for hiding this comment

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

The preferred name in scipy has been trapezoid since 1.6.0, so you should probably try-import that first, and only use import trapz as a fallback with a note that the fallback can be removed if 1.6.0 becomes the minimum supported scipy version (it's 1.5.0 now).

We'll remove trapz at some point, so this would be more future-proof.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's clear - I added try except for all three imports.

betatim
betatim approved these changes Aug 30, 2023
View reviewed changes
Copy link
Member

@betatim betatim left a comment

Choose a reason for hiding this comment

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

Thanks for coming along to fix things that will be broken :)

I think the changes looks reasonable.

Is there a reason to be in a hurry with merging this? It looks like the PR hasn't been merged yet. And once it has been merged it will take a moment to trickle through to the CI run against the nightlies

@rgommers
Copy link
Contributor

You're testing against numpy nightlies, so merging the fixes before the breaking numpy nightly arrives is always useful. We try to be careful in numpy to not merge things that we can see are going to break CI for scikit-learn or another project, but we won't wait for weeks.

@mtsokol mtsokol force-pushed the remove-in1d-usages branch from 0a406d6 to e522e01 Compare August 30, 2023 17:59
rgommers
rgommers approved these changes Sep 1, 2023
View reviewed changes
Copy link
Contributor

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

LGTM now

@rgommers
Copy link
Contributor

rgommers commented Sep 5, 2023

This change was merged into numpy's main branch just now. I recommend hitting the green button here.

@lesteve
Copy link
Member

lesteve commented Sep 6, 2023 •
edited
Loading

Thanks a lot for the PR:

  • I move the try/except import to sklearn/utils/fixes.py which is where we generally put this kind of compatibility code.
  • I removed the .ravel because I think they are not needed, let's see what the CI has to say about this 🤞

@lesteve
Copy link
Member

lesteve commented Sep 6, 2023

Thanks a lot for the PR, merging this one!

Not too sure about the coverage drop and why it only appears in the last commit and not previously so I am going to ignore it. I did test manually this code branch and it seems fine.

@lesteve lesteve merged commit cb15a82 into scikit-learn:main Sep 6, 2023
@mtsokol mtsokol deleted the remove-in1d-usages branch September 6, 2023 16:34
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Sep 18, 2023
@mtsokol @lesteve @glemaitre
MAINT: Remove np.in1d and np.trapz usages (scikit-learn#27140)
10cd21d 
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
jeremiedbb pushed a commit that referenced this pull request Sep 20, 2023
@mtsokol @lesteve @jeremiedbb
MAINT: Remove np.in1d and np.trapz usages (#27140)
54eeee2 
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
@mtsokol @lesteve @REDVM
MAINT: Remove np.in1d and np.trapz usages (scikit-learn#27140)
53c7812 
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgommers rgommers rgommers approved these changes

@betatim betatim betatim approved these changes

Assignees
No one assigned
Labels
No Changelog Needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mtsokol @rgommers @lesteve @betatim