Repository navigation
Review of coordinate transformations (esp. #4254) #4268
Description
Activity
The issue here is that the layers of abstraction and other code elements that make
coordinatesa powerful and easy-to-use package also serve to make it difficult to review the mathematical / scientific content. I still consider the actual transformation code to be a deep black box that I am hesitant to look into. I'm sure there is just a learning curve, but that's where we're starting from. Since I also don't personally use any coordinate system besides ICRS (and have almost no domain knowledge of all those transforms), let me walk down a different path --What about focusing effort on documentation and testing vs. independent implementations? So the first thing is being very clear about the intended mathematics and literature reference for each of the frames / transformations. We have the See Also reference section, but it seems in many cases there is no specific citation to a set of equations that are the intended transformation. Having a cleanly documented set of transform equations is a prerequisite in any case to determining that the astropy code is doing the right thing.
On testing, I am imagining that there are independent implementations of most or all of the astropy transforms that could be used to generate reference data. I know this is already done to some extent, so is there an issue of completeness there? Could this strategy have avoided the #4254 issue?
@taldcroft - I totally get what you're saying on the abstraction/complexity issue. I'm quite happy to help someone who might be interested figure out where to look to actually do the review. That is, if someone who's up for this wants, they can treat everything else as a big black surrounding and have them focus only on what's specifically relevant for the transformations. I'm happy to provide guidance on that (although that's probably better done in a more interactive medium like face-to-face or hangouts or whatever).
As for the independent implementations, in fact, part of the problem is that there are not independent implementations of some of this. The IAU2000-based transformation chain is not used by many (any?) other projects, at least as far as I know not with the same level of flexibility as what we're doing here. We do have the various end-to-end tests that do stuff like going from ICRS to AltAz, and naturally that covers many of the most important use cases. That probably could have caught some or all of #4254, at least if we'd had moon ephemerides working at the time the GCRS stuff was originally implemented. But I think there are other still relevant use cases (e.g. satellites, planetary or other in-solar-system dynamics that might use GCRS or similar) that are not tested. So someone with that expertise (which is not really my area) might be a valuable person to both implement some relevant tests and perhaps also review the transformations.
On the docs side, you do raise a good point that it might make this a lot easier if there was a narrative section that actually describes how the included frames fit together. So far I've avoided that because it's a lot of repetition with the See Also resources... But also because it would probably take a lot of time that I haven't been able to find. But maybe you're saying I need to bump it up the queue for this review to happen?
And @astrofrog - good idea re: email to the list, I'll probably do that once we get 1.1 out the door since it's labeled a 1.2 issue...
@eteq - now having scanned through some of the See Also resources I understand better the significant level of effort in documenting the transformations. You're right that duplicating large bits of those documents is probably not the best use of time.
So maybe the best starting place really is @astrofrog's idea of mailing to the larger list and looking for some brave souls to champion their favorite transformations.
I am slowly working through the coordinate transforms that I have alternative implementations of, or understand. I can confirm that the chain ITRS->GCRS->ICRS is correct, in the sense that it looks conceptually correct to me, and that it returns ICRS coordinates of Earth observatories that agrees with a SLALIB based implementation to within 6km, or a light travel time of 20 microsecs. This is equal to the precision of the SLALIB implantation.
@StuartLittlefair - great! If possible (and not doing it already), do also write a few test cases where you explicitly compare with SLALIB.
Thanks @StuartLittlefair!
If you're feeling really ambitious, you might try implementing tests like what @mhvk said here in https://github.com/astropy/coordinates-benchmark . That's intended for larger-scale accuracy comparisons that might otherwise take too long/include to much data to be in the astropy test suite. But if that seems like too much work, smaller-scale comparisons in the
astropytests are good too.Moved the milestone to "future", since really this is an issue that needs someone to take the time, which is hard to enforce for a given deadline.
This is coming out of the discussion in #4254 . Basically, that got merged without a thorough review of the actual transformation methodology (there was code review, but not necessarily a methodology/mathematical correctness review). So, when possible, someone should review that code for the details of how the transformations are implemented. (In particular for #4254, there's a specific question at #4254 (diff) that probably requires someone who's willing to go in and wrap their head around the geometry of the situation.)
More generally, this issue encapsulates my concern that quite a bit of the transformation code in
coordinateshas not been "conceptually" reviewed. That is, all of the code has been reviewed from a coding style/interface perspective, but not necessarily a review of the mathematical/scientific content.I realize this is a pretty big ask because the transform code is quite complex and involves a lot of overlapping frames and concepts... But specifically because of that, I think it's quite important that we make use of the hive mind on these topics.
Hopefully someone is willing to volunteer for this - I shouldn't do so because I am the original author of the majority of this stuff. I'm milestoning this for v1.2 mainly just as a way to give whoever might volunteer a deadline: as we all know, deadlines are the best way to get things done!
cc @adrn @mhvk @astrofrog @taldcroft @bmorris3