Quansight-Labs / numpy_pytorch_interop

19 stars 4 forks source link

ENH: NEP 50 "weak scalars" with dtype and PyTorch defaults #143

Closed lezcano closed 1 year ago

lezcano commented 1 year ago

Note that this PR is against the branch from https://github.com/Quansight-Labs/numpy_pytorch_interop/pull/140.

This PR tries to solve in a clean way some simpler cases first (dtype is not None, or we have two tensors) to be able to deal with the tricky part having some preconditions on the data. This fixes a couple bugs that were latent in the other branch. It also adds a test to test for this case.

The logic to solve NEP 50 is still that of https://github.com/Quansight-Labs/numpy_pytorch_interop/pull/140 for the tricky case.

It also tries to avoid iterations over lists that can be very slow. See if the timings of CI agree with this.

ev-br commented 1 year ago

The error on CI is that a test is bad under NEP50:

In [24]: np._set_promotion_state('weak')

In [25]: np.add(1.0, 1e-15, dtype=bool)
Out[25]: True

In [26]: np._set_promotion_state('legacy')

In [27]: np.add(1.0, 1e-15, dtype=bool)
---------------------------------------------------------------------------
UFuncTypeError                            Traceback (most recent call last)
Cell In[27], line 1
----> 1 np.add(1.0, 1e-15, dtype=bool)

UFuncTypeError: Cannot cast ufunc 'add' input 0 from dtype('float64') to dtype('bool') with casting rule 'same_kind'

The test itself is ours, not vendored from numpy, so can be changed easily.

The fact that it started failing now indicates that the test itself is not as strong as it could have been: it's testing the dtype= argument to ufuncs with scalar arguments, so it only probes the scalar + scalar code path. It used to be the only one, but not anymore :-).

lezcano commented 1 year ago

This is so annoying. The uniform treatment of the dtype arg before doing anything else was so clean. ugh.

lezcano commented 1 year ago

Ah, nevermind! The test passes under NEP 50 lol

lezcano commented 1 year ago

removed the test. Also, CI still takes 20 min... Well, I tried.

ev-br commented 1 year ago

Thanks Mario.