-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Avoid unsafe casts from float to unsigned int #9964
base: main
Are you sure you want to change the base?
Conversation
6b366de
to
fefab07
Compare
if xp == np: | ||
# numpy currently doesn't have a view: | ||
return data.view(*args, **kwargs) | ||
return xp.view(data, *args, **kwargs) |
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.
Which package supports this? I can't find xp.view in the standard https://data-apis.org/array-api/draft/API_specification/index.html
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, I didn't realize this was implementing a standard when @kmuehlbauer brought it up in #9815. As in #9815, it works for NumPy and Dask. I don't know about any other packages other than tests don't seem to fail about it specifically.
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.
I didn't check this fully, but to be honest, I also tried it the same way locally.
@Illviljan can you suggest a way forward here, so that it is usable for numpy and dask?
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.
I can also try view
and fall back to astype(copy=False)
if that's more portable. And also not expose view
so publicly if desired.
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.
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.
Using hasattr(__array_namespace__)
implies the method is included in the array api standard. But the standard does not include a .view.
That's why I simply don't think the __array_namespace__
should be used in this case.
But using .astype(copy=False)
does indeed seem like a nice way forward, no?
Note that this adds
.view
toduck_array_ops
; I'm not certain if this is new user API or not.This works across all non-x86_64 architectures in Fedora. I did not test RISC-V, as that is not a primary architecture, but it is likely the same issue and maybe @U2FsdGVkX1 can give it a try.
whats-new.rst
api.rst