Merge pull request #11061 from mhvk/tete-location - #11065

Well, I looked at one of them:

        # test TETE-ITRS transform by comparing GCRS-CIRS-ITRS to GCRS-TETE-ITRS
        itrs1 = moon.transform_to(CIRS()).transform_to(ITRS())
        itrs2 = moon.transform_to(TETE()).transform_to(ITRS())
>       assert_allclose(itrs1.separation_3d(itrs2), 0*u.mm, atol=1*u.mm)

Of course, that's not going to work due to the bug in CIRS.

I notice this in the current version. Maybe that might help.

    # this won't be as close since it will round trip through ICRS until
    # we have some way of translating between the GCRS obsgeoloc/obsgeovel
    # attributes and the CIRS location attributes
    assert_allclose(itrs1.separation_3d(itrs2), 0*u.mm, atol=100*u.mm)

Looks like it will:

>       np.testing.assert_allclose(*_unquantify_allclose_arguments(
            actual, desired, rtol, atol), **kwargs)
E       AssertionError: 
E       Not equal to tolerance rtol=1e-07, atol=1e-06
E       
E       Mismatched elements: 1 / 1 (100%)
E       Max absolute difference: 7.67570602e-05
E       Max relative difference: inf
E        x: array(7.675706e-05)
E        y: array(0.)

That's 76.7 mm, so it should pass if the tolerance is increased to 100 mm.

Looks like that accounts for 12 of the failures, at least.

Yes - that tolerance definitely needs increasing. It was more precise before since the transform recognised it's the same observer and time and didn't back out to ICRS.

If/when we fix GCRS so that the transforms can recognise an obsgeoloc/vel and a location as the same place, it can go back down again.

The only other failure seems to be an issue with the what's new entry - in the readthedocs build

OK, thanks both - I'll relax the restriction here since backporting the whole CIRS stuff is likely too much at this stage, and for most applications, this lack of precision matters less than avoiding deprecation.

OK, thanks, this passes locally, so should now be OK (the readthedocs failure is unrelated to this). It is good to know that with this change, we just get consistency with current master for TETE precision.

@bsipocz - this is really the single commit we hoped to get in still, to avoid having to deprecate something introduced on a new class immediately. I'll close the larger PR which would seem well out of scope for 4.2-rc2.

is this to replace #11061? then that one should not be milestoned an 4.2

Yes, indeed! The backport was slightly tricky and given how late in the game it is, I thought I'd do it and make sure it worked. I now moved the milestone of #11061. Thanks!!

This slight lack of precision is interesting. At the distance to the Moon, this is 2 parts in e-10 which is too large for round off error. Yet the angular equivalence of 41 uas seems too small to be an error in one of the transforms. At first I thought this might be due to some limit of precision in the calculation of GAST, but looking at the code there, I see that they are calculating the equation of the origins precisely by (yet again) calculating the BPN and extracting the equation of the origins from it. This, of course, means that deriving the observer's GCRS position and velocity using the current algorithm for TETE is actually no more efficient than just calling location.get_gcrs_posvel(). I know of two simple ways to get around this, but since we are in a bit of a hurry to get this into 4.2, I'll raise a new issue to discuss these.

@mkbrewer - since we go through ICRS, maybe a rounding error relative to those? In any case, for 4.2 this is good enough - let's indeed discuss ways to improve efficiency and precision separately, based on what we have in current master (the chaining of matrices was another possibility).

I would expect a rounding error to be several orders of magnitude less than 2e-10. For my thoughts on efficiency, please see issue #11067.

Regarding precision, although this error is quite curious, it is much less than the ~0.5 mas precision of the coordinates themselves.

Just a note here. The minor precision issue has been resolved for 4.3.

@mhvk - could you please rebase this? I have backported all the other PRs in the milestone as well as some GH actions to get the CI running, and it would be nice to wrap up the release soon.

Sure, rebased! Tests pass locally still, but definitely good to see the whole slew.

@pllim - do you recall seeing this numerical timer issue for 3.9 before? If yes, could you advise which PR to backport for the fix?

@mhvk - is this good to go know? both failures are unrelated.

utils.timer? It was removed in #10281 for 4.3, so no need to waste time on it. Just xfail or skip (maybe skip is safest).

But it now lives in astroquery, so if you see the same thing over there, it is something that needs fixing but not in this repo. Hope this clarifies the matter. Thanks!

Oh, indeed, skip will be good enough here. Thanks.

@mhvk - please let me know when this is ready to go.

@bsipocz - this is ready to go - the discussion her veered of quite a bit, but was really relevant only on how we are proceeding for 4.3. The nice thing now is that we won't have to deprecate anything in TETE! Thanks so much!

Last one standing in the milestone, so let's done it.

Thanks @mhvk!

Thanks, @bsipocz! And sorry to hear that the backporting more generally was such a pain...

@mhvk - It's just the nature of diverging branches and dependencies. None of those backports were challenging on their own, just overall they adds up to a lot of back and forth to fix smallish issues.

OK, makes sense. Thanks again for all the work!