Follow-on to 2586 - #2629

@eteq - haven't had a chance to try this or read the code in detail, but my question about the controversial change is whether all the frames know about reasonable units (deg) for Cylindrical and PhysicsSpherical and other (yet-to-be-registered) classes?

@eteq @taldcroft I like the idea of not giving the representations any name or unit information as they are just supposed to represent an arbitrary point in 3D space.

@taldcroft You can have any representation in any units? We have a Cartesian system in angular units. It should be up to the frame to decide if it is sensible to have the supplied units or not. You could define a default as long as it easily override able.

OK, I checked out the code and to mind there is now inconsistency. The frames that @eteq cares about have been nicely defined to always represent in degrees, but the others he doesn't like (cylindrical, physicsspherical, and user-defined representations) are left out the wind.

In [10]: SkyCoord(1, 0.2, 3, unit='hourangle,rad,m', representation='spherical')
Out[10]: <SkyCoord (NoFrame): ra=15.0 deg, dec=11.4591559026 deg, distance=3.0 m>

In [11]: SkyCoord(1, 0.2, 3, unit='hourangle,rad,m', representation='physicsspherical')
Out[11]: <SkyCoord (NoFrame): phi=1.0 hourangle, theta=0.2 rad, r=3.0 m>

In [12]: SkyCoord(1, 0.2, 3, unit='m,hourangle,m', representation='cylindrical')
Out[12]: <SkyCoord (NoFrame): rho=1.0 m, phi=0.2 hourangle, z=3.0 m>

In the original design SkyCoord would provide a consistent view of all representations, which includes coercing the units to degrees. To get this right we are back to a lot of boilerplate in every frame class definition.

In this PR, how can a user defined representation tell all frames what would be nice units for display?

What about meeting half-way and having a recommended_units attribute in representations?

I would call recommended_units a very weak coupling between representations and frames. Basically the representation is just giving a recommendation which users are free to ignore, but in our case frames will heed the advice of representations by default. Practicality beats purity? Our frames should be required to work OK with there is no recommended_units.

@taldcroft Why is the behaviour you demonstrate so undesirable? You are creating a NoFrame which means it could be representing anything, why is having the output units sterilized important?

And if it isn't obvious, dropping the default names in representations is a good idea, that always made a bit uncomfortable too.

Doesn't icrs do the same?

Answer: yes

In [13]: SkyCoord(1, 0.2, 3, frame='icrs', unit='m,hourangle,m', representation='cylindrical')
Out[13]: <SkyCoord (ICRS): rho=1.0 m, phi=0.2 hourangle, z=3.0 m>

test_skycoord.test_units is missing a test for this, the expected behavior is that phi has units of deg. (That is a test that will pass in current master).

Slightly OT, but the comment from @Cadair got me thinking about our default class name of NoFrame. What about changing that to Equatorial, as in a generic equatorial frame. The reason I say that is that our default looks and feels like equatorial because it does have ra and dec as the spherical attribute names. The fact that we are singling out equatorial with RA and Dec as a default doesn't bother me, this is Astropy and equatorial is the most common generic frame in astronomy as a whole.

BTW, then our equatorial systems could, in theory, inherit from Equatorial.

Then we could have a NoFrame which is really agnostic and has default units of (None, None, None) across the board.

I like your idea about Equatorial, @taldcroft - I had thought about making an Equatorial base class when I started putting back in the representation units, but then though better of it because it seemed like it would be confusing to have a frame that's not actually used for anything.

On the recommended_units idea: I guess I'm pretty much neutral. Generally, I dislike the idea that we want to attach specific default units at all: the beauty of Quantity et al. is that you only need to worry about units at the beginning and end, and shouldn't have to worry about anything in-between. So in that mindset, I think any "intermediate" units are bad, because they are needless complication.

All that said, I see that most of the examples here are string representations that are semi-automatic, so that's sort of an output you want control over. So hence the neutral.

FYI, @taldcroft - I think the test you refer to for cylindrical is already implicitly in because there's documentation of that behavior - I had to switch something from deg to rad, anyway.

... the beauty of Quantity et al. is that you only need to worry about units at the beginning and end, and shouldn't have to worry about anything in-between.

I fully agree, but I consider the output of a coordinate for human consumption to be an important "end" point. In defining recommended_units we are doing nothing more than defining a consistent end point in the repr and attributes, while the underlying data Quantities are unaffected. That's part (a) of why I feel fairly strongly about this.

Part (b) is just overall consistency. Through all the API discussions we came to agreement that coordinate RA and Dec values would always be shown to the user in degrees no matter how they are entered. But of course RA and Dec were only specifically called out because those are the most common attributes, but I would say that by extension APE5 implicitly calls for output of all angular quantities in degrees. To say that we will control the units for Spherical and UnitSpherical but not PhysicsSpherical or Cylindrical is giving a special status to the former that isn't consistent with the overall view that all representations are equal.

Alright, since you clearly have a stronger opinion that I, @taldcroft, I'm ok with putting back in default_units as recommended_units. @Cadair, are you ok with that given @taldcroft's discussion here?

This leaves one question that I side-stepped by removing it, though: does RepresentationMapping(..., ..., defaultunit=None) mean "no preference" (i.e., the current behavior) or "fall back on whatever is recommended"? I lean towards the former, as that means any time a RepresentationMapping is declared, it has to specify the unit to get the special behavior. But that might be confusing because the current implementation allows just RepresentationMapping(..., ...), so that defaultunit defaults to None. That means if you don't specify it you get no default unit rather than whatever is recommended. I could easily enough remove that, though, and just require that the unit or None is always given... What do you think, @taldcroft?

I think that it's not too confusing to have defaultunit=Noneforce a "no preference" situation while not supplying a value falls through to the recommended units. We do want to have a mechanism for both so that users can define a frame where the representations have no preferred units.

frame_specific_representation_info = {
    'spherical': [RepresentationMapping('lon', 'ra', u.deg),  # Force degrees
                  RepresentationMapping('lat', 'dec', u.deg)]

frame_specific_representation_info = {
    'spherical': [RepresentationMapping('lon', 'ra'),  # Use recommended units
                  RepresentationMapping('lat', 'dec')]

frame_specific_representation_info = {
    'spherical': [RepresentationMapping('lon', 'ra', None),  # Never coerce units so that
                  RepresentationMapping('lat', 'dec', None)]  # attrs are always in user-supplied units

In this scheme, how should we define the built-in frames? My inclination is to use the second example of using recommended units ('lon', 'ra') instead of forcing degrees. I think this should be the usual pattern for users to follow, which is to say most of the time you should be heeding the recommended units of the representation.

What is the status of this? Can we wrap it up over the next day or so for 0.4?

This is now rebased on master (which was quite a task, it turns out!). It still needs the change discussed in #2543 and (making 'icrs' default and removing NoFrame), as well as switching back to having recommended_units. I'll work on that later tonight, but I wanted this up now so that I can see if travis reports any problems.

I think this now accounts for everything planned, and hence is ready to merge if the tests pass.

@taldcroft and/or @astrofrog may want to review before merging, though.

This looks good to me! @taldcroft - please review and merge if you are happy with it too.

Excellent, I'll review this later today.

@eteq - on the topic of frame_attr_names:

I think the GenericFrame references to _frame_attr_names are fine. The sky_coordinates.py ones are fine since they are distinct from the original BaseCoordinateFrame usage). The test_api_ape5.py usage should probably be updated.

Finally I'm worried about the usage in transformations.py since that seems like an actual problem, but I haven't understood that bit of code so I'll defer to you. I get a little confused by the way GenericFrame is used...

(astropy)neptune$ find astropy/coordinates -name '*.py' | xargs grep frame_attr_names | grep -v get_frame_attr_names
astropy/coordinates/baseframe.py:        # for frame_attr_names, and leading underscore for others
astropy/coordinates/baseframe.py:    * `frame_attr_names`
astropy/coordinates/baseframe.py:    frame_attr_names = {}  # maps attribute to default value
astropy/coordinates/baseframe.py:        super(GenericFrame, self).__setattr__('_frame_attr_names', frame_attrs)
astropy/coordinates/baseframe.py:        return self._frame_attr_names
astropy/coordinates/baseframe.py:        if name in self._frame_attr_names:
astropy/coordinates/builtin_frames.py:    frame_attr_names = {}  # not necessary if empty, but this makes it clearer
astropy/coordinates/sky_coordinate.py:                frame_attr_names = frame.representation_component_names.keys()
astropy/coordinates/sky_coordinate.py:                for arg, frame_attr_name, repr_attr_name, unit in zip(args, frame_attr_names,
astropy/coordinates/sky_coordinate.py:        # Anything in the set of all possible frame_attr_names is handled
astropy/coordinates/sky_coordinate.py:        # Add all possible frame_attr_names
astropy/coordinates/sky_coordinate.py:    frame_attr_names = frame.representation_component_names.keys()
astropy/coordinates/sky_coordinate.py:                frame_attr_names, repr_attr_classes, values, units):
astropy/coordinates/sky_coordinate.py:    frame_attr_names = frame.representation_component_names.keys()
astropy/coordinates/sky_coordinate.py:    for frame_attr_name, repr_attr_class, unit in zip(frame_attr_names, repr_attr_classes, units):
astropy/coordinates/tests/test_api_ape5.py:    # `frame_attr_names` will mainly be used by the high-level class (discussed
astropy/coordinates/transformations.py:        self.overlapping_frame_attr_names = overlap = []
astropy/coordinates/transformations.py:        if (hasattr(fromsys, 'frame_attr_names') and
astropy/coordinates/transformations.py:                hasattr(tosys, 'frame_attr_names')):
astropy/coordinates/transformations.py:            keys of ``tosys.frame_attr_names``. Typically this is of class
astropy/coordinates/transformations.py:                           for attrnm in self.overlapping_frame_attr_names])

@eteq - I've finished my review. Apart from the comment about frame_attr_names in transformations.py, and other minor comments, this looks great! I'm sorry you had to deal with the rebase, I had a feeling it would be a pain...

Alright, I think I've addressed everything except for the question about defining the equinox with a string (which I think is fine, but @taldcroft might want to confirm). So I think frame_attr_names is taken care of everywhere that it seems like it could matter, @taldcroft, and if so I think this is ready if the tests pass.

Looks good now. Doing Time('B1950', scale='utc') (or the the equiv Time('B1950') does not produce a warning for me, but to be honest I don't know why. In any case the code now is fine. There was one random fail and I restarted that job, so assuming it passes this is good for merge.

@eteq et al. - with apologies for taking the honor, I just merged this after it passed tests.

Looks like coordinates 0.4 is done!! 🎇 👏

Hooray 👏 !! (fyi, @astrofrog, this is merged now)