Skip to content
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

Issue with repr() for differential components on frames after set_representation_cls #7001

Open
adrn opened this issue Dec 19, 2017 · 2 comments

Comments

@adrn
Copy link
Member

adrn commented Dec 19, 2017

Found a bug in BaseCoordinateFrame._data_repr(). I haven't tracked down exactly what the issue is, but here's how to reproduce:

>>> import astropy.coordinates as coord
>>> import astropy.units as u
>>> c = coord.ICRS(ra=150*u.deg, dec=-11*u.deg, 
...                pm_ra_cosdec=100*u.mas/u.yr,
...                pm_dec=199*u.mas/u.yr)
>>> c
<ICRS Coordinate: (ra, dec) in deg
    ( 150., -11.)
 (pm_ra_cosdec, pm_dec) in mas / yr
    ( 100.,  199.)>
>>> c.set_representation_cls(s=coord.CartesianDifferential)
>>> c
<ICRS Coordinate: (ra, dec) in deg
    ( 150., -11.)
 (d_lon_coslat, d_lat) in mas / yr
    ( 100.,  199.)>

After setting the differential class, the repr for the differential components reverts to the UnitSphericalCosLatDifferential component names. This has something to do with the

if isinstance(dif_data, (r.UnitSphericalDifferential,
                                         r.UnitSphericalCosLatDifferential,
                                         r.RadialDifferential)):

in _data_repr.

cc @mhvk

@mhvk
Copy link
Contributor

mhvk commented Dec 19, 2017

@adrn - git blames you for putting that code in... So, if I change the if statement to be closer to what happens above for the representation:

                if (issubclass(dif_cls, (r.SphericalDifferential,
                                         r.SphericalCosLatDifferential)) and
                    isinstance(dif_data, (r.UnitSphericalDifferential,
                                          r.UnitSphericalCosLatDifferential,
                                          r.RadialDifferential))):

then I get

<ICRS Coordinate: (ra, dec) in deg
    ( 150., -11.)
 (v_x, v_y, v_z) in mas / (rad yr)
    (-82.88384202, -67.61704534,  195.34380951)>

The units are of course awkward since there is no distance, but arguably reasonable.

@mhvk
Copy link
Contributor

mhvk commented Dec 19, 2017

Note: I do wonder whether we should have these if-statements at all - I think this is really to guard against changes from UnitSpherical to Spherical in transformations, but perhaps we should treat such occurrences as bugs rather than paper over them. After all, from the dimension of the distance it is trivial to infer whether something should be unit-spherical or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants