From 86486803b65000f18cb07a768087e37d194229ff Mon Sep 17 00:00:00 2001 From: dmichalowicz Date: Thu, 19 May 2016 16:33:15 -0400 Subject: [PATCH] BUG: custom factor outputs naming collisions --- tests/pipeline/test_term.py | 20 +++ zipline/errors.py | 11 ++ zipline/pipeline/data/dataset.py | 8 +- zipline/pipeline/expression.py | 4 +- zipline/pipeline/factors/factor.py | 40 +++--- zipline/pipeline/filters/filter.py | 12 +- zipline/pipeline/term.py | 189 ++++++++++++++++------------- 7 files changed, 171 insertions(+), 113 deletions(-) diff --git a/tests/pipeline/test_term.py b/tests/pipeline/test_term.py index 206e38fd..e6db7d1d 100644 --- a/tests/pipeline/test_term.py +++ b/tests/pipeline/test_term.py @@ -7,6 +7,7 @@ from unittest import TestCase from zipline.errors import ( DTypeNotSpecified, + InvalidOutputName, NonWindowSafeInput, NotDType, TermInputsNotSpecified, @@ -24,6 +25,7 @@ from zipline.pipeline import ( ) from zipline.pipeline.data import Column, DataSet from zipline.pipeline.data.testing import TestingDataSet +from zipline.pipeline.factors import RecarrayField from zipline.pipeline.term import AssetExists, NotSpecified from zipline.pipeline.expression import NUMEXPR_MATH_FUNCS from zipline.testing import parameter_space @@ -90,6 +92,9 @@ class MultipleOutputs(CustomFactor): inputs = [SomeDataSet.foo, SomeDataSet.bar] outputs = ['alpha', 'beta'] + def some_method(self): + return + def gen_equivalent_factors(): """ @@ -476,6 +481,21 @@ class ObjectIdentityTestCase(TestCase): errmsg, "GenericCustomFactor does not have multiple outputs.", ) + # Public method, user-defined method. + # Accessing these attributes should return the output, not the method. + conflicting_output_names = ['zscore', 'some_method'] + + mo = MultipleOutputs(outputs=conflicting_output_names) + for name in conflicting_output_names: + self.assertIsInstance(getattr(mo, name), RecarrayField) + + # Non-callable attribute, private method, special method. + disallowed_output_names = ['inputs', '_init', '__add__'] + + for name in disallowed_output_names: + with self.assertRaises(InvalidOutputName): + GenericCustomFactor(outputs=[name]) + def test_require_super_call_in_validate(self): class MyFactor(Factor): diff --git a/zipline/errors.py b/zipline/errors.py index 44b88c03..e17e4af5 100644 --- a/zipline/errors.py +++ b/zipline/errors.py @@ -450,6 +450,17 @@ class TermOutputsEmpty(ZiplineError): ) +class InvalidOutputName(ZiplineError): + """ + Raised if a term's output names conflict with any of its attributes. + """ + msg = ( + "{output_name!r} cannot be used as an output name for {termname}. " + "Output names cannot start with an underscore or be contained in the " + "following list: {disallowed_names}." + ) + + class WindowLengthNotSpecified(ZiplineError): """ Raised if a user attempts to construct a term without specifying window diff --git a/zipline/pipeline/data/dataset.py b/zipline/pipeline/data/dataset.py index 0fd19cfe..d77f21b6 100644 --- a/zipline/pipeline/data/dataset.py +++ b/zipline/pipeline/data/dataset.py @@ -14,7 +14,7 @@ from zipline.pipeline.term import ( AssetExists, LoadableTerm, NotSpecified, - Term, + validate_dtype, ) from zipline.utils.input_validation import ensure_dtype from zipline.utils.numpy_utils import NoDefaultMissingValue @@ -56,7 +56,7 @@ class _BoundColumnDescr(object): # (e.g. int64), but still enables us to provide an error message that # points to the name of the failing column. try: - self.dtype, self.missing_value = Term.validate_dtype( + self.dtype, self.missing_value = validate_dtype( termname="Column(name={name!r})".format(name=name), dtype=dtype, missing_value=missing_value, @@ -131,9 +131,9 @@ class BoundColumn(LoadableTerm): return super(BoundColumn, self)._init(*args, **kwargs) @classmethod - def static_identity(cls, dataset, name, *args, **kwargs): + def _static_identity(cls, dataset, name, *args, **kwargs): return ( - super(BoundColumn, cls).static_identity(*args, **kwargs), + super(BoundColumn, cls)._static_identity(*args, **kwargs), dataset, name, ) diff --git a/zipline/pipeline/expression.py b/zipline/pipeline/expression.py index 0aa83d59..d99ab341 100644 --- a/zipline/pipeline/expression.py +++ b/zipline/pipeline/expression.py @@ -194,9 +194,9 @@ class NumericalExpression(ComputableTerm): return super(NumericalExpression, self)._init(*args, **kwargs) @classmethod - def static_identity(cls, expr, *args, **kwargs): + def _static_identity(cls, expr, *args, **kwargs): return ( - super(NumericalExpression, cls).static_identity(*args, **kwargs), + super(NumericalExpression, cls)._static_identity(*args, **kwargs), expr, ) diff --git a/zipline/pipeline/factors/factor.py b/zipline/pipeline/factors/factor.py index fcd18625..115cb057 100644 --- a/zipline/pipeline/factors/factor.py +++ b/zipline/pipeline/factors/factor.py @@ -934,9 +934,9 @@ class GroupedRowTransform(Factor): return super(GroupedRowTransform, self)._init(*args, **kwargs) @classmethod - def static_identity(cls, transform, *args, **kwargs): + def _static_identity(cls, transform, *args, **kwargs): return ( - super(GroupedRowTransform, cls).static_identity(*args, **kwargs), + super(GroupedRowTransform, cls)._static_identity(*args, **kwargs), transform, ) @@ -1020,9 +1020,9 @@ class Rank(SingleInputMixin, Factor): return super(Rank, self)._init(*args, **kwargs) @classmethod - def static_identity(cls, method, ascending, *args, **kwargs): + def _static_identity(cls, method, ascending, *args, **kwargs): return ( - super(Rank, cls).static_identity(*args, **kwargs), + super(Rank, cls)._static_identity(*args, **kwargs), method, ascending, ) @@ -1205,20 +1205,24 @@ class CustomFactor(PositiveWindowLengthMixin, CustomTermMixin, Factor): ''' dtype = float64_dtype - def __getattr__(self, attribute_name): - if self.outputs is NotSpecified: - return getattr(super(CustomFactor, self), attribute_name) - if attribute_name in self.outputs: - return RecarrayField(factor=self, attribute=attribute_name) + def __getattribute__(self, name): + outputs = object.__getattribute__(self, 'outputs') + if outputs is NotSpecified: + return super(CustomFactor, self).__getattribute__(name) + elif name in outputs: + return RecarrayField(factor=self, attribute=name) else: - raise AttributeError( - 'Instance of {factor} has no output named {attr!r}.' - ' Possible choices are: {choices}.'.format( - factor=type(self).__name__, - attr=attribute_name, - choices=self.outputs, + try: + return super(CustomFactor, self).__getattribute__(name) + except AttributeError: + raise AttributeError( + 'Instance of {factor} has no output named {attr!r}. ' + 'Possible choices are: {choices}.'.format( + factor=type(self).__name__, + attr=name, + choices=self.outputs, + ) ) - ) def __iter__(self): if self.outputs is NotSpecified: @@ -1248,9 +1252,9 @@ class RecarrayField(SingleInputMixin, Factor): return super(RecarrayField, self)._init(*args, **kwargs) @classmethod - def static_identity(cls, attribute, *args, **kwargs): + def _static_identity(cls, attribute, *args, **kwargs): return ( - super(RecarrayField, cls).static_identity(*args, **kwargs), + super(RecarrayField, cls)._static_identity(*args, **kwargs), attribute, ) diff --git a/zipline/pipeline/filters/filter.py b/zipline/pipeline/filters/filter.py index d39a9d67..5fcad60f 100644 --- a/zipline/pipeline/filters/filter.py +++ b/zipline/pipeline/filters/filter.py @@ -280,9 +280,9 @@ class PercentileFilter(SingleInputMixin, Filter): return super(PercentileFilter, self)._init(*args, **kwargs) @classmethod - def static_identity(cls, min_percentile, max_percentile, *args, **kwargs): + def _static_identity(cls, min_percentile, max_percentile, *args, **kwargs): return ( - super(PercentileFilter, cls).static_identity(*args, **kwargs), + super(PercentileFilter, cls)._static_identity(*args, **kwargs), min_percentile, max_percentile, ) @@ -411,9 +411,9 @@ class ArrayPredicate(SingleInputMixin, Filter): return super(ArrayPredicate, self)._init(*args, **kwargs) @classmethod - def static_identity(cls, op, opargs, *args, **kwargs): + def _static_identity(cls, op, opargs, *args, **kwargs): return ( - super(ArrayPredicate, cls).static_identity(*args, **kwargs), + super(ArrayPredicate, cls)._static_identity(*args, **kwargs), op, opargs, ) @@ -445,9 +445,9 @@ class SingleAsset(Filter): return super(SingleAsset, self)._init(*args, **kwargs) @classmethod - def static_identity(cls, asset, *args, **kwargs): + def _static_identity(cls, asset, *args, **kwargs): return ( - super(SingleAsset, cls).static_identity(*args, **kwargs), asset, + super(SingleAsset, cls)._static_identity(*args, **kwargs), asset, ) def _compute(self, arrays, dates, assets, mask): diff --git a/zipline/pipeline/term.py b/zipline/pipeline/term.py index c6091e2a..cd29cec0 100644 --- a/zipline/pipeline/term.py +++ b/zipline/pipeline/term.py @@ -2,12 +2,14 @@ Base class for Filters, Factors and Classifiers """ from abc import ABCMeta, abstractproperty +from bisect import insort from weakref import WeakValueDictionary from numpy import array, dtype as dtype_class, ndarray from six import with_metaclass from zipline.errors import ( DTypeNotSpecified, + InvalidOutputName, NonWindowSafeInput, NotDType, TermInputsNotSpecified, @@ -82,14 +84,14 @@ class Term(with_metaclass(ABCMeta, object)): if window_safe is NotSpecified: window_safe = cls.window_safe - dtype, missing_value = cls.validate_dtype( + dtype, missing_value = validate_dtype( cls.__name__, dtype, missing_value, ) params = cls._pop_params(kwargs) - identity = cls.static_identity( + identity = cls._static_identity( domain=domain, dtype=dtype, missing_value=missing_value, @@ -161,72 +163,6 @@ class Term(with_metaclass(ABCMeta, object)): param_values.append(value) return tuple(zip(cls.params, param_values)) - @staticmethod - def validate_dtype(termname, dtype, missing_value): - """ - Validate a `dtype` and `missing_value` passed to Term.__new__. - - Ensures that we know how to represent ``dtype``, and that missing_value - is specified for types without default missing values. - - Returns - ------- - validated_dtype, validated_missing_value : np.dtype, any - The dtype and missing_value to use for the new term. - - Raises - ------ - DTypeNotSpecified - When no dtype was passed to the instance, and the class doesn't - provide a default. - NotDType - When either the class or the instance provides a value not - coercible to a numpy dtype. - NoDefaultMissingValue - When dtype requires an explicit missing_value, but - ``missing_value`` is NotSpecified. - """ - if dtype is NotSpecified: - raise DTypeNotSpecified(termname=termname) - - try: - dtype = dtype_class(dtype) - except TypeError: - raise NotDType(dtype=dtype, termname=termname) - - if not can_represent_dtype(dtype): - raise UnsupportedDType(dtype=dtype, termname=termname) - - 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): """ Noop constructor to play nicely with our caching __new__. Subclasses @@ -244,12 +180,12 @@ class Term(with_metaclass(ABCMeta, object)): pass @classmethod - def static_identity(cls, - domain, - dtype, - missing_value, - window_safe, - params): + def _static_identity(cls, + domain, + dtype, + missing_value, + window_safe, + params): """ Return the identity of the Term that would be constructed from the given arguments. @@ -445,15 +381,15 @@ class ComputableTerm(Term): return super(ComputableTerm, self)._init(*args, **kwargs) @classmethod - def static_identity(cls, - inputs, - outputs, - window_length, - mask, - *args, - **kwargs): + def _static_identity(cls, + inputs, + outputs, + window_length, + mask, + *args, + **kwargs): return ( - super(ComputableTerm, cls).static_identity(*args, **kwargs), + super(ComputableTerm, cls)._static_identity(*args, **kwargs), inputs, outputs, window_length, @@ -466,8 +402,29 @@ class ComputableTerm(Term): if self.inputs is NotSpecified: raise TermInputsNotSpecified(termname=type(self).__name__) - if not self.outputs: + if self.outputs is NotSpecified: + pass + elif not self.outputs: raise TermOutputsEmpty(termname=type(self).__name__) + else: + # Raise an exception if there are any naming conflicts between the + # term's output names and certain attributes. + disallowed_names = [ + attr for attr in dir(ComputableTerm) + if not attr.startswith('_') + ] + + # The name 'compute' is an added special case that is disallowed. + # Use insort to add it to the list in alphabetical order. + insort(disallowed_names, 'compute') + + for output in self.outputs: + if output.startswith('_') or output in disallowed_names: + raise InvalidOutputName( + output_name=output, + termname=type(self).__name__, + disallowed_names=disallowed_names, + ) if self.window_length is NotSpecified: raise WindowLengthNotSpecified(termname=type(self).__name__) @@ -543,6 +500,72 @@ class ComputableTerm(Term): ) +def validate_dtype(termname, dtype, missing_value): + """ + Validate a `dtype` and `missing_value` passed to Term.__new__. + + Ensures that we know how to represent ``dtype``, and that missing_value + is specified for types without default missing values. + + Returns + ------- + validated_dtype, validated_missing_value : np.dtype, any + The dtype and missing_value to use for the new term. + + Raises + ------ + DTypeNotSpecified + When no dtype was passed to the instance, and the class doesn't + provide a default. + NotDType + When either the class or the instance provides a value not + coercible to a numpy dtype. + NoDefaultMissingValue + When dtype requires an explicit missing_value, but + ``missing_value`` is NotSpecified. + """ + if dtype is NotSpecified: + raise DTypeNotSpecified(termname=termname) + + try: + dtype = dtype_class(dtype) + except TypeError: + raise NotDType(dtype=dtype, termname=termname) + + if not can_represent_dtype(dtype): + raise UnsupportedDType(dtype=dtype, termname=termname) + + 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 _assert_valid_categorical_missing_value(value): """ Check that value is a valid categorical missing_value.