Repository: rsokl/MyGrad

Issue:
Define InvalidBackprop exception
Currently `Tensor.backprop` and `Operation.backprop` raise a bare `Exception` for when raising for invalid backprop states. We should raise a more descriptive exceptions so that people don't have to do a bare try-except to catch these.

Discussion:
rsokl: Thanks for the review @davidmascharka . Could you give this a second pass once the test suite passes?

Patch:
diff --git a/docs/source/changes.rst b/docs/source/changes.rst
new file mode 100644
index 0000000..0dad65d
--- /dev/null
+++ b/docs/source/changes.rst
@@ -0,0 +1,20 @@
+=========
+Changelog
+=========
+
+This is a record of all past mygrad releases and what went into them,
+in reverse chronological order. All previous releases should still be available
+on pip.
+
+.. _v1.2.0:
+
+-------------------
+1.2.0 - 2019-08-03
+-------------------
+
+We're finally keeping a formal changelog!
+
+This release makes substantial improvements to MyGrad's error-checking and handling, in order to make much simpler the process of debugging issues with buggy custom operations. Specifically, :func:`~mygrad.operation_base.Operation.backward` now checks for an invalid-gradients on each call of :func:`~mygrad.operation_base.Operation.backward_var`, and raises a descriptive error message.
+
+`mygrad.errors` was introduced to provide descriptive, mygrad-specific exceptions. For example, we no longer raise bare exceptions for scenarios like invalid backprop through a scalar-only graph, rather we now raise a descriptive `InvalidBackprop` exception.
+
diff --git a/docs/source/index.rst b/docs/source/index.rst
index 02eebff..5fc0bd5 100644
--- a/docs/source/index.rst
+++ b/docs/source/index.rst
@@ -70,3 +70,4 @@ than is suggested here.
    math
    nnet
    graph_viz
+   changes
diff --git a/mygrad/_utils.py b/mygrad/_utils.py
index 62d1436..ad56b33 100644
--- a/mygrad/_utils.py
+++ b/mygrad/_utils.py
@@ -1,3 +1,9 @@
+from numbers import Real
+from typing import Any
+
+import numpy as np
+
+
 def reduce_broadcast(grad, var_shape):
     """ Sum-reduce axes of `grad` so its shape matches `var_shape.
 
@@ -35,3 +41,19 @@ def reduce_broadcast(grad, var_shape):
         grad = grad.sum(axis=keepdims, keepdims=True)
 
     return grad
+
+
+def is_invalid_gradient(grad: Any) -> bool:
+    """Returns ``True`` if ``grad`` is not array-like.
+
+    Parameters
+    ----------
+    grad : Any
+
+
+    Returns
+    -------
+    ``True`` if ``grad`` is invalid"""
+    return not isinstance(grad, (np.ndarray, Real)) or not np.issubdtype(
+        np.asarray(grad).dtype, np.number
+    )
diff --git a/mygrad/errors.py b/mygrad/errors.py
new file mode 100644
index 0000000..7e6b0d4
--- /dev/null
+++ b/mygrad/errors.py
@@ -0,0 +1,12 @@
+class MyGradException(Exception):
+    """Generic parent class for exceptions thrown by MyGrad."""
+
+
+class InvalidGradient(MyGradException):
+    """An invalid gradient (i.e. a non-numeric or non-array-like object)
+    was produced by an operation or was supplied by a user."""
+
+
+class InvalidBackprop(MyGradException):
+    """Backpropagation was invoked through a partially-cleared graph
+    or from a non-scalar for a scalar-only graph"""
diff --git a/mygrad/operation_base.py b/mygrad/operation_base.py
index cda10dd..b93e633 100644
--- a/mygrad/operation_base.py
+++ b/mygrad/operation_base.py
@@ -2,9 +2,12 @@
 Defines the base class for mathematical operations capable of back-propagating
 gradients to their input tensors."""
 
+from typing import Optional, Set
+
 import numpy as np
 
-from mygrad._utils import reduce_broadcast
+from mygrad._utils import is_invalid_gradient, reduce_broadcast
+from mygrad.errors import InvalidBackprop, InvalidGradient
 
 __all__ = ["Operation", "BroadcastableOp"]
 
@@ -35,9 +38,16 @@ class Operation:
         unambiguously.
         """
 
-    scalar_only = False
+    # tracks if a given operation-instance performs a
+    # non-vectorized or broadcasted operation , which
+    # requires that backpropagation be invoked from a scalar
+    scalar_only = False  # type: bool
+
+    # stores a set of all the operation-instances that participate in
+    # the computational graph up to and including the present operation
+    graph = None  # type: Optional[Set[Operation]]
 
-    def __call__(self, *input_vars):
+    def __call__(self, *input_vars, **kwargs):
         """ Performs a forward pass, f, of this Operation::
 
             f(x1, ...., xn) -> out
@@ -48,6 +58,9 @@ class Operation:
             The input-arguments of f. The tuple (x1, ...., xn)
             should be bound to the instance-attribute `self.variables`
 
+        **kwargs : Any
+            Additional arguments for the operation
+
         Returns
         -------
         numpy.ndarray
@@ -101,13 +114,26 @@ class Operation:
         for index, var in enumerate(self.variables):
             if not var.constant:
                 if not var._ops:
-                    raise Exception(
-                        "Invalid Backprop: part of the computational graph containing "
-                        "this tensor was cleared prior to backprop"
+                    raise InvalidBackprop(
+                        "Part of the computational graph containing "
+                        "this tensor was 'cleared' prior to backprop."
+                    )
+                backed_grad = self.backward_var(grad, index, **kwargs)
+
+                if is_invalid_gradient(backed_grad):
+                    raise InvalidGradient(
+                        "An invalid gradient-value was passed to:"
+                        "\n\t`{call_signature}`"
+                        "\nGradients are expected to be real-valued scalars or "
+                        "numpy arrays, got a gradient of type: {_type}".format(
+                            call_signature="{name}.backward_var(<gradient>, index={index})".format(
+                                name=type(self).__name__, index=index
+                            ),
+                            _type=type(grad),
+                        )
                     )
-
                 if var.grad is None:
-                    tmp_grad = np.asarray(self.backward_var(grad, index, **kwargs))
+                    tmp_grad = np.asarray(backed_grad)
 
                     if _reduction is not None:
                         tmp_grad = _reduction(tmp_grad, var.shape)
@@ -119,12 +145,9 @@ class Operation:
                     )
                 else:
                     if _reduction is None:
-                        var.grad += self.backward_var(grad, index, **kwargs)
+                        var.grad += backed_grad
                     else:
-                        var.grad += _reduction(
-                            self.backward_var(grad, index, **kwargs), var.shape
-                        )
-
+                        var.grad += _reduction(backed_grad, var.shape)
         for var in {
             i for i in self.variables if not i.constant and i.creator is not None
         }:
diff --git a/mygrad/tensor_base.py b/mygrad/tensor_base.py
index 27c0b37..ea1c6f3 100644
--- a/mygrad/tensor_base.py
+++ b/mygrad/tensor_base.py
@@ -9,6 +9,8 @@ from typing import List, Set, Type, Union
 
 import numpy as np
 
+from mygrad._utils import is_invalid_gradient
+from mygrad.errors import InvalidGradient, InvalidBackprop
 from mygrad.linalg.ops import MatMul
 from mygrad.math.arithmetic.ops import *
 from mygrad.operation_base import BroadcastableOp, Operation
@@ -84,7 +86,7 @@ class Tensor:
     derivatives.
 
     >>> f.null_gradients()
-    >>> x.grad is None and y.grad is None and f.grad is Nonw
+    >>> x.grad is None and y.grad is None and f.grad is None
     True
 
     Accessing the Underlying NumPy Array
@@ -171,7 +173,7 @@ class Tensor:
         Op : Type[Operation]
             Operation-class, used to perform forward-pass on `input_vars`.
 
-        input_vars : array_like
+        input_vars : Tuple[array_like]
             An arbitrary number of input-tensors. These can take any form that
             can be converted to an array.  This includes numbers, sequences, nested
             numerical sequences, numpy-ndarrays, and mygrad-tensors.
@@ -274,11 +276,25 @@ class Tensor:
 
         if grad is not None:
             self.grad = np.asarray(grad.data if isinstance(grad, Tensor) else grad)
+            if is_invalid_gradient(self.grad):
+                raise InvalidGradient(
+                    "An invalid gradient-value was passed to "
+                    "\n\t`{call_signature}`"
+                    "\nGradients are expected to be real-valued scalars or "
+                    "numpy arrays, got a gradient of type: {_type}".format(
+                        call_signature="{name}.backward(<gradient>)".format(
+                            name=type(self).__name__
+                        ),
+                        _type=type(grad),
+                    )
+                )
+
         else:
             if self.ndim > 0 and self._scalar_only:
-                raise Exception(
-                    "Invalid Backprop: backpropagation must be triggered by a "
-                    "scalar for this computational graph"
+                raise InvalidBackprop(
+                    "Backpropagation must be invoked from a "
+                    "scalar-tensor (a 0D tensor) for this computational "
+                    "graph."
                 )
             dtype = float if np.issubdtype(self.dtype, np.signedinteger) else self.dtype
             self.grad = (
diff --git a/mygrad/tensor_core_ops/indexing.py b/mygrad/tensor_core_ops/indexing.py
index 89e8782..77b3c7d 100644
--- a/mygrad/tensor_core_ops/indexing.py
+++ b/mygrad/tensor_core_ops/indexing.py
@@ -11,17 +11,22 @@ class GetItem(Operation):
         Supports back-propagation through all valid numpy-indexing (basic, advanced, mixed, etc.)"""
 
     def __call__(self, a, index):
-        """ a[index]
+        """ ``a[index]``
 
-            Parameters
-            ----------
-            a : mygrad.Tensor
-                The tensor whose entries are being accessed.
+        Parameters
+        ----------
+        a : mygrad.Tensor
+            The tensor whose entries are being accessed.
 
-            index : valid-array-index
-                An n-dimensional index for specifying entries or subregions of `a`.
-                All means of numpy-array indexing (basic, advanced, mixed, etc) are
-                supported."""
+        index : valid-array-index
+            An n-dimensional index for specifying entries or subregions of `a`.
+            All means of numpy-array indexing (basic, advanced, mixed, etc) are
+            supported.
+
+        Returns
+        -------
+        numpy.ndarray
+            The array returned by the get-item operation"""
         self.variables = (a,)
         self.index = index
         return a.data[index]


Test patch:
diff --git a/tests/custom_strategies/__init__.py b/tests/custom_strategies/__init__.py
index 8604f7b..d0f13bd 100644
--- a/tests/custom_strategies/__init__.py
+++ b/tests/custom_strategies/__init__.py
@@ -2,6 +2,7 @@
 import math
 from collections.abc import Sequence
 from numbers import Integral
+from typing import Any, Tuple, Union
 
 import hypothesis.extra.numpy as hnp
 import hypothesis.strategies as st
@@ -9,14 +10,36 @@ import numpy as np
 
 __all__ = [
     "adv_integer_index",
+    "basic_index",
     "broadcastable_shape",
     "choices",
+    "everything_except",
     "valid_axes",
-    "basic_index",
 ]
 
 
+def everything_except(
+    excluded_types: Union[type, Tuple[type, ...]]
+) -> st.SearchStrategy[Any]:
+    """Returns hypothesis strategy that generates values of any type other than
+    those specified in ``excluded_types``."""
+    return (
+        st.from_type(type)
+        .flatmap(st.from_type)
+        .filter(lambda x: not isinstance(x, excluded_types))
+    )
+
+
 def _check_min_max(min_val, min_dim, max_dim, param_name, max_val=None):
+    """Ensures that:
+        min_val <= min_dim
+        min_val <= max_dim
+        min_val <= max_val
+    If `max_val` is specified, ensures that `max_dim <= max_val`
+
+    Raises
+    ------
+    ValueError"""
     if not isinstance(min_dim, Integral) or min_dim < min_val:
         raise ValueError(
             "`min_{name}` must be larger than {min_val}. "
@@ -36,6 +59,13 @@ def _check_min_max(min_val, min_dim, max_dim, param_name, max_val=None):
             "Got {val}".format(max_val=max_val, name=param_name, val=max_dim)
         )
 
+    if max_dim < min_dim:
+        raise ValueError(
+            "`min_{name}={_min}` cannot be larger than max_{name}={_max}.".format(
+                _min=min_dim, _max=max_dim, name=param_name
+            )
+        )
+
 
 def choices(seq, size, replace=True):
     """Randomly choose elements from `seq`, producing a tuple of length `size`.
diff --git a/tests/math/unary/test_scipy_mirror.py b/tests/math/unary/test_scipy_mirror.py
index fe5f073..563ad15 100644
--- a/tests/math/unary/test_scipy_mirror.py
+++ b/tests/math/unary/test_scipy_mirror.py
@@ -1,7 +1,7 @@
 import hypothesis.extra.numpy as hnp
 import hypothesis.strategies as st
 import numpy as np
-from hypothesis import given
+from hypothesis import given, settings
 from numpy.testing import assert_array_equal
 from scipy import special
 
@@ -9,6 +9,7 @@ from mygrad.math._special import logsumexp
 from tests.custom_strategies import valid_axes
 
 
+@settings(deadline=None)
 @given(
     data=st.data(),
     x=hnp.arrays(
diff --git a/tests/nnet/test_conv.py b/tests/nnet/test_conv.py
index 4a2fbb3..66f448b 100644
--- a/tests/nnet/test_conv.py
+++ b/tests/nnet/test_conv.py
@@ -3,7 +3,7 @@
 import hypothesis.extra.numpy as hnp
 import hypothesis.strategies as st
 import numpy as np
-from hypothesis import assume, given, settings
+from hypothesis import assume, given, settings, HealthCheck
 from numpy.testing import assert_allclose
 from pytest import raises
 
@@ -240,7 +240,7 @@ def test_conv_1d_bkwd():
     behavior with `constant` arg, good behavior of null_gradients, etc."""
 
 
-@settings(deadline=None)
+@settings(deadline=None, suppress_health_check=(HealthCheck.filter_too_much,))
 @given(
     data=st.data(),
     shape=hnp.array_shapes(min_dims=1, max_dims=3, max_side=12),
@@ -280,7 +280,7 @@ def test_conv_ND_fwd(data, shape, num_filters, num_batch, num_channel):
     assert_allclose(actual=mygrad_conv, desired=numpy_conv, atol=1e-6, rtol=1e-6)
 
 
-@settings(deadline=None)
+@settings(deadline=None, suppress_health_check=(HealthCheck.filter_too_much,))
 @given(
     data=st.data(),
     shape=hnp.array_shapes(min_dims=1, max_dims=3, max_side=6),
diff --git a/tests/tensor_base/test_broadcast.py b/tests/tensor_base/test_broadcast.py
index a233620..10aa682 100644
--- a/tests/tensor_base/test_broadcast.py
+++ b/tests/tensor_base/test_broadcast.py
@@ -1,7 +1,7 @@
 import hypothesis.extra.numpy as hnp
 import hypothesis.strategies as st
 import numpy as np
-from hypothesis import assume, given
+from hypothesis import given
 from numpy.testing import assert_allclose
 from pytest import raises
 
diff --git a/tests/tensor_base/test_graph_tracking.py b/tests/tensor_base/test_graph_tracking.py
new file mode 100644
index 0000000..5c60405
--- /dev/null
+++ b/tests/tensor_base/test_graph_tracking.py
@@ -0,0 +1,28 @@
+import mygrad as mg
+from mygrad.math.arithmetic.ops import Add, Power, Divide
+
+
+def test_op_tracks_graph():
+    """Ensures that ``Operation.graph`` tracks operations as expected"""
+    x = mg.Tensor(1)
+    y = mg.Tensor(2)
+
+    z = x * y
+    assert z.creator.graph == {z.creator}
+
+    f = z + 2
+    assert f.creator.graph == {z.creator, f.creator}
+
+    h = z - f
+    assert h.creator.graph == {h.creator} | f.creator.graph
+
+    i = ((h + 3) ** 2) / 5
+    assert h.creator.graph < i.creator.graph
+    assert (
+        len(i.creator.graph - h.creator.graph) == 3
+    ), "should be {{Add, Subtract, Divide}}, but got {}".format(
+        i.creator.graph - h.creator.graph
+    )
+    assert all(
+        isinstance(x, (Add, Power, Divide)) for x in i.creator.graph - h.creator.graph
+    )
diff --git a/tests/tensor_base/test_scalar_only.py b/tests/tensor_base/test_scalar_only.py
index ff050a1..c0a01df 100644
--- a/tests/tensor_base/test_scalar_only.py
+++ b/tests/tensor_base/test_scalar_only.py
@@ -6,6 +6,7 @@ from pytest import raises
 
 from mygrad.operation_base import BroadcastableOp, Operation
 from mygrad.tensor_base import Tensor
+from mygrad.errors import InvalidBackprop
 
 
 class ScalarOnlyOp(BroadcastableOp):
@@ -44,7 +45,7 @@ def test_scalar_only_op(a_const, a_scalar_only, b_const, b_scalar_only):
 
     # check out.backward()
     if scalar_only:
-        with raises(Exception):
+        with raises(InvalidBackprop):
             out.backward()
     else:
         out.backward()  # a, b, out are const (nothing computed)
@@ -70,7 +71,7 @@ def test_standard_op(a_const, a_scalar_only, b_const, b_scalar_only):
 
     # check out.backward()
     if scalar_only:
-        with raises(Exception):
+        with raises(InvalidBackprop):
             out.backward()
     else:
         if a.constant and b.constant:
@@ -90,5 +91,5 @@ def test_practical_scalar_only(constant, operation):
     if constant:
         out.backward()
     else:
-        with raises(Exception):
+        with raises(InvalidBackprop):
             out.backward()
diff --git a/tests/tensor_base/test_tensor.py b/tests/tensor_base/test_tensor.py
index 6d546f8..23c75a3 100644
--- a/tests/tensor_base/test_tensor.py
+++ b/tests/tensor_base/test_tensor.py
@@ -8,9 +8,11 @@ from pytest import raises
 
 import mygrad as mg
 from mygrad import Tensor
+from mygrad.errors import InvalidBackprop, InvalidGradient
 from mygrad.linalg.ops import MatMul
 from mygrad.math.arithmetic.ops import Add, Divide, Multiply, Negative, Power, Subtract
 from mygrad.operation_base import Operation
+from tests.utils import does_not_raise
 
 
 @given(
@@ -66,11 +68,17 @@ def test_repr(tensor, repr_):
     assert repr(tensor) == repr_
 
 
+@given(constant=st.booleans())
+def test_invalid_gradient_raises(constant: bool):
+    x = Tensor(3, constant=constant) * 2
+    with (pytest.raises(InvalidGradient) if not constant else does_not_raise()):
+        x.backward("bad")
+
+
 @pytest.mark.parametrize("element", (0, [0, 1, 2]))
 def test_contains(element):
     t = Tensor([[0, 1, 2], [3, 4, 5]])
-    assert element in t
-    assert element in t.data
+    assert (element in t) is (element in t.data)
 
 
 @given(
@@ -89,6 +97,7 @@ def test_properties(a, constant, scalar, creator):
         ref = Operation()
         tensor = Tensor(a, constant=constant, _creator=ref, _scalar_only=scalar)
     else:
+        ref = None
         tensor = Tensor(a, constant=constant, _scalar_only=scalar)
 
     assert tensor.ndim == array.ndim
@@ -160,7 +169,11 @@ dtype_strat_numpy = st.sampled_from(
     numpy_dtype=dtype_strat_numpy,
 )
 def test_init_params(data, creator, constant, scalar_only, dtype, numpy_dtype):
-    elements = st.floats if np.issubdtype(numpy_dtype, np.floating) else st.integers
+    elements = (
+        (lambda x, y: st.floats(x, y, width=8 * np.dtype(numpy_dtype).itemsize))
+        if np.issubdtype(numpy_dtype, np.floating)
+        else st.integers
+    )
     a = data.draw(
         hnp.arrays(
             shape=hnp.array_shapes(max_side=3, max_dims=5),
@@ -243,7 +256,7 @@ def test_neg(x):
     x=hnp.arrays(
         shape=hnp.array_shapes(),
         dtype=hnp.floating_dtypes(),
-        elements=st.floats(-10, 10),
+        elements=st.floats(-10, 10, width=16),
     ),
     x_constant=st.booleans(),
     y_constant=st.booleans(),
@@ -252,7 +265,9 @@ def test_neg(x):
 def test_comparison_ops(
     op: str, x: np.ndarray, x_constant: bool, y_constant: bool, data: st.SearchStrategy
 ):
-    y = data.draw(hnp.arrays(shape=x.shape, dtype=x.dtype, elements=st.floats(-10, 10)))
+    y = data.draw(
+        hnp.arrays(shape=x.shape, dtype=x.dtype, elements=st.floats(-10, 10, width=16))
+    )
     x = Tensor(x, constant=x_constant)
     y = Tensor(y, constant=y_constant)
     assert hasattr(Tensor, op), "`Tensor` is missing the attribute {}".format(op)
@@ -431,5 +446,5 @@ def test_clear_graph(x, y, z):
     assert z.creator is None
     assert f.creator is None
 
-    with raises(Exception):
+    with raises(InvalidBackprop):
         g.backward()
diff --git a/tests/test_operation_base.py b/tests/test_operation_base.py
new file mode 100644
index 0000000..c4567b5
--- /dev/null
+++ b/tests/test_operation_base.py
@@ -0,0 +1,44 @@
+import hypothesis.strategies as st
+import numpy as np
+import pytest
+from hypothesis import given
+from hypothesis.extra import numpy as hnp
+
+from mygrad import Tensor
+from mygrad.errors import InvalidGradient
+from mygrad.operation_base import Operation
+from tests.utils import does_not_raise
+
+
+class OldOperation(Operation):
+    """Implements old version of MyGrad back-propagation"""
+
+    def __call__(self, a):
+        self.variables = (a,)
+        return a.data
+
+    def backward_var(self, grad, index, **kwargs):
+        self.variables[index].backward(grad)
+
+
+def old_op(a):
+    return Tensor._op(OldOperation, a)
+
+
+@given(constant=st.booleans(), arr=hnp.arrays(dtype=float, shape=hnp.array_shapes()),
+       op_before=st.booleans(), op_after=st.booleans())
+def test_backpropping_non_numeric_gradient_raises(constant: bool, arr: np.ndarray, op_before: bool,
+                                                  op_after):
+    x = Tensor(arr, constant=constant)
+
+    if op_before:
+        x += 1
+
+    x = old_op(x)
+
+    if op_after:
+        x = x * 2
+
+    # if constant tensor, backprop should not be triggered - no exception raised
+    with (pytest.raises(InvalidGradient) if not constant else does_not_raise()):
+        x.backward()
diff --git a/tests/test_utils.py b/tests/test_utils.py
new file mode 100644
index 0000000..b99a6fb
--- /dev/null
+++ b/tests/test_utils.py
@@ -0,0 +1,36 @@
+from numbers import Real
+
+import hypothesis.extra.numpy as hnp
+import hypothesis.strategies as st
+import numpy as np
+import pytest
+from hypothesis import HealthCheck, given, settings
+
+from mygrad._utils import is_invalid_gradient
+from tests.custom_strategies import everything_except
+
+
+@pytest.mark.parametrize(
+    ("grad", "is_invalid"),
+    [
+        (everything_except((np.ndarray, Real)), True),
+        (None, True),
+        (np.ndarray([1], dtype="O"), True),
+        (
+            hnp.arrays(
+                shape=hnp.array_shapes(),
+                dtype=hnp.floating_dtypes(),
+                elements=st.floats(width=16),
+            ),
+            False,
+        ),
+        ((st.integers(min_value=-1e6, max_value=1e6) | st.floats()), False),
+    ],
+)
+@settings(deadline=None, suppress_health_check=(HealthCheck.too_slow,))
+@given(data=st.data())
+def test_is_invalid_gradient(grad, is_invalid, data: st.DataObject):
+    if isinstance(grad, st.SearchStrategy):
+        grad = data.draw(grad, label="grad")
+
+    assert is_invalid_gradient(grad) is is_invalid, grad
diff --git a/tests/utils/__init__.py b/tests/utils/__init__.py
index e69de29..d4ec2b0 100644
--- a/tests/utils/__init__.py
+++ b/tests/utils/__init__.py
@@ -0,0 +1,17 @@
+from contextlib import contextmanager
+
+
+@contextmanager
+def does_not_raise():
+    """An 'empty' constext manager that yields ``None``. This is
+    to be used in conjunction with ``pytest.raises`` in scenarios
+    where the tested function ought not raise any exception.
+
+    Examples
+    --------
+    >>> import pytest
+    >>> x = "hello"
+    >>> with (pytest.raises(AttributeError) if not isinstance(x, str) else does_not_raise()):
+        ... x.lower()
+    """
+    yield