Repository navigation
Implement TEME frame in astropy. - #10149
Conversation
|
Hello @StuartLittlefair 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style. There are no PEP8 style issues with this pull request - thanks! 🎉 Comment last updated at 2020-05-01 17:13:34 UTC |
pllim
added
whatsnew-needed
needs-copyediting
|
Thank you! cc @c3-14 |
|
If anyone can advise why the documentation is failing in the travis tests I'd appreciate the help. The python looks fine to me; can't understand why we are getting a SyntaxError |
|
I believe the line after it (https://github.com/astropy/astropy/pull/10149/files#diff-58e29e965fccf81ca4bcf30af0bef3f6R38) needs |
|
Before merging this, please squash the last few commits about the rst docs into one. |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Don't know much about TEME, which probably shows, but some comments nevertheless...
|
something is not right with coverage, it thinks that none of the new code is tested, so the CI log needs to be double checked whether the new ones are indeed picked up. |
Isn't the coverage issue the same one as with my previous PRs? That all the tests involving coordinate transforms need to be marked as remote_data if they use By-hand I can't find any new lines of code that are not covered by the provided tests... |
|
But the coverage run uses |
|
@bsipocz no - i don't think so. Accessing |
|
Ah ok, you are right of course. I wonder then whether we should switch it to that job, but it's certainly beyond the scope here. |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Looks good with only a tiny thing about the docs.
eteq
left a comment
eteq
left a comment
There was a problem hiding this comment.
Broadly speaking this looks good to me. I think we want to be a bit more explicit that this is a particular TEME (i.e. a specific equator/equinox definition), but given this is the definition assumed for TLEs, I think it's reasonable to have that in the docstring as opposed to baking it into the name or having a generator function or something.
lglattly
left a comment
lglattly
left a comment
There was a problem hiding this comment.
I made a few copy editing suggestions you can take or leave as you see fit! I would also suggest breaking up the example markup so that the "Transforming TEME to Other Coordinate Systems" heading (satellites.rst line 76) isn't in the middle of the flagged example but rather its own example.
Thanks @lglattly ! |
Co-Authored-By: Erik Tollerud <[email protected]>
Co-Authored-By: Erik Tollerud <[email protected]>
Co-Authored-By: Erik Tollerud <[email protected]>
Co-Authored-By: Adrian Price-Whelan <[email protected]>
Co-Authored-By: Adrian Price-Whelan <[email protected]>
|
thanks @adrn. Should be fine now. Looks like I didn't muck up the commit history during rebase for once! |
StuartLittlefair
added
Ready-for-final-review
and removed
needs-copyediting
bsipocz
left a comment
bsipocz
left a comment
There was a problem hiding this comment.
while fixing the CI failures, please also squash the many single liner commits into one
|
|
||
| >>> from astropy.coordinates import EarthLocation, AltAz | ||
| >>> siding_spring = EarthLocation.of_site('aao') # doctest: +REMOTE_DATA | ||
| >>> aa = teme.transform_to(AltAz(obstime=t, location=siding_spring)) # doctest: +REMOTE_DATA |
There was a problem hiding this comment.
my understanding is that the examples should be standalone. So while this may pass the doctests for now, in the long term it have to be fixed.
There was a problem hiding this comment.
OK - I've been back and forth on this; no-one seems to know what the right way is!
There was a problem hiding this comment.
the example directive is not working yet, but when it does, these examples will be extracted to a separate place, part of a gallery. I would be very surprised if they see outside of their directive container, but the infra is still in the works, so I cannot say this for certain.
There was a problem hiding this comment.
I think it will probably start with them needing to be stand-alone, but might expand to have ways to see wider context? But anyway, I think it doesn't need to block this PR
Co-authored-by: lglattly <[email protected]>
|
Alright this looks all good to me - I'm pretty sure the coverage is sort of a red herring in that the remote data tests are where this is being tested (and they are passing). I'll make an issue to suggest we re-work those tests into something that doesn't require remote data, but I think that's not critical so shouldn't block merging. |
Earth satellites described by the SGP4 model (and distributed as Two-Line Elements) return positions and velocities relative to the True Equator, Mean Equinox (TEME) frame, an Earth-centred inertial frame somewhat like GCRS. This used to be handled by Pyorbital but since version 4.1 Astropy will have builtin support for this frame (see astropy/astropy#10149). Since we don't need the (az, el) of an observer in this calculation anymore, it is now safe to call `EarthSatelliteBody.compute` without a location (it defaults to the centre of the Earth). Update the tests to reflect this. Add tests against Skyfield which has decent satellite support. Consolidate the TLE test parameters as module constants in the process. The new approach is within 0.25 arcseconds of Skyfield, instead of 1.7 arcseconds with the Pyorbital approach. This addresses JIRA ticket SPAZA-120.
Description
One frame missing from the builtin frames is the True Equator Mean Equinox frame, which is often used in combination with satellite data. This PR implements that frame, and provides some simple examples in using it.
Fixes #10141