Repository navigation
Fix 4 intertwined coordinates bugs - #4039
Conversation
…ed to arrays after a transform
…so be UnitSphericalRepresentation
|
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). |
|
I changed the milestone for #3920 to v1.0.4 to reflect this too. |
|
Looks great--thanks for getting this done in time. Is there anyone else you'd like to review or should we merge? |
|
Also were you really working on this at 2 in the morning, or are you in Hawaii right now? :D |
|
@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 ;) |
|
I 👏 your dedication. |
|
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. |
|
👍 |
Fix 4 intertwined coordinates bugs
Fix 4 intertwined coordinates bugs
Fix 4 intertwined coordinates bugs
Fix 4 intertwined coordinates bugs
Fix 4 intertwined coordinates bugs
4 participants
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
SkyCoordwith a distance and converting to a locatAltAzwill 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