-
Notifications
You must be signed in to change notification settings - Fork 4
Tests for converting NumPy dtypes to their equivalent #128
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
Conversation
Ah, I see. I think this is easier fixed in the Two options: either do a local import of numpy (local imports we frown upon though) and add a check along the lines of
or go the duck-typed route: if the argument has a
Our name aliases we delibrately made to follow numpy, so it should work (or maybe a rarer alias will need to be added) to one of the dicts between https://github.com/Quansight-Labs/numpy_pytorch_interop/blob/main/torch_np/_dtypes.py#L164 and |
8ecc725
to
20fab73
Compare
See my latest commit which uses I've left those namespaced dtype tests xfailed so this PR is actully good to merge if you're happy with the |
b0c1a7a
to
aefe64f
Compare
Test failures are real I'm afraid. The problem is not your code though, it's a crutch left from some messy prototyping. Consider
This (surprise!) goes through This is ugly indeed. Let's add an extra |
We don't want to import numpy in this project if we can avoid it, as NumPy is an optional dependency in PyTorch |
As discussed, I don't think this is high prio at the moment, and given the limited budget we have, let's just punt on this for now. Leaving it open in case we decide to fix this later on. |
aefe64f
to
5a0ce8f
Compare
I've removed the (attempted) fix from me, so this PR now only contains the tests I introduced, where anything failing has been xfailed. IMO we can merge these tests at least? |
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
Starts from #116 as that contains a few useful tidbits.
@ev-br see
test_dtype.py
for the failing tests I wrote for converting NumPy dtypes (and thus arrays) to the equivalent interop dtypes. Currently its a mixed bag on what gets converted correctlyI think the solution is pretty simple—we make sure to try using the
str()
of dtypes as a fallback (special casing"bool_"
as"bool"
) before raising an error.Not a priority, just to say I think fixing this is very useful for "comparison testing" between libraries that consume NumPy-proper arrays (including NumPy) and this interop library. See
test_put
as an example of how powerful comparison testing can be with relatively little work.