Make ones/zeros/empty/full dtype handling more uniform #51
Conversation
ev-br
force-pushed
the
infer-full-dtype-followup
branch
from
74cdad2 to
47e8adf
Compare
|
@honno what to you think this further tweak to |
ev-br
force-pushed
the
infer-full-dtype-followup
branch
from
a87462d to
8f1b501
Compare
Also implements `DType.kind` to raise `NotImplementedError`
ev-br
force-pushed
the
infer-full-dtype-followup
branch
from
8f1b501 to
89a8d54
Compare
|
1b41e14 implements |
|
Niche behaviour I found in NumPy >>> np.zeros((1, 5), dtype="c") # character dtype special case
array([[b'', b'', b'', b'', b'']], dtype='|S1')Doubt we care to implement this, just wondering if it would be nice to check for this and raise TypeError: data type 'c' not understood |
|
ISTM this is the best we can do with non-numeric dtypes, which are not supported by pytorch. Do you think NotImplementedError is more descriptive than TypeError? |
|
IMO |
| @@ -288,12 +288,15 @@ def arange(start=None, stop=None, step=1, dtype=None, *, like=None): | |||
| raise ValueError("Maximum allowed size exceeded") | |||
|
|
|||
|
|
|||
| @_decorators.dtype_to_torch | |||
| def empty(shape, dtype=float, order="C", *, like=None): | |||
| _util.subok_not_ok(like) | |||
There was a problem hiding this comment.
There's a bit of a mismatch here?
There was a problem hiding this comment.
shouldn't we have a function that's _util.like_not_implemented(like) or so? Why are we passing like to subok?
There was a problem hiding this comment.
Ah, so it's the util name which is confusing. Conceptually, I'd say that like and subok are similar: one is specifically for ndarray subclasses, the other one is for a newer __array_wrap__ protocol (I think). Most functions which work for subclasses or the protocol, have both arguments, some have one or the other. We are not going to support either of them. The utility is literally two lines:
https://github.com/Quansight-Labs/numpy_pytorch_interop/blob/main/torch_np/_detail/_util.py#L22
So if we go with utility functions I'd keep it as is, can rename it if really wanted. If we transform to the normalization approach of gh-32, these two would likely get their own type annotations.
So overall I'd rather not touch it now.
| fill_value = asarray(fill_value).get() | ||
| if dtype is None: | ||
| torch_dtype = asarray(fill_value).get().dtype | ||
| else: | ||
| torch_dtype = _dtypes.torch_dtype_from(dtype) | ||
| return asarray(torch.full(shape, fill_value, dtype=torch_dtype)) | ||
| dtype = fill_value.dtype |
There was a problem hiding this comment.
A better way to implement this would be having a function that given a tensor or a number returns its torch dtype. The reason here is that we don't want to wrap scalars into tensors if we don't need to. Scalars are easier to treat in a compiler than tensors, and often get extra optimisations (no memory allocations, no synchronisations when accessed if we are working on CUDA, you can specialise on them...)
Note that this applies in general, so we should be mindful about this going all across.
There was a problem hiding this comment.
Let's move this to #56, to not conflate a small cleanup of this PR with a rather large rework.
|
Merging to keep the ball rolling. Comments ask for a codebase wide tweaks, so can be addressed in a sequel. |
while at it, un-xfail test_numeric.py::TestTypes.