Skip to content

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

Merged
merged 1 commit into from
May 17, 2023

Conversation

honno
Copy link
Contributor

@honno honno commented Apr 26, 2023

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 correctly

'bool'                    PASSED
np.dtype('bool')          FAILED
'bool_'                   FAILED
np.bool_                  FAILED
np.dtype('bool_')         FAILED
'int8'                    PASSED
np.int8                   FAILED
np.dtype('int8')          PASSED
'int16'                   PASSED
np.int16                  FAILED
np.dtype('int16')         PASSED
'int32'                   PASSED
np.int32                  FAILED
np.dtype('int32')         PASSED
'int64'                   PASSED
np.int64                  FAILED
np.dtype('int64')         PASSED
'uint8'                   PASSED
np.uint8                  FAILED
np.dtype('uint8')         PASSED
'float16'                 PASSED
np.float16                FAILED
np.dtype('float16')       PASSED
'float32'                 PASSED
np.float32                FAILED
np.dtype('float32')       PASSED
'float64'                 PASSED
np.float64                FAILED
np.dtype('float64')       PASSED
'complex64'               PASSED
np.complex64              FAILED
np.dtype('complex64')     PASSED
'complex128'              PASSED
np.complex128             FAILED
np.dtype('complex128')    PASSED

I 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.

@ev-br
Copy link
Collaborator

ev-br commented Apr 27, 2023

Ah, I see. I think this is easier fixed in the elif chain at https://github.com/Quansight-Labs/numpy_pytorch_interop/blob/main/torch_np/_dtypes.py#L275

Two options: either do a local import of numpy (local imports we frown upon though) and add a check along the lines of

In [23]: issubclass(np.empty(3).dtype.type, np.generic)
Out[23]: True

or go the duck-typed route: if the argument has a name attribute, feed arg.name to sctype_from_string:

In [25]: dt = np.empty(3).dtype

In [26]: dt.name
Out[26]: 'float64'

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 def sctype_from_string

@honno honno force-pushed the asarray-cast-dtypes branch from 8ecc725 to 20fab73 Compare April 27, 2023 07:35
@honno honno marked this pull request as ready for review April 27, 2023 08:33
@honno
Copy link
Contributor Author

honno commented Apr 27, 2023

See my latest commit which uses str(arg) as the fallback in DType.__init__(). The only place we'd really need to import numpy is if we want to support namespaced dtypes (e.g. np.int64, np.float64)... or we could just regex search the str repr to match known dtype strings haha. Supporting namespaced dtypes would be nice as that's what you usually get from numpy.ndarray.dtype.

I've left those namespaced dtype tests xfailed so this PR is actully good to merge if you're happy with the str(arg) fallback.

@honno honno force-pushed the asarray-cast-dtypes branch from b0c1a7a to aefe64f Compare April 27, 2023 08:36
@ev-br
Copy link
Collaborator

ev-br commented Apr 27, 2023

Test failures are real I'm afraid. The problem is not your code though, it's a crutch left from some messy prototyping. Consider

In [12]: tnp.dtype(float)
Out[12]: dtype("float64")

This (surprise!) goes through sctype_from_string accepting python types, via https://github.com/Quansight-Labs/numpy_pytorch_interop/blob/main/torch_np/_dtypes.py#L244.

This is ugly indeed. Let's add an extra elif for this in DType.__init__ instead.

@lezcano
Copy link
Collaborator

lezcano commented May 1, 2023

We don't want to import numpy in this project if we can avoid it, as NumPy is an optional dependency in PyTorch

@lezcano
Copy link
Collaborator

lezcano commented May 17, 2023

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.

@honno honno force-pushed the asarray-cast-dtypes branch from aefe64f to 5a0ce8f Compare May 17, 2023 11:38
@honno honno changed the title Convert NumPy dtypes to their equivalent Tests for converting NumPy dtypes to their equivalent May 17, 2023
@honno
Copy link
Contributor Author

honno commented May 17, 2023

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?

Copy link
Collaborator

@lezcano lezcano left a comment

Choose a reason for hiding this comment

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

LGTM

@lezcano lezcano merged commit 736c29a into Quansight-Labs:main May 17, 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.

3 participants