Skip to content

Conversation

@jarrodmillman
Copy link
Contributor

@jarrodmillman jarrodmillman commented May 31, 2023

Follow-up to #6969 and #6973. This re-enables the tests for NumPy 1.25 we still need to sort out what to do about the 7 failing tests

===== 7 failed, 8369 passed, 20 skipped, 10 warnings in 356.97s (0:05:56) ======

@jarrodmillman jarrodmillman added the 🔧 type: Maintenance Refactoring and maintenance of internals label May 31, 2023
@stefanv
Copy link
Member

stefanv commented May 31, 2023

Most of the failures are related to changes in NumPy's cos/sin implementation. These changes widely affect other packages in the ecosystem as well, so I presume it will be discussed on tomorrow's triage meeting @seberg?

If we are guaranteed that the new implementation is more accurate than the previous one across the board, then I'd be okay fixing all those tests. But if it varies from place to place, the change may warrant further discussion?

@tacaswell
Copy link
Contributor

For reference we already dealt with this in Matplotlib : matplotlib/matplotlib#25813 and matplotlib/matplotlib#25789 .

@jni
Copy link
Member

jni commented Jun 2, 2023

fwiw I do think the change in NumPy should be reverted (agree with all the comments in the mailing list thread arguing for that, including the point about teaching), but at the same time, maybe we should also fix our tests to not depend on exact matches for those special values...

@stefanv
Copy link
Member

stefanv commented Jun 2, 2023

@jni The way many of these tests were constructed is to take an image, transform it with some parameters (which affect the output shape), and to confirm that it's correct. Then, that gets enshrined in the test suite. Now, it's still correct, but the shapes are slightly off :/ And it's strange to test for shape +- 1 on all dimensions, that's a bit rough. So, how do we do it? Perhaps choose better angles for the transforms?

@tacaswell
Copy link
Contributor

Do you see any flakyness of these same tests across architectures / platforms?

@stefanv
Copy link
Member

stefanv commented Jun 2, 2023

I think our CI is limited that way (chip instruction sets), but at least across numpy versions during the transition we'll have problems.

@tacaswell
Copy link
Contributor

For Matplotlib we occasionally get reports from the Debian packager of test failures on exotic architectures / endianess / bit size systems (most of them are RBGA values changing by +/-1 presumably due to float rounding issues), I was wondering if skimage got the same and if these tests were the ones that also had issues there. If that were the case it might be extra motivation to make these tests less brittle.

@jni
Copy link
Member

jni commented Jun 3, 2023

@stefanv

And it's strange to test for shape +- 1 on all dimensions, that's a bit rough. So, how do we do it? Perhaps choose better angles for the transforms?

np.testing.assert_allclose(shape_actual, shape_ref, atol=1.5)?

jarrodmillman added a commit to jarrodmillman/scikit-image that referenced this pull request Jun 4, 2023
stefanv pushed a commit that referenced this pull request Jun 5, 2023
@jni
Copy link
Member

jni commented Jun 27, 2023

Can we just close this now that NumPy reverted those changes? Or do we want to keep this as an improvement?

@lagru
Copy link
Member

lagru commented Jun 27, 2023

I think this needs to be merged to remove the skip of NumPy 1.25 on Azure?

@jarrodmillman jarrodmillman marked this pull request as draft June 28, 2023 02:34
@jarrodmillman
Copy link
Contributor Author

We still need to sort out what to do about the 7 failing tests

===== 7 failed, 8369 passed, 20 skipped, 10 warnings in 356.97s (0:05:56) ======

@jarrodmillman
Copy link
Contributor Author

I just noticed linux-cp3.11-pre has

===== 10 failed, 8366 passed, 20 skipped, 2 warnings in 338.15s (0:05:38) ======

@stefanv
Copy link
Member

stefanv commented Aug 1, 2023

 skimage/morphology/tests/test_util.py::test_offsets_to_raveled_neighbors_explicit_0
 skimage/feature/tests/test_corner.py::test_subpix_border

@lagru
Copy link
Member

lagru commented Aug 5, 2023

Enabling stable sorting in _raveled_offsets_and_distances seemed to do the trick for test_offsets_to_raveled_neighbors_explicit_*. I think we can justify the changed ordering and output with the reasoning that from now on it should be guaranteed to be stable.

jarrodmillman and others added 3 commits August 14, 2023 05:54
Comment on lines 158 to +160
_, indices = np.unique(sorted_raveled_offsets, return_index=True)
sorted_raveled_offsets = sorted_raveled_offsets[np.sort(indices)]
sorted_distances = sorted_distances[np.sort(indices)]
indices = np.sort(indices, kind="stable")
sorted_raveled_offsets = sorted_raveled_offsets[indices]
Copy link
Member

Choose a reason for hiding this comment

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

indices contains only unique values, so "quicksort" and "stable" should have the same behavior.

Copy link
Member

Choose a reason for hiding this comment

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

These sorts probably don't differ by much, but why specify stable here if it's not necessary?

@lagru
Copy link
Member

lagru commented Aug 15, 2023

Okay fixed the two remaining failing test by switching to stable sorting at the appropriate place. Going forward, we should probably consider stable sorting as the default and use quicksort only if there is enough to warrant the trade-off of unstable sorting.

Details
Subject: [PATCH] Use stable sort everywhere
---
Index: skimage/util/_regular_grid.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/skimage/util/_regular_grid.py b/skimage/util/_regular_grid.py
--- a/skimage/util/_regular_grid.py	(revision 8a59eda7bb4609636ba8e0c9ecfa122f0b4bad7c)
+++ b/skimage/util/_regular_grid.py	(date 1692119211772)
@@ -60,7 +60,7 @@
     """
     ar_shape = np.asanyarray(ar_shape)
     ndim = len(ar_shape)
-    unsort_dim_idxs = np.argsort(np.argsort(ar_shape))
+    unsort_dim_idxs = np.argsort(np.argsort(ar_shape, kind="stable"))
     sorted_dims = np.sort(ar_shape)
     space_size = float(np.prod(ar_shape))
     if space_size <= n_points:
Index: skimage/transform/tests/test_hough_transform.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/skimage/transform/tests/test_hough_transform.py b/skimage/transform/tests/test_hough_transform.py
--- a/skimage/transform/tests/test_hough_transform.py	(revision 8a59eda7bb4609636ba8e0c9ecfa122f0b4bad7c)
+++ b/skimage/transform/tests/test_hough_transform.py	(date 1692119211772)
@@ -260,7 +260,7 @@
                                        min_ydistance=1, threshold=None,
                                        num_peaks=np.inf,
                                        total_num_peaks=np.inf)
-    s = np.argsort(out[3])  # sort by radii
+    s = np.argsort(out[3], kind="stable")  # sort by radii
     assert_equal(out[1][s], np.array([y_0, y_1]))
     assert_equal(out[2][s], np.array([x_0, x_1]))
     assert_equal(out[3][s], np.array([rad_0, rad_1]))
Index: skimage/transform/hough_transform.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/skimage/transform/hough_transform.py b/skimage/transform/hough_transform.py
--- a/skimage/transform/hough_transform.py	(revision 8a59eda7bb4609636ba8e0c9ecfa122f0b4bad7c)
+++ b/skimage/transform/hough_transform.py	(date 1692119211772)
@@ -354,9 +354,9 @@
     cy = np.array(cy)
     accum = np.array(accum)
     if normalize:
-        s = np.argsort(accum / r)
+        s = np.argsort(accum / r, kind="stable")
     else:
-        s = np.argsort(accum)
+        s = np.argsort(accum, kind="stable")
     accum_sorted, cx_sorted, cy_sorted, r_sorted = \
         accum[s][::-1], cx[s][::-1], cy[s][::-1], r[s][::-1]
 
Index: skimage/feature/peak.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/skimage/feature/peak.py b/skimage/feature/peak.py
--- a/skimage/feature/peak.py	(revision 8a59eda7bb4609636ba8e0c9ecfa122f0b4bad7c)
+++ b/skimage/feature/peak.py	(date 1692119211772)
@@ -404,7 +404,7 @@
     xcoords_peaks = np.array(xcoords_peaks)
 
     if num_peaks < len(img_peaks):
-        idx_maxsort = np.argsort(img_peaks)[::-1][:num_peaks]
+        idx_maxsort = np.argsort(img_peaks, kind="stable")[::-1][:num_peaks]
         img_peaks = img_peaks[idx_maxsort]
         ycoords_peaks = ycoords_peaks[idx_maxsort]
         xcoords_peaks = xcoords_peaks[idx_maxsort]
Index: skimage/transform/radon_transform.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/skimage/transform/radon_transform.py b/skimage/transform/radon_transform.py
--- a/skimage/transform/radon_transform.py	(revision 8a59eda7bb4609636ba8e0c9ecfa122f0b4bad7c)
+++ b/skimage/transform/radon_transform.py	(date 1692119211772)
@@ -338,7 +338,7 @@
     """
     interval = 180
 
-    remaining_indices = list(np.argsort(theta))   # indices into theta
+    remaining_indices = list(np.argsort(theta, kind="stable"))   # indices into theta
     # yield an arbitrary angle to start things off
     angle = theta[remaining_indices[0]]
     yield remaining_indices.pop(0)
Index: skimage/morphology/grayreconstruct.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/skimage/morphology/grayreconstruct.py b/skimage/morphology/grayreconstruct.py
--- a/skimage/morphology/grayreconstruct.py	(revision 8a59eda7bb4609636ba8e0c9ecfa122f0b4bad7c)
+++ b/skimage/morphology/grayreconstruct.py	(date 1692119211772)
@@ -179,7 +179,8 @@
     images = images.reshape(-1)
 
     # Erosion goes smallest to largest; dilation goes largest to smallest.
-    index_sorted = np.argsort(images).astype(signed_int_dtype, copy=False)
+    index_sorted = np.argsort(images, kind="stable")
+    index_sorted = index_sorted.astype(signed_int_dtype, copy=False)
     if method == 'dilation':
         index_sorted = index_sorted[::-1]
 

Though, now we get a memory corruption or something like that on pre. It looks reproducable. 🤔 windows job with more details

@stefanv
Copy link
Member

stefanv commented Aug 15, 2023

Well done figuring out the problem!

@lagru
Copy link
Member

lagru commented Aug 16, 2023

Okay, after some digging I am still clueless:

Details
FAILED [ 20%]
test_basic_features.py:39 (test_multiscale_basic_features_channel_axis[0])
channel_axis = 0

    @pytest.mark.parametrize('channel_axis', [0, 1, 2, -1, -2])
    def test_multiscale_basic_features_channel_axis(channel_axis):
        num_channels = 5
        shape_spatial = (10, 10)
        ndim = len(shape_spatial)
        shape = tuple(
            np.insert(shape_spatial, channel_axis % (ndim + 1), num_channels)
        )
        img = np.zeros(shape)
        img[:10] = 1
        img += 0.05 * np.random.randn(*img.shape)
        n_sigmas = 2
    
        # features for all channels are concatenated along the last axis
        features = multiscale_basic_features(img, sigma_min=1, sigma_max=2,
                                             channel_axis=channel_axis)
        assert features.shape[-1] == 5 * n_sigmas * 4
        assert features.shape[:-1] == np.moveaxis(img, channel_axis, -1).shape[:-1]
    
        # Consider channel_axis as spatial dimension
>       features = multiscale_basic_features(img, sigma_min=1, sigma_max=2)

test_basic_features.py:60: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../_basic_features.py:180: in multiscale_basic_features
    features = list(itertools.chain.from_iterable(all_results))
../_basic_features.py:168: in <genexpr>
    _mutiscale_basic_features_singlechannel(
../_basic_features.py:87: in _mutiscale_basic_features_singlechannel
    out_sigmas = list(
/usr/lib/python3.11/concurrent/futures/_base.py:619: in result_iterator
    yield _result_or_cancel(fs.pop())
/usr/lib/python3.11/concurrent/futures/_base.py:317: in _result_or_cancel
    return fut.result(timeout)
/usr/lib/python3.11/concurrent/futures/_base.py:449: in result
    return self.__get_result()
/usr/lib/python3.11/concurrent/futures/_base.py:401: in __get_result
    raise self._exception
/usr/lib/python3.11/concurrent/futures/thread.py:58: in run
    result = self.fn(*self.args, **self.kwargs)
../_basic_features.py:89: in <lambda>
    lambda s: _singlescale_basic_features_singlechannel(
../_basic_features.py:28: in _singlescale_basic_features_singlechannel
    results += (*_texture_filter(gaussian_filtered),)
../_basic_features.py:14: in _texture_filter
    eigvals = feature.hessian_matrix_eigvals(H_elems)
../corner.py:510: in hessian_matrix_eigvals
    return _symmetric_compute_eigenvalues(H_elems)
../corner.py:412: in _symmetric_compute_eigenvalues
    eigs = np.linalg.eigvalsh(matrices)[..., ::-1]
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

a = array([[[[[-1.76429749e-05, -3.95774841e-05, -8.44955444e-04],
          [-3.95774841e-05, -6.33984804e-04,  1.6325712...00112772e-04,  2.67672539e-03],
          [ 9.18269157e-04,  2.67672539e-03,  0.00000000e+00]]]]],
      dtype=float32)
UPLO = 'L'

    @array_function_dispatch(_eigvalsh_dispatcher)
    def eigvalsh(a, UPLO='L'):
        """
        Compute the eigenvalues of a complex Hermitian or real symmetric matrix.
    
        Main difference from eigh: the eigenvectors are not computed.
    
        Parameters
        ----------
        a : (..., M, M) array_like
            A complex- or real-valued matrix whose eigenvalues are to be
            computed.
        UPLO : {'L', 'U'}, optional
            Specifies whether the calculation is done with the lower triangular
            part of `a` ('L', default) or the upper triangular part ('U').
            Irrespective of this value only the real parts of the diagonal will
            be considered in the computation to preserve the notion of a Hermitian
            matrix. It therefore follows that the imaginary part of the diagonal
            will always be treated as zero.
    
        Returns
        -------
        w : (..., M,) ndarray
            The eigenvalues in ascending order, each repeated according to
            its multiplicity.
    
        Raises
        ------
        LinAlgError
            If the eigenvalue computation does not converge.
    
        See Also
        --------
        eigh : eigenvalues and eigenvectors of real symmetric or complex Hermitian
               (conjugate symmetric) arrays.
        eigvals : eigenvalues of general real or complex arrays.
        eig : eigenvalues and right eigenvectors of general real or complex
              arrays.
        scipy.linalg.eigvalsh : Similar function in SciPy.
    
        Notes
        -----
    
        .. versionadded:: 1.8.0
    
        Broadcasting rules apply, see the `numpy.linalg` documentation for
        details.
    
        The eigenvalues are computed using LAPACK routines ``_syevd``, ``_heevd``.
    
        Examples
        --------
        >>> from numpy import linalg as LA
        >>> a = np.array([[1, -2j], [2j, 5]])
        >>> LA.eigvalsh(a)
        array([ 0.17157288,  5.82842712]) # may vary
    
        >>> # demonstrate the treatment of the imaginary part of the diagonal
        >>> a = np.array([[5+2j, 9-2j], [0+2j, 2-1j]])
        >>> a
        array([[5.+2.j, 9.-2.j],
               [0.+2.j, 2.-1.j]])
        >>> # with UPLO='L' this is numerically equivalent to using LA.eigvals()
        >>> # with:
        >>> b = np.array([[5.+0.j, 0.-2.j], [0.+2.j, 2.-0.j]])
        >>> b
        array([[5.+0.j, 0.-2.j],
               [0.+2.j, 2.+0.j]])
        >>> wa = LA.eigvalsh(a)
        >>> wb = LA.eigvals(b)
        >>> wa; wb
        array([1., 6.])
        array([6.+0.j, 1.+0.j])
    
        """
        UPLO = UPLO.upper()
        if UPLO not in ('L', 'U'):
            raise ValueError("UPLO argument must be 'L' or 'U'")
    
        extobj = get_linalg_error_extobj(
            _raise_linalgerror_eigenvalues_nonconvergence)
        if UPLO == 'L':
            gufunc = _umath_linalg.eigvalsh_lo
        else:
            gufunc = _umath_linalg.eigvalsh_up
    
        a, wrap = _makearray(a)
        _assert_stacked_2d(a)
        _assert_stacked_square(a)
        t, result_t = _commonType(a)
        signature = 'D->d' if isComplexType(t) else 'd->d'
>       w = gufunc(a, signature=signature, extobj=extobj)
E       ValueError: On entry to DSYEVD parameter number 8 had an illegal value

/home/lg/.local/lib/venv/skimagedev/lib/python3.11/site-packages/numpy/linalg/linalg.py:1181: ValueError

I'll open an issue in NumPy and ask nicely for support., not sure yet if this is our fault..

@lagru
Copy link
Member

lagru commented Aug 16, 2023

For local debugging with NumPy v1.26.0b1

import numpy as np
from skimage.feature import multiscale_basic_features

channel_axis = 0
num_channels = 5
shape_spatial = (10, 10)
ndim = len(shape_spatial)
shape = tuple(
    np.insert(shape_spatial, channel_axis % (ndim + 1), num_channels)
)
img = np.zeros(shape)
img[:10] = 1
img += 0.05 * np.random.randn(*img.shape)

features = multiscale_basic_features(img, sigma_min=1, sigma_max=2)

Is it possible that np.linalg.eigenvals no longer supports multi-threading somehow? If I set the number of workers in

with ThreadPoolExecutor(max_workers=num_workers) as ex:

to 1, the error disappears...

@jarrodmillman jarrodmillman marked this pull request as ready for review August 17, 2023 01:55
@jarrodmillman
Copy link
Contributor Author

jarrodmillman commented Aug 17, 2023

@stefanv @lagru This PR was started with the goal of supporting NumPy 1.25, so I just upperpinned numpy<1.26 in a5f328f. Let's merge this and and open a new PR undoing a5f328f and add @lagru comments above about numpy 1.26.

@jarrodmillman jarrodmillman enabled auto-merge (squash) August 17, 2023 02:15
@jarrodmillman jarrodmillman requested a review from stefanv August 17, 2023 04:28
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.

5 participants