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

SkyCoord initialized with two CartesianRepresentations each with different units generates nonsense units #7754

Open
thevorpalblade opened this issue Aug 22, 2018 · 5 comments

Comments

@thevorpalblade
Copy link

thevorpalblade commented Aug 22, 2018

Here's a MWE:

from astropy.time import Time
from astropy import coordinates as coord

now = Time.now()
# create a tuple of CartesianRepresentations with different units
earth = coord.get_body_barycentric_posvel("earth", now)
print(earth)
(<CartesianRepresentation (x, y, z) in AU
    (0.86689474, -0.47190455, -0.2046662)>,
 <CartesianRepresentation (x, y, z) in AU / d
    (0.00857658, 0.01346705, 0.00583879)>)

bad_units = coord.SkyCoord(earth)
print(bad_units)
<SkyCoord (ICRS): (ra, dec, distance) in (deg, deg, AU / d)
    [(331.43780716, -11.7147513 , 1.00801222),
     ( 57.50870768,  20.08731918, 0.01700031)]>

Notably, the resulting SkyCoord splits the difference in terms of units, giving the angular components units of degrees and the radial component units of velocity. I'd suggest that initialization of the SkyCoord should fail rather than silently produce a inconsistent result if given inconsistent units.

I'm using astropy version 3.0.4 and numpy version 1.15.0 with python 3.7 under archlinux.

@pllim
Copy link
Member

pllim commented Aug 22, 2018

cc @eteq and @adrn

@adrn
Copy link
Member

adrn commented Dec 3, 2018

Ah, here this specific issue is because the second object returned by earth is a velocity. But it's strange that it's returned as a separate object instead of as a differential attached to the position...

@mhvk
Copy link
Contributor

mhvk commented Dec 3, 2018

Yes, returning a tuple predates us having the ability of storing velocities as part of a representation. I don't know that we can trivially change that, but at least we should give the option of including it as a derivative (either via an argument or another function; e.g., it could use get_body, which already returns a SkyCoord of the position).

Separately, though, the example should really error since (I think) the result is interpreted as a stack of two coordinates, but those have inconsistent units. So, it is definitely a bug from that perspective.

@mhvk mhvk added the Bug label Dec 3, 2018
@adrn
Copy link
Member

adrn commented Dec 3, 2018

Agreed!

@thevorpalblade
Copy link
Author

thevorpalblade commented Dec 3, 2018

Indeed, thinking that it should return a differential attached to a position is how I discovered this bug!
You can see the discussion and my subsequent enlightenment here: #7743

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

4 participants