Fix gcrs posvel - #6699

Hi there @StuartLittlefair 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labelled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here

Also fixes #6680, subject to the precision required by that user.

Also fixes #6680, subject to the precision required by that user.

can you mention that in the PR description commit, so merging will autoclose that issue, too?

I think astrobot doesn't recognize the PR number because it's off by 2.

ASTROPY-BOT IS ALWAYS RIGHT!

@eteq for comparison, this is what the routine would look like using differentials:

    def get_gcrs_posvel(self, obstime):
        """
        Calculate the GCRS position and velocity of this object at the
        requested ``obstime``.

        Parameters
        ----------
        obstime : `~astropy.time.Time`
            The ``obstime`` to calculate the GCRS position/velocity at.

        Returns
        --------
        obsgeoloc : `~astropy.coordinates.CartesianRepresentation`
            The GCRS position of the object
        obsgeovel : `~astropy.coordinates.CartesianRepresentation`
            The GCRS velocity of the object
        """
        # do this here to prevent a series of complicated circular imports
        from .builtin_frames import CIRS
        from .builtin_frames.intermediate_rotation_transforms import gcrs_to_cirs_mat

        itrs = self.get_itrs(obstime)
        cirs_frame = CIRS(obstime=obstime)
        cirs_cart = itrs.transform_to(cirs_frame).cartesian
        rotation_vector = CartesianRepresentation(0/u.s, 0/u.s, OMEGA_EARTH)
        cirs_vel = rotation_vector.cross(cirs_cart)
        cirs_posvel=cirs_cart.with_differentials({'s': coord.CartesianDifferential(*cirs_vel.xyz)})
        gcrs_posvel = cirs_posvel.transform(matrix_transpose(gcrs_to_cirs_mat(t)))
        return gcrs_posvel.without_differentials(), gcrs_posvel.differentials['s'].to_cartesian()

Personally I find it a bit harder to follow, but it is marginally faster, as it has one fewer transformation...

Documentation failures look unrelated to changes in this PR @bsipocz ?

Yes, those warnings are due to #6695

This looks good to me. I actually like the use of the standard transformation routines, but think it could also be left to a separate issue.

Continuing separate though, might it be more logical to make a new routine get_gcrs that returns the full gcrs position with derivative, and is used here to split out the two parts. Or even to have a more general .transform_to method in analogy with SkyCoord?

(Just as a temporary note: this is on my radar and aim to review it ASAP)

Superceded by #6861