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

Fix 4 intertwined coordinates bugs #4039

Merged
merged 9 commits into from Aug 6, 2015

Conversation

Projects
None yet
4 participants
@eteq
Member

eteq commented Aug 5, 2015

This PR addresses a series of quirk/bugs in astropy.coordinates, dealing with a mix of specific inputs that result in inconsistent or incorrect outputs.

Specifically, this closes #3920, closes #3938, closes #3998, and closes #4033. While the fixes are mostly independent, they do interact enough that a single PR seemed more straightforward (e.g., the fix for some of the transforms giving vectors when they should give scalars makes writing the regression test for some of the others easier).

The one questionable piece here is about @astrofrog's item from #3920 . There's a not totally ignorable argument that the change to fix that is backwards-incompatible because the current version (incorrectly) yields an array coordinate when it should be a scalar. But that's been in since v1.0, so users may have written workarounds. It's still definitely a bug, but the question is whether we want to, say, delay that to v1.1. @astrofrog asked for a use case that might lead to it: the simple use case of starting from a scalar SkyCoord with a distance and converting to a locat AltAz will do that, which is a pretty standard use... So it is possible this is used in the wild. On the other hand, no one brought it up, and it is not what the documentation claims, so maybe that's over-worrying?

cc @embray @bmorris3 @adrn

@eteq eteq added this to the v1.0.4 milestone Aug 5, 2015

@embray

This comment has been minimized.

Member

embray commented Aug 5, 2015

I don't think we should worry about backwards compatibility so much when the previous behavior was a bug (especially if no one has reported the issue).

@embray

This comment has been minimized.

Member

embray commented Aug 5, 2015

I changed the milestone for #3920 to v1.0.4 to reflect this too.

@embray

This comment has been minimized.

Member

embray commented Aug 5, 2015

Looks great--thanks for getting this done in time. Is there anyone else you'd like to review or should we merge?

@embray

This comment has been minimized.

Member

embray commented Aug 5, 2015

Also were you really working on this at 2 in the morning, or are you in Hawaii right now? :D

@eteq

This comment has been minimized.

Member

eteq commented Aug 5, 2015

@embray - In theory it would be nice if someone both reviewed it and wrapped their head around it, but that's a pretty tall order because these are all very subtle/complex in actual use. I think it would be good to hear from @astrofrog about what he thinks re: #3920, but I'm pretty sure from his last comment there that he's ok with it, so if you want to merge to get v1.0.4 out the door that's probably OK.

And yes, it really was 2am. Given how complex my debugging setup ended up to track these down, I was afraid I'd never reproduce the fixes if I didn't do them all at the same time ;)

@bmorris3

This comment has been minimized.

Contributor

bmorris3 commented Aug 5, 2015

I 👏 your dedication.

@embray

This comment has been minimized.

Member

embray commented Aug 5, 2015

I spent some time yesterday reviewing what was going on in #3938, and so I've partially wrapped my head around this, but I don't fully understand how all the other issues relate.

@astrofrog

This comment has been minimized.

Member

astrofrog commented Aug 5, 2015

👍

embray added a commit that referenced this pull request Aug 6, 2015

Merge pull request #4039 from eteq/fix-coords
Fix 4 intertwined coordinates bugs

@embray embray merged commit 198343b into astropy:master Aug 6, 2015

3 checks passed

continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.007%) to 77.469%
Details

@eteq eteq deleted the eteq:fix-coords branch Aug 6, 2015

embray added a commit that referenced this pull request Aug 7, 2015

Merge pull request #4039 from eteq/fix-coords
Fix 4 intertwined coordinates bugs

embray added a commit that referenced this pull request Aug 7, 2015

Merge pull request #4039 from eteq/fix-coords
Fix 4 intertwined coordinates bugs

embray added a commit that referenced this pull request Aug 11, 2015

Merge pull request #4039 from eteq/fix-coords
Fix 4 intertwined coordinates bugs

embray added a commit that referenced this pull request Aug 11, 2015

Merge pull request #4039 from eteq/fix-coords
Fix 4 intertwined coordinates bugs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment