TST: add `xfail_xp_backends` #21334

lucascolley · 2024-08-07T13:15:06Z

Reference issue

Towards gh-21280

What does this implement/fix?

Adds an xfail_xp_backends decorator to complement skip_xp_backends.
I've only applied it in one place for now for demonstration.

Additional information

@rgommers @ev-br a few questions:

Does this implementation look okay?
If so, should I go ahead with replacing the ad-hoc xfails in ndimage?
Is it worth replacing skips that would technically be better as xfails in other places?

rgommers · 2024-08-07T13:44:54Z

This looks good to me based on just a look at the diff. I'd go ahead with using this in ndimage indeed, that should show whether things work as advertised.

Is it worth replacing skips that would technically be better as xfails in other places?

It depends a bit. xfails can be slow and fragile, so I'd only do it for known upstream bugs that are likely to be fixed in the near future.

ev-br · 2024-08-07T13:51:50Z

Agreed with what Ralf said. To me, an xfail compared to a skip carries a bit more of "it's failing now, but we'd like it get fixed" message. But the semantic difference is minor and not that clear-cut, so I don't believe it's worth much effort converting one to the other. And speed / stability argument applies, too.

lucascolley · 2024-08-07T20:45:52Z

scipy/ndimage/tests/test_filters.py

+    @xfail_xp_backends('cupy', reasons=["cupy/cupy#8405"])
+    @skip_xp_backends(np_only=True,
+                      reasons=["output=dtype is numpy-specific"],
+                      exceptions=['cupy'],)


@ev-br what do you think about cases like this? Is this the nicest way to express what we want? I'm having a hard time thinking how we would condense this logic into one decorator rather than two, and whether that would even be preferable.

Well, from readability POV this looks horrible TBH. Not that the original was pretty, but at least I can quickly see what's going on (maybe place is_cupy check first --- then the original reads "if cupy, there's a know cupy issue to fix in cupy, otherwise it's numpy and cupy only").

This version I simply cannot read without going back and forth and thinking for a while. This skip_xp_backends(np_only=True --- ok, so it's numpy only, what is "exceptions"? Ah, OK, it's probably allows cupy (got to go back and find the docs/implementation of skip_xp_backends, but let me guess for now), so cupy is an exception, but this was on the previous line, is it skipped or xfailed on neither? I'm confused and I surely won't be able to type this myself if I ever need something like this elsewhere without copy-pasting.
Unlike the previous version which is ugly but is possible to recreate.

ISTM thisis a good indicator that skip_xp_backends is already not very intuitive and overloading it further is not a very good idea.

One thing to note is that skip_xp_backends operates on specific backend names, while regular pytest xfail and skip markers operate on predicates (and so does unittest.skipIf for that matter, and however the analog was called by nose --- so essentially every other testing framework).

So instead of overloading with reasons=list and exceptions=list, maybe there's a way to teach pytest to recognize is cupy(xp) or not is_numpy(xp) as a boolean.

I agree that it is not intuitive, however np_only=True, exceptions=['cupy'] is the best I have come up with so far implementation-wise to get is cupy(xp) or not is_numpy(xp). Feel free to propose an alternative syntax and the necessary adjustments to the infrastructure!

My comment could be condensed to "if you ask me, I'd leave it as it was until there's a tweak to the decorator infra; but do roll with whatever best syntax you came up with if you feel strongly about it".

lucascolley · 2024-08-08T21:20:18Z

scipy/ndimage/tests/test_filters.py

+    @xfail_xp_backends('cupy', reasons=["cupy/cupy#8405",])
+    @skip_xp_backends(np_only=True,
+                      reasons=["output=dtype is numpy-specific"],
+                      exceptions=['cupy'],)


This is one of the most complex cases. Here is my understanding of what the logic seems to be doing. Let me know if this is correct @ev-br .

At the module level, we skip for all non-CPU backends apart from CuPy and JAX. CuPy and JAX have some delegation implemented, but other alternative backends do not.

At the level of this test, "output=dtype is numpy-specific" hence we want to skip for CPU backends and JAX as well. But CuPy is okay here (?) so we make an exception for CuPy.

At the level of this test, we xfail for CuPy due to the linked CuPy issue.

If that is correct, then personally I am "okay" with this syntax, although it would be nice to have something more simple.

lucascolley · 2024-08-08T22:16:15Z

Some local CUDA failures:

_______________________________________________________________ TestNdimageFourier.test_fourier_uniform_real01[float64-14-shape2-jax.numpy] ________________________________________________________________
scipy/ndimage/tests/test_fourier.py:69: in test_fourier_uniform_real01
    assert_almost_equal(ndimage.sum(a), xp.asarray(1.0), decimal=dec)
        a          = Array([[ 0.44940199,  0.2844783 , -0.01392904,  0.00791456, -0.00578776,
         0.00524593, -0.00578776,  0.00791456, -0.01392904,  0.2844783 ]],      dtype=float64)
        dec        = 14
        dtype      = 'float64'
        fft        = <module 'jax.numpy.fft' from '/home/lucas/dev/pixi-dev-scipystack/scipy/.pixi/envs/array-api-cuda/lib/python3.12/site-packages/jax/numpy/fft.py'>
        self       = <scipy.ndimage.tests.test_fourier.TestNdimageFourier object at 0x75e811438320>
        shape      = (1, 10)
        xp         = <module 'jax.numpy' from '/home/lucas/dev/pixi-dev-scipystack/scipy/.pixi/envs/array-api-cuda/lib/python3.12/site-packages/jax/numpy/__init__.py'>
scipy/_lib/_array_api.py:403: in assert_almost_equal
    return xp_assert_close(actual, desired,
        actual     = Array(1.00000001, dtype=float64)
        args       = ()
        atol       = 1.5e-14
        decimal    = 14
        desired    = Array(1., dtype=float64, weak_type=True)
        kwds       = {}
        rtol       = 0
../../../../../.pixi/envs/array-api-cuda/lib/python3.12/contextlib.py:81: in inner
    return func(*args, **kwds)
E   AssertionError: 
E   Not equal to tolerance rtol=0, atol=1.5e-14
E   
E   Mismatched elements: 1 / 1 (100%)
E   Max absolute difference among violations: 1.49011612e-08
E   Max relative difference among violations: 1.49011612e-08
E    ACTUAL: array(1.)
E    DESIRED: array(1.)
        args       = (<function assert_allclose.<locals>.compare at 0x75e6b5c3d6c0>, array(1.00000001), array(1.))
        func       = <function assert_array_compare at 0x75ea11b84040>
        kwds       = {'equal_nan': True, 'err_msg': '', 'header': 'Not equal to tolerance rtol=0, atol=1.5e-14', 'strict': False, ...}
        self       = <contextlib._GeneratorContextManager object at 0x75ea13d6e6f0>
____________________________________________________________ TestNdimageFourier.test_fourier_uniform_complex01[complex128-14-shape1-jax.numpy] _____________________________________________________________
scipy/ndimage/tests/test_fourier.py:85: in test_fourier_uniform_complex01
    assert_almost_equal(ndimage.sum(xp.real(a)), xp.asarray(1.0), decimal=dec)
        a          = Array([[ 8.94593083e-02+0.00000000e+00j,  5.59267962e-02+1.58899741e-18j,
        -2.30043465e-03-1.95622719e-18j,  1....0582e-03+1.02154248e-18j, -2.35566085e-03+1.73931036e-18j,
         5.72694226e-02-1.81208745e-18j]], dtype=complex128)
        dec        = 14
        dtype      = 'complex128'
        fft        = <module 'jax.numpy.fft' from '/home/lucas/dev/pixi-dev-scipystack/scipy/.pixi/envs/array-api-cuda/lib/python3.12/site-packages/jax/numpy/fft.py'>
        self       = <scipy.ndimage.tests.test_fourier.TestNdimageFourier object at 0x75e811439910>
        shape      = (31, 15)
        xp         = <module 'jax.numpy' from '/home/lucas/dev/pixi-dev-scipystack/scipy/.pixi/envs/array-api-cuda/lib/python3.12/site-packages/jax/numpy/__init__.py'>
scipy/_lib/_array_api.py:403: in assert_almost_equal
    return xp_assert_close(actual, desired,
        actual     = Array(1.00000002, dtype=float64)
        args       = ()
        atol       = 1.5e-14
        decimal    = 14
        desired    = Array(1., dtype=float64, weak_type=True)
        kwds       = {}
        rtol       = 0
../../../../../.pixi/envs/array-api-cuda/lib/python3.12/contextlib.py:81: in inner
    return func(*args, **kwds)
E   AssertionError: 
E   Not equal to tolerance rtol=0, atol=1.5e-14
E   
E   Mismatched elements: 1 / 1 (100%)
E   Max absolute difference among violations: 2.23517405e-08
E   Max relative difference among violations: 2.23517405e-08
E    ACTUAL: array(1.)
E    DESIRED: array(1.)
        args       = (<function assert_allclose.<locals>.compare at 0x75e6b5c7c0e0>, array(1.00000002), array(1.))
        func       = <function assert_array_compare at 0x75ea11b84040>
        kwds       = {'equal_nan': True, 'err_msg': '', 'header': 'Not equal to tolerance rtol=0, atol=1.5e-14', 'strict': False, ...}
        self       = <contextlib._GeneratorContextManager object at 0x75ea13d6e6f0>
________________________________________________________________ TestNdimageFourier.test_fourier_shift_real01[float64-11-shape1-jax.numpy] _________________________________________________________________
scipy/ndimage/tests/test_fourier.py:100: in test_fourier_shift_real01
    assert_array_almost_equal(a[1:, 1:], expected[:-1, :-1], decimal=dec)
        a          = Array([[ 4.64000010e+02,  4.50000010e+02,  4.51000010e+02,
         4.52000010e+02,  4.53000010e+02,  4.54000010e+02,
...+02,  4.44000010e+02,  4.45000010e+02,
         4.46000010e+02,  4.47000010e+02,  4.48000010e+02]],      dtype=float64)
        dec        = 11
        dtype      = 'float64'
        expected   = Array([[  0.,   1.,   2.,   3.,   4.,   5.,   6.,   7.,   8.,   9.,  10.,
         11.,  12.,  13.,  14.],
       [ 15...    [450., 451., 452., 453., 454., 455., 456., 457., 458., 459., 460.,
        461., 462., 463., 464.]], dtype=float64)
        fft        = <module 'jax.numpy.fft' from '/home/lucas/dev/pixi-dev-scipystack/scipy/.pixi/envs/array-api-cuda/lib/python3.12/site-packages/jax/numpy/fft.py'>
        self       = <scipy.ndimage.tests.test_fourier.TestNdimageFourier object at 0x75e81143ae70>
        shape      = (31, 15)
        xp         = <module 'jax.numpy' from '/home/lucas/dev/pixi-dev-scipystack/scipy/.pixi/envs/array-api-cuda/lib/python3.12/site-packages/jax/numpy/__init__.py'>
scipy/_lib/_array_api.py:394: in assert_array_almost_equal
    return xp_assert_close(actual, desired,
        actual     = Array([[-2.93385379e-14,  1.00000002e+00,  2.00000004e+00,
         3.00000007e+00,  4.00000009e+00,  5.00000011e+00,
...,
         4.44000010e+02,  4.45000010e+02,  4.46000010e+02,
         4.47000010e+02,  4.48000010e+02]], dtype=float64)
        args       = ()
        atol       = 1.5e-11
        decimal    = 11
        desired    = Array([[  0.,   1.,   2.,   3.,   4.,   5.,   6.,   7.,   8.,   9.,  10.,
         11.,  12.,  13.],
       [ 15.,  16...],
       [435., 436., 437., 438., 439., 440., 441., 442., 443., 444., 445.,
        446., 447., 448.]], dtype=float64)
        kwds       = {}
        rtol       = 0
../../../../../.pixi/envs/array-api-cuda/lib/python3.12/contextlib.py:81: in inner
    return func(*args, **kwds)
E   AssertionError: 
E   Not equal to tolerance rtol=0, atol=1.5e-11
E   
E   Mismatched elements: 419 / 420 (99.8%)
E   Max absolute difference among violations: 1.00135795e-05
E   Max relative difference among violations: 2.23517695e-08
E    ACTUAL: array([[-2.933854e-14,  1.000000e+00,  2.000000e+00,  3.000000e+00,
E            4.000000e+00,  5.000000e+00,  6.000000e+00,  7.000000e+00,
E            8.000000e+00,  9.000000e+00,  1.000000e+01,  1.100000e+01,...
E    DESIRED: array([[  0.,   1.,   2.,   3.,   4.,   5.,   6.,   7.,   8.,   9.,  10.,
E            11.,  12.,  13.],
E          [ 15.,  16.,  17.,  18.,  19.,  20.,  21.,  22.,  23.,  24.,  25.,...
        args       = (<function assert_allclose.<locals>.compare at 0x75e6b5c7eb60>, array([[-2.93385379e-14,  1.00000002e+00,  2.00000004e...1., 432., 433.],
       [435., 436., 437., 438., 439., 440., 441., 442., 443., 444., 445.,
        446., 447., 448.]]))
        func       = <function assert_array_compare at 0x75ea11b84040>
        kwds       = {'equal_nan': True, 'err_msg': '', 'header': 'Not equal to tolerance rtol=0, atol=1.5e-11', 'strict': False, ...}
        self       = <contextlib._GeneratorContextManager object at 0x75ea13d6e6f0>
_____________________________________________________________ TestNdimageFourier.test_fourier_shift_complex01[complex128-11-shape1-jax.numpy] ______________________________________________________________
scipy/ndimage/tests/test_fourier.py:115: in test_fourier_shift_complex01
    assert_array_almost_equal(xp.real(a)[1:, 1:], expected[:-1, :-1], decimal=dec)
        a          = Array([[ 4.64000010e+02-5.50097614e-16j,  4.50000010e+02-2.48421392e-16j,
         4.51000010e+02-2.82587881e-16j,  4....0010e+02-3.39321479e-16j,  4.47000010e+02-1.78652907e-16j,
         4.48000010e+02-7.44099714e-16j]], dtype=complex128)
        dec        = 11
        dtype      = 'complex128'
        expected   = Array([[  0.+0.j,   1.+0.j,   2.+0.j,   3.+0.j,   4.+0.j,   5.+0.j,
          6.+0.j,   7.+0.j,   8.+0.j,   9.+0.j,  1...  456.+0.j, 457.+0.j, 458.+0.j, 459.+0.j, 460.+0.j, 461.+0.j,
        462.+0.j, 463.+0.j, 464.+0.j]], dtype=complex128)
        fft        = <module 'jax.numpy.fft' from '/home/lucas/dev/pixi-dev-scipystack/scipy/.pixi/envs/array-api-cuda/lib/python3.12/site-packages/jax/numpy/fft.py'>
        self       = <scipy.ndimage.tests.test_fourier.TestNdimageFourier object at 0x75e8114644a0>
        shape      = (31, 15)
        xp         = <module 'jax.numpy' from '/home/lucas/dev/pixi-dev-scipystack/scipy/.pixi/envs/array-api-cuda/lib/python3.12/site-packages/jax/numpy/__init__.py'>
scipy/_lib/_array_api.py:394: in assert_array_almost_equal
    return xp_assert_close(actual, desired,
        actual     = Array([[-2.93385379e-14,  1.00000002e+00,  2.00000004e+00,
         3.00000007e+00,  4.00000009e+00,  5.00000011e+00,
...,
         4.44000010e+02,  4.45000010e+02,  4.46000010e+02,
         4.47000010e+02,  4.48000010e+02]], dtype=float64)
        args       = ()
        atol       = 1.5e-11
        decimal    = 11
        desired    = Array([[  0.+0.j,   1.+0.j,   2.+0.j,   3.+0.j,   4.+0.j,   5.+0.j,
          6.+0.j,   7.+0.j,   8.+0.j,   9.+0.j,  1....j,
        441.+0.j, 442.+0.j, 443.+0.j, 444.+0.j, 445.+0.j, 446.+0.j,
        447.+0.j, 448.+0.j]], dtype=complex128)
        kwds       = {}
        rtol       = 0
../../../../../.pixi/envs/array-api-cuda/lib/python3.12/contextlib.py:81: in inner
    return func(*args, **kwds)
E   AssertionError: 
E   Not equal to tolerance rtol=0, atol=1.5e-11
E   
E   Mismatched elements: 419 / 420 (99.8%)
E   Max absolute difference among violations: 1.00135795e-05
E   Max relative difference among violations: 2.23517549e-08
E    ACTUAL: array([[-2.933854e-14,  1.000000e+00,  2.000000e+00,  3.000000e+00,
E            4.000000e+00,  5.000000e+00,  6.000000e+00,  7.000000e+00,
E            8.000000e+00,  9.000000e+00,  1.000000e+01,  1.100000e+01,...
E    DESIRED: array([[  0.+0.j,   1.+0.j,   2.+0.j,   3.+0.j,   4.+0.j,   5.+0.j,
E             6.+0.j,   7.+0.j,   8.+0.j,   9.+0.j,  10.+0.j,  11.+0.j,
E            12.+0.j,  13.+0.j],...
        args       = (<function assert_allclose.<locals>.compare at 0x75e6b5df67a0>, array([[-2.93385379e-14,  1.00000002e+00,  2.00000004e... 439.+0.j, 440.+0.j,
        441.+0.j, 442.+0.j, 443.+0.j, 444.+0.j, 445.+0.j, 446.+0.j,
        447.+0.j, 448.+0.j]]))
        func       = <function assert_array_compare at 0x75ea11b84040>
        kwds       = {'equal_nan': True, 'err_msg': '', 'header': 'Not equal to tolerance rtol=0, atol=1.5e-11', 'strict': False, ...}
        self       = <contextlib._GeneratorContextManager object at 0x75ea13d6e6f0>

ev-br · 2024-08-09T09:12:34Z

This is one of the most complex cases. Here is my understanding of what the logic seems to be doing. Let me know if this is correct @ev-br .

At the module level, we skip for all non-CPU backends apart from CuPy and JAX. CuPy and JAX have some delegation implemented, but other alternative backends do not.
At the level of this test, "output=dtype is numpy-specific" hence we want to skip for CPU backends and JAX as well. But CuPy is okay here (?) so we make an exception for CuPy.
At the level of this test, we xfail for CuPy due to the linked CuPy issue.

Basically, yes. output=dtype is only implemented on numpy and cupy (cupy is via delegation to cupyx). For this test specifically, there's an unrelated cupy issue.

Implementing output=dtype across backends is possible even for JAX. What needs to be done is a mapping from backend dtypes to numpy dtypes (array_api_strict.float32 -> np.float32 etc). This probably needs to be done for each supported backend.

ev-br · 2024-08-09T09:14:21Z

Cupy failures look to be rather straightforward tolerance bump material.

[skip cirrus] [skip circle]

lucascolley · 2024-08-26T12:40:07Z

This is ready to go in IMO - still very open to suggestions for a cleaner interface, but we can iterate. We can also iterate on any local CuPy failures separately.

ev-br

While this may or may not be the end of the story for skip/xfail syntax, this is definitely an improvement over the status quo, so let's land this indeed. If we ever come up with more ergonomic syntax, we'll iterate.

Thanks Lucas, Ralf.

github-actions bot added scipy.ndimage maintenance Items related to regular maintenance tasks labels Aug 7, 2024

lucascolley force-pushed the xfail-xp branch from b148f53 to f4d83f2 Compare August 7, 2024 20:42

lucascolley commented Aug 7, 2024

View reviewed changes

lucascolley force-pushed the xfail-xp branch 2 times, most recently from 8e98ccd to 62d6cff Compare August 8, 2024 21:14

lucascolley commented Aug 8, 2024

View reviewed changes

lucascolley force-pushed the xfail-xp branch from 62d6cff to 662fa1b Compare August 8, 2024 21:39

lucascolley marked this pull request as ready for review August 8, 2024 21:39

lucascolley requested review from rgommers and tylerjereddy August 8, 2024 21:39

lucascolley force-pushed the xfail-xp branch 2 times, most recently from 36db7cb to 85560e7 Compare August 8, 2024 21:44

lucascolley added this to the 1.15.0 milestone Aug 8, 2024

lucascolley added the array types Items related to array API support and input array validation (see gh-18286) label Aug 8, 2024

lucascolley force-pushed the xfail-xp branch from 85560e7 to cb857b4 Compare August 8, 2024 22:16

lucascolley mentioned this pull request Aug 9, 2024

TST/MAINT: ndimage: follow-ups for array API standard support #21280

Open

5 tasks

lucascolley force-pushed the xfail-xp branch from cb857b4 to 01a57fa Compare August 26, 2024 11:41

TST: add xfail_xp_backends

acbcb72

[skip cirrus] [skip circle]

lucascolley force-pushed the xfail-xp branch from 01a57fa to acbcb72 Compare August 26, 2024 12:12

ev-br approved these changes Aug 27, 2024

View reviewed changes

ev-br merged commit be11e29 into scipy:main Aug 27, 2024
38 checks passed

lucascolley deleted the xfail-xp branch August 27, 2024 11:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

TST: add `xfail_xp_backends` #21334

TST: add `xfail_xp_backends` #21334

Uh oh!

lucascolley commented Aug 7, 2024

Uh oh!

rgommers commented Aug 7, 2024

Uh oh!

ev-br commented Aug 7, 2024

Uh oh!

lucascolley Aug 7, 2024 •

edited

Loading

Uh oh!

ev-br Aug 8, 2024

Uh oh!

lucascolley Aug 8, 2024

Uh oh!

ev-br Aug 8, 2024

Uh oh!

lucascolley Aug 8, 2024 •

edited

Loading

Uh oh!

lucascolley commented Aug 8, 2024

Uh oh!

ev-br commented Aug 9, 2024

Uh oh!

ev-br commented Aug 9, 2024

Uh oh!

lucascolley commented Aug 26, 2024

Uh oh!

ev-br left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TST: add xfail_xp_backends #21334

TST: add xfail_xp_backends #21334

Uh oh!

Conversation

lucascolley commented Aug 7, 2024

Reference issue

What does this implement/fix?

Additional information

Uh oh!

rgommers commented Aug 7, 2024

Uh oh!

ev-br commented Aug 7, 2024

Uh oh!

lucascolley Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ev-br Aug 8, 2024

Choose a reason for hiding this comment

Uh oh!

lucascolley Aug 8, 2024

Choose a reason for hiding this comment

Uh oh!

ev-br Aug 8, 2024

Choose a reason for hiding this comment

Uh oh!

lucascolley Aug 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucascolley commented Aug 8, 2024

Uh oh!

ev-br commented Aug 9, 2024

Uh oh!

ev-br commented Aug 9, 2024

Uh oh!

lucascolley commented Aug 26, 2024

Uh oh!

ev-br left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TST: add `xfail_xp_backends` #21334

TST: add `xfail_xp_backends` #21334

lucascolley Aug 7, 2024 •

edited

Loading

lucascolley Aug 8, 2024 •

edited

Loading