Skip to content

Avoid calling hh.arrays with dtype=None for complex types #313

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

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

cbourjau
Copy link
Contributor

Issue description

ndonnx does not (yet) support complex data types. We signal that by setting ARRAY_API_TESTS_SKIP_DTYPES="complex64,complex128". During the collection phase hypothesis_helpers.complex_dtypes is set to None. However, downstream calls to hypothesis_helpers.arrays appear to not be able to handle dtype=None as an argument, ultimately leading to an error when looking up the data type such as:

________________ ERROR collecting array_api_tests/test_operators_and_elementwise_functions.py ________________________
api-coverage-tests/array_api_tests/test_operators_and_elementwise_functions.py:1066: in <module>
    @given(hh.arrays(dtype=hh.complex_dtypes, shape=hh.shapes()))
api-coverage-tests/array_api_tests/hypothesis_helpers.py:73: in arrays
    elements = from_dtype(dtype)
api-coverage-tests/array_api_tests/hypothesis_helpers.py:44: in from_dtype
    min_, max_ = dh.dtype_ranges[component_dtype]
api-coverage-tests/array_api_tests/dtype_helpers.py:85: in __getitem__
    raise KeyError(f"{key!r} not found")
E   KeyError: 'None not found'

Since data types are looked up using __eq__ things appear to work if any of the data types compare equal to None which is the case for numpy.dtype("float64"), for example.

Solution

This PR simply guards each affected tests (i.e. tests that work on hypothesis_helpers.complex_dtypes) with a respective if statement. This works just fine in practice for us, but I'm sure there is a better fix out there. I tried to set complex_dtypes = (), which seems reasonable, but that lead to other errors.

@asmeurer
Copy link
Member

Does using pytest.skip not work here? That would be preferred if it can work.

@asmeurer
Copy link
Member

You could also try modifying hh.arrays so that it fails at example draw time instead of at function call time when dtypes=().

I'm curious how this is able to work when complex dtypes aren't enabled due to older standard versions. Maybe that isn't ever actually tested anywhere, since even array-api-strict's older standard flags doesn't actually support versions prior to adding complex support.

We definitely do want to support skipping complex dtypes with ARRAY_API_TESTS_SKIP_DTYPES, but that hasn't been tested before.

@cbourjau
Copy link
Contributor Author

cbourjau commented Dec 1, 2024

Apologies for the late reply!

Does using pytest.skip not work here? That would be preferred if it can work.

I'm not sure how we may use pytest.skip here. The error occurs already during pytest's collection phase meaning that neither pytest.mark.skipif nor pytest.skip are applicable, AFAICT. Calling the latter from hypothesis_helpers.arrays yields:

============================================== ERRORS ==============================================
___________________________ ERROR collecting array_api_tests/test_fft.py ___________________________
Using pytest.skip outside of a test will skip the entire module. If that's your intention, pass `allow_module_level=True`. If you want to skip a specific test or an entire class, use the @pytest.mark.skip or @pytest.mark.skipif decorators.

You could also try modifying hh.arrays so that it fails at example draw time instead of at function call time when dtypes=().

Might you have an example where something like that is already done in this project? I'm rather inexperienced with hypothesis.

@cbourjau cbourjau force-pushed the guard-tests-no-complex-dtypes branch from 523bf4c to c43c83a Compare January 13, 2025 03:28
@cbourjau
Copy link
Contributor Author

After digging a bit into the hypothesis documentation it seems that using the nothing strategy is the way to go here.

@cbourjau
Copy link
Contributor Author

How does this look to you, @asmeurer ?

@cbourjau
Copy link
Contributor Author

@ev-br do you have feedback on this PR?

Copy link
Member

@ev-br ev-br left a comment

Choose a reason for hiding this comment

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

TIL about a pretty arcane hypothesis feature, as in, the nothing strategy. Wow.

The PR looks good to me, merging.

Sorry @cbourjau it took us this long.


def all_floating_dtypes() -> SearchStrategy[DataType]:
strat = floating_dtypes
if api_version >= "2022.12" and complex_dtypes is not None:
if api_version >= "2022.12" and not complex_dtypes.is_empty:
Copy link
Member

Choose a reason for hiding this comment

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

For the record, is_empty here and below relies on the following:

In [16]: sampled_from([1, 2, 3]).is_empty
Out[16]: False

In [17]: nothing().is_empty
Out[17]: True

@ev-br ev-br merged commit d982a62 into data-apis:master Jan 30, 2025
3 checks passed
@cbourjau cbourjau deleted the guard-tests-no-complex-dtypes branch January 30, 2025 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants