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

Decide if obstime transformations of coordinates with velocities should result in motion #6280

Open
eteq opened this issue Jun 26, 2017 · 16 comments

Comments

@eteq
Copy link
Member

eteq commented Jun 26, 2017

This issues is a follow-up discussion to an unresolved question from #6219 (in that PR we are punting on the question by raising an error).

The starting point for the topic is #6219 (comment) - @StuartLittlefair's position is that if you transform to a frame with a different obstime, a coordinate with velocities should be moved to a different position based on its velocity vector. @StuartLittlefair added more in #6219 (comment) and #6219 (comment) and @mhvk agreed in #6219 (comment)

By contrast, I think this is not what we want and will be extremely confusing and inconsistent with the other coordinate machinery - see #6219 (comment). In #6219 (comment) and #6219 (comment) @adrn agreed.

This issue is hence a placeholder, because this is a somewhat significant philosophical question for coordinates, so we should resolve it for the next version.

@eteq eteq added this to the v3.0.0 milestone Jun 26, 2017
@eteq
Copy link
Member Author

eteq commented Jun 26, 2017

(not that this issue is explicitly referenced in a NotImplementedError I am proposing to add in #6219)

@astrofrog
Copy link
Member

I would be in favor of not updating the position based on the velocity. The velocity vector may be that of a flow/stream rather than a point object that is moving for example. This is not to say that there couldn't be a keyword argument or a separate method to update the positions though.

@mhvk
Copy link
Contributor

mhvk commented Jun 26, 2017

If I input Barnard's Star with its Hipparcos position and proper motion, and try to determine where it will be at a give time, then surely by default it should account for the time difference.... More generally, one thing I do very often is to register images to, say, UCAC4 or other position + proper motion catalogues. For this to work generally, one again does want to apply proper motions.

I must admit I cannot see the general use case for not taking into account obstime - it seems this is sensible only for coordinate frames in which obstime is not needed in the first place.

@astrofrog
Copy link
Member

What about the case of a binary star? If I have the velocity and proper motion of a star in a binary, any linear extrapolation would be very wrong.

@astrofrog
Copy link
Member

We could have a required keyword argument for cases like this to avoid any ambiguity?

@adrn
Copy link
Member

adrn commented Jun 26, 2017

Also solar system objects

@mhvk
Copy link
Contributor

mhvk commented Jun 26, 2017

But what use would be to transform those proper motions and velocities to another frame with another obstime - then the data has simply become inconsistent. I would think that in that case you would always want to have, say, ICRS, with the obstime still associated. (And if not, you'd better be forced to be very explicit about what you want!) In contrast, for the "normal" case, it is completely clear what should happen.

@mhvk
Copy link
Contributor

mhvk commented Jun 26, 2017

p.s. Yes, we need something like a keyword to tell what should happen. I think the main question is about what should be the default. It is quite possible that it should just be error on anything ambiguous.

@eteq
Copy link
Member Author

eteq commented Nov 17, 2017

After lengthy conversation with @mhvk and @adrn, we (@adrn, initially) are thinking to implement a SkyCoord.move method that does the operation of evolving based on velocity in the absence of an obstime on the frame itself.

I think we also came up with a clearer concept of the core issue here: there's actually two things that change when obstime changes: the motion of the frame, and the motion of an object in the frame. So if, say, you have an asteroid in GCRS coordinates, it's really not clear if you mean to evolve the asteroid including the reflex velocity due to the Earth's motion around the sun in addition to it's motion in the solar system (which is what just taking the raw velocity would do), or if you just want the linear space motion in a barycentric frame (which is what you would get if you transformed to ICRS and then applied the motion).

But it's much trickier to understand how to evolve this at the same time as an obstime on the frame. Our current thinking is probably to have transform_to get a new keyword along that if True applies move before the frame transofrmation, if False, does not, and if None, checks for obstime and a velocity and raises an exception (or warning?) if both are present with a message along the lines of "you need to think a bit more about what you mean in this case, and set this keyword appropriately to determine what you really mean"

@mhvk
Copy link
Contributor

mhvk commented Dec 22, 2017

@eteq - I think for 3.0 we're just erroring if obstime changes, correct? If so, we probably should move the milestone to 3.1

@adrn
Copy link
Member

adrn commented Dec 22, 2017

Agreed

@adrn adrn modified the milestones: v3.0.0, v3.1 Dec 22, 2017
@bsipocz bsipocz removed this from the v3.1 milestone Oct 28, 2018
hrishikeshgoyal referenced this issue in poliastro/poliastro Feb 1, 2019
  - Handled the case where the coordinate instance has multiple representations
  - Handled the case where coordinates instance doesn't have shape 0
  - Do not transform coordinates when coordinate instance is already in required
    frame(i.e. in a frame parallel to ICRS and centered at attractor)
  - Added tests for above changes

Issue: #434
@astrofrog
Copy link
Member

I ran into the following example today. I am trying to transform the position of the ALMA observatory with velocities to be in the frame of reference of the LSR. Note that the LSRD frame/skycoord has no explicit obstime:

In [1]: from astropy.coordinates import SkyCoord                                                                                                       

In [2]: from astropy import units as u 
   ...:                                                                                                                                                

In [3]: LSRD = SkyCoord(u=0 * u.km, v=0 * u.km, w=0 * u.km, 
   ...:                 U=9 * u.km / u.s, V=12 * u.km / u.s, W=7 * u.km / u.s, 
   ...:                 frame='galactic', representation_type='cartesian', differential_type='cartesian')                                              

In [4]: from astropy.coordinates import EarthLocation 
   ...:                                                                                                                                                

In [5]: alma = EarthLocation.of_site('ALMA').itrs 
   ...:                                                                                                                                                

In [6]: from astropy.coordinates import CartesianDifferential 
   ...: vel_to_add = CartesianDifferential(0*u.km/u.s, 0*u.km/u.s, 0*u.km/u.s) 
   ...: new_alma_data = alma.data.to_cartesian().with_differentials(vel_to_add) 
   ...: alma_with_vel = alma.realize_frame(new_alma_data)                                                                                              

In [7]: alma_with_vel.transform_to(LSRD)                                                                                                               
---------------------------------------------------------------------------
NotImplementedError                       Traceback (most recent call last)
<ipython-input-7-3999c54fb874> in <module>
----> 1 alma_with_vel.transform_to(LSRD)

~/python/dev/lib/python3.7/site-packages/astropy/coordinates/baseframe.py in transform_to(self, new_frame)
   1177            hasattr(self, 'obstime') and hasattr(new_frame, 'obstime') and
   1178            np.any(self.obstime != new_frame.obstime)):
-> 1179             raise NotImplementedError('You cannot transform a frame that has '
   1180                                       'velocities to another frame at a '
   1181                                       'different obstime. If you think this '

NotImplementedError: You cannot transform a frame that has velocities to another frame at a different obstime. If you think this should (or should not) be possible, please comment at https://github.com/astropy/astropy/issues/6280

In [8]: LSRD.obstime                                                                                                                                   

So I wonder if the restriction could be lifted if the target obstime is not set?

@mhvk
Copy link
Contributor

mhvk commented Oct 28, 2019

But wouldn't the velocity of ALMA in the LSR depend on the observation time? It would seem that the code is doing the right thing here...

@astrofrog
Copy link
Member

@mhvk - yes good point that an error is expected but I don't think the error message is correct then - it should mention that the obstime is missing?

@mhvk
Copy link
Contributor

mhvk commented Oct 29, 2019

Indeed, the error message is most confusing! In part this is because the if statement before works when it probably shouldn't: alma actually does have obstime=Time('J2000'), which I guess is the default (possibly a bad idea...), but LSRD has obstime=None and so the hasattr(LSRD, 'obstime') works, but shouldn't have.

@mhvk
Copy link
Contributor

mhvk commented Oct 29, 2019

p.s. The above seems like a bug regardless, but should be a separate issue!

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

5 participants