Provide more useful error message for UnitSpherical transformations without a common origin

[barentsen]

#3749 recently added support for ecliptic coordinates (which makes me super happy, yay!! 👍 @eteq 👍)

I started playing with it and noted two glitches.

First, the GeocentricTrueEcliptic class has a lambda parameter, which causes trouble because lambda is a reserved word, e.g. resulting in a syntax error when trying to access the value:

In [1]: from astropy.coordinates import SkyCoord
In [2]: crd = SkyCoord(0, 0, unit="deg")
In [3]: crd.geocentrictrueecliptic
Out[3]: 
<SkyCoord (GeocentricTrueEcliptic: equinox=J2000.000): (lambda, beta, delta) in (deg, deg, )
    (359.99508548, -0.00000571, 1.0)>
In [4]: crd.geocentrictrueecliptic.lambda
  File "<ipython-input-4-64ca382d5d94>", line 1
    crd.geocentrictrueecliptic.lambda
                                    ^
SyntaxError: invalid syntax

It looks like lambda will need to be renamed, but I can't offer a suggestion into what. I was already quite surprised that BarycentricTrueEcliptic uses l whereas GeocentricTrueEcliptic uses lambda, is that an accident or do both frames indeed use different language?

Secondly, I ran into a UnitConversionError when accessing HelioCentricTrueEcliptic.

In [5]: crd.heliocentrictrueecliptic
---------------------------------------------------------------------------
UnitConversionError                       Traceback (most recent call last)
/home/gb/dev/astropy/astropy/units/quantity_helper.py in get_converter(from_unit, to_unit)
     20     try:
---> 21         scale = from_unit._to(to_unit)
     22     except UnitsError:

/home/gb/dev/astropy/astropy/units/core.py in _to(self, other)
    934         raise UnitConversionError(
--> 935             "'{0!r}' is not a scaled version of '{1!r}'".format(self, other))
    936 

UnitConversionError: 'Unit("AU")' is not a scaled version of 'Unit(dimensionless)'

During handling of the above exception, another exception occurred:

UnitConversionError                       Traceback (most recent call last)
/home/gb/dev/astropy/astropy/units/quantity_helper.py in get_converters_and_unit(f, *units)
    278             converters[changeable] = get_converter(units[changeable],
--> 279                                                    units[fixed])
    280         except UnitsError:

/home/gb/dev/astropy/astropy/units/quantity_helper.py in get_converter(from_unit, to_unit)
     23         return from_unit._apply_equivalencies(
---> 24                 from_unit, to_unit, get_current_unit_registry().equivalencies)
     25     if scale == 1.:

/home/gb/dev/astropy/astropy/units/core.py in _apply_equivalencies(self, unit, other, equivalencies)
    859             "{0} and {1} are not convertible".format(
--> 860                 unit_str, other_str))
    861 

UnitConversionError: 'AU' (length) and '' (dimensionless) are not convertible

During handling of the above exception, another exception occurred:

UnitConversionError                       Traceback (most recent call last)
<ipython-input-8-0007f5ba8fbd> in <module>()
----> 1 crd.heliocentrictrueecliptic

/home/gb/dev/astropy/astropy/coordinates/sky_coordinate.py in __getattr__(self, attr)
    430             frame_cls = frame_transform_graph.lookup_name(attr)
    431             if frame_cls is not None and self.frame.is_transformable_to(frame_cls):
--> 432                 return self.transform_to(attr)
    433 
    434         # Fail

/home/gb/dev/astropy/astropy/coordinates/sky_coordinate.py in transform_to(self, frame)
    396         # Do the transformation, returning a coordinate frame of the desired
    397         # final type (not generic).
--> 398         new_coord = trans(self.frame, generic_frame)
    399 
    400         # Finally make the new SkyCoord object from the `new_coord` and

/home/gb/dev/astropy/astropy/coordinates/transformations.py in __call__(self, fromcoord, toframe)
    916 
    917             curr_toframe = t.tosys(**frattrs)
--> 918             curr_coord = t(curr_coord, curr_toframe)
    919 
    920         # this is safe even in the case where self.transforms is empty, because

/home/gb/dev/astropy/astropy/coordinates/transformations.py in __call__(self, fromcoord, toframe)
    704 
    705     def __call__(self, fromcoord, toframe):
--> 706         res = self.func(fromcoord, toframe)
    707         if not isinstance(res, self.tosys):
    708             raise TypeError('the transformation function yielded {0} but '

/home/gb/dev/astropy/astropy/coordinates/builtin_frames/ecliptic_transforms.py in icrs_to_helioecliptic(from_coo, to_frame)
     73 
     74     #first offset to heliocentric
---> 75     heliocart = from_coo.cartesian.xyz + delta_bary_to_helio * u.au
     76 
     77     # now compute the matrix to precess to the right orientation

/home/gb/dev/astropy/astropy/units/quantity.py in __array_prepare__(self, obj, context)
    321         # the unit the output from the ufunc will have.
    322         if function in UFUNC_HELPERS:
--> 323             converters, result_unit = UFUNC_HELPERS[function](function, *units)
    324         else:
    325             raise TypeError("Unknown ufunc {0}.  Please raise issue on "

/home/gb/dev/astropy/astropy/units/quantity_helper.py in helper_twoarg_invariant(f, unit1, unit2)
    288 
    289 def helper_twoarg_invariant(f, unit1, unit2):
--> 290     return get_converters_and_unit(f, unit1, unit2)
    291 
    292 UFUNC_HELPERS[np.add] = helper_twoarg_invariant

/home/gb/dev/astropy/astropy/units/quantity_helper.py in get_converters_and_unit(f, *units)
    282                 "Can only apply '{0}' function to quantities "
    283                 "with compatible dimensions"
--> 284                 .format(f.__name__))
    285 
    286         return converters, units[fixed]

UnitConversionError: Can only apply 'add' function to quantities with compatible dimensions

I verified that these issues affect both 3.4 and legacy Python.

[eteq]

Thanks for the test run, @barentsen! The first one is definitely a bug - it never even occurred to me that lambda is a reserved word in this context, but it's obvious as soon as you said it! I'm thinking the best solution is to change it to lamb. I'll write a PR to do that shortly.

And that's because yes, my understanding is that there is indeed a distinction - if you are using a geocentric ecliptic system, the traditional variables are apparently lambda/beta/delta, while for heliocentric (which by inference I decided also makes sense for barycentric), the l/b/r or d convention seems to be more standard. This is based on a pretty flimsy set of examples, though - a textbook that I promptly lost track of, and the wikipedia page on ecliptic coordinates. So if you or someone else that actually uses these systems knows better, it could certainly be changed!

For the second item, though, I don't think that's a bug, but rather a feature. The reason why is that the crd object you created has no distance. BarycentricTrueEcliptic is just a rotation from ICRS (with now origin shift), so it's perfectly reasonable to transform from ICRS to that frame. But HeliocentricTrueEcliptic has a different origin from ICRS, so asking to switch to that frame when you gave no distance is ambiguous. But the following will work:

>>> crd3d = SkyCoord(0*u.deg, 0*u.deg, distance=100*u.au)
>>> crd3d.heliocentrictrueecliptic
<SkyCoord (HeliocentricTrueEcliptic: equinox=J2000.000): (l, b, r) in (deg, deg, AU)
    (359.9977336, -0.00012388, 100.0071365)>
>>> crd3d.barycentrictrueecliptic
<SkyCoord (BarycentricTrueEcliptic: equinox=J2000.000): (l, b, r) in (deg, deg, AU)
    (359.99613188, -0.00000574, 100.0)>

with the key difference being the presence of the distance attribute.

You may have been fooled by the fact that crd.geocentrictrueecliptic worked (which also has a different origin). That's also a bug, but a bug in who the ICRS->GCRS transformation works - it should be giving the same sort of error, but it's not (and that's related to some other important but more subtle ICRS<->GCRS issues). So once that's fixed hopefully everything will be more consistent. Does that all sound reasonable?

[barentsen]

Thanks for the detailed and enlightening explanation @eteq! I am going to try to read about this to get a better understanding, but all sounds reasonable at first read :-)

Note that it would be nice to have the second item raise an exception that says something like:
CoordinateTransformException: frame has different origin
rather than the current error, which is more obscure, i.e.:
UnitConversionError: Can only apply 'add' function to quantities with compatible dimensions
(I say this in the comfort of not knowing how difficult it would be to implement a better exception.)

By the way, I was wondering if we could offer the user a clear default, i.e. crd.ecliptic, which would return whatever ecliptic frame is the most commonly used? (If such a thing exist?) Or would we be encouraging semantic mistakes that way?

[eteq]

#4211 should fix the lambda problem. As I said above if you have any reason to think the lamb/beta/delta scheme is not actually used I'm happy to hear it, as I've never actually used ecliptic coordinates myself for anything. But absent someone who can speak to the matter I'm sticking with wikipedia ;)

But you do raise a very good point, @barentsen, that a more useful error message would make things a lot clearer. Perhaps We should consider #4211 as closing this, but you could create a separate issue to make error messages better when a frame that's UnitSpherical is transformed to something with a different origin? If you're feeling adventurous you could even try to implement it yourself (I'm happy to point you in the right direction if so).

[barentsen]

@eteq Could you point me in the right direction, in a few words, re. the improved error message? Happy to do things that allows you to work on other stuff :-)

[eteq]

@barentsen - the basic idea is to try to surround the place that gives the UnitConversionError in a try/except block, and then in the except block you re-raise a new error with the more useful message (you'll probably also want to create a new exception class for this and stick it in astropy/coordinates/errors.py).

The trick is where in the call stack - ideally this would be somewhere that makes it so that other frames (not just the ecliptic ones) also give the same message. Perhaps in the BaseCoordinateFrame.transform_to method? I'm not sure exactly where, and you'll have to be a bit careful to make sure you don't accidentally catch the wrong kinds of exceptions and re-throw them as frame related errors. But hopefully this can get you started?

[eteq]

I'm re-assigning this to v1.2 and changed the name to reflect what it has now morphed into.

[bsipocz]

It seems to have been solved in the meantime. The first example works on master and the message for UnitesError looks more sensible:

In [1]: In [1]: from astropy.coordinates import SkyCoord
   ...: In [2]: crd = SkyCoord(0, 0, unit="deg")
   ...: In [3]: crd.geocentrictrueecliptic
   ...: 
Out[1]: 
<SkyCoord (GeocentricTrueEcliptic: equinox=J2000.000): (lon, lat, distance) in (deg, deg, )
    ( 359.99508544,  -5.70722397e-06,  1.)>

In [2]: crd.heliocentrictrueecliptic
---------------------------------------------------------------------------
UnitsError                                Traceback (most recent call last)
<ipython-input-2-0007f5ba8fbd> in <module>()
----> 1 crd.heliocentrictrueecliptic

/Users/bsipocz/munka/devel/astropy/astropy/coordinates/sky_coordinate.py in __getattr__(self, attr)
    481             frame_cls = frame_transform_graph.lookup_name(attr)
    482             if frame_cls is not None and self.frame.is_transformable_to(frame_cls):
--> 483                 return self.transform_to(attr)
    484 
    485         # Fail

/Users/bsipocz/munka/devel/astropy/astropy/coordinates/sky_coordinate.py in transform_to(self, frame)
    437         # Do the transformation, returning a coordinate frame of the desired
    438         # final type (not generic).
--> 439         new_coord = trans(self.frame, generic_frame)
    440 
    441         # Finally make the new SkyCoord object from the `new_coord` and

/Users/bsipocz/munka/devel/astropy/astropy/coordinates/transformations.py in __call__(self, fromcoord, toframe)
    905 
    906             curr_toframe = t.tosys(**frattrs)
--> 907             curr_coord = t(curr_coord, curr_toframe)
    908 
    909         # this is safe even in the case where self.transforms is empty, because

/Users/bsipocz/munka/devel/astropy/astropy/coordinates/transformations.py in __call__(self, fromcoord, toframe)
    705 
    706     def __call__(self, fromcoord, toframe):
--> 707         res = self.func(fromcoord, toframe)
    708         if not isinstance(res, self.tosys):
    709             raise TypeError('the transformation function yielded {0} but '

/Users/bsipocz/munka/devel/astropy/astropy/coordinates/builtin_frames/ecliptic_transforms.py in icrs_to_helioecliptic(from_coo, to_frame)
     68 def icrs_to_helioecliptic(from_coo, to_frame):
     69     if not u.m.is_equivalent(from_coo.cartesian.x.unit):
---> 70         raise UnitsError(_NEED_ORIGIN_HINT.format(from_coo.__class__.__name__))
     71 
     72     # get barycentric sun coordinate

UnitsError: The input ICRS coordinates do not have length units. This probably means you created coordinates with lat/lon but no distance.  Heliocentric<->ICRS transforms cannot function in this case because there is an origin shift.

[pllim]

So, can we close this?

[bsipocz]

Ideally we should wait for @barentsen to confirm that he's happy with the current status.

[barentsen]

I confirm that @barentsen is happy with the improved error message!