(As a warning to others, @adrn and I have been doing a lot of out-of-band discussion about this so it might be difficult to follow the doc...)

On the questions:

How do we modify the repr() on BaseRepresentation when there are differentials associated?

I'd vote that if there are no differentials, it should look just like the current version. E.g. (for comparison with the others below,

<CartesianRepresentation (x, y, z) in kpc
    ( 0.0663711,  0.75566441, -1.29113804)>

If differentials are present, the most straightforward option would be:

<CartesianRepresentation (x, y, z) in kpc
    ( 0.0663711,  0.75566441, -1.29113804),
 differentials=(<CartesianDifferential (d_x, d_y, d_z) in kpc
    ( 1.82124186,  1.48662901, -1.21030373)>,)>

which is pretty trivial to implement.

I could also see an argument for this, though:

<CartesianRepresentation (x, y, z) in kpc
    ( 0.0663711,  0.75566441, -1.29113804),
 differentils=(CartesianDifferential(...), )>

e.g., don't show the actual data, but expect the user to do .differentials if they want to see the data. That prevents the repr from getting awkwardly long if there's lots of differentials. I think I slightly favor this over the above, but either seem ok.

Where should I document the subtleties of this? We want to document it somewhere, but also we don't really want people to use it explicitly, or at least have a clear warning on top. Should I just add a section to docs/coordinates/representations.rst near the bottom?

Yes, I like just what you suggested here. But be sure to add a .. warning:: this code is experimental and may change in future versions! at the top of the section.

One slightly annoying side effect of this is that any subclass of BaseRepresentation has to remember to pass through the differentials in to_cartesian() and from_cartesian() (see examples in this PR, e.g., SphericalRepresentation. Should we just accept that and document it?

Hmm, there is an alternative: have to_cartesian/from_cartesian be on BaseRepresentation and have the subclasses instead implement _real_to_cartesian/_real_from_cartesian, which does not ever include the differentials in its result. Then BaseRepresentation.to_cartesian would be something like:

def to_cartesian(self):
    repr = self._real_to_cartesian()
    repr._differentials = self._differentials 
    return repr

@eteq First two points: sounds good!

On the to_cartesian/from_cartesian idea: I was thinking something similar. My idea was to have the subclasses implement (something like) _to_cartesian_components(), which returns an OrderedDict with just the x, y, z data, and _from_cartesian_components(), which returns an OrderedDict with keys set to the component names of the given representation. I guess I was thinking about moving the actual creation of the object up to the superclass (since right now, they all do copy=False in the initializer as well.)

@adrn: the commits I just pushed up add the with_differential method. The last of the three actually does something different: it explicitly addresses passing in a string to represent_as. That wasn't being accounted for, but on reflection I think it's best to do this explicitly so as to yield a more helpful error message. But if you disagree feel free to back that out or do something different.

One other thing: I noticed the machinery for adding differentials is on BaseRepresentationOrDifferential, not BaseRepresentation. I think that's not what we want: differentials attached to other differentials creates an ambiguity: if I have an acceleration, should it be rep.differential[0].differential[0], or rep.differential[0]? Also, it's impossible to do the re-representations anyway if you don't have the representation and all the lower-order differentials. So I think differentials should only be attachable to representations, not other differentials. (Note that if you change this, you'll also need to move the BaseRepresentationOrDifferential._validate_differentials method which I added in those last few commits.)

@eteq ah, yea, I agree - I just moved the relevant code to BaseRepresentation locally. Will push some more changes shortly.

@adrn, @eteq - only high-level comments and questions in the first instance. From highest to lowers:

1. Question: Why is this best done inside representations rather than at the frame level? I ask mostly because it makes sense for coordinates to have just RV or PM, but not really as much for representations, which suggests that this doesn't belong in representations.
2. Comment: I do think we do need to get this right in one go: to have a "hack" just ahead of a LTS version means that we're stuck with the hack. Better it seems to bite the bullet and go for a design where we create a new class that holds both a representation and a differential. (I doubt that there are that many implementations of custom representations that we would break at this point.) [Badly thought out idea: could the new ones have the old names, and the old representations get new names? Does this even help?]
3. Comment: from some of the tests, it seemed mix-and-match representations and differentials were allowed (spherical and cylindrical); is this the case and if so was this indeed intended? I would suggest that for every case, it should be possible to use the differential with as a base the representation it is attached to (i.e., without further converting the base). So, a spherical representation could hold spherical, unispherical and radial differential (plus coslat variants), but a cylindrical representation only a cylindrical differential.
4. Comment: I feel operations on representations should not drop the differential but rather just error. (This allows them to start working with differentials later, for cases where that makes sense.)

@mhvk @eteq

1. My recollection is that when I looked into this idea, I decided that it would end up requiring a lot of detailed changes to the frame classes and duplicated code because of some unfortunate choices in the development of the frames. Right now, the representations are stored as ._data on the frame classes (ugh), and most methods work with this attribute alone. We'd have to modify everything to also look for ._differentials (or whatever). @eteq and I briefly discussed the possibility of creating a new container class for representations and differentials that would replace ._data, but we're really down to the wire here and trying to carve out a minimal path towards getting something in before the feature freeze.

2. I just don't think we have the time (or at least I don't) before next weekend. I do agree in principle and think that it would be the cleaner way to go, but I worry about the required fixes and downstream changes we'd need to make elsewhere for any API-breaking changes we make to the representations. Our compromise was to add the minimal functionality we needed and document it as experimental; wouldn't that mean we don't need to provide LTS for this functionality? In any case, I think we need to make a clear decision ASAP about this because we can't proceed with any of the further pull requests without this...

3. Right now, yes, mix-and-match representations and differentials are allowed.

4. That's a fair point!

@mhvk:
On 1. Fair point - I think I originally did test velocity implementations just as you propose, but eventually we realized attaching them to the representations is important for the reasons @adrn gave above, and because it makes certain aspects of velocity support easier to implement (@adrn and I are working on a couple of PRs for that now). More philosophically, after thinking and talking with a few folks about it, I think this actually does make more sense: basically, you can't do really anything with differentials (other than cartesian) without the representation (and any lower-order derivatives if we implement acceleration et al. in the future), so it makes as much sense to attach them to the representation as shared in the frames.

On 2. I disagree that we're doing a "quick hack" for LTS. We've been thinking about this for over a year now and its time to get something in to support velocities. The feature freeze is sort of acting as a stick so that the perfect isn't the enemy of the good. I also think it's not unreasonable to have this release include a warning in the docs that says "this might change in future versions" which is fairly standard for big changes like this.

As for whether there should be a container class with both representations and differentials, that was @adrn's initial idea, but I think I convinced him otherwise on the basis that this would be a major a API-breaking change, and @adrn realized it would be a lot of work with a variety of subtle consequences to change ._data to be a container class (as @adrn said above about 1). And there are people I know of using this representation machinery (most notably Sunpy), which means there are probably more who we don't know about (and won't know about until the start using it and it's all borken). So I think that ship has already sailed.

On 3. Good point... We ended up with the mix-and-match option because as long as its attached to the representation, the differential can always be re-cast into whatever representation is desired, so one can always get whatever one wants. But sometimes you don't want to. E.g. if you have a cartesian representation and differential and just want to get the .lat, you shouldn't be forced to transform the differential as it's a performance waste. On some level this is an argument for your point 1, but given the restrictions we described above, it seems like this is the lesser of the various evils.

(I'm ambivalent on 4)

OK, pushed my changes, including a second commit with also __repr__ (and a bug fix of the first commit, so be careful if one wants to revert that).

I would suggest that if we want to have a method that transforms differentials to cartesian as well, we discuss that in another PR, so that we can merge this one.

Thanks @mhvk for the careful reading and for submitting changes! From your perspective, is this ready to go if tests all pass? I'd love to have the satisfaction of hitting merge on this one 😄 , but will wait until @eteq chimes in to see if he has any remaining comments.

Yes, I'm 👍 on merging. This has become a really nice integration of the differentials!!

Next up is #6218, correct? That one I think was very close as it was; hopefully the rebase will not be too painful.

Yes, next is #6218 - talk to you over there!

(Remote data build failures)

One more suggestion after some out-of-band questioning with @adrn : there should be a test to ensure that the following is not accepted as a differential {u.s: some_diff, u.yr: some_other_diff}. That shouldn't work b/c it's the same physical type so its ambiguous what's going on. @adrn thinks that's already the case, but is going to add a test.

The new test I added passes locally, but since I added quite a few lines of documentation I am trying out the new [docs only] commit message flag to run the documentation builds.

Hm, it seems that the [docs only] flag didn't work, so I manually killed all tests except for the docs build on travis (and circleci since it's usually fast).
cc @bsipocz

Hmm, what do you mean not working? I think it was: https://travis-ci.org/astropy/astropy/jobs/244281543#L381

(Sadly we can't avoid to start up the jobs, but can just cancel them before any conda install and the testing happens. So by no means cancelling them manually is more efficient but less convenient).

Oh, ignore my comment! I saw the jobs spinning up and jumped to conclusions. I take it back!

I'm satisfied, so when the doc tests pass feel free to pull the trigger @adrn . You've definitely earned that! 💯

Doh - the docs build failure is because of 8 warnings about mistaken single backticks I had in the docstrings I added. I fixed and ran build_docs locally to verify those warnings went away.

Thanks all for the review -- moving on to #6218

🚀 🤘 👏

🔁