Skip to content

Conversation

@mhvk
Copy link
Contributor

@mhvk mhvk commented Mar 31, 2015

This is a regression introduced in 1.0. The problem is here

I'm not a huge fan of duck-typing by simply looking for a to attribute (and hoping that object is sufficiently Quantity-like), but given the existing code the best way out might just be to handle the possibility of a unit attribute that is None.

@mhvk

In [1]: from astropy.table import Column
In [2]: from astropy.time import Time
In [3]: x = Column([1])
In [4]: Time(x, format='jd')
---------------------------------------------------------------------------
UnitsError                                Traceback (most recent call last)
<ipython-input-4-2acb9071d863> in <module>()
----> 1 Time(x, format='jd')

/Users/aldcroft/anaconda/lib/python2.7/site-packages/astropy/time/core.pyc in __init__(self, val, val2, format, scale, precision, in_subfmt, out_subfmt, location, copy)
    196                 self._set_scale(scale)
    197         else:
--> 198             self._init_from_vals(val, val2, format, scale, copy)
    199 
    200         if self.location:

/Users/aldcroft/anaconda/lib/python2.7/site-packages/astropy/time/core.pyc in _init_from_vals(self, val, val2, format, scale, copy)
    238 
    239         # Parse / convert input values into internal jd1, jd2 based on format
--> 240         self._time = self._get_time_fmt(val, val2, format, scale)
    241         self._format = self._time.name
    242 

/Users/aldcroft/anaconda/lib/python2.7/site-packages/astropy/time/core.pyc in _get_time_fmt(self, val, val2, format, scale)
    272             try:
    273                 return FormatClass(val, val2, scale, self.precision,
--> 274                                    self.in_subfmt, self.out_subfmt)
    275             except (ValueError, TypeError):
    276                 pass

/Users/aldcroft/anaconda/lib/python2.7/site-packages/astropy/time/core.pyc in __init__(self, val1, val2, scale, precision, in_subfmt, out_subfmt, from_jd)
   1296             self.jd2 = val2
   1297         else:
-> 1298             val1, val2 = self._check_val_type(val1, val2)
   1299             self.set_jds(val1, val2)
   1300 

/Users/aldcroft/anaconda/lib/python2.7/site-packages/astropy/time/core.pyc in _check_val_type(self, val1, val2)
   1323             # set possibly scaled unit any quantities should be converted to
   1324             _unit = u.CompositeUnit(getattr(self, 'unit', 1.), [u.day], [1])
-> 1325             val1 = val1.to(_unit).value
   1326             if val2 is not None:
   1327                 val2 = val2.to(_unit).value

/Users/aldcroft/anaconda/lib/python2.7/site-packages/astropy/table/column.pyc in to(self, unit, equivalencies, **kwargs)
    686             ``unit``.
    687         """
--> 688         return self.quantity.to(unit, equivalencies)
    689 
    690     def _copy_attrs(self, obj):

/Users/aldcroft/anaconda/lib/python2.7/site-packages/astropy/units/quantity.pyc in to(self, unit, equivalencies)
    603         unit = Unit(unit)
    604         new_val = np.asarray(
--> 605             self.unit.to(unit, self.value, equivalencies=equivalencies))
    606         return self._new_view(new_val, unit)
    607 

/Users/aldcroft/anaconda/lib/python2.7/site-packages/astropy/units/core.pyc in to(self, other, value, equivalencies)
    943             If units are inconsistent
    944         """
--> 945         return self._get_converter(other, equivalencies=equivalencies)(value)
    946 
    947     def in_units(self, other, value=1.0, equivalencies=[]):

/Users/aldcroft/anaconda/lib/python2.7/site-packages/astropy/units/core.pyc in _get_converter(self, other, equivalencies)
    847         except UnitsError:
    848             return self._apply_equivalences(
--> 849                 self, other, self._normalize_equivalencies(equivalencies))
    850         return lambda val: scale * _condition_arg(val)
    851 

/Users/aldcroft/anaconda/lib/python2.7/site-packages/astropy/units/core.pyc in _apply_equivalences(self, unit, other, equivalencies)
    838         raise UnitsError(
    839             "{0} and {1} are not convertible".format(
--> 840                 unit_str, other_str))
    841 
    842     def _get_converter(self, other, equivalencies=[]):

UnitsError: '' (dimensionless) and 'd' (time) are not convertible

@mhvk
Copy link
Contributor

mhvk commented Mar 31, 2015

I guess we're back to the problem that Column only sometimes behaves as a unit. This case may be most easily solved by replacing hasattr(val1, 'to') with getattr(val1, 'unit', None) is not None. PR on its way.

@mhvk
Copy link
Contributor

mhvk commented Mar 31, 2015

@taldcroft - this fixes it locally, and adds regression tests.

@mhvk
Copy link
Contributor

mhvk commented Mar 31, 2015

Hmm, this is not good: when I give [skip ci], coverage still gets run and github thinks all is OK, even though the other things gave errors. Anyway, I'll get an update in once I figured out what went wrong.

@mhvk
Copy link
Contributor

mhvk commented Mar 31, 2015

OK, that was just a "out-of-space" issue on travis. Hopefully, my restart will fix it.

@embray
Copy link
Member

embray commented Mar 31, 2015

Why rely on duck-typing? Why not just an isinstance there?

@mhvk
Copy link
Contributor

mhvk commented Mar 31, 2015

@embray - Changing to isinstance(val1, Quantity) would mean the unit of a Column instance would be ignored. I don't think this matters too much in principle (since it is used inconsistently anyway), but in the end it seems to make more sense to use the information one has.

@astrofrog
Copy link
Member

Couldn't we do isinstance(val1, (Quantity, Column))?

@mhvk
Copy link
Contributor

mhvk commented Mar 31, 2015

@astrofrog - why bother? If something has a unit attribute but is not a Column or Quantity, should it just be treated as an array? I think it is much better that in that case an error is raised unless it also has a .to method that can convert to a new unit. Also, the current implementation is fast and takes care of the actual problem here, that columns can have unit unset.

@mhvk
Copy link
Contributor

mhvk commented Mar 31, 2015

@taldcroft - finally appveyor got to it as well, and everything now seems OK.

@embray
Copy link
Member

embray commented Mar 31, 2015

Would help if there were an ABC that mean "Thing with units". Maybe call it QuantityABC or something.

@mhvk
Copy link
Contributor

mhvk commented Mar 31, 2015

@embray - yes, that would be a good solution; I am becoming more and more enchanted with metaclasses, where someone can, if they wish, register a certain type of object, thus guaranteeing that it behaves like it. I think it should be discussed separately of this PR, however (let's get the bug fixed!): see #3650.

taldcroft added a commit that referenced this pull request Mar 31, 2015
Fix regression when initializing Time from a Column with no unit set
@taldcroft taldcroft merged commit 66d3a10 into astropy:master Mar 31, 2015
@taldcroft
Copy link
Member Author

Thanks @mhvk!

@mhvk mhvk deleted the time-from-column-fix branch March 31, 2015 19:47
taldcroft added a commit that referenced this pull request Apr 13, 2015
Fix regression when initializing Time from a Column with no unit set
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants