-
Notifications
You must be signed in to change notification settings - Fork 45
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
Fix some issues #181
Conversation
+ [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)] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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)] |
There was a problem hiding this comment.
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.
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. |
Some issues that came up with testing pytorch against the 2022 spec (this doesn't address any of the issues I opened).