Repository: quantopian/zipline
File: zipline/pipeline/data/dataset.py (Python)

Code before:

    @coerce_types(currency=(str, Currency))
    def fx(self, currency):
        """
        Construct a currency-converted version of this column.

        Parameters
        ----------
        currency : str or zipline.currency.Currency
            Currency into which to convert this column's data.

        Returns
        -------
        column : BoundColumn
            Column producing the same data as ``self``, but currency-converted
            into ``currency``.
        """
        if hasattr(self.dataset, 'extra_coords'):
            name = self.dataset.extra_coords['item']
        else:
            name = self.name

        if self.dtype != float64_dtype:
            raise TypeError(
                'The .fx method cannot be called on {} because {} does not '
                'have a float64 dtype.'.format(self, name)
            )

        conversion = self._currency_conversion
        if conversion is Disallowed:
            raise TypeError(
                'The .fx method cannot be called on {} because {} is not '
                'currency aware.'.format(self, name)
            )
        elif conversion is not None and conversion.currency == currency:
            return self

        return self._replace(
            currency_conversion=CurrencyConversion(
                currency=currency,
                field=DEFAULT_FX_RATE,
            )
        )

    @property
    def currency_conversion(self):
        """Specification for currency conversions applied for this term.
        """
        if self._currency_conversion is Disallowed:
            return None
        return self._currency_conversion

Review comment (suggestion):
I'm not sure we need a separate error message for this case vs. the one below. It's always accurate to say that `.fx()` isn't supported because the column doesn't support currency conversion, and I don't think it helps the user much to distinguish between "this isn't allowed because it's a float that doesn't represent a dollar value", and "this isn't allow because it's not a float".

I think a simpler model might be to only check `if not self.currency_aware`, and to then emit an error saying that the column doesn't support `.fx()`. 

As an extra sanity check, we might want to assert in the constructor of `BoundColumn` that self.dtype is float64 if self.currency_aware is true, since we're never going to be able to currency-convert non-float data.

Code after:
            metadata=self._metadata,
            currency_conversion=self._currency_conversion,
            currency_aware=self._currency_aware,
        )
        kw.update(kwargs)

        return type(self)(**kw)

    def specialize(self, domain):
        """Specialize ``self`` to a concrete domain.
        """
        if domain == self.domain:
            return self

        return self._replace(dataset=self._dataset.specialize(domain))

    def unspecialize(self):
        """
        Unspecialize a column to its generic form.

        This is equivalent to ``column.specialize(GENERIC)``.
        """
        return self.specialize(GENERIC)

    @coerce_types(currency=(str, Currency))
    def fx(self, currency):
        """
        Construct a currency-converted version of this column.

        Parameters
        ----------
        currency : str or zipline.currency.Currency
            Currency into which to convert this column's data.

        Returns
        -------
        column : BoundColumn
            Column producing the same data as ``self``, but currency-converted
            into ``currency``.
        """
        conversion = self._currency_conversion

        if not self._currency_aware:
            raise TypeError(
                'The .fx() method cannot be called on {} because it does not '
                'produce currency-denominated data.'.format(self.qualname)
            )
        elif conversion is not None and conversion.currency == currency:
            return self

        return self._replace(