Dynamic coordinate preferred representation - #2586

I like this approach. It's simple from a user perspective, and maintains the seperation of representation and frame, while not converting under the hood.

Spherical just got a lot less special...

>>> from astropy.coordinates import *
>>> import astropy.units as u

>>> SkyCoord(1, 2, 3, unit='deg,deg,m')  # Distance works as 3rd arg now
<SkyCoord (NoFrame): ra=1.0 deg, dec=2.0 deg, distance=3.0 m>

>>> SkyCoord([1,2], [2,3], [3,4], unit='deg,deg,m')
<SkyCoord (NoFrame): (ra, dec, distance) in (deg, deg, m)
    [(1.0, 2.0, 3.0), (2.0, 3.0, 4.0)]>

>>> c = ICRS(1*u.m, 2*u.m, 3*u.m, representation=CartesianRepresentation)
>>> SkyCoord(c)
<SkyCoord (ICRS): ra=63.4349488229 deg, dec=53.3007747995 deg, distance=3.74165738677 m>

>>> sc = SkyCoord([1,2], [2,3], [3,4], unit='m', representation=CartesianRepresentation)
>>> sc
<SkyCoord (NoFrame): (x, y, z) in m
    [(1.0, 2.0, 3.0), (2.0, 3.0, 4.0)]>
>>> sc.frame.preferred_representation = SphericalRepresentation
>>> sc
<SkyCoord (NoFrame): (ra, dec, distance) in (deg, deg, m)
    [(63.43494882292201, 53.30077479951012, 3.7416573867739413),
     (56.309932474020215, 47.96888622580272, 5.385164807134504)]>

>>> SkyCoord(x=[1,2], y=[2,3], z=[3,4], unit='m', representation=CartesianRepresentation)
<SkyCoord (NoFrame): (x, y, z) in m
    [(1.0, 2.0, 3.0), (2.0, 3.0, 4.0)]>

Some of test failures are (I think) not related to this PR exactly, but something has gotten whacked in configuration. Locally the full test suite passes except a couple of coordinate doctest fails.

If anyone actually tries to look at the code changes, I'd recommend going one commit at a time since there are a lot of somewhat intricate changes. Otherwise the key will be testing, which doesn't exist yet...

@vaticancameos @Cadair - As you have volunteered to help out, I will happily accept PR's on my branch for tests and documentation to support these changes (even if the tests you write are currently failing but should pass). I'll be basically offline all day Sunday, climbing.

What about below for API, i.e. replace preferred_representation (which is very long and not intuitive from the user perspective) with just representation:

sc = SkyCoord(..)  # default spherical
sc.representation = CartesianRepresentation  # Update view of object, no data changed
# And same for BaseCoordinateFrame objects

One might not like that this is making coordinates quasi-mutable and suggest:

sc_cart = sc.to_representation(Carte...)
# OR
sc_cart = SkyCoord(sc, representation='cartesian')  # this already works BTW.

I think that the first option (quasi-mutability) is good here because no data are changing and it saves making a copy of the internal data. All that changes is the exterior representation of those coordinates, but it makes the process easier and more transparent for users.

I think the quasi-mutable option is the one to go with too. You've effectively made it so that data does not change, but what is shown to the user does. The first option comes more naturally (setting a property) to a user than the second (calling a function).
But I see where the mutable concern is coming from - requiring that the user calls the relevant API method. Wouldn't the user expect this kind of quasi mutability on most other objects too? If we go forward with this that is. Ping @Cadair .

You haven't really left much for us to pitch in unless I'm mistaken :D. Is there any part in your mind that remains which you would like me to do?

@vaticancameos @taldcroft mentioned tests and docs earlier.

@vaticancameos Interesting that you think the property option is more natural than the method call. I wouldn't totally agree with that. I think I prefer the property as well.

There was a length gitter discussion today that convinced me this is worth doing. But one high-level comment to leave for @taldcroft here: the code this PR has in builtin_frames for defining the representations is now much more difficult to read than before (IMHO). This matters a lot because one of the primary aims of APE5 is to make it easy/clear how to make new frames such that users can do it without too much trouble.

So I'm fine with the nested-dict approach here, but I don't think we want all this to be done with private attributes - so dropping the leading underscore would help. I'll try to think a bit more about how to make defining this clearer and leave another comment later.

Also note that the sgr-example has to be updated for this along with the other docs (both the .rst and the ..py file)

@eteq - Agreed about the messy code in builtin_frames. I was already thinking a bit about ways to clean that up and reduce the repetitive boilerplate code. For now I'll focus elsewhere and if you have ideas please toss them out.

@eteq - pushed some more commits that change to using representation. Despite my promise to leave this be, I implemented one way of making the builtin_frames code a bit cleaner. This is pretty much confined to 7596394 so we can back that out if you have a different idea.

I noticed infrastructure there to handle a frame with no special representation. I'm pretty sure some of the new code will break in that case, but there are currently no tests that bring this out. I suspect that a frame with no representation will break other parts as well. What was the intended use case? Some code could be simplified if we make the requirement that any frame object stores the data internally as a proper Representation class object.

BTW, coordinate tests passed locally (maybe not doc testing, didn't run that).

Known issue: SkyCoord needs a representation setter.

If I understand then the sc.spherical representation coming out in rad is expected.

In [2]: sc = SkyCoord(x=1, y=2, z=3, frame='icrs', unit='m', representation=CartesianRepresentation)

In [3]: sc
Out[3]: <SkyCoord (ICRS): x=1.0 m, y=2.0 m, z=3.0 m>

In [4]: sc.spherical
Out[4]: <SphericalRepresentation lon=1.10714871779 rad, lat=0.930274014115 rad, distance=3.74165738677 m>

Without this PR then sc.spherical would not know anything about preferred units for this frame in a spherical representation (assuming that the frame preferred representation was Cartesian). I think that we can now have sc.spherical displayed in the preferred units.

SkyCoord representation property added.

@taldcroft - something seems to now be conflicting in master... can you rebase?

@eteq - I added a possibly-controversial commit 8acef8e just now. Please read the full commit message for explanation.

About rebasing, I was planning to hold off until getting more comments. This avoids needless Travis churning for now. I think that this is now feature complete as far as changes to the low-level modules and you can review. There might be some minor tweaks still in SkyCoord.

The only test fail now (on python 2.7) is due to removing preferred_attr_units as a class attribute:

        #but that *shouldn't* hold if we turn off preferred_attr_units
        class FakeICRS(ICRS):
            preferred_attr_units = {}

        fi = FakeICRS(i4.data)
        ri2 = ''.join(rexrepr.split(repr(i2)))
        rfi = ''.join(rexrepr.split(repr(fi)))
        rfi = re.sub('FakeICRS', 'ICRS', rfi)  # Force frame name to match
>       assert ri2 != rfi
E       assert '<ICRS Coordinate: ra=1. deg, dec=2. deg>' != '<ICRS Coordinate: ra=1. deg, dec=2. deg>'

Do we just remove this test, or ??

By the way, I have not touched the variable names with pref in them that refer to the legacy preferred_representation. For future clarity it might be worth changing, but there are already a ton of diffs so I tried to change as little as possible.

Just noticed a typo in the 8acef8e commit message. representation_attrs[<ReprClass>]['frame_units'] should have been representation_attrs[<ReprClass>]['attr_units']

@eteq - rebased and passing all tests locally.

@taldcroft what else needs doing on this?

@Cadair - we can still use tests as outlined in the discussion for taldcroft#8. I think that in the end I had better write the docs, which I'll do today or tomorrow. That PR on my branch needs rebasing as there has been quite a bit of churn since then.

I'd like to propose changing preferred_attr_names to representation_names and preferred_attr_units to representation_units, and representation_attrs to just representations. And finally make the representations dictionary keys be names and units instead of attr_names and attr_unts. Opinions?

I did already lift a commit from @vaticancameos in a rush to get the doc tests passing just now.

I have argued myself back to leaving representation_attrs.

OK, all the representations we built in can now be trivially used in every frame! Yay.

13cdcae and 32e9468 have further detail in the commit messages, please read. Tomorrow will add code to allow representation='cylindrical', but this is just a couple lines of code at this point.

>>> from astropy.coordinates import *
>>> import astropy.units as u
>>> sc = SkyCoord(rho=1, phi=1*u.deg, z=1, frame='icrs', representation=CylindricalRepresentation)
>>> sc
<SkyCoord (ICRS): rho=1.0 , phi=1.0 deg, z=1.0 >
>>> sc.representation = SphericalRepresentation
>>> sc
<SkyCoord (ICRS): ra=1.0 deg, dec=45.0 deg, distance=1.41421356237 >
>>> sc.representation = PhysicsSphericalRepresentation
>>> sc
<SkyCoord (ICRS): phi=1.0 deg, theta=45.0 deg, r=1.41421356237 >
>>> sc.r
<Quantity 1.4142135623730951>

>>> ICRS(rho=1*u.m, phi=1*u.deg, z=1*u.m, representation=CylindricalRepresentation)
<ICRS Coordinate: rho=1.0 m, phi=1.0 deg, z=1.0 m>
>>> ICRS(rho=1, phi=1*u.deg, z=1, representation=CylindricalRepresentation)
WARNING: AstropyDeprecationWarning: Initializing frames using non-Quantity arguments is now deprecated. Use SkyCoord or pass Quantity instances instead. [astropy.coordinates.baseframe]
<SkyCoord (ICRS): rho=1.0 , phi=1.0 deg, z=1.0 >
# I don't yet understand why that deprecation message is showing up here.

>>> pprint( sc.representation_attrs)
{<class 'astropy.coordinates.representation.CartesianRepresentation'>: {u'names': (u'x',
                                                                                   u'y',
                                                                                   u'z'),
                                                                        u'units': (None,
                                                                                   None,
                                                                                   None)},
 <class 'astropy.coordinates.representation.SphericalRepresentation'>: {u'names': (u'ra',
                                                                                   u'dec',
                                                                                   u'distance'),
                                                                        u'units': (Unit("deg"),
                                                                                   Unit("deg"),
                                                                                   None)},
 <class 'astropy.coordinates.representation.UnitSphericalRepresentation'>: {u'names': (u'ra',
                                                                                       u'dec'),
                                                                            u'units': (Unit("deg"),
                                                                                       Unit("deg"))},
 <class 'astropy.coordinates.representation.PhysicsSphericalRepresentation'>: {u'names': (u'phi',
                                                                                          u'theta',
                                                                                          u'r'),
                                                                               u'units': (Unit("deg"),
                                                                                          Unit("deg"),
                                                                                          None)},
 <class 'astropy.coordinates.representation.CylindricalRepresentation'>: {u'names': (u'rho',
                                                                                     u'phi',
                                                                                     u'z'),
                                                                          u'units': (None,
                                                                                     Unit("deg"),
                                                                                     None)}}

@taldcroft @vaticancameos and I shall try and get tests done asap.

The change was introduced in taldcroft@ab82474

This did not work since it doesn't know about that commit even when inside astropy_helpers.

git pull origin master
git reset --hard 40fdee319e9a5d3226b6202844bc613c9ecd3f5f

The rebase and amend worked, though required stack overflow and git reset HEAD^1 astropy_helpers.

I can't say I'm particularly thrilled with our new sub-repo, it's been a bit of learning curve and I'm still a bit low on that curve.

I don't quite understand why the submodule changed version. @embray, is it possible for the submodule to change what version it's pointing to without explicitly going in it and doing a git pull? ah_bootstrap.py doesn't update the git version, right?

@astrofrog @taldcroft - What commit was the submodule change introduced? I've noticed this a couple times I think when people do something like git add -A before they do python setup.py build - the latter updates the submodule, but if you add before that, it will use whatever your current local version is. The way I understand it, anything you do inside the astropy_helpers dir is not reflected in the astropy repo unless you do a git add from the astropy repo. As long as you never add astropy_helpers from there, it should be safe.

I've changed my process recently but I used to never do the python setup.py build step in favor of develop and running tests within ipython.

@eteq - which is why git add -A and git commit -a should be banned...

agreed, @astrofrog ! I never use them, but some other people do...

@taldcroft - I think python setup.py develop should do the same thing about updating the helpers. If not, I'd say it's a bug.

Holy cow, tests finally passed. @eteq - this is really ready for final review.

One thing you'll notice is that there is still an underscore on _frame_specific_representation_info in builtin_frames. I know you wanted to make it public, but I really think this particular class attribute should be private and marked as such. The user should not see this because it is just specifying how to update the defaults which are built in to representation classes. The final representation_info (which is the one place where _frame_specific_representation_info gets used) is all they should care about. Finally, I should say that one of the first things I did with the old preferred_attr_units was to change it in-place on a frame instance. My first reaction was "neat, that worked" and I actually wrote some documentation to that effect. Only later did I realize that I had modified the actual class attribute, and every new frame instance had different units. Bad. If frame_specific_representation_info were public and mutable it would be an inviting target for making mistakes.

This is great! @eteq - is it good to go?

I think so... I want to take one last look later tonight, but will merge it if there are no major items. Can always make any remaining items as separate PRs, and merging this will unblock #2620 and one other that @taldcroft wants to do.

Alright @taldcroft @astrofrog - merged! I saw a few typos in the docs and just fixed them in a new commit before merging.

I've got a couple more significant changes to this, but I'll issue those as separate PRs so as not to gum up the works any further.