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

Test and prepare for NumPy 2.0 #7282

Open
2 tasks done
lagru opened this issue Jan 5, 2024 · 26 comments
Open
2 tasks done

Test and prepare for NumPy 2.0 #7282

lagru opened this issue Jan 5, 2024 · 26 comments
Labels
⬆️ Upstream Needs help from or involves an upstream project 🔧 type: Maintenance Refactoring and maintenance of internals

Comments

@lagru
Copy link
Member

lagru commented Jan 5, 2024

It turns out that "Test nightly" only builds with NumPy 2.0 but tests with NumPy 1.26.2. 🙈 In the last run of "Test nightly" it looks like matplotlib is (indirectly) forcing the downgrade to 1.26? Or maybe its a dependency of matplotlib?

Testing with NumPy 2.0 locally shows that we need to fix our code in a few places. Let's address this in two steps:

  • Fix the issues with NumPy 2.0
  • Make sure we can test the above fixes with NumPy 2.0 in our CI

For reference see #7249 (comment)

@lagru lagru added 🔧 type: Maintenance Refactoring and maintenance of internals ⬆️ Upstream Needs help from or involves an upstream project labels Jan 5, 2024
@lagru lagru added this to the 0.23 milestone Jan 5, 2024
@hmaarrfk
Copy link
Member

hmaarrfk commented Jan 5, 2024

we could add skips to the matplotlib code to remove it as a test dependency for the "--pre" cases.

@rgommers
Copy link

rgommers commented Jan 10, 2024

It'd also be useful if you could do a new release with a numpy<2 constraint, preferably before NumPy 2.0.0rc1 comes out around 1 Feb. The latest release on PyPI only has a lower bound: https://github.com/scikit-image/scikit-image/blob/v0.22.x/pyproject.toml#L29

That release is otherwise going to break with something like: pip install --pre numpy scikit-image.

EDIT: see https://numpy.org/devdocs/dev/depending_on_numpy.html#numpy-2-0-specific-advice for more details.

@lagru
Copy link
Member Author

lagru commented Jan 11, 2024

Thanks for the advice. What I take from the linked guide is:

  • Before NumPy 2.0.0rc1, use an upper bound <2.0 on skimage
  • After NumPy 2.0.0rc1, build with that and remove the bound and quickly release

@rgommers
Copy link

Yes, exactly.

@lagru
Copy link
Member Author

lagru commented Jan 17, 2024

@jarrodmillman, in yesterday's meeting I suggested that I make a minor release 0.22.1 with the numpy<2 constraint. My thoughts were that (a) it's quick to do and we can override / yank it if we get skimage into a state that works with both NumPy 1 & 2 (see #7288). (b) I'd like to try making a release myself and a minor one seems like a good start.

Stéfan suggested to ping you about it. What do you think? :)

@jarrodmillman
Copy link
Contributor

jarrodmillman commented Jan 22, 2024

I am not sure I understand the motivation. As things currently stand, if (in the near future) someone installs NumPy 2 and does pip install scikit-image it will try to install scikit-image 0.22.0. If we release 0.22.1 with the numpy<2 constraint, then if (in the near future) someone installs NumPy 2 and does pip install scikit-image it will try to install scikit-image 0.22.0 (but not 0.22.1). So it seems like the user experience would be identical. Am I missing something?

@lagru
Copy link
Member Author

lagru commented Jan 22, 2024

I think the argument is that if someone uses the --pre flag then pip install --pre numpy scikit-image would yield a broken install once NumPy 2c comes around.

@lagru
Copy link
Member Author

lagru commented Jan 22, 2024

Hmm, but as we've seen already this issue can be annoying for up-stream libraries. matplotlib limiting to NumPy<2.0 now is annoying for us, but we'll probably break other's tests with release candidates if we are not ready by then...

@jarrodmillman
Copy link
Contributor

I think pip will backtrack from 0.22.1 and try 0.22.0, but I am not sure. I believe I tested this before and confirmed that that's what happens. But I may very well be misremembering. I may try to create a test case and check.

@rgommers
Copy link

I think pip will backtrack from 0.22.1 and try 0.22.0, but I am not sure.

That is probably what happens now, but there are discussions around changing the pip behavior to not trying older releases. Not sure of the current status, but it could be improved in pip in the future. At that point, things would work correctly. If there are zero releases with <2 (which is a bug in metadata of older releases), there is no way for it to pick the ever correct numpy.

Even if the pip behavior never changes, other installers (e.g., the new uv) may be better-behaved. And it serves as documentation to distro packagers etc. It's just a "do the right thing here".

For all future releases, please add <3 in release branches to avoid this exact same problem in the future. Not doing so is guaranteed to be wrong, since if there'll ever be a numpy 3.0 it'll again be because of a change in ABI.

@rgommers
Copy link

PyWavelets 1.6.0rc1 is up on PyPI now, in case that helps. 1.6.0 final release will happen as soon as numpy 2.0.0rc1 is up.

@rgommers
Copy link

rgommers commented Apr 3, 2024

With SciPy 1.13.0 and PyWavelets 1.6.0 released, I think you should be able to do a release built against numpy 2.0.0rc1, right?

@lagru
Copy link
Member Author

lagru commented Apr 3, 2024

Oh, we should get on that asap. @jarrodmillman, I was under the impression that 0.23.0rc0 was already built with NumPy 2.0 but local testing and CI (e.g. this job) tells me that's not the case?

I think the easiest way to do so is to temporarily pin our build dependency to NumPy 2.0.0rc1. I think pip then picks up the pre-release even without us having to sneak the --pre flag into the workflow...

@rgommers
Copy link

rgommers commented Apr 3, 2024

I think the easiest way to do so is to temporarily pin our build dependency to NumPy 2.0.0rc1. I think pip then picks up the pre-release even without us having to sneak the --pre flag into the workflow...

Yes indeed - with >= preferably, not ==. See numpy/numpy#24300 (comment).

@lagru
Copy link
Member Author

lagru commented Apr 4, 2024

Addressed in #7367. 🚀

@lagru
Copy link
Member Author

lagru commented Apr 4, 2024

Confirmed that pip install --pre scikit-image pulls down NumPy 2 and skimage.test() passes locally. Note our optional dep astropy still pulls in NumPy <2.

@neutrinoceros
Copy link

neutrinoceros commented Apr 4, 2024

astropy 6.1 will have compat with NumPy 2 and is now on the launchpad

@jarrodmillman
Copy link
Contributor

https://pypi.org/project/scikit-image/0.23.0rc2/

@neutrinoceros
Copy link

https://pypi.org/project/astropy/6.1.0rc1/ 🤗

@jarrodmillman
Copy link
Contributor

@jarrodmillman jarrodmillman removed this from the 0.23 milestone Apr 10, 2024
@rgommers
Copy link

Awesome, thanks @jarrodmillman!

@bnavigator
Copy link
Contributor

I see these errors with scikit-image 0.23.2, numpy 2.0.0rc1 and pywavelet 1.6.0:

[   84s] =================================== FAILURES ===================================
[   84s] _______ test_wavelet_denoising_scaling[True-True-int16-2d multichannel] ________
[   84s] [gw3] linux -- Python 3.10.14 /usr/bin/python3.10
[   84s] 
[   84s] case = '2d multichannel', dtype = <class 'numpy.int16'>, convert2ycbcr = True
[   84s] estimate_sigma = True
[   84s] 
[   84s]     @pytest.mark.parametrize(
[   84s]         "case", ["1d", pytest.param("2d multichannel", marks=xfail_without_pywt)]
[   84s]     )
[   84s]     @pytest.mark.parametrize(
[   84s]         "dtype",
[   84s]         [np.float16, np.float32, np.float64, np.int16, np.uint8],
[   84s]     )
[   84s]     @pytest.mark.parametrize(
[   84s]         "convert2ycbcr", [True, pytest.param(False, marks=xfail_without_pywt)]
[   84s]     )
[   84s]     @pytest.mark.parametrize(
[   84s]         "estimate_sigma", [pytest.param(True, marks=xfail_without_pywt), False]
[   84s]     )
[   84s]     def test_wavelet_denoising_scaling(case, dtype, convert2ycbcr, estimate_sigma):
[   84s]         """Test cases for images without prescaling via img_as_float."""
[   84s]         rstate = np.random.default_rng(1234)
[   84s]     
[   84s]         if case == '1d':
[   84s]             # 1D single-channel in range [0, 255]
[   84s]             x = np.linspace(0, 255, 1024)
[   84s]         elif case == '2d multichannel':
[   84s]             # 2D multichannel in range [0, 255]
[   84s]             x = data.astronaut()[:64, :64]
[   84s]         x = x.astype(dtype)
[   84s]     
[   84s]         # add noise and clip to original signal range
[   84s]         sigma = 25.0
[   84s]         noisy = x + sigma * rstate.standard_normal(x.shape)
[   84s]         noisy = np.clip(noisy, x.min(), x.max())
[   84s]         noisy = noisy.astype(x.dtype)
[   84s]     
[   84s]         channel_axis = -1 if x.shape[-1] == 3 else None
[   84s]     
[   84s]         if estimate_sigma:
[   84s]             sigma_est = restoration.estimate_sigma(noisy, channel_axis=channel_axis)
[   84s]         else:
[   84s]             sigma_est = None
[   84s]     
[   84s]         if convert2ycbcr and channel_axis is None:
[   84s]             # YCbCr requires multichannel == True
[   84s]             with pytest.raises(ValueError):
[   84s]                 denoised = restoration.denoise_wavelet(
[   84s]                     noisy,
[   84s]                     sigma=sigma_est,
[   84s]                     wavelet='sym4',
[   84s]                     channel_axis=channel_axis,
[   84s]                     convert2ycbcr=convert2ycbcr,
[   84s]                     rescale_sigma=True,
[   84s]                 )
[   84s]             return
[   84s]     
[   84s]         denoised = restoration.denoise_wavelet(
[   84s]             noisy,
[   84s]             sigma=sigma_est,
[   84s]             wavelet='sym4',
[   84s]             channel_axis=channel_axis,
[   84s]             convert2ycbcr=convert2ycbcr,
[   84s]             rescale_sigma=True,
[   84s]         )
[   84s]         assert denoised.dtype == _supported_float_type(noisy.dtype)
[   84s]     
[   84s]         data_range = x.max() - x.min()
[   84s]         psnr_noisy = peak_signal_noise_ratio(x, noisy, data_range=data_range)
[   84s]         clipped = np.dtype(dtype).kind != 'f'
[   84s]         if not clipped:
[   84s]             psnr_denoised = peak_signal_noise_ratio(x, denoised, data_range=data_range)
[   84s]     
[   84s]             # output's max value is not substantially smaller than x's
[   84s]             assert denoised.max() > 0.9 * x.max()
[   84s]         else:
[   84s]             # have to compare to x_as_float in integer input cases
[   84s]             x_as_float = img_as_float(x)
[   84s]             f_data_range = x_as_float.max() - x_as_float.min()
[   84s]             psnr_denoised = peak_signal_noise_ratio(
[   84s]                 x_as_float, denoised, data_range=f_data_range
[   84s]             )
[   84s]     
[   84s]             # output has been clipped to expected range
[   84s]             assert denoised.max() <= 1.0
[   84s]             if np.dtype(dtype).kind == 'u':
[   84s]                 assert denoised.min() >= 0
[   84s]             else:
[   84s]                 assert denoised.min() >= -1
[   84s]     
[   84s] >       assert psnr_denoised > psnr_noisy
[   84s] E       assert 25.809793363059022 > nan
[   84s] 
...
[   84s] =============================== warnings summary ===============================
[   84s] restoration/tests/test_denoise.py::test_wavelet_denoising_scaling[True-True-int16-2d multichannel]
[   84s] restoration/tests/test_denoise.py::test_wavelet_denoising_scaling[True-False-int16-1d]
[   84s] restoration/tests/test_denoise.py::test_wavelet_denoising_scaling[True-False-int16-2d multichannel]
[   84s] restoration/tests/test_denoise.py::test_wavelet_denoising_scaling[False-True-int16-2d multichannel]
[   84s] restoration/tests/test_denoise.py::test_wavelet_denoising_scaling[False-False-int16-1d]
[   84s] restoration/tests/test_denoise.py::test_wavelet_denoising_scaling[False-False-int16-2d multichannel]
[   84s]   /usr/lib64/python3.10/site-packages/skimage/metrics/simple_metrics.py:167: RuntimeWarning: invalid value encountered in log10
[   84s]     return 10 * np.log10((data_range**2) / err)
[   84s] 
[   84s] -- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
[   84s] =========================== short test summary info ============================
[   84s] FAILED restoration/tests/test_denoise.py::test_wavelet_denoising_scaling[True-True-int16-2d multichannel]
[   84s] FAILED restoration/tests/test_denoise.py::test_wavelet_denoising_scaling[True-False-float16-1d]
[   84s] FAILED restoration/tests/test_denoise.py::test_wavelet_denoising_scaling[True-False-float32-1d]
[   84s] FAILED restoration/tests/test_denoise.py::test_wavelet_denoising_scaling[True-False-int16-1d]
[   84s] FAILED restoration/tests/test_denoise.py::test_wavelet_denoising_scaling[True-False-int16-2d multichannel]
[   84s] FAILED restoration/tests/test_denoise.py::test_wavelet_denoising_scaling[False-True-int16-2d multichannel]
[   84s] FAILED restoration/tests/test_denoise.py::test_wavelet_denoising_scaling[False-False-float16-1d]
[   84s] FAILED restoration/tests/test_denoise.py::test_wavelet_denoising_scaling[False-False-float32-1d]
[   84s] FAILED restoration/tests/test_denoise.py::test_wavelet_denoising_scaling[False-False-int16-1d]
[   84s] FAILED restoration/tests/test_denoise.py::test_wavelet_denoising_scaling[False-False-int16-2d multichannel]
[   84s] ========== 10 failed, 8164 passed, 125 skipped, 6 warnings in 54.27s ===========

The tests succeeds with numpy 1.26.4

Full logs:

Build Tests
Numpy 1 skimage-np1-build.txt ✔️ skimage-np1-test.txt
Numpy 2 skimage_np2_buildlog.txt skimage_np2-testfail.txt

@lagru
Copy link
Member Author

lagru commented May 7, 2024

I can reproduce, thanks for the report. Seems like we are starting to see these failures only now, because PyWavelets or some other dependency is no longer blocking NumPy 2.0, and now these tests are run with NumPy 2 for the first time. There are also failures on main that are cause by pyamg which dosen't use an upper bound for NumPy<2.0 but is not yet compatible with it (see pyamg/pyamg#406).

@jakirkham
Copy link
Contributor

PyAMG released 5.2.1, which fixes the aforementioned issue

On a different note, Mark just shipped scikit-image with NumPy 2 compatible builds in conda-forge: conda-forge/scikit-image-feedstock#110

@hmaarrfk
Copy link
Member

hmm i somewhat forgot about the failures.

But generally, things seem to have gotten down to the "normal bug level", and not "catastrophic failure level".

@jakirkham
Copy link
Contributor

jakirkham commented Jun 28, 2024

Think PyAMG 5.2.1 fixed those already

In any event having an easier way for more people to install, test, and report will speed up identification and resolution of remaining issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⬆️ Upstream Needs help from or involves an upstream project 🔧 type: Maintenance Refactoring and maintenance of internals
Projects
None yet
Development

No branches or pull requests

7 participants