Repository navigation
added transforms between HCRS and ICRS frames - #4929
Conversation
|
@StuartLittlefair - you will need to rebase this to get rid of the merge commits. In general, to avoid this, I never actually use |
|
@astrofrog thanks for the tip. |
| """ | ||
| Check HCRS<->ICRS coordinate conversions. | ||
|
|
||
| Uses ICRS Solar positions from get_body_barycentric. |
There was a problem hiding this comment.
Can you clarify what this means? It doesn't seem to use get_body_barycentric anywhere... I assume you mean that's how you got the values in the first place? If so, maybe include just a quick snippet of that.
|
Looks good to me aside from that one minor comment. Weird that coverage went down... But that must be wrong because I can't see any way the full transforms aren't getting tested, right @StuartLittlefair ? |
|
@eteq it looks like the GCRS to HCRS code is not getting called by the tests. This is because the previously written tests converted ITRS to HCRS. This transform is, I suspect, going through ICRS and bypassing GCRS! |
|
This looks all OK to me. I think I'll defer to @eteq for merging, though. |
|
@StuartLittlefair - do you think it's bad that this was being bypassed? That is, is it better to do the pathway you're forcing it to go through here? If so, there's a way to give weights to transforms to prefer one route over the other... I can go into more if you think that's necessary (and don't already know about it). But other than that this looks good to me. |
|
@eteq No, I'm not worried. Results are consistent and I think the transformations chose the right path under the circumstances. |
|
Great, then I think this is all set. Merging! |
added transforms between HCRS and ICRS frames
|
And this is a bugfix, so it can be backported to 1.2. Done in 813a39d |
This PR addresses #4922 by adding transforms between the HCRS and ICRS frames in both directions, and adding tests to check that these frames work correctly for the case of the Sun, where the HCRS coordinates should always be 0!