Skip to content

Fix some issues #181

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 2 commits into from
Apr 28, 2023
Merged

Fix some issues #181

merged 2 commits into from
Apr 28, 2023

Conversation

asmeurer
Copy link
Member

Some issues that came up with testing pytorch against the 2022 spec (this doesn't address any of the issues I opened).

+ [make_param("__int__", d, int) for d in dh.all_int_dtypes]
+ [make_param("__index__", d, int) for d in dh.all_int_dtypes]
+ [make_param("__int__", d, int) for d in dh._filter_stubs(*dh.all_int_dtypes)]
+ [make_param("__index__", d, int) for d in dh._filter_stubs(*dh.all_int_dtypes)]
Copy link
Member Author

Choose a reason for hiding this comment

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

Let me know if there's a better way to do this. I'm not sure if this internal function is supposed to be used outside of the dtype_helpers module. I'm a little unclear why all_int_dtype isn't just filtered to begin with?

Copy link
Member

Choose a reason for hiding this comment

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

Alternative approach which skips undefined stubs

def test_scalar_casting(method_name, dtype, stype, data):
+    if isinstance(dtype, xp._UndefinedStub):
+        pytest.skip(f"{dtype} not in xp namespace")

I could go either way with this. I like the idea of having the test case for uint{16,32,64} just exist as it is expected behaviour in the spec afterall, but I also get its noisy.

I'm a little unclear why all_int_dtype isn't just filtered to begin with?

IIRC it's for the above reason, so it exists for such test cases and we can subsequently skip. Again, could go either way on that now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yeah skipping is probably better.

Copy link
Member

@honno honno left a comment

Choose a reason for hiding this comment

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

LGTM!

+ [make_param("__int__", d, int) for d in dh.all_int_dtypes]
+ [make_param("__index__", d, int) for d in dh.all_int_dtypes]
+ [make_param("__int__", d, int) for d in dh._filter_stubs(*dh.all_int_dtypes)]
+ [make_param("__index__", d, int) for d in dh._filter_stubs(*dh.all_int_dtypes)]
Copy link
Member

Choose a reason for hiding this comment

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

Alternative approach which skips undefined stubs

def test_scalar_casting(method_name, dtype, stype, data):
+    if isinstance(dtype, xp._UndefinedStub):
+        pytest.skip(f"{dtype} not in xp namespace")

I could go either way with this. I like the idea of having the test case for uint{16,32,64} just exist as it is expected behaviour in the spec afterall, but I also get its noisy.

I'm a little unclear why all_int_dtype isn't just filtered to begin with?

IIRC it's for the above reason, so it exists for such test cases and we can subsequently skip. Again, could go either way on that now.

@asmeurer
Copy link
Member Author

Skipping would be better, but unfortunately these tests fail at collection time (in the parameterize call), so your fix doesn't work. If you know how to do it, we can change it. In the meantime I'm going to merge this because I need it to fix the array-api-compat CI.

@asmeurer asmeurer merged commit 1a73804 into data-apis:master Apr 28, 2023
@asmeurer asmeurer mentioned this pull request May 1, 2023
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.

2 participants