Skip to content
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

ENH: add result_type_false: check the minimum promotion rules according to the spec #321

Closed
wants to merge 1 commit into from

Conversation

ev-br
Copy link
Contributor

@ev-br ev-br commented Nov 21, 2024

https://data-apis.org/array-api/latest/API_specification/type_promotion.html#type-promotion specifies the minimum required promotion rules, and states that

A conforming implementation of the array API standard may support additional type promotion rules beyond those described in this specification.

array_api_strict only allows this minimal promotion rules. So, add a test that promotions beyond this minimum set are not allowed. The test is mostly informational: an array provider may want signal that it allows extra promotion paths by xfailing the test.

…ng to the spec

Conforming array libraries may extend the minimum promotion rules. If they do, they
will fail this new test and may want to xfail it.
@ev-br
Copy link
Contributor Author

ev-br commented Nov 21, 2024

This PR is basically to

  • gauge the feeling towards this sort of tests. I think it makes sense to use the test suite to sort of document all sorts of deviations from the strict spec, including where array provider libs extend the standard, not only where they lack something that standard stipulates;
  • ask for a review of the added hypothesis strategy

@rgommers
Copy link
Member

So, add a test that promotions beyond this minimum set are not allowed.

This feels wrong to me. This test suite should not start including tests for behavior that is allowed by the standard (and common in practice), but just not standardized. That just leads to a lot of busywork I think?

@asmeurer
Copy link
Member

I agree this test doesn't make sense. We could use something like this in array_api_strict when testing strictness. Currently those tests don't use hypothesis, but rather just test each combination.

As far as the strategy, it looks like it would do what it says it does. I don't understand the max_size parameter that does nothing. Also, to be fully complete, it would need to include uint64. Maybe a cleaner way to implement it would be to take all possible combinations of dtypes, remove the ones that are promotable, and sample_from that. (but again, I don't know that we actually need this strategy)

@ev-br
Copy link
Contributor Author

ev-br commented Nov 23, 2024

I still think it's a not totally invalid approach, the decision is only where the documentation is, here or in the array libraries. But okay, the reaction is pretty clear :-). Let's close this. Thanks for the review!

@ev-br ev-br closed this Nov 23, 2024
@ev-br ev-br added the wontfix This will not be worked on label Nov 23, 2024
@rgommers
Copy link
Member

the decision is only where the documentation is

It's more than that. Note that "should" and "must" have precise meanings, see the discussion at data-apis/array-api#796 (we should really close that issue by clarifying the docs). The test suite by design has to verify all the "must" requirements, but not enforce "should" recommendations like they are "must" requirements, nor enforce things that aren't specified.

In the case of type promotion rules, the spec lays out what must be implemented and nothing else. It says:

"Mixed integer and floating-point type promotion rules are not specified because behavior varies between implementations."

If it had said something like "mixed integer and floating-point ... must raise" or "...must not be implemented" then it needs a test like this. In the absence of that language, it doesn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants