Repository navigation
Fix two bugs in get_sun and related bug in GCRS->ICRS transformation - #3750
Conversation
|
I think the travis-ci fail is a fluke? |
There was a problem hiding this comment.
Would it make sense to extract these aberration formulae into a helper function?
Might not be worth exposing in __all__, but could come in handy in other places in astropy.coordinates or other packages that use astropy.coordinates?
Just a thought ... I'm not sure.
There was a problem hiding this comment.
Yeah, that make senses - there's a builtin_frames/utils.py that has helper functions used in some of the other frames.
I'll give it some time to see if there are other comments on this, but it's easy enough to pull this out and put it there.
There was a problem hiding this comment.
Actually, @cdeil, after trying to do this I realized I couldn't really figure out a way to make it any simpler to actually invoke than the already existing usage, because in all other cases you would want to specify the earth->something else axis. Here it's only simpler because the "something else" is the sun itself. So I don't really see a way to generalize it more than what's in the existing ERFA ab function. (except for the invlorentz bit... not sure why ERFA decided that should be stored separately... But that's a one-liner and it doesn't seem to me like it justifies a helper at this point)
|
@taldcroft @adrn or @astrofrog - do you or anyone you can think of want to review this? I feel like another brain separate from mine should try to wrap their brain around this, most because I got it wrong once already and am now afraid I might be doing it again, just in a different way... |
|
@eteq I'm happy to review, but am definitely not an expert. If someone else wants to step up, please do! Otherwise, I'll take a look this afternoon. |
|
@adrn - I think the experts at this don't generally hang out on github, so I think your perspective is valuable even just to make sure it makes internal sense... |
|
@eteq - this is well beyond my coordinates-fu... |
|
@adrn - were you still planning to review this? I see it needs a rebase, but it's a trivial one-liner, so you can review it first so we don't waste testing cycles |
There was a problem hiding this comment.
This is getting super picky, but I liked to put in axis=-1 -- did a double take on this because I thought it was missing the axis specification.
Also, curious as to why you transpose and use the last axis? Won't this be a tiny tiny tiny bit faster if you just do
distance = np.sum((astrom['eb']*u.au + gcrs_coo.cartesian.xyz)**2, axis=0)**0.5
?
There was a problem hiding this comment.
will add the axis part - I completely agree with you but was just being lazy here.
Re: the transpose: it's needed because astrom['eb'] is sometimes a vector of length equal to the non-3 dimension of xyz, so the transpose there was the easiest way to get the broadcasting to work. I think it would also work to reshape the astrom['ab'] to (1,n), but this seems simpler...
|
I'm no expert on the details of the transformation, but I'm happy with the code itself! |
[ci skip]
|
Alright, rebased and I addressed @adrn's comment, as well as adding a changelog entry. Will merge once the tests pass if there are no objections. |
Fix two bugs in get_sun and related bug in GCRS->ICRS transformation
Fix two bugs in get_sun and related bug in GCRS->ICRS transformation
5 participants
This fixes two bugs I found in
astropy.coordinates.funcs.get_sun:obstimecorresponding to the input times. That's not strictly speaking wrong, but it probably is confusing.These effects are quite small for what actually ends up observable (i.e. the AltAz frame), so things like the observing example are unaffected, but this is slightly more correct.
Also included is a fix to a definite bug I uncovered while testing
get_sunfor this fix: the GCRS->ICRS transformation was computing the distance wrong, yielding strange incorrect results like a coordinate near the ICRS origin comes back via the ICRS->GCRS->ICRS transformation as though it were 2 AU away from the barycenter. The actual fix is just the removal of a minus sign.