Implementation of APE5 coordinates scheme - #2422

Question for @astrofrog @Cadair or @mhvk : in c787d4e I had to xfail a few tests that broke when I rebased against master. It appears that some of the Quantity objects in the representations refuse to reference, and copy despite copy=False being given. Do you know what changed about this? Is this now expected behavior for Latitude, Longitude, or Angle?

+many on this :D

@eteq I know of nothing that should have changed.

I've started to test this, and one thing that doesn't seem to work anymore is:

In [8]: ICRS('13:13:13', '14:14:14', unit=(u.degree, u.degree))
[snip]
UnitsError: No unit specified

Is there a reason why we can't continue to support this? After all, if the units are unambiguous, we can pass strings:

In [10]: ICRS('13h13m13s', '14d14m14s')
Out[10]: <ICRS Coordinate: ra=198.304166667 deg, dec=14.2372222222 deg>

so why not keep the ability to specify the units explicitly? This would help maintain backward-compatibility and is actually quite a handy feature.

@eteq - this will need a rebase following the merging of the APE3 changes. I think the conflicts will be relatively minimal though - I checked beforehand and I could only see a couple of places where a conflict might occur.

The only part not yet implemented that was in the original APIis parsing and output of strings like SDSS J123456.89-012345.6 or similar formats (that can be added later).

For output I have a function with tests here that you or I could adapt ... but I think this changeset is big enough as is, so 👍 to doing this after this is merged in a separate issue.

@eteq Could you please post the docs online and share a link here?
I sometimes prefer looking over API HTML docs instead of browsing the code on GitHub.

Is this planned for Astropy 0.4 or will this only be available in 0.5?

ConvertError: Cannot transform from <class 'astropy.coordinates.builtin_frames.ICRS'> to <class 'astropy.coordinates.builtin_frames.AltAz'>

I always have to go to the docs to remember how to create coordinate objects.
Can you make the docstring more helpful?

In [22]: ICRS?
Type:            FrameMeta
String form:     <class 'astropy.coordinates.builtin_frames.ICRS'>
File:            /Users/deil/code/astropy/astropy/coordinates/builtin_frames.py
Init definition: ICRS(self, *args, **kwargs)
Docstring:
A coordinate or frame in the ICRS system.

If you're looking for "J2000" coordinates, and aren't sure if you want to
use this or `FK5`, you probably want to use ICRS. It's more well-defined as
a catalog coordinate and is an inertial system, and is very close (within
tens of arcseconds) to J2000 equatorial.

Parameters
----------
representation : `BaseRepresentation` or None
    A representation object or None to have no data (or use the other keywords)
ra : `Angle`, optional, must be keyword
    The RA for this object (`dec` must also be given and ``representation``
    must be None).
dec : `Angle`, optional, must be keyword
    The Declination for this object (`ra` must also be given and
    ``representation`` must be None).
distance : `~astropy.units.Quantity`, optional, must be keyword
    The Distance for this object along the line-of-sight.
    (``representation`` must be None).

Or maybe casual users are not supposed to use that constructor any more?
If so, maybe point them to SkyCoord in the ICRS docstring?

Is there a reason why we can't continue to support this?

@astrofrog I've been thinking about that too -- I'm in favor of adding the unit kwarg back to the frame classes.

I'm relatively neutral on what the low-level classes really should support, but I believe that @eteq will say that the APE5 API strongly encourages users to always use the high-level SkyCoord interface. In other words the low-level interface is, by design (and by agreement of the community in accepting APE5) not especially feature-full for end-user work. All the features being asked for here (including better docs) are (or will be) in the high-level interface.

I will say that I was a little surprised at reaching this conclusion despite all the previous discussion, but I've gotten used to the idea, especially being one of the original proponents of the one-class high-level interface.

It's also worth noting that the input interface for SkyCoord is reasonably intricate in order to handle all of the required inputs. There is a good case to be made for keeping the low-level interface simple in not parsing string / list inputs and requiring angle inputs with units.

The only part not yet implemented that was in the original APIis parsing and output of strings like SDSS J123456.89-012345.6 or similar formats (that can be added later).

I'm not convinced that allowing somewhat free form input (i.e. complex catalog identifiers) as an initializer to SkyCoord is a good idea (should have argued this before, I know). The from_name method now exists and is a much better place because it is unambiguously designed to handle catalog and typical naming conventions. As an input to SkyCoord you immediately have to start doing some complex heuristics to distinguish from legal coordinate values.

I agree that users should make use of the high-level interface in most cases, but I personally still find the following behavior inconsistent:

In [8]: ICRS('13:13:13', '14:14:14', unit=(u.degree, u.degree))
[snip]
UnitsError: No unit specified
In [10]: ICRS('13h13m13s', '14d14m14s')
Out[10]: <ICRS Coordinate: ra=198.304166667 deg, dec=14.2372222222 deg>

I see several options:

1. Disallow anything that is not a quantity
2. Disallow anything that is not a quantity or a string with unambiguous units in it. This is the current behavior, but in that case the error message is wrong in the first case above (it shouldn't ask for units). Also, I feel that passing unit should raise an error since it's not technically a supported keyword argument.
3. Re-instate the unit argument temporarily and mark it as deprecated in order to provide a smoother transition.
4. Re-instate the unit argument and preserve the behavior from 0.3 in terms of the initialization of frames.

I'll have a think about it!

(I guess my main thought about the unit argument is that if it's the only thing needed to provide backward-compatibility for most users, then I'm not sure it's worth breaking backward-compatibility.)

@astrofrog @adrn - What @taldcroft said is right: it's by design that the low-level classes are supposed to support a small subset of initialization techniques, and SkyCoord is supposed to do more complex parsing. I think I also agree with @taldcroft (now that he says it), that SDSS-like parsing better belongs in a class/static method something like from_name.

On the more specific question of unit: the reason I'm resistent to adding that because it leads to two different ways of initializing in the most common cases: ICRS(1*u.deg, 2*u.deg) or ICRS(1, 2, units=(u.deg, u.deg)). That was awkward to account for in the original design, and I was hoping to avoid it here in the low-level classes so that user-created frames don't have as much work to do. To do what @astrofrog wants with units, the "better" way is:

ICRS(Longitude('13:13:13',u.degree), Latitude('14:14:14', u.degree)) #could also be Angle

That's all it would do internally, anyway.

So I'm +1 for @astrofrog option 2, and -1 to the others. Note that passing in unit actually does raise an error (try ICRS('2d','3d', unit=(u.deg,u.deg))), but it's never reached because the coordinates are recognized and fail before the kwargs are all checked. But that could easily be rectified.

I'd like to add an option 5, too, though: @astrofrog option 2, but with "compatibility" layers for users of pre-0.4 coordinates, which would work like this:

>>> from astropy.coordinates.compat import ICRS
>>> ICRS('13:13:13', '14:14:14', unit=(u.degree, u.degree))
Warning: The pre-v0.4 style of initializing coordinates is deprecated and will 
go away in the next version. Use astropy.coordinates.SkyCoord directly, instead.
<SkyCoord (ICRS): ra=13.2202777778 deg, dec=14.2372222222 deg>

That would be trivial to code up, too, as long as we don't consider that a violation of the feature freeze rule (I'm willing to call it a "bug fix" if the rest of you are...)

Now, for @cdeil's comments/questions:

Is this planned for Astropy 0.4 or will this only be available in 0.5?

Unfortunately, it's a lot more work to do the AltAz stuff right, but it will now be a lot easier with these changes. So not in 0.4, but it will be in 1.0 (that's my top priority after we get 0.4 out the door).

Can you make the docstring more helpful?

Just so I understand: what's unclear about that? Is it that you want examples in the docstrings? The typical instatiation would just be: ICRS(ra=1*u.deg, dec=2*u.deg). But I do see the point that examples would help.

That said, just as you surmised, the recommended "user" route is now SkyCoord, though.

Could you please post the docs online and share a link here?

See http://eteq.github.io/astropy/coordinates/index.html#reference-api - that should now have the docs from this branch as of when I left this comment. Note that the narrative docs still need a fair amount of updating, but the API should be right.

(And I'll rebase this once we come to a conclusion on the unit question)

@eteq - I agree a compatibility layer is a good idea, but currently I think your suggestion is the wrong way around:

>>> from astropy.coordinates.compat import ICRS
>>> ICRS('13:13:13', '14:14:14', unit=(u.degree, u.degree))
Warning: The pre-v0.4 style of initializing coordinates is deprecated and will 
go away in the next version. Use astropy.coordinates.SkyCoord directly, instead.
<SkyCoord (ICRS): ra=13.2202777778 deg, dec=14.2372222222 deg>

because users aren't going to change to use the compat sub-package, only to find a deprecation warning. Also:

>>> ICRS('13d13m13s', '14h14m14s')

would not emit a deprecation warning. Technically, it's unambiguous, but we still want people to use SkyCoord instead, right?

This leads me to the following suggestion:

For this pull request

We do not make the frame classes available at the top-level of the coordinates package anymore, so in order to import the frames, users have to do:

from astropy.coordinates.builtin_frames import FK5

we then leave (for 2 versions) the old frames as compatibility layers at the top-level of astropy.coordinates so that:

>>> from astropy.coordinates import ICRS
>>> ICRS('13:13:13', '14:14:14', unit=(u.degree, u.degree))
Warning: The pre-v0.4 style of initializing coordinates is deprecated and will 
go away in the next version. Use astropy.coordinates.SkyCoord directly, instead.
<SkyCoord (ICRS): ra=13.2202777778 deg, dec=14.2372222222 deg>
>>> ICRS('13d13m13s', '14m14d14s')  # no unit
Warning: The pre-v0.4 style of initializing coordinates is deprecated and will 
go away in the next version. Use astropy.coordinates.SkyCoord directly, instead.
<SkyCoord (ICRS): ra=13.2202777778 deg, dec=14.2372222222 deg>

i.e. any usage of the old coordinate classes to initialize values raises a deprecation warning.

This is just a slightly modified version of @eteq's suggestion number 5 - except that the compatibility layer stays where the classes are currently defined, and the 'new' classes are moved to a frames sub-package.

A bonus is that this separates the compatibility classes from the frame classes - that is, the frame classes in frames are clean and don't have any backward-compatibility stuff.

Minor - I would actually suggest renaming builtin_frames to frames so that in the suggestion above, importing frames is a little easier:

from astropy.coordinates.frames import FK5

Post-0.4

Post-0.4 I would like to make the suggestion that we do not allow data to be attached to frames, because there really should only be one way to initialize coordinates, and it would keep a better distinction between SkyCoord and ICRS or other frame classes. A SkyCoord is then a coordinate classes that uses frame and representation classes to represent the coordinates. However, since that is a departure from the current agreed API, I am suggesting this should be ignored for now.

@eteq No, I don't think adding examples to the ICRS docstring is necessary, but pointing users to SkyCoordinate in the text or a see also section would IMO be helpful.

What is unclear at the moment when typing ICRS? in IPython and reading the docstring (pasted in my comment above) is the various ways in which ICRS objects can be constructed, e.g. that one can pass strings that will be parsed ... but based on discussion in this PR it looks like those convenience construction methods might be moved elsewhere, and they are already described for SkyCoordinate, so that's OK.

@cdeil - I think the current confusion is due to the fact that the frame classes are designed both for users and for internal use, and the features required for internal use (passing e.g. representation or coordinates) are not things users should have to do.

One solution for the docstrings is:

• Make an Other Parameters section (allowed by numpydoc) in the docstring for parameters that are intended only for internal use, and keep the Parameters section only for parameters relating to what the user is meant to use.
• Add an Examples section that shows how users should use it.
  now is simply to add an Examples section to the frame docstrings to show that initializing a frame is actually easy in most cases.

So then a docstring would look like:

"""
A coordinate or frame in the FK5 system.

The parmeters described in `Other parameters` are intended for internal use
only.

Parameters
----------
equinox : `~astropy.time.Time`, optional, must be keyword
    The equinox of this frame.

Examples
--------

    >>> FK5()  # FK5 frame with default equinox
    >>> FK5(equinox="J2010")  # FK5 with custom equinox

Other parameters
----------------
representation : `BaseRepresentation` or None
    A representation object or None to have no data (or use the other keywords)
ra : `Angle`, optional, must be keyword
    The RA for this object (``dec`` must also be given and ``representation``
    must be None).
dec : `Angle`, optional, must be keyword
    The Declination for this object (``ra`` must also be given and
    ``representation`` must be None).
distance : `~astropy.units.Quantity`, optional, must be keyword
    The Distance for this object along the line-of-sight.
    (``representation`` must be None).
 """

@astrofrog - If I understand, your suggestion is to mix the old SphericalCoordinateBase classes with the new BaseCoordinateFrame frames (via SkyCoord) infrastructure within astropy.coordinates. My initial reaction on that is that it could lead to a lot of confusion. Maybe that's misplaced, or maybe I'm misunderstanding.

On a question asked much earlier by @eteq, I looked at the SkyCoord docstring and I have ideas for changes that I plan to put in (hopefully sooner rather than later).

@astrofrog - Hmm... Let me clarify the intent of my idea, first: The compatibility layer is intended to be explicit, because otherwise users won't necessarily know anything changed, and perhaps miss more subtle pieces that have changed. For example, precess_to no longer exists as a method, instead you transform to a frame with the appropriate equinox, and some of the transformation meanings are subtly different. So my intent was to make it "opt-in" rather than automatic.

There's one major issue I see with e.g. astropy.coordinates.FK5 being a compatibility layer: right now, you have to use the frames to describe any transformation that requires attributes. E.g., coord.transform_to(FK5(equinox='J1950') requires a reference to the FK5 frame class, and having the base-level FK5 be the compatibility layer and not the low-level/frame class strikes me as confusing. That's the main motivation for having the frames at the top-level.

(And in case it's not apparent, to add to what @taldcroft said, it's a non-starter to actually bring back in old-style SphericalCoordinateBase without a lot of added work, Although that may or may not have been what you meant?)

All that said, I see your point that this might be a source of many complaints. And maybe these subtleties I'm worrying about aren't relevant for the vast majority of users... So I could be convinced we should go your option (we'll call it option 6 😉) And in that case, I'm 👍 on builtin_frames -> frames.

Any other opinions on this? (I.e., astropy.coordinates.ICRS being the new frames, or the old classes?)

Also:
>>> ICRS('13d13m13s', '14h14m14s')
would not emit a deprecation warning. Technically, it's unambiguous, but we still want people to use SkyCoord instead, right?

No, the low-level frames are intended to be used in that way, as long as it's unambiguous. So that's by design not giving any warnings. The guideline in my head for the low-level classes is that any unambiguous individual component is acceptable, but anything beyond that should instead be an error. So it's primarily a "translation layer" between the representations and the frame-specific mappings to representations (e.g., "ra" = "spherical lon"). SkyCoord then makes decisions about anything beyond that.

Post-0.4

I disagree that frame classes should be divorced from the data. SkyCoord will have to be much more complex if it's going to be expected to do all of the data-related operations. It's deceptively simple now because the current frames are quite similar (all equatorial-like). But once we start having systems that are more naturally represented as e.g. cartesian (e.g., ITRS) or cylindrical (some of the SunPy coordinates, if I understood @Cadair right), SkyCoord would quickly devolve into a huge mess if it has to do something separate for each of those. So the idea is that it delegates to the low-level classes for everything that's specific to that frame's data.

I think it's also important for allowing user-created coordinate frames/transforms. We don't want to tell users to subclass SkyCoord for each of their custom frames, after all, but we should allow them to have their own methods that operate on the data (if it makes sense). That's impossible if the data doesn't live in the frame class.

(This discussion is moderately relevant for the PR, in that it partially motivates whether or not the frames should be top-level...)

@cdeil - OK, I agree that ICRS et al. should reference SkyCoord so that people just doing ICRS? will be directed to the right place. Maybe less important if we go with @astrofrog's option 6 though?

@eteq - I see your points, and I'm happy to defer to your judgment. I understand that implementing a full compatibility layer would be a nightmare, so maybe a clean break is needed. One could even simply move the frames out of the top-level so that any old imports break, and users are forced to switch to SkyCoord (but in that case I have a better solution, see below). My main worry with the current situation is that users doing

c = ICRS('13d13m13s', '14h14m14s')

won't see any warnings at initialization time, even though users are never meant to initialize frames with coordinates.

This discussion and the discussion about the docstrings made me realize that the fundamental issue here is that users should now no longer instantiate frames with data (correct?) and should use SkyCoord instead. Initializing with data is something used only internally, correct? If so, then I will leave you with my last option of the day, option 7, after which I will present no more options! This option consists of keeping everything exactly as it is right now, with the exception that initializing frames with data is done via a private class method (or a public one if you do want to keep the option for advanced users to do it). That is, one can only initialize the frames with the frame parameters:

>>> ICRS()
>>> FK5(equinox='J2010')

and initializing frames with data requires using a class method:

>>> ICRS.with_data(lon, lat, equinox='J2010')

with any kwargs passed to the original __init__.

This then also solves the docstring issue because the __init__ docstring is much simpler.

This does go back to having a backward-incompatible API, but it will at least be consistently backward-incompatible, in that even ICRS('13d13m13s', '14h14m14s') will not work.

I'm going to be on gitter if anyone wants to discuss this option further :)

Some of us (me, @astrofrog, and @adrn) discussed this on gitter (for more detail look in the log there for May 7). We have a plan to move forward: @astrofrog's option 1 + an option 8: the low-level classes will be top-level, but they will only accept Quantity-like objects, not strings (which are now the purview of the SkyCoord class)... unless the unit keyword is present. In that case, they act like the compatibility layer described above and yield corresponding SkyCoord objects, as well as showing a warning indicating that unit will disappear in the next version.

The one catch with this is that it may take some effort to re-write SkyCoord to work with this plan. If it looks like that's going to be a big pain to deal with, plan B for 0.4 is to continue to accept strings in the low-level classes, but do the aforementioned behavior if the unit kwarg is given.

Alright, @astrofrog, I've skipped the relevant doctests.

I also just realized an obvious solution to the unicode literals problem: remove the u. There's a __future__ import of unicode_literals, and this is exactly what it's meant for, duh... will push up a commit shortly, and if we're lucky, the tests may actually all pass!

@astrofrog - The tests are now passing! 🎆 😤

As far as I'm concerned, this is all set to merge, now, but someone else should do the honors!

👏

Just push the button!

All right - let's do this! I gave it one final look over and it's good to go! :)

Thanks @eteq for leading the effort, and @Cadair @adrn @taldcroft @mhvk @cdeil for all your contributions to this!

👍 to that!! Nice work everyone!

Woohoo!

Do we have a plan for getting some wider community testing of this from astropy-dev or even the astropy list (pre-release stress testing)? I aim to work on docs for SkyCoord this weekend and have something reasonable that people could look at next week. I think we will benefit from at least a week or two of people outside of the coordinates dev group pounding on it.

@taldcroft SunPy have a GSOC student who is going to be adapting this to our uses, so we will be giving it a hammering from the dev perspective, but that will not entirely be in time for 0.4.

ping @vaticancameos.

@taldcroft - this is a good point: how about we (you and I) try to get the docs hammered out by the start of next week, and then we can make a post on the astropy list. That still leaves a couple weeks for testing.

@eteq - sounds good. To the extent possible it would be good if you first laid out the overall structure and left SkyCoord-related sections for me, either empty or with material that is marked as NEEDS REWORK. I won't start on this until the weekend to give you a head start.

@taldcroft - sounds like a plan!