Repository navigation
Fix gcrs transforms for non geocentric frames - #4894
StuartLittlefair wants to merge 9 commits into
Conversation
|
@StuartLittlefair - can you do a rebase? It should get rid of the merge commits (you don't need to squash, I think merge commits get removed automatically) |
|
|
||
| # now add rotational velocity of GCRS frame w.r.t to ITRS | ||
| # rotation of earth | ||
| V = u.Quantity([0, 0, 7.292115855306589e-5])*u.rad/u.s |
There was a problem hiding this comment.
Can this be defined as a constant, along with a reference for where it comes from?
|
@eteq will need to review this, but in the mean time @StuartLittlefair can you add a regression test, as well as a changelog entry in 1.1.3? |
|
@StuartLittlefair - ah sorry. What I mean is that you should add a test with an example that would have failed before this fix and that now works. |
|
I think you could probably add the test in https://github.com/astropy/astropy/blob/ffc0a89b2c42fd440eb19bcb2f93db90cab3c98b/astropy/coordinates/tests/test_intermediate_transformations.py |
| 3434985.71204565])*u.m) | ||
| icrs_coo = ICRS(ra=184.285*u.deg, dec=-1.8802*u.deg, distance=148101810.2*u.km) | ||
| gcrs_coo = icrs_coo.transform_to(gcrs_frame) | ||
|
|
There was a problem hiding this comment.
@StuartLittlefair - remove the spaces from line 143 (empty lines should not have spaces). This is causing a PEP8 failure on Travis.
|
@StuartLittlefair - an annoying whitespace error... astropy/coordinates/tests/test_intermediate_transformations.py:143:1: W293 blank line contains whitespace [EDITED] sorry, I see this was caught already... |
|
|
||
|
|
||
| """ | ||
| rotational velocity of Earth in radians per (UT1) second |
There was a problem hiding this comment.
This confused me at first, since if it really was per UT1 seconds, it should just be 2 pi / 24 / 3600. But the rotational velocity given is actually per TAI second. Maybe rephrase the docstring
Rotational velocity of the Earth. In UT1 seconds, this would be 2 pi / (24 * 3600),
but we need the value in SI seconds.
|
See some further small comments. I'm convinced enough that this is right to merge it with those in place, but ideally @eteq has a look too. |
| - Transformations between ``GCRS`` frames and other frames now properly handles | ||
| frames with non-zero ```obsgeoloc```. Previously this was in slight error for | ||
| nearby objects, leading to position errors of a few arcseconds for solar system | ||
| planets, to a degree for the Moon. |
There was a problem hiding this comment.
Add [#4894] to the end of this just so interested parties get a link to this issue.
|
@StuartLittlefair - Sorry, I'm confused by this. Why do you say |
|
@eteq poo. the documentation does say this, I had missed that. This PR came about because @bmorris was initialising the This raises a slight issue: The documentation also says |
|
@StuartLittlefair - yep, your solution of putting As for "3-vectors", I'm 95% sure that's a subtle lie: it's actually that the trailing (or maybe leading?) dimension has to be 3, but otherwise it can be whatever dimensionality is required. "3-vector" is just there to indicate they are cartesian x/y/z in the same frame as GCRS, rather than spherical or however else someone might want to represent a 3-coordinate. Feel free to also clear up those docs (ideally once you've figured out if it's trailing or leading... easiest way to find out is try one or the other and see which raises exceptions...) |
|
Oh, and I'm going to kill the tests here just because I'm pretty sure you'll want to close this given the discussion above. But I can re-start them if we end up thinking this is needed after all. |
|
No - I'm pretty sure this is not needed. I'm going to add the changes to EarthLocation in #4890. |
|
OK, great. Once you've decided that for sure, @StuartLittlefair, feel free to just close this. |
5 participants
There was a bug when transforming to and from GCRS frames with non-geocentric coordinates (i.e those with non-zero values of
obsgeolocandobsgeovel. This is because theerfaroutineapcs13expects a position and velocity in GCRS coordinates.Previously
obsgeolocandobsgeovelwere being passed naively, but these are defined in ITRS coordinates. I have added a helper routineget_gcrs_posveltoicrs_cirs_transformsto compute the proper pv array to supply toapcs13.