Small comments are in your own PR, but here some larger ones:

• it might be good for to_cartesian to also transform the differentials, if they are present.
• The translation vectiors should probably be CartesianRepresentation; indeed, I'd argue for a CartesianRepresentation with an associated differential (which means one for the case of no positional offset, but a velocity offset, one has to use pos=0,0,0 -- does this happen much?)
• Instead of _apply_transform, how about expanding expand CartesianRepresentation.transform to take an optional offset (which can carry a differential).

1. Yes, @eteq noticed that and added an option to do this as well - see the new commits I pushed from one of his branches in Allow representations to contain differentials #6169
2. Ah, yes, I like that idea!
3. OK, I'll look in to implementing that.

@mhvk I think I took care of your first two points. On the third, I'm not totally sold on adding the offset functionality to CartesianRepresentation, but am happy to discuss more. Already implementing the other changes I think the code has become a bit cleaner.

@adrn - up late, like I last night... On .transform -- I guess it is obvious that it is useful generally for those who want to use representations as vectors, but thinking again about that case I now see it is of course already quite easy to do: what would be r2 = r1.transform(M, t) is just as short and arguably more comprehensible as r2 = r1.transform(M) + t. OK, so, drop that suggestion and just use the latter in the code!

Love this PR. Nice work @adrn et al!

I've done the rebase already, just fixing a few lingering issues now.

Just so comments don't get lost on a sub-PR, on to_cartesian_with_differentials() - my preference is doing this in baseframe

"""
Am still not too keen to add this to representation, as I don't like the duplication of to_cartesian and find the name rather long (but don't have a better suggestion).

How about using it for .cartesian on a frame, though? For that case, I feel there is no doubt one wants to transform the derivatives as well, and we know that we only have first-order to worry about. I guess that means adapting represent_as. We can always decide to move it down to representation later.
"""

I'd be fine with that - that is, just make frame.cartesian do what is in to_cartesian_with_differentials. @eteq would that satisfy your needs?

OK, now this diff should be much cleaner. Closing the sub-PR on my fork. Please leave any future comments here.

Very nice! I'm toast, though, and have to get up early, so will leave it at this for tonight.

p.s. Just to be clear, my only real request for change is to go with frame.cartesian for the "with differentials" route -- which at least in the context of this PR seems very logical. With that, this PR can be merged as far as I'm concerned.

After looking back at #6226, esentially all of my use cases were someframe.data.to_carteisan(includediff=True). So I think @mhvk is right that someframe.cartesian ends up much cleaner!

@mhvk @eteq In implementing BaseCoordinateFrame.cartesian, I found two bugs that I don't know how to resolve, both related to having a UnitSphericalRepresentation base.

• The first is that I don't know how to handle calling .cartesian on a frame with a UnitSphericalRepresentation & SphericalDifferential (a fairly common situation: sky position, but proper motions and radial velocity for a target). Converting the SphericalDifferential
  to CartesianDifferential fails because of the UnitSphericalRepresentation base. But, we need to use .cartesian in BaseAffineTransform._apply_transform(), even if only a matrix transform gets applied.
• The second bug I found noticed is when a RadialDifferential is attached to a UnitSphericalRepresentation, the auto-determination of the key for the differentials dict fails. If you look at how it works now, it pairs up a repr component name with a diff component name and divides. But, in this case there are no pairs (UnitSphericalRepresentation has lon and lat, RadialDifferential has d_distance).

@adrn - on your points:

1. Coordinates + PM + RV: here, I think the failure is real in case of a proper AffineTransform, since you need the distance to see how PM and RV change into each other. I think at least for now this should raise an error, which the error message stating that the only way to solve this is to put an (estimated) distance in the coordinate (and that for large distances this is not going to change most transformations within the solar system). For MatrixTransform, there should not be a real problem, so perhaps the MatrixtTransform has to explicitly not exclude any radial velocity component (i.e., transform SphericalDifferential to a combination of UnitSphericalDifferential and RadialDiffferential).
2. For RV + UnitSphericalRepresentation: I think in this case expected_key returns None, correct? I think for this case we just skip the checking (for now) and trust the key the user puts in -- hence, your baseframe code has to ensure it puts in the right key...

1. UnitSpherical + SphericalDifferential: This is a critical one. For now maybe just special-case it as we do with a lot of the UnitSpherical stuff? I think it's absolutely necessary we support this, but should think of UnitSpherical as having "infinite" distance, where applicable. This is arguable the most common case (and in most implementations, the easiest because its only rotations), and most users will find it very strange if they can't do that. I'd also say that the general behavior should be that AffineTransform only applies the matrix part and not the (spatial) offset for the UnitsSpherical case.
2. UnitSpherical + RadialDifferential: I'd say special-case this also, as a specialer case of the above, thinking of it as having pm=0 . Thats' what's implicitly meant by this construction, I think. (I think that's also what @mhvk was suggeting?)

Cool and agreed, working on this logic now.

Yes, indeed, taking UnitSpherical as being at infinite distance is also a reasonable choice for transformations - though it is perhaps more special-casing than is immediately obvious. E.g., in a naive implementation, taking .cartesian on a UnitSphericalRepresentation with proper motion you'd get infinite velocities. This is in part why I suggested asking users to put in an estimated distance -- typically this would be "infinite enough" (but has the advantage that you actually get to see whether that assumption is correct).

But perhaps the answer is to have a configuration parameter that would allow unitspherical=inf.distance to happen. I think that, by default, this should be False (I don't like warnings for things like this), and at least my recommendation would be for this to be a ScienceState one, so that it can be used as a context manager (which would warn anybody reading code that something funny may happen).

Obviously, I may be too paranoid, but ideally we don't help people make silly mistakes -- while ignoring distances for observation planning is probably fine, anything to go with, say, galactic motion is not.

Tests passing locally so I canceled the CI runs. Tests from previous commit were all clear, except stalled remote-data tests.

LGTM now, @adrn - thanks!

Thanks for the clarification!

OK, while @mhvk is still "requesting changes", I'm pretty sure that's a hold-over and not intended based on the comment that it looks good pending the one change, which @adrn did indeed make. So I'm going to merge this to make rebasing others easier, but @mhvk, if you had a concern here that I missed, we can still entertain that between now and release!

Thanks @adrn for all the work on this, and @mhvk and @StuartLittlefair for the reviews! It's really good to see this many eyes on this stuff.