Skip to content

Don't test values that are on/near the boundary of an array's dtype #226

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 3 commits into from
Dec 15, 2023

Conversation

honno
Copy link
Member

@honno honno commented Dec 14, 2023

This PR should prevent Hypothesis testing boundary numbers that are unrepresentable for libraries like JAX @jakevdp @rgommers. There might be a performance gain but not sure—we know vectorisation would be the biggest win anywho.

I'll sleep on it but I think its probably fine to just get this in, and a TODO would be to make this optional behaviour and/or check the library we're testing on runtime.

Just to say, this is achieved by setting the min/max value arguments in Hypothesis strategies. Historically there were issue where setting boundaries caused undesirable example generation (HypothesisWorks/hypothesis#2907), but that should be all sorted 🤔

Relevant discussion from #206 (review)

Although I will say it has generally been hard for us to keep up with all the various CI failures in this repo, mostly due to the nature of the test suite that there are expected failures.

I think we should try a little harder to prevent Hypothesis from using floats like 1.5e+308 or ints like 2**63. It results in a lot of spurious failures that are costing us a lot of time to deal with and make using the test suite harder, but are of little interest. Isn't there something like a "use only reasonable ranges" setting?

Yeah we do try avoiding spurious failures, but its been on an adhoc basis when issues have come up with adopters. A blanket "use only reasonable ranges" of Hypothesis would require some re-thinking on how the test suite is architected 🤔

Like 1.5e+308 shouldn't ever be a problem for manipulation functions, so for me it is pretty nice to make sure that is actually the case—on the other hand, yeah we don't really care if some element-wise functions don't play nice with massive floats. And well "nice to have" testing isn't also that important if it shouldn't matter anyway, so I defo see your point.

and

It's very unlikely to fail though, and very unlikely to be real-world relevant. So there's 2 reasons that it's basically a dont-care. I encourage you to think about something like limiting the default range to something like 1e20 for float64, 1e8 for float32, etc. This should be easy to do I'd expect, and a significant improvement in making the test suite more usable/robust, and hence more useful for folks implementing support in their array library.

@honno honno requested a review from asmeurer December 14, 2023 20:05
@honno honno marked this pull request as ready for review December 15, 2023 16:32
@honno
Copy link
Member Author

honno commented Dec 15, 2023

Seems all good so YOLO

@honno honno merged commit 974367f into data-apis:master Dec 15, 2023
@honno
Copy link
Member Author

honno commented Dec 15, 2023

Note to self: remember to set JAX_ENABLE_X64 as 1 when testing JAX!!

@honno honno deleted the jax-niceties branch February 28, 2024 13:19
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.

1 participant