Skip to content

added transforms between HCRS and ICRS frames - #4929

Merged
eteq merged 5 commits into
astropy:masterfrom
StuartLittlefair:hcrs_transforms
May 18, 2016
Merged

eteq merged 5 commits into
astropy:masterfrom
StuartLittlefair:hcrs_transforms

Conversation

@StuartLittlefair

Copy link
Copy Markdown
Contributor

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!

@astrofrog

Copy link
Copy Markdown
Member

@StuartLittlefair - you will need to rebase this to get rid of the merge commits. In general, to avoid this, I never actually use git merge - I always rebase instead.

@StuartLittlefair

Copy link
Copy Markdown
Contributor Author

@astrofrog thanks for the tip.

@eteq eteq added the Bug label May 16, 2016
@eteq eteq added this to the v1.2.0 milestone May 16, 2016
"""
Check HCRS<->ICRS coordinate conversions.

Uses ICRS Solar positions from get_body_barycentric.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@eteq eteq added the Affects-dev PRs and issues that do not impact an existing Astropy release label May 16, 2016
@eteq

eteq commented May 16, 2016

Copy link
Copy Markdown
Member

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 ?

@StuartLittlefair

Copy link
Copy Markdown
Contributor Author

@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!

@mhvk

mhvk commented May 18, 2016

Copy link
Copy Markdown
Contributor

This looks all OK to me. I think I'll defer to @eteq for merging, though.

@eteq

eteq commented May 18, 2016

Copy link
Copy Markdown
Member

@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.

@StuartLittlefair

Copy link
Copy Markdown
Contributor Author

@eteq No, I'm not worried. Results are consistent and I think the transformations chose the right path under the circumstances.

@eteq

eteq commented May 18, 2016

Copy link
Copy Markdown
Member

Great, then I think this is all set. Merging!

@eteq
eteq merged commit 90cac24 into astropy:master May 18, 2016
eteq added a commit that referenced this pull request May 18, 2016
added transforms between HCRS and ICRS frames
@eteq

eteq commented May 18, 2016

Copy link
Copy Markdown
Member

And this is a bugfix, so it can be backported to 1.2. Done in 813a39d

@StuartLittlefair
StuartLittlefair deleted the hcrs_transforms branch May 18, 2016 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Affects-dev PRs and issues that do not impact an existing Astropy release Bug coordinates no-changelog-entry-needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants