ENH/TST: xp_assert_ enhancements
#267
Conversation
There was a problem hiding this comment.
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.
| @@ -9,13 +9,15 @@ | |||
| from types import ModuleType | |||
| from typing import cast | |||
|
|
|||
| import numpy as np | |||
There was a problem hiding this comment.
Is this OK? Sometimes it was imported within test functions below.
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We'll just need to add a test that import array_api_extra doesn't import numpy.
Note to do this in a follow-up
| 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: |
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
scalar sounds fine to me.
There was a problem hiding this comment.
I think I prefer check_0d, but we can consider this in a follow-up that makes these public
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. |
|
The failures are XPASSes: Judging by the message, we're expecting the test to fail due to |
|
That's what I figured, which is why I tried to set the xfail The trouble is that when 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 So what would you prefer:
If it were me, I would just do option 1. I have no qualms about using |
|
Multiple of those options seem fine with me (at least for now), but I'll ping @crusaderky as the original author first |
xp_assert_ enhancementsxp_assert_ enhancements
xp_assert_ enhancementsxp_assert_ enhancements
We recently released |
|
@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 |
|
Whoops: It's in a non-standard experimental part of the codebase; not exposed to the user. My bad. |
https://labs.quansight.org/blog/sparse-array-ecosystem is probably a good start |
|
Fix pyright failures and some cosmetic design tweaks: mdhaber#2 |
Fix failures in data-apis#267
|
coverage is complaining about cupy being untested in CI #60 |
|
are you looking to add anything else to the scope of this PR @mdhaber ? If not, we can probably merge and iterate on what else is needed for SciPy/scikit-learn in future PRs |
|
@mdhaber gentle ping |
|
Please hold off on merge, but I will get back to this shortly. |
|
Never mind, we don't have to add anything here. |
xp_assert_ enhancementsxp_assert_ enhancements
| @@ -9,13 +9,15 @@ | |||
| from types import ModuleType | |||
| from typing import cast | |||
|
|
|||
| import numpy as np | |||
There was a problem hiding this comment.
We'll just need to add a test that import array_api_extra doesn't import numpy.
Note to do this in a follow-up
| 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: |
There was a problem hiding this comment.
I think I prefer check_0d, but we can consider this in a follow-up that makes these public
|
|
||
| return desired_xp | ||
|
|
||
|
|
||
| def _prepare_for_test(array: Array, xp: ModuleType) -> Array: | ||
| def as_numpy_array(array: Array, *, xp: ModuleType) -> np.t F438 yping.NDArray[Any]: # type: ignore[explicit-any] |
There was a problem hiding this comment.
@jorenham I don't want to open a can of worms by looking at the typing across everywhere this PR touches (this is very close to merge, any improvements could come in a follow-up), but is this single line idiomatic np.typing use?
[skip ci] Co-authored-by: Lucas Colley <lucas.colley8@gmail.com>
Toward #17. As discussed in mdhaber/marray#110 (comment).
This adds a few of the features from SciPy's
xp_assert_functions to thexpxxp_assertfunctions. Tests will probably fail in this first commit; I'll comment inline.I decided not to add a
check_namespacetoggle: ifcheck_namespaceis 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 usescheck_namespace=False, and it probably shouldn't.Once this is looking good, I'll add
xp_assert_lessas a clone ofxp_assert_equal.