Skip to content
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

MAINT: update minimal NumPy version #5340

Merged
merged 9 commits into from
May 3, 2021
Merged

Conversation

tupui
Copy link
Contributor

@tupui tupui commented Apr 21, 2021

Description

Since the minimal version of Python is 3.6 and NumPy is 1.16.5, you could update minimal SciPy to 1.5.4. SciPy 1.2.3 is quite old now and was the last to be fully compatible with Python 2.

https://scipy.github.io/devdocs/dev/toolchain.html#numpy

Since the minimal version of Python is 3.6 and NumPy is 1.16.5, you could update minimal SciPy to 1.5.4. SciPy 1.2.3 is quite old now and was the last to be fully compatible with Python 2.

https://scipy.github.io/devdocs/dev/toolchain.html#numpy
@jni
Copy link
Member

jni commented Apr 21, 2021

@tupui Thanks for contributing! But we use NEP-29 as a guide for how long to keep our minimal dependencies. This means we support SciPy versions released 24 months ago, which works out to 1.2.0. In May we can switch to 1.3.0. Try:

$ pip install nep29

then

$ nep29 scipy
| version |    date    |
|---------|------------|
|  1.6.0  | 2020-12-31 |
|  1.5.0  | 2020-06-21 |
|  1.4.0  | 2019-12-16 |
|  1.3.0  | 2019-05-17 |
|  1.2.0  | 2018-12-18 |

@jni jni closed this Apr 21, 2021
@tupui
Copy link
Contributor Author

tupui commented Apr 21, 2021

Thanks @jni for pointing out this module. But then you are closing this a bit fast as NumPy is not correctly set to 1.17.0. And doing this, you would not be compliant with SciPy as we are stating that the maximal for 1.2.x is NumPy 1.16.x.

So if you don't plan a release before May, I would suggest to use SciPy 1.3.x and NumPy 1.17.x.

@jni
Copy link
Member

jni commented Apr 21, 2021

we are stating that the maximal for 1.2.x is NumPy 1.16.x.

But that is something for pip's resolver to work out, not humans. =) I think! Anyway I'll gladly merge a PR updating our minimal NumPy to 1.7.0! pip can then dismiss 1.2 for us until May. =) (It's unclear whether we'll release before that deadline...)

@tupui
Copy link
Contributor Author

tupui commented Apr 21, 2021

I updated the branch with the NumPy update. (I guess this needs to be re-open in order to see the new commit.)

@grlee77 grlee77 reopened this Apr 21, 2021
@grlee77 grlee77 changed the title MAINT: minimal SciPy version MAINT: update minimal NumPy version Apr 21, 2021
@grlee77
Copy link
Contributor

grlee77 commented Apr 21, 2021

Thanks @tupui, I have reopened the issue and modified its title.

I don't think bumping SciPy from 1.2 to 1.3 buys us any new features we need. The two things in newer SciPy I am aware of that will help us here from a code maintenance standpoint are:
1.) SciPy 1.4 will let us use the scipy.fft module unconditionally
2.) SciPy 1.6 will let us always use the new scipy.ndimage features in that release

@hmaarrfk
Copy link
Member

@tupui there are a few other places where the numpy version is used. Please review the changes that I pushed. so you can get a good picture of them.

A bump in numpy also requires a review of the TODO.txt knocking out the items in the TODO list.
https://github.com/scikit-image/scikit-image/blob/main/TODO.txt#L85

@tupui
Copy link
Contributor Author

tupui commented Apr 21, 2021

Thanks @tupui, I have reopened the issue and modified its title.

I don't think bumping SciPy from 1.2 to 1.3 buys us any new features we need. The two things in newer SciPy I am aware of that will help us here from a code maintenance standpoint are:
1.) SciPy 1.4 will let us use the scipy.fft module unconditionally
2.) SciPy 1.6 will let us always use the new scipy.ndimage features in that release

Thanks! On my side, it was more to encourage users not to rely on an old version. Since then we did not just add features but fixed tons of issues.

@tupui
Copy link
Contributor Author

tupui commented Apr 21, 2021

@tupui there are a few other places where the numpy version is used. Please review the changes that I pushed. so you can get a good picture of them.

Oh ok, sorry I did not see that you defined the dependencies in other places.

A bump in numpy also requires a review of the TODO.txt knocking out the items in the TODO list.
https://github.com/scikit-image/scikit-image/blob/main/TODO.txt#L85

I will remove the warning assertion. Not sure about the rest, also maybe better for another PR?

@tupui
Copy link
Contributor Author

tupui commented Apr 21, 2021

@hmaarrfk I updated the PR with 2 items from the list. I did not touch the _round_safe so there is still a loop.

do not make this warning optional now that numpy 1.16 has been dropped
@pep8speaks
Copy link

pep8speaks commented Apr 21, 2021

Hello @tupui! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-04-21 16:48:35 UTC

Comment on lines -85 to -88
* When ``numpy`` is set to >= 1.16, remove the warning assertion in
``skimage/exposure/tests/test_exposure.py::test_rescale_nan_warning``
regarding ``invalid value encountered in reduce``.
regarding ``Passing `np.nan` to mean no clipping in np.clip``.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this instruction was incorrect. The warning did not occur on old NumPy, but does on 1.17+. I pushed a commit to restore the warning, but make it non-optional now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, this does not actually appear to warn in my local test environment. I cancelled CI here for now and will investigate further...

Copy link
Contributor

Choose a reason for hiding this comment

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

Hopefully CI will pass now. It turns out that only some of the parameterized test cases try to clip using NaN values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah ok! Thanks for working on the PR btw, really cool!

@tupui
Copy link
Contributor Author

tupui commented Apr 21, 2021

As a side note, since we are having NP >= 1.17 now, I could propose a follow-up PR to update the usage of np.random.RandomState to the recommended np.random.Generator. I did quite some work on SciPy around the topic so I have a good view of the problem and possible solutions.

@hmaarrfk
Copy link
Member

As a side note, since we are having NP >= 1.17 now, I could propose a follow-up PR to update the usage of np.random.RandomState to the recommended np.random.Generator. I did quite some work on SciPy around the topic so I have a good view of the problem and possible solutions.

That would be great. Random number usage is always finicky and i think the new api improves on that.

Could you please open a separate issue so we can track progress.

Also, i would like to suggest postponing merging this due to the impending release of 0.18.2.

Since this touches infrastructure, it would make certain infrastructure fixes harder to backport. (Unless we backport this too)

@tupui
Copy link
Contributor Author

tupui commented Apr 21, 2021

Sure no problem 👌

Copy link
Member

@jni jni left a comment

Choose a reason for hiding this comment

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

Awesome, thank you @tupui! (and @hmaarrfk and @grlee77!)

@tupui
Copy link
Contributor Author

tupui commented Apr 22, 2021

If this is not merged in May, I would then bump SciPy here too as to follow this nep29.

@alexdesiqueira alexdesiqueira added the 🔧 type: Maintenance Refactoring and maintenance of internals label Apr 26, 2021
Copy link
Contributor

@grlee77 grlee77 left a comment

Choose a reason for hiding this comment

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

This looks good now. The only failure on CI was a common timeout, so I will go ahead and merge.

@grlee77 grlee77 merged commit 3ccd918 into scikit-image:main May 3, 2021
@tupui tupui deleted the patch-1 branch May 3, 2021 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔧 type: Maintenance Refactoring and maintenance of internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants