From 8b1136d9d5ff4ec5ee59ed6baa4f9a74a9a03763 Mon Sep 17 00:00:00 2001 From: Scott Sanderson Date: Tue, 10 May 2016 18:03:21 -0400 Subject: [PATCH] ENH: Validate missing_values at term construction. Finds bugs in several bad tests that were constructing invalid terms. --- tests/pipeline/test_classifier.py | 4 +-- tests/pipeline/test_factor.py | 2 +- tests/pipeline/test_term.py | 23 +++++++++++++++ zipline/pipeline/term.py | 47 ++++++++++++++++++++++++++++++- 4 files changed, 72 insertions(+), 4 deletions(-) diff --git a/tests/pipeline/test_classifier.py b/tests/pipeline/test_classifier.py index 3eb02020..aba52225 100644 --- a/tests/pipeline/test_classifier.py +++ b/tests/pipeline/test_classifier.py @@ -8,7 +8,6 @@ from zipline.pipeline import Classifier from zipline.testing import parameter_space from zipline.utils.numpy_utils import ( categorical_dtype, - coerce_to_dtype, int64_dtype, ) @@ -162,7 +161,8 @@ class ClassifierTestCase(BasePipelineTestCase): dtype_=[int64_dtype, categorical_dtype], ) def test_disallow_comparison_to_missing_value(self, missing, dtype_): - missing = coerce_to_dtype(dtype_, missing) + if dtype_ == categorical_dtype: + missing = str(missing) class C(Classifier): dtype = dtype_ diff --git a/tests/pipeline/test_factor.py b/tests/pipeline/test_factor.py index 4eee9f2e..fc4eef69 100644 --- a/tests/pipeline/test_factor.py +++ b/tests/pipeline/test_factor.py @@ -444,7 +444,7 @@ class FactorTestCase(BasePipelineTestCase): f = self.f m = Mask() c = C() - str_c = C(dtype=categorical_dtype) + str_c = C(dtype=categorical_dtype, missing_value=None) factor_data = array( [[1.0, 2.0, 3.0, 4.0], diff --git a/tests/pipeline/test_term.py b/tests/pipeline/test_term.py index 502d6e48..4b7bf0de 100644 --- a/tests/pipeline/test_term.py +++ b/tests/pipeline/test_term.py @@ -602,3 +602,26 @@ class SubDataSetTestCase(TestCase): with self.assertRaises(ValueError) as e: SomeClassifier() self.assertEqual(str(e.exception), expected_error) + + def test_unreasonable_missing_values(self): + + for base_type, dtype_, bad_mv in ((Factor, float64_dtype, 'ayy'), + (Filter, bool_dtype, 'lmao'), + (Classifier, int64_dtype, 'lolwut'), + (Classifier, categorical_dtype, 7)): + class SomeTerm(base_type): + inputs = () + window_length = 0 + missing_value = bad_mv + dtype = dtype_ + + with self.assertRaises(TypeError) as e: + SomeTerm() + + prefix = ( + "^Missing value {mv!r} is not a valid choice " + "for term SomeTerm with dtype {dtype}.\n\n" + "Coercion attempt failed with:" + ).format(mv=bad_mv, dtype=dtype_) + + self.assertRegexpMatches(str(e.exception), prefix) diff --git a/zipline/pipeline/term.py b/zipline/pipeline/term.py index 1bffc3e7..4b04d465 100644 --- a/zipline/pipeline/term.py +++ b/zipline/pipeline/term.py @@ -4,7 +4,7 @@ Base class for Filters, Factors and Classifiers from abc import ABCMeta, abstractproperty from weakref import WeakValueDictionary -from numpy import dtype as dtype_class, ndarray +from numpy import array, dtype as dtype_class, ndarray from six import with_metaclass from zipline.errors import ( DTypeNotSpecified, @@ -16,10 +16,12 @@ from zipline.errors import ( WindowLengthNotSpecified, ) from zipline.lib.adjusted_array import can_represent_dtype +from zipline.lib.labelarray import LabelArray from zipline.utils.input_validation import expect_types from zipline.utils.memoize import lazyval from zipline.utils.numpy_utils import ( bool_dtype, + categorical_dtype, default_missing_value_for_dtype, ) from zipline.utils.sentinel import sentinel @@ -177,6 +179,7 @@ class Term(with_metaclass(ABCMeta, object)): """ if dtype is NotSpecified: raise DTypeNotSpecified(termname=termname) + try: dtype = dtype_class(dtype) except TypeError: @@ -188,6 +191,31 @@ class Term(with_metaclass(ABCMeta, object)): if missing_value is NotSpecified: missing_value = default_missing_value_for_dtype(dtype) + try: + if (dtype == categorical_dtype): + # This check is necessary because we use object dtype for + # categoricals, and numpy will allow us to promote numerical + # values to object even though we don't support them. + _assert_valid_categorical_missing_value(missing_value) + + # For any other type, we can check if the missing_value is safe by + # making an array of that value and trying to safely convert it to + # the desired type. + # 'same_kind' allows casting between things like float32 and + # float64, but not str and int. + array([missing_value]).astype(dtype=dtype, casting='same_kind') + except TypeError as e: + raise TypeError( + "Missing value {value!r} is not a valid choice " + "for term {termname} with dtype {dtype}.\n\n" + "Coercion attempt failed with: {error}".format( + termname=termname, + value=missing_value, + dtype=dtype, + error=e, + ) + ) + return dtype, missing_value def __init__(self, *args, **kwargs): @@ -498,3 +526,20 @@ class ComputableTerm(Term): inputs=self.inputs, window_length=self.window_length, ) + + +def _assert_valid_categorical_missing_value(value): + """ + Check that value is a valid categorical missing_value. + + Raises a TypeError if the value is cannot be used as the missing_value for + a categorical_dtype Term. + """ + label_types = LabelArray.SUPPORTED_SCALAR_TYPES + if not isinstance(value, label_types): + raise TypeError( + "Categorical terms must have missing values of type " + "{types}.".format( + types=' or '.join([t.__name__ for t in label_types]), + ) + )