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.

Sign up for GitHub

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

Jump to bottom

Velocities in spherical coordinates #11402

Open
al-jshen opened this issue Mar 19, 2021 · 8 comments
Open

Velocities in spherical coordinates #11402

al-jshen opened this issue Mar 19, 2021 · 8 comments
Labels
coordinates Effort-medium Package-intermediate question

Comments

@al-jshen
Copy link

Hi, I'm converting some positions and velocities from ICRS to Galactocentric, and then turning that into a spherical representation. The velocities are given as angular velocities, which is a little strange since I would have expected them in km/s.

c = coord.SkyCoord(
        ra = ra * u.deg, 
        dec = dec * u.deg, 
        pm_ra_cosdec = pmracosdec * u.mas/u.yr, 
        pm_dec = pmdec * u.mas / u.yr, 
        radial_velocity = vrad * u.km / u.s, 
        distance = dist * u.kpc,
        frame = 'icrs',
    )
cg = c.transform_to(coord.Galactocentric())
cgs = cg.represent_as('spherical')
diffs = cgs.differentials['s']
diffs
<SphericalDifferential (d_lon, d_lat, d_distance) in (mas / yr, mas / yr, kpc mas / (rad yr))
    (-0.17801241, -0.06221044, -12.62912254)>

How can I get these in km/s? For d_distance I can do

cgs.differentials['s'].d_distance.to(u.km/u.s)

but for d_lon and d_lat I can't do the conversion, even when I multiply by the distance:

(cgs.differentials['s'].d_lon * dist * u.kpc).to(u.km/u.s)

gives the following error:

UnitConversionError: 'kpc mas / yr' and 'km / s' (speed) are not convertible

Thanks.
@github-actions
Copy link
Contributor

Welcome to Astropy 👋 and thank you for your first issue!

A project member will respond to you as soon as possible; in the meantime, please double-check the guidelines for submitting issues and make sure you've provided the requested details.

If you feel that this issue has not been responded to in a timely manner, please leave a comment mentioning our software support engineer @embray, or send a message directly to the development mailing list. If the issue is urgent or sensitive in nature (e.g., a security vulnerability) please send an e-mail directly to the private e-mail [email protected].

@ayshih
Copy link
Contributor

ayshih commented Mar 19, 2021

You need to specify the dimensionless_angles() equivalency to convert mas in the way that you want:

(cgs.differentials['s'].d_lon * dist * u.kpc).to(u.km/u.s, equivalencies=u.dimensionless_angles())

@adrn
Copy link
Member

adrn commented Mar 19, 2021

Yes, for an immediate solution, I use what @ayshih suggests. But it has been a longstanding issue for me that non-cartesian representations don't provide access to velocities in velocity units when possible (similar issues for PhysicsSpherical and Cylindrical representations). If you have any energy to think about that (specifically what the API should look like for that), please let us know your thoughts! (see also #7655 and cc @mhvk)

@al-jshen
Copy link
Author

al-jshen commented Mar 19, 2021 •
edited
Loading

@ayshih @adrn Thanks for the help. Unfortunately I don't think I know enough to give any meaningful input about how to solve that in a nice way.

Follow-up question: is there anything like this (https://docs.astropy.org/en/stable/coordinates/galactocentric.html) explaining the math for the velocity coordinate transform? I tried looking at this (https://github.com/astropy/astropy/blob/master/astropy/coordinates/builtin_frames/galactocentric.py) but didn't find anything there. Thanks.

@mhvk
Copy link
Contributor

mhvk commented Mar 31, 2021

About a solution for this: we already have a SphericalCosLatDifferential to deal with motion in longitude that includes cos(lat) to make it arcsec/yr on the sky. I think it should not be too difficult to make a new differential class, say SphericalRCosLatDifferential (better name would be desirable!) that for lat and lon also includes the radial term and thus gives km/s. One would then also need to adjust the scale factor calculation, at

astropy/astropy/coordinates/representation.py

Lines 1856 to 1862 in 7811614

def scale_factors(self, omit_coslat=False):
sf_lat = self.distance / u.radian
sf_lon = sf_lat if omit_coslat else sf_lat * np.cos(self.lat)
sf_distance = np.broadcast_to(1.*u.one, self.shape, subok=True)
return {'lon': sf_lon,
'lat': sf_lat,
'distance': sf_distance}

@adrn
Copy link
Member

adrn commented Jul 9, 2021

Coming back to this because of velocity conversion headaches the students are facing at our Galactic Dynamics summer school — we should really just implement your proposed solution @mhvk (and same for PhysicsSpherical, Cylindrical, etc.)!

@mhvk
Copy link
Contributor

mhvk commented Jul 9, 2021

Sounds good! Mostly just needs agreement on the name! Also for the attributes: v_ra, v_dec, radial_velocity, I guess?

@adrn
Copy link
Member

adrn commented Jul 9, 2021

It could also be nice to have a shorthand, like c.spherical.velocity, that has [v_ra, c_dec, radial_velocity] as a single quantity, like an analog of c.cartesian.xyz - something to think about / discuss on Monday or at the next coordinates call?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
coordinates Effort-medium Package-intermediate question
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@adrn @ayshih @pllim @mhvk @al-jshen