Repository: tinygrad/tinygrad
Pull request: Add check for negative dimension in view
fixes: #5774

Is it okay to check at a view level?

~~Regression tests pending~~

Problem statement:
better error message for Tensor creation methods with negative dims
currently `Tensor.full((-2,-2), 2)` and `Tensor.full((-2,-2), 2).realize()` both have no error.

torch error: `RuntimeError: Trying to create tensor with negative dimension -2: [-2, -2]`
numpy error: `ValueError: negative dimensions are not allowed`

fix this generically for all Tensor creation methods

Hints from issue discussion:

Commit message: move check to tensor level

Patch to review:
diff --git a/test/test_ops.py b/test/test_ops.py
index b7e57c2834269..1cca365e8154d 100644
--- a/test/test_ops.py
+++ b/test/test_ops.py
@@ -1,5 +1,6 @@
 import time, math, unittest
 import numpy as np
+from typing import List, Callable
 import torch
 from tinygrad.helpers import getenv, IMAGE, DEBUG, CI
 from tinygrad import Tensor, Device, dtypes
@@ -96,6 +97,42 @@ def test_full_like(self):
   def test_full(self):
     helper_test_op([], lambda: torch.full((45,65), 4, dtype=torch.int32), lambda: Tensor.full((45,65), 4), forward_only=True)
 
+  def test_negative_dims(self):
+    creation_methods: List[Callable[..., Tensor]] = [
+      Tensor.empty,
+      Tensor.rand,
+      Tensor.zeros,
+      Tensor.ones,
+      Tensor.randn,
+      Tensor.randint,
+      Tensor.normal,
+      Tensor.uniform,
+      Tensor.scaled_uniform,
+      Tensor.glorot_uniform
+    ]
+
+    for method in creation_methods:
+      with self.assertRaises(RuntimeError): method(-3, 2)
+      with self.assertRaises(RuntimeError): method((2, -3))
+      with self.assertRaises(RuntimeError): method((2, -3, 0))
+
+  def test_negative_dims_full(self):
+    with self.assertRaises(RuntimeError): Tensor.full(-3, 2)
+    with self.assertRaises(RuntimeError): Tensor.full((2, -3), 4)
+    with self.assertRaises(RuntimeError): Tensor.full((2, -3, 0), 4)
+
+  def test_negative_dims_eye(self):
+    with self.assertRaises(RuntimeError): Tensor.eye(-3, 3)
+    with self.assertRaises(AssertionError): Tensor.eye(3, -3)
+    with self.assertRaises(RuntimeError): Tensor.eye(-3, -3)
+
+  def test_negative_dims_kaiming(self):
+    creation_methods = [Tensor.kaiming_uniform, Tensor.kaiming_normal]
+    for method in creation_methods:
+      with self.assertRaises(RuntimeError): method(-3, 3)
+      with self.assertRaises(ValueError): method((-3, 3), 3)
+      with self.assertRaises(ValueError): method((-3, -3), 3)
+
   def test_zeros(self):
     helper_test_op([], lambda: torch.zeros(45,65), lambda: Tensor.zeros(45,65), forward_only=True)
     helper_test_op([], lambda: torch.zeros([45,65]), lambda: Tensor.zeros([45,65]), forward_only=True)
diff --git a/tinygrad/shape/view.py b/tinygrad/shape/view.py
index a194b3a4e8b9c..2d0d7596e0a1e 100644
--- a/tinygrad/shape/view.py
+++ b/tinygrad/shape/view.py
@@ -99,6 +99,7 @@ def size(self) -> int:
   @staticmethod
   @functools.lru_cache(maxsize=None)
   def create(shape:Tuple[sint, ...], strides:Optional[Tuple[sint, ...]]=None, offset:sint=0, mask:Optional[Tuple[Tuple[sint, sint], ...]]=None):
+    assert all(s >= 0 for s in shape), f"invalid view with negative dimension: {shape=}"
     strides = canonicalize_strides(shape, strides) if strides else strides_for_shape(shape)
     # canonicalize 0 in shape
     if 0 in shape: return View(shape, (0,) * len(shape), offset=0, mask=None, contiguous=True)
diff --git a/tinygrad/tensor.py b/tinygrad/tensor.py
index 03468396a9745..a5270f0b761f5 100644
--- a/tinygrad/tensor.py
+++ b/tinygrad/tensor.py
@@ -348,6 +348,7 @@ def from_node(y:Node, **kwargs) -> Tensor:
 
   @staticmethod
   def _metaop(op, shape, device:Optional[Union[Tuple[str, ...], str]]=None, dtype:Optional[DType]=None, arg=None, **kwargs):
+    if not all(s >= 0 for s in shape): raise RuntimeError(f"Trying to create Tensor with negative dimension: {shape=}")
     if isinstance(device, tuple):
       return Tensor(MultiLazyBuffer([LazyBuffer.metaop(op, shape, dtype or dtypes.default_float, Device.canonicalize(d), arg) \
                                       for d in device], None), device, dtype, **kwargs)
@@ -420,6 +421,7 @@ def rand(*shape, device:Optional[Union[Tuple[str, ...], str]]=None, dtype:Option
     counts1, counts2 = (x & 0xffffffff).cast(dtypes.uint32), ((x >> 32) & 0xffffffff).cast(dtypes.uint32)
 
     out = counts1.cat(counts2).rshift(8).cast(dtypes.float32).div(2 ** 24)[:num]
+    if not all(s >= 0 for s in shape): raise RuntimeError(f"Trying to create view with negative dimension: {shape=}")
     out = out.reshape(shape).cast(dtypes.default_float if dtype is None else dtype)
     out.requires_grad = kwargs.get("requires_grad")
     return out.contiguous()
@@ -441,6 +443,7 @@ def full(shape:Tuple[sint, ...], fill_value:ConstType, **kwargs):
     print(Tensor.full((2, 3), False).numpy())
     ```
     """
+    if not all(s >= 0 for s in argfix(shape)): raise RuntimeError(f"Trying to create view with negative dimension: {shape=}")
     return Tensor(fill_value, **kwargs).reshape((1, )*len(new_shape := argfix(shape))).expand(new_shape)
 
   @staticmethod


Code review comments:
diff_hunk: @@ -441,6 +443,7 @@ def full(shape:Tuple[sint, ...], fill_value:ConstType, **kwargs):
     print(Tensor.full((2, 3), False).numpy())
     ```
     """
+    if not all(s >= 0 for s in argfix(shape)): raise RuntimeError(f"Trying to create view with negative dimension: {shape=}")
path: tinygrad/tensor.py
text: @user1:
argfix is called twice, also don't just copy paste error messages. the message does not make sense in Tensor

@author:
Fixed error message, sorry about that

@author:
Also, python won't let me use walrus operator in that context:

```
if not all(s >= 0 for s in (new_shape := argfix(shape))): raise RuntimeError(f"Trying to create Tensor with negative dimension: {shape=}")
```
highlights `:=` as being wrong