Change ordering of GCRS/ITRS transforms to make more sense #4255
Conversation
|
Looks good. I restarted the python 3.4 travis build which for some reason couldn't find jinja2 (though it had been installed). |
|
As for versions, I'd be in favour of putting it in 1.1 still, and could see the argument for backporting to 1.0 together with #4254. |
|
The travis build now keeps failing (twice) on hdf5; see https://travis-ci.org/astropy/astropy/jobs/86514064 It may be that another restart will just resolve it, but thought I'd let people know before doing it. |
|
@mhvk - known issue with the conda package, will be fixed today - ContinuumIO/anaconda-issues#486 |
|
Oops, forgot to push up the changelog entry, which I pushed up now as a second ci-skip commit. @astrofrog, what do you think re: 1.1 or 1.0.x? |
eteq
force-pushed
the
change-gcrs-itrs-cirs
branch
from
3675d1b to
16f1483
Compare
|
Had to rebase due to #4238 (although, ironically, the typo being fixed is actually deleted by this commit...) |
|
@eteq - in my opinion we can backport all the way back to 1.0.x provided that we have adequate numerical tests that will ensure no regression. |
|
But since it's not really critical, we can just include in 1.1 and not 1.0.x |
|
Alright, I will go ahead and merge this now and backport to 1.1 but not 1.0.x . Basically, I suspect the tests are not exhaustive enough to ensure that this does not cause a substantive change for this part of the transformation graph - there are tests that cover this, but they're primarily "end-to-end" tests that pass through these frames rather than specifically checking related cases. Moreover, this is actually a subtansive conceptual change in how this part of the transform graph works. Nominally that's part of the API, but it depends on whether we think the API is set more by what the transform graph looks like or instead by what we think it should look like... I'm hoping we can get some more tests in that might cover some of this (e.g., #4269), so we may end up wanting to backport this for 1.0.7 if those seem to cover it. |
Change ordering of GCRS/ITRS transforms to make more sense
|
Oops, my bad: I just realized this is now in the wrong part of the changelog for going into 1.1 . Will fix directly in master. |
Change ordering of GCRS/ITRS transforms to make more sense
|
backported to v1.1.x in e9b2cca |
4 participants
This is an adjustment to the
coordinatestransformation graph that was inspired by (but not directly related to) #4254 . Basically, while working on that, I realized that the transformations since v0.4 included a rather illogical transform chain, which I think was originally just a workaround because we didn't have it all implemented yet. If you look at Fig 2 of sofa's precession/nutation guide (or the simpler version in Fig 1 of the astrometry guide, which I've put at the bottom of this issue), you'll see how the IAU2000 transform chain is supposed to work.Right now we don't do all of these steps because ERFA lets us jump over some of them in a single step. But if you want to go from GCRS to AltAz, right now there's an extra transform than necessary: GCRS->ITRS->CIRS->AltAz ... worse yet, the CIRS->AltAz transform internally goes through ITRS, so it's completely wasted computation.
This PR removes the GCRS<->ITRS transformation and replaces it with a GCRS<->CIRS transformation. That serves to make the whole thing more linear where relevant, and also reduces the wasted computation going from GCRS<->AltAz
cc @mhvk @adrn @astrofrog
One logistical question - I'm not sure if this is something we should backport to v1.1.x vs. leaving as new in v1.2. It's not clear whether you call this a bug or a feature - it's a performance enhancement but also fixing something that was originally (in v0.4) intended as a temporary measure to get stuff up-and-running quickly. So it's sort of just changing the behavior to what was originally intended. @astrofrog or @embray have any thoughts here?