Repository navigation
Use pmsafe instead of starpm for applying space motion to SkyCoords - #10296
Conversation
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
This is super-nice! pmsafe was introduced in 2013, but maybe missed because the more logical starpm does not have a cross-reference to it. Only some nit-pick comments.
My sense is that this fixes a bug so should go in 4.0.2, but perhaps good to get @eteq's opinion.
| if override_plx: | ||
| new_distance = None | ||
| else: | ||
| new_distance = Distance(parallax=starpm[4] * u.arcsec, copy=False) |
There was a problem hiding this comment.
Tiny detail, but starpm[4] << u.arcsec avoids a copy. (or just do u.Quantity(starpm[4], u.arcsec, copy=False) in analogy to what follows). In either case, might as well drop the copy=False on Distance, since it has to make a copy in order to calculate from a parallax.
| frame='galactic', | ||
| obstime=Time('1988-12-18 05:11:23.5')) | ||
|
|
||
| with catch_warnings(ErfaWarning): |
There was a problem hiding this comment.
I'd use `pytest.warns(ErfaWarning, match='some piece of text that matches the expected warning')
| c1 = SkyCoord(frame, obstime=t1, pressure=101*u.kPa) | ||
| applied1 = c1.apply_space_motion(new_obstime=t2) | ||
| applied2 = c1.apply_space_motion(dt=12*u.year) | ||
| with catch_warnings(ErfaWarning): |
There was a problem hiding this comment.
As above, use pytest.warns (allows you to remove the import too...). And maybe add a comment that one gets a warning because the PM chosen is very high.
|
Thanks @mhvk. Really useful comments, as per usual! |
eteq
left a comment
eteq
left a comment
There was a problem hiding this comment.
Thanks for this @StuartLittlefair ! It looks basically good to me, except for one minor suggested variable-name-change (I tried to catch it in all spots so you can use the "add suggestion to batch" and do it all in github if you're happy with it...). But I'm fine with this with or without, so approving!
|
Note: A rebase will be needed to get rid of that merge commit. FYI. |
Eteq’s variable name change Co-authored-by: Erik Tollerud <[email protected]>
|
@pllim I think the circleci failure is unrelated... |
Use pmsafe instead of starpm for applying space motion to SkyCoords
Use pmsafe instead of starpm for applying space motion to SkyCoords
5 participants
Description
This pull request fixes
SkyCoord.apply_space_motionfor the case where no distance is present on theSkyCoordand the proper motion is relatively high.Fixes #10092