New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly handle obstime in coordinate transforms and add HGS>HGS #2461

Merged
merged 3 commits into from Feb 20, 2018

Conversation

Projects
None yet
4 participants
@Cadair
Copy link
Member

Cadair commented Feb 16, 2018

This fixes the way we handle obstime in the transform functions to be more compliant with what Astropy and SkyCoord in particular are expecting.

It also adds a HGS>HGS transform for different obstime, which does a HGS - HCRS - ICRS - HCRS - HGS transform to calculate the new HGS coordinates when the point is fixed relative to the barycentre.

Hopefully, this will not break any code that's using SkyCoord. Code using Frames will have to be more explicit about the obstime.

@Cadair Cadair added the coordinates label Feb 16, 2018

@Cadair Cadair added this to the 0.9 milestone Feb 16, 2018

@Cadair Cadair requested a review from ayshih Feb 16, 2018

@pep8speaks

This comment has been minimized.

Copy link

pep8speaks commented Feb 16, 2018

Hello @Cadair! Thanks for updating the PR.

Line 219:1: E303 too many blank lines (3)
Line 234:101: E501 line too long (109 > 100 characters)
Line 237:101: E501 line too long (109 > 100 characters)
Line 308:101: E501 line too long (104 > 100 characters)

Comment last updated on February 17, 2018 at 17:51 Hours UTC

@Cadair Cadair force-pushed the Cadair:coordinates-obstime branch from 005ddb0 to 02a3b99 Feb 16, 2018

Cadair added some commits Feb 16, 2018

Correctly handle obstime in the coordinate transforms.
Following a discussion with @eteq I believe this is the correct behaviour. Users
using SkyCoord should notice no change (I hope)

@Cadair Cadair force-pushed the Cadair:coordinates-obstime branch from 02a3b99 to c844a92 Feb 16, 2018

@ayshih

This comment has been minimized.

Copy link
Contributor

ayshih commented Feb 16, 2018

Ah, I see what you're doing for obstime. Makes sense. Definitely needs a changelog in case of any breakage.

@Cadair

This comment has been minimized.

Copy link
Member

Cadair commented Feb 19, 2018

@ayshih can you give this a look over again, I think it's "ready"

@ayshih

ayshih approved these changes Feb 20, 2018

Copy link
Contributor

ayshih left a comment

Looks good to me. What could go wrong? =P

@Cadair Cadair requested a review from nabobalis Feb 20, 2018

@nabobalis
Copy link
Contributor

nabobalis left a comment

Looks clean.

@nabobalis nabobalis merged commit be4f073 into sunpy:master Feb 20, 2018

@Cadair Cadair deleted the Cadair:coordinates-obstime branch Feb 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment