Skip to content

Make astrometric frames be on [-180, 180] and change their name to SkyOffsetFrame#5023

Merged
eteq merged 2 commits intoastropy:masterfrom
eteq:astrometric-tweaks
Jun 7, 2016
Merged

Make astrometric frames be on [-180, 180] and change their name to SkyOffsetFrame#5023
eteq merged 2 commits intoastropy:masterfrom
eteq:astrometric-tweaks

Conversation

@eteq
Copy link
Copy Markdown
Member

@eteq eteq commented Jun 3, 2016

This closes #5002 by implementing the two changes discussed there: astrometric frames now have longitudes that wrap at 180 (i.e. they are on [-180, 180] degrees), and they have been renamed SkyOffsetFrame, due to the ambiguity of the name.

cc @alexrudy @Kevtron @astrofrog @mcara

I've marked it for 1.2 because it's clearly a significant change to a feature that's new there (Assuming you agree, @astrofrog and @taldcroft?)

Note that this needs to be backported after #4992, as there would otherwise be a ton of conflicts.

@eteq eteq added coordinates Affects-dev PRs and issues that do not impact an existing Astropy release labels Jun 3, 2016
@eteq eteq added this to the v1.2.0 milestone Jun 3, 2016
@taldcroft
Copy link
Copy Markdown
Member

👍 on the concept (both name change and wrapping). Obviously the widespread test fail needs fixing.

@eteq
Copy link
Copy Markdown
Member Author

eteq commented Jun 3, 2016

Oh, fascinating: the test failures are not due to this change itself: they're due to the problem that should be fixed by #5021. That bug turns out to be order-dependent in that if the tests that use spherical_offset run before the SkyOffsetFrame get run, that triggers the bug. I think what happened is that this changed the names of the tests, so they now get run in a different order, at least some of which apparently trigger the bug.

I just checked now, and if I run the full testing suite on this merged with master I get the error, but if both this and #5021 are merged in, the error goes away. So probably the best thing is to merge #5021, then I can rebase this on that to make sure all the tests pass, and then this can be merged.

@taldcroft
Copy link
Copy Markdown
Member

Sometimes "fascinating" is just not what you want in life.

@eteq eteq force-pushed the astrometric-tweaks branch from c95408f to 7ab503f Compare June 6, 2016 22:46
@eteq
Copy link
Copy Markdown
Member Author

eteq commented Jun 6, 2016

rebased on #5021. Hopefully the test will now pass (they did for me locally in various circumstances, anyway).

@astrofrog
Copy link
Copy Markdown
Member

👍

@eteq
Copy link
Copy Markdown
Member Author

eteq commented Jun 7, 2016

OK, this looks good now, so I'm going to merge and backport to 1.2 .

@eteq eteq merged commit c96053d into astropy:master Jun 7, 2016
@eteq eteq deleted the astrometric-tweaks branch June 7, 2016 21:05
eteq added a commit that referenced this pull request Jun 7, 2016
Make astrometric frames be on [-180, 180] and change their name to SkyOffsetFrame
@eteq
Copy link
Copy Markdown
Member Author

eteq commented Jun 7, 2016

backported to v1.2.x in b7b93b4

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 coordinates no-changelog-entry-needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AstrometricFrame should wrap longitudes to -180 to 180

3 participants