WIP: ENH/TST: `xp_assert_` enhancements #267

mdhaber · 2025-04-16T17:55:03Z

Toward #17. As discussed in mdhaber/marray#110 (comment).

This adds a few of the features from SciPy's xp_assert_ functions to the xpx xp_assert functions. Tests will probably fail in this first commit; I'll comment inline.

I decided not to add a check_namespace toggle: if check_namespace is False and the namespaces don't match, other operations might fail. I didn't want to work on fixing this if it's not an important feature. There is only one file in SciPy that uses check_namespace=False, and it probably shouldn't.

Once this is looking good, I'll add xp_assert_less as a clone of xp_assert_equal.

mdhaber

Easier to review with "Hide whitespace". @lucascolley do you see what is going on with

FAILED tests/test_testing.py::test_assert_close_equal_shape[sparse-True-xp_assert_close]
FAILED tests/test_testing.py::test_assert_close_equal_dtype[sparse-True-xp_assert_close]

?

Also, it looks to me like the codecov failures are misleading. It doesn't look to me like the new code is actually missing coverage if tests are run with all backends.

mdhaber · 2025-04-16T17:55:21Z

src/array_api_extra/_lib/_testing.py

@@ -9,13 +9,15 @@
 from types import ModuleType
 from typing import cast

+import numpy as np


Is this OK? Sometimes it was imported within test functions below.

Today it is OK, as this module is not imported automatically from the outer scope.
In the long run though, we want to move this module to public at which point it won't be a good design anymore (although it remains to be seen if any Array library in real life can achieve not to have numpy as a hard dependency...)

Yes, I think it would be fine to make these public API once they are ready, with the caveat that NumPy is required. We are really striving for minimal runtime dependencies rather than test time dependencies downstream, at least for now.

This module heavily relies on np.testing.assert* anyway.
We'll just need to add a test that import array_api_extra doesn't import numpy.

mdhaber · 2025-04-16T17:56:11Z

src/array_api_extra/_lib/_testing.py

+        msg = f"dtypes do not match: {actual.dtype} != {desired.dtype}"
+        assert actual.dtype == desired.dtype, msg
+
+    if is_numpy_namespace(actual_xp) and check_scalar:


Suggested change

if is_numpy_namespace(actual_xp) and check_scalar:

if is_numpy_namespace(actual_xp) and check_arrayness:

?
I seem to remember some discussion about naming this parameter when adding to SciPy (check_0d). I don't really care what it is called. check_type is also on the table.

scalar sounds fine to me.

src/array_api_extra/_lib/_testing.py

tests/test_testing.py

src/array_api_extra/_lib/_testing.py

lucascolley · 2025-04-16T18:41:07Z

It doesn't look to me like the new code is actually missing coverage if tests are run with all backends.

Yeah, this is a constant problem. It eventually flips to passed once it registers the backends run. I think initial investigation didn't find a solution, but there is probably some sort of config/workaround to fix it.

lucascolley · 2025-04-16T18:57:04Z

The failures are XPASSes: [XPASS(strict)] sparse:no isdtype

Judging by the message, we're expecting the test to fail due to sparse not implementing isdtype, but it is now passing. Could that be because of any of your changes?

mdhaber · 2025-04-16T21:31:56Z

That's what I figured, which is why I tried to set the xfail strict=False in param_assert_equal_close. I guess that option is not respected as written.

The trouble is that when check_shape or check_dtype is False, _check_ns_shape_dtype passes, the code continues, and then we run into the AttributeError.

        xp = _check_ns_shape_dtype(actual, desired, check_dtype, check_shape, check_scalar)
    
       floating = xp.isdtype(actual.dtype, ("real floating", "complex floating"))
       # AttributeError: module 'sparse' has no attribute 'isdtype'. Did you mean: 'astype'?

So whether the test will fail with sparse arrays due to the AttributeError depends on check_shape/check_dtype. We cannot simply use the param_assert_equal_close decorator to xfail (strict) the sparse tests, nor can we remove it.

So what would you prefer:

Mark the test with pytest.xfail inside the test function considering the value of check_shape/check_dtype (in addition to the backend)
Create some alternative to param_assert_equal_close that effectively does the same thing as the previous option (but is harder for me to write)
Make the XFAIL with sparse arrays not strict (perhaps I can figure out why strict=False was not enough?)
Just skip the test with sparse arrays
Remove the parameterization and write two separate tests just to accommodate for sparse arrays
Something else?

If it were me, I would just do option 1. I have no qualms about using pytest.xfail within a function, but since someone went through the trouble of creating param_assert_equal_close, it seems that is not desirable to everyone.

lucascolley · 2025-04-16T21:49:03Z

Multiple of those options seem fine with me (at least for now), but I'll ping @crusaderky as the original author first

hameerabbasi · 2025-04-17T17:46:03Z

The failures are XPASSes: [XPASS(strict)] sparse:no isdtype

Judging by the message, we're expecting the test to fail due to sparse not implementing isdtype, but it is now passing. Could that be because of any of your changes?

We recently released 0.16.0; I believe that may have added isdtype.

mdhaber · 2025-04-17T18:26:36Z

@hameerabbasi If so, it does not appear to be documented at https://sparse.pydata.org/en/stable/api/. Is that where I should be looking?

Also - I'm not very familiar with the sparse project - is there somewhere that summarizes the current relationship between scipy.sparse and sparse projects?

hameerabbasi · 2025-04-17T20:13:29Z

Whoops: It's in a non-standard experimental part of the codebase; not exposed to the user. My bad.

lucascolley · 2025-04-17T20:27:48Z

is there somewhere that summarizes the current relationship between scipy.sparse and sparse projects?

https://labs.quansight.org/blog/sparse-array-ecosystem is probably a good start

crusaderky · 2025-04-21T11:49:25Z

src/array_api_extra/_lib/_testing.py

+        assert (np.isscalar(actual) and np.isscalar(desired)) or (
+            not np.isscalar(actual) and not np.isscalar(desired)
+        ), _msg


Suggested change

assert (np.isscalar(actual) and np.isscalar(desired)) or (

not np.isscalar(actual) and not np.isscalar(desired)

), _msg

assert np.isscalar(actual) == np.isscalar(desired), _msg

src/array_api_extra/_lib/_testing.py

crusaderky · 2025-04-21T11:57:03Z

src/array_api_extra/_lib/_testing.py

+    check_scalar : bool, default: False
+        NumPy only: whether to check agreement between actual and desired types -
+        0d array vs scalar.


The default for this is the opposite as in scipy

I meant to mention that, so thanks for bringing it up. I think it should be True, but that would make a lot of tests fail internally. I figured that could be fixed in a follow-up.

array-api-extra tests? If so, yes, that sounds fine, but we should open an issue for that before merging this

Yes. Sure, I'll open an issue.

src/array_api_extra/_lib/_testing.py

crusaderky · 2025-04-21T11:59:37Z

src/array_api_extra/_lib/_testing.py

-            err_msg=err_msg,
-        )
+    # JAX/Dask arrays work directly with `np.testing`
+    assert isinstance(rtol, float)


isinstance(0, float) returns False

Suggested change

assert isinstance(rtol, float)

assert isinstance(rtol, int | float)

That's fine, but that code was here - do we even need it?

It wasn't here. Maybe in scipy? Agree that the assert is overkill.

it was:

array-api-extra/src/array_api_extra/_lib/_testing.py

Line 188 in 6e3ad0f

assert isinstance(rtol, float)

I stand corrected. 🙏

tests/test_testing.py

crusaderky · 2025-04-21T14:48:09Z

tests/test_testing.py

+            marks=pytest.mark.xfail_xp_backend(
+                Backend.SPARSE, reason="no isdtype", strict=False
+            ),


#269 adds support for strict=False, which was previously quietly ignored.

crusaderky · 2025-04-21T12:16:48Z

src/array_api_extra/_lib/_testing.py

+        actual_shape = actual.shape
+        desired_shape = desired.shape


This may fail if we start using it in scipy, because scipy overrides array_namespace to return numpy for scalars and lists. Maybe out of scope for this PR I though?

Huh. Yeah let's do l consider that in a SciPy PR that attempts to use this there. Then we can decide whether/what changes are needed.

crusaderky · 2025-04-21T14:48:09Z

tests/test_testing.py

+            marks=pytest.mark.xfail_xp_backend(
+                Backend.SPARSE, reason="no isdtype", strict=False
+            ),


#269 adds support for strict=False, which was previously quietly ignored.

mdhaber

I'm leaving the pyright failures so you can decide how you want to deal with them. I think all of them are false positives, as usual with static type checking.

mdhaber · 2025-04-21T17:53:32Z

src/array_api_extra/_lib/_testing.py

+    check_scalar : bool, default: False
+        NumPy only: whether to check agreement between actual and desired types -
+        0d array vs scalar.


mdhaber · 2025-04-21T17:54:52Z

src/array_api_extra/_lib/_testing.py

-        import numpy as np  # pylint: disable=import-outside-toplevel
-
-        # JAX/Dask arrays work directly with `np.testing`
-        assert isinstance(rtol, float)


This was probably added to avoid the pyright error below? I don't think pyright should make us do this sort of thing.

mdhaber · 2025-04-21T17:55:16Z

tests/test_testing.py

+            marks=pytest.mark.xfail_xp_backend(
+                Backend.SPARSE, reason="no isdtype", strict=False
+            ),


mdhaber · 2025-04-21T17:57:07Z

tests/test_testing.py

+        else nullcontext()
+    )
+    with context:
+        func(xp.asarray([xp.nan, xp.nan]), xp.asarray(xp.nan), check_shape=check_shape)


NaNs pass NumPy's _close, _equal, and _less checks, so using NaNs here (and below) allows us to parametrize over all three functions.

WIP: xp_assert enhancements

1140a1a

mdhaber commented Apr 16, 2025

View reviewed changes

mdhaber marked this pull request as draft April 16, 2025 18:17

lucascolley changed the title ~~WIP: xp_assert_ enhancements~~ WIP: ENH: xp_assert_ enhancements Apr 16, 2025

lucascolley added enhancement New feature or request xpx.testing testing labels Apr 16, 2025

lucascolley changed the title ~~WIP: ENH: xp_assert_ enhancements~~ WIP: ENH/TST: xp_assert_ enhancements Apr 16, 2025

crusaderky reviewed Apr 21, 2025

View reviewed changes

crusaderky mentioned this pull request Apr 21, 2025

TST: xfail_xp_backend(strict=False) #269

Merged

crusaderky reviewed Apr 21, 2025

View reviewed changes

mdhaber added 2 commits April 21, 2025 09:52

Merge remote-tracking branch 'upstream/main' into xp_assert_enhancements

054fba0

ENH: add xp_assert_less

d7f7549

mdhaber mentioned this pull request Apr 21, 2025

API/TST: array-ness of desired and expected values does not match in several tests #271

Open

mdhaber commented Apr 21, 2025

View reviewed changes

	if is_numpy_namespace(actual_xp) and check_scalar:
	if is_numpy_namespace(actual_xp) and check_arrayness:

	assert isinstance(rtol, float)
	assert isinstance(rtol, int \| float)

WIP: ENH/TST: xp_assert_ enhancements #267

Are you sure you want to change the base?

WIP: ENH/TST: xp_assert_ enhancements #267

Conversation

mdhaber commented Apr 16, 2025 • edited Loading

mdhaber left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crusaderky Apr 21, 2025 • edited Loading

Choose a reason for hiding this comment

mdhaber Apr 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucascolley commented Apr 16, 2025 • edited Loading

lucascolley commented Apr 16, 2025

mdhaber commented Apr 16, 2025 • edited Loading

lucascolley commented Apr 16, 2025 • edited Loading

hameerabbasi commented Apr 17, 2025

mdhaber commented Apr 17, 2025 • edited Loading

hameerabbasi commented Apr 17, 2025

lucascolley commented Apr 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdhaber Apr 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdhaber Apr 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crusaderky Apr 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdhaber left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WIP: ENH/TST: `xp_assert_` enhancements #267

WIP: ENH/TST: `xp_assert_` enhancements #267

mdhaber commented Apr 16, 2025 •

edited

Loading

mdhaber left a comment •

edited

Loading

crusaderky Apr 21, 2025 •

edited

Loading

mdhaber Apr 16, 2025 •

edited

Loading

lucascolley commented Apr 16, 2025 •

edited

Loading

mdhaber commented Apr 16, 2025 •

edited

Loading

lucascolley commented Apr 16, 2025 •

edited

Loading

mdhaber commented Apr 17, 2025 •

edited

Loading

mdhaber Apr 21, 2025 •

edited

Loading

mdhaber Apr 21, 2025 •

edited

Loading

crusaderky Apr 21, 2025 •

edited

Loading

mdhaber left a comment •

edited

Loading