Repository navigation
Fix conversion of cartesian to unit-spherical coordinates with velocities - #11469
Conversation
| raise u.UnitsError('d_lon and d_lat should have equivalent units.') | ||
|
|
||
| @classmethod | ||
| def from_cartesian(cls, other, base): |
There was a problem hiding this comment.
If this is new API, probably wanna re-milestone to 4.3.
There was a problem hiding this comment.
No, it is actually a bug fix... I had not quite realized that the base class method would not work correctly for the UnitSphericalDifferental. Though if difficult to backport, it can be for 4.3.
There was a problem hiding this comment.
Well, if this bug is in LTS, I think the current milestone is fine and we'll leave it up to the release manager(s) to decide later. Thanks!
adrn
left a comment
adrn
left a comment
There was a problem hiding this comment.
Ah, yes, thanks for this fix! Looks good to me, except for an ignorable nitpick!
| base : `BaseRepresentation` | ||
| The points for which the differentials are to be converted: each of | ||
| the components is multiplied by its unit vectors and scale factors. | ||
| Will be converted to ``self.base_representation`` if needed. |
There was a problem hiding this comment.
Nitpick, but should this be
| Will be converted to ``self.base_representation`` if needed. | |
| Will be converted to ``cls.base_representation`` if needed. |
?
There was a problem hiding this comment.
OK, thanks! I made the change as part of rebasing - might as well check that more tests pass now...
Previously, the distance was ignored, which meant the units of the
proper motions would be wrong:
c = CartesianRepresentation([1, 0, 0]*u.kpc)
cd = CartesianDifferential([-10, 0, 4.74]*u.km/u.s)
cd.represent_as(UnitSphericalDifferential, base=c)
<UnitSphericalDifferential (d_lon, d_lat) in km rad / s
(0., 4.74)>
Previously, the following gave the wrong unit for the proper motion:
c = SkyCoord(x=1*u.kpc, y=0*u.kpc, z=0*u.kpc,
v_x=10*u.km/u.s, v_y=0*u.km/u.s, v_z=4.74*u.km/u.s,
representation_type='cartesian')
c.representation_type = 'unitspherical'
repr(c)
<SkyCoord (ICRS): (ra, dec) in deg
(0., 0.)
(pm_ra, pm_dec) in km rad / s
(0., 4.74)>
f9d7501 to
ce45028
Compare
|
CI failure is unrelated, so merging. Thanks for the quick review, @adrn! |
…scale Fix conversion of cartesian to unit-spherical coordinates with velocities
3 participants
Previously, the following gave the wrong unit for the proper motion:
This was because
CartesianDifferentialwas incorrectly transformed toUnitSphericalDifferential.p.s. Not quite sure about the milestone.
cc @nstarman - this may be relevant for getting #11467 to work as well...