Add velocity support to coordinate frames and transformations by adrn · Pull Request #6219 · astropy/astropy

adrn · 2017-06-15T17:23:25Z

This is the 3rd of several PR's that add support for velocities in astropy.coordinates.

This PR focuses on the high-level API: passing velocity data in to frames and SkyCoord.

mhvk · 2017-06-16T20:23:57Z

@adrn - I have some smaller comments in the PR, but overall this looks very nice. Good to see that it all meshes together so well!

I guess the largest comment is worth bringing up here too: should representation_names just give all the names, of representation and differential combined. Maybe not entirely illogical, since the two are in fact stored together.

One other comment, just to be sure: I guess you and @eteq decided that pm_ra will always include the cos(dec) term - I do this myself, so OK on that account, but obviously we have to be sure to document this well. It is also a one-way street: now it will essentially be impossible to have a nicely named ra derivative that does not include the term - you'd have to use d_lon and d_lat. Again, I actually agree with this decision, but just want to be sure that was indeed the plan.

mhvk · 2017-06-18T13:33:41Z

Note: Will need minor rebase after #6236 is merged.

adrn · 2017-06-18T21:26:52Z

should representation_names just give all the names, of representation and differential combined. Maybe not entirely illogical, since the two are in fact stored together.

Do you mean representation_component_names? My gut reaction is no, I disagree that and think the differential components should not be stored in there. Elsewhere we use elements from this to retrieve attributes from the underlying representation data. We'd have to insert a bunch of checks of whether the attribute exists on the representation data or the differential...But worth discussing more.

I guess you and @eteq decided that pm_ra will always include the cos(dec) term

Yes, that was my (our?) plan -- will make sure we clearly document this!

cc @mhvk

StuartLittlefair · 2017-06-19T10:21:25Z

An API question here, also relevant for #6226. I'm sure this has been discussed, so forgive me for that.

This PR handles transformations between frames, but what should we do with transformations when the obstime changes, and the frame has velocity data? Should we update the positions for the velocities (i.e update star positions for proper motion, a la erfa.starpv)? As a naive user, I might expect the following to update the stellar positions:

Star = coord.ICRS(ra=130*u.deg, dec=22*u.deg, pm_ra=100*u.mas/u.y, pm_dec=2*u.mas/u.yr, distance=100*u.pc, obstime=Time(57923.430, format='mjd'))
Star.transform_to(coord.ICRS(obstime="J2000"))

mhvk · 2017-06-19T11:25:58Z

@StuartLittlefair - I don't think the update currently happens -- one can clearly not assume much! I agree with your expectation, so think we either have to implement it (not difficult in itself, though it will become much more useful once we properly include errors), or raise if the obstime changes.

mhvk · 2017-06-19T11:51:44Z

p.s. The "not difficult in itself" is of course an understatement. It means we need to presume 0 for PM/RV values that are not present. My own sense would be strictness here: raise for missing values, with the exception message saying one can put in 0 if needed (just like I suggested one has to put a nominal distance for transformations that change the observer's perspective).

StuartLittlefair · 2017-06-19T11:53:12Z

Agreed - this is the approach used inside erfa as well, except that they automatically insert 0 for missing data and return a warning flag. I agree we should be more cautious.

turns out there were *two* problems: self._representation was being used instead of self.representation (which initializes an absent _representation), and self._data.differential was failing when _data was None or differentials were absent

…seCoordinateFrame

…ion and velocity of galactic center

mhvk

I went over this again with a fairly fine comb, and found mostly nitpicks, but also some more serious issues, in particular the test for unequal obstime.

mhvk · 2017-06-26T15:51:05Z

astropy/coordinates/baseframe.py


+        if (getattr(self.data, 'differentials', None) and
+           hasattr(self, 'obstime') and hasattr(new_frame, 'obstime') and
+           np.all(self.obstime == new_frame.obstime)):


Hmm, this surely has to be np.any(self.obstime != new_frame.obstime). And that this should be tested!

Also, just to be sure, since ICRS does not have an obstime, does this mean transforming to it will always work?

Hmm, this surely has to be np.any(self.obstime != new_frame.obstime). And that this should be tested!

Good point! I recall writing a test for this, but I don't see it now. It's possible I did not commit it ... (it was ~1am local time, so I will blame that...)

Also, just to be sure, since ICRS does not have an obstime, does this mean transforming to it will always work?

No, because in most (all?) cases, to get to ICRS from something that has an obstime you first have to do a transform into J2000 obstime. Certainly the AltAz (the most important case) works that way.

I take back that last part... I was getting those confused with the FK5 which have a specified equinox. So yes, it does work, and I'm adding a test for that.

Possibly, it should be that if obstime is not given on a frame, it is assumed to be J2000 for this purposes. Of course, on SkyCoord, this would just work, since there the obstime is propagated.

mhvk · 2017-06-26T15:52:26Z

astropy/coordinates/baseframe.py

            data_repr = 'Data:\n' + data_repr

+        if (getattr(self.data, 'differentials', None) and
+           's' in self.data.differentials):


Alignment off.

mhvk · 2017-06-26T16:00:03Z

astropy/coordinates/builtin_frames/altaz.py

+            RepresentationMapping('lat', 'alt')
+        ],
+        r.SphericalCosLatDifferential: [
+            RepresentationMapping('d_lon_coslat', 'pm_az', u.mas/u.yr),


This should be pm_az_cosalt - and needs a test case...

Though, what do you mean by a test case? You mean tests for proper motions in AltAz in general?

The test case I meant was iniatializing - presumably, as it was, it would fail with AltAz(..., pm_az_cosalt=..., ...).

mhvk · 2017-06-26T16:00:26Z

astropy/coordinates/builtin_frames/altaz.py

+            RepresentationMapping('d_distance', 'radial_velocity', u.km/u.s),
+        ],
+        r.SphericalDifferential: [
+            RepresentationMapping('d_lon', 'pm_ra', u.mas/u.yr),


Oops, copy & paste mistake: should be pm_lon and pm_lat.

mhvk · 2017-06-26T16:01:17Z

astropy/coordinates/builtin_frames/baseradec.py

+        A representation object or ``None`` to have no data (or use the other
+        keywords below).
+
+    ra : `Angle`, optional, must be keyword


It is not true that this must be a keyword... (and same for all up to radial_velocity)

All of these instances of "must be keyword" are from master...would you like me to address them here?
e.g., https://github.com/astropy/astropy/blob/master/astropy/coordinates/builtin_frames/icrs.py#L27

I'd call this either a "bug" or "docfix", so should do it in a separate PR

It mostly annoys me, but as long as it gets done, that's fine.

I added a note to #6260 to remind me to do this.

mhvk · 2017-06-26T16:18:50Z

astropy/coordinates/builtin_frames/lsr.py

+    representation : `BaseRepresentation` or None
+        A representation object or None to have no data (or use the other keywords)
+
+    l : `Angle`, optional, must be keyword


"must be keyword" is not true.

mhvk · 2017-06-26T16:23:01Z

astropy/coordinates/builtin_frames/skyoffset.py

                        rmlat = RepresentationMapping('lat', 'lat', 'recommended')
                        component_list.insert(0, rmlat)
+
                    lists_done.append(component_list)


Ah, now see that we don't yet support velocities. Maybe add TODO here?

mhvk · 2017-06-26T16:24:35Z

astropy/coordinates/builtin_frames/supergalactic.py

    representation : `BaseRepresentation` or None
        A representation object or None to have no data (or use the other keywords)
+
    sgl : `Angle`, optional, must be keyword


The "must be keyword" again is not true.

mhvk · 2017-06-26T16:26:41Z

astropy/coordinates/frame_attributes.py

+        if allowed_classes is not None:
+            self.allowed_classes = tuple(allowed_classes)
+        else:
+            self.allowed_classes = None


Maybe just set to BaseDifferential? That makes the check below more straightforward.

mhvk · 2017-06-26T16:31:01Z

astropy/coordinates/tests/test_frames.py

+
+    # two classes
+    rep2 = icrs.represent_as(r.CartesianRepresentation,
+                             r.SphericalCosLatDifferential)


Do we really want a test that checks unwanted behaviour? The reason that this doesn't feel is only because of the work-around for UnitSphericalRepresentation and RadialDifferential! I'd remove this test and write a TODO that things like this should fail in the future (or make them fail already...).

mhvk · 2017-06-26T16:44:28Z

On including this in 2.0, I think it is quite close, but am not sure how much gain it is to people if SkyCoord does not support proper motions, and if we specifically do not guarantee that the API is stable. I worry somewhat that soon we'll be quite far ahead with this in master, and it will become a drag to support what we have now for multiple years. Still, perhaps the largest benefit would be to have people who want proper motions become aware of this and start testing master...

Overall, I'm neutral on whether to include this in 2.0 (though if forced to choose, I'd not).

eteq · 2017-06-26T17:27:10Z

@mhvk

Still, perhaps the largest benefit would be to have people who want proper motions become aware of this and start testing master...

My thoughts exactly - in particular, the notable time pressure is the Gaia data release in ~April 2018. If no PMs are in until 3.0, then users have at best a few months to learn/experiment, whereas this way, they have a fighting chance of getting code for Gaia in that actually has a stable API (assuming we do that in 3.0).

I agree an LTS is not the best time to do this, but I think the externalities are forcing our hand here...

eteq · 2017-06-26T19:02:22Z

(I just pushed up a PR to address the obstime bit, since that was my doing anyway)

adrn · 2017-06-26T20:44:57Z

I addressed @mhvk 's comments, except for the "must be keyword" comments.

This was in master, so before I change (in all frame classes), I just want to make sure those weren't there for a reason? @eteq

eteq · 2017-06-26T21:23:57Z

This was in master, so before I change (in all frame classes), I just want to make sure those weren't there for a reason?

Pretty sure this is a holdover. As I mentioned above I think this is best addressed in a follow-on PR (and is docs so we can do it later, so I'd say not in this PR)

eteq

Guess I should have done this before... but I'm happy with it as it stands.

adrn · 2017-06-27T01:47:34Z

Added tests to check that the frame initializers accept argument names that we expect. Test is passing locally, and previous tests passed. @eteq or @mhvk any last thoughts?

adrn · 2017-06-27T01:53:15Z

Sorry I missed @mhvk's comments above:

but am not sure how much gain it is to people if SkyCoord does not support proper motions

I agree with @eteq's points above, and about preparing for Gaia.

and if we specifically do not guarantee that the API is stable

I think we can guarantee that the frame-level initialization of coordinates with proper motions will be stable, no? That is, I think it is reasonable to assume that ICRS(ra=.., dec=.., pm_ra_cosdec=..., pm_dec=...) will work in to the future no matter what changes we make to the representation internals.

[ci skip]

eteq · 2017-06-27T03:19:30Z

Alright, since everything is passing, I think we can go ahead and put this in, as @adrn and I think it's good to get this in for an experimental basis. @mhvk, I took your "neutral" above as being "if you're the deciding vote you vote no". But if you really think we shouldn't, there's still a window where we could back it (and #6226) out between now and release, as it's a straightforward revert commit of the merge.

But I do think this is the best way forward, following from the original implementation of coordinates where we started with an experimental API, kept as much of it as we could (especially the higher-level parts) and used that to build the better system.

Appreciate everyone's help with this, especially @mhvk and @StuartLittlefair ! (and @adrn of course)

Add velocity support to coordinate frames and transformations

mhvk · 2017-06-27T13:20:49Z

🎆 🎆 🎆

(And now on to SkyCoord for 3.0..........)

adrn added the coordinates label Jun 15, 2017

adrn added this to the v2.0.0 milestone Jun 15, 2017

adrn requested review from eteq and mhvk June 15, 2017 17:23

adrn force-pushed the coordinates/frame-velocity-support branch 2 times, most recently from 856e89b to 68457ec Compare June 16, 2017 04:27

eteq mentioned this pull request Jun 16, 2017

Support Velocities in coordinates by having transforms that use finite differences #6226

Merged

adrn mentioned this pull request Jun 16, 2017

Allow representations to contain differentials #6169

Merged

adrn mentioned this pull request Jun 18, 2017

Ensure realize_frame does not overwrite representation class. #6236

Merged

adrn mentioned this pull request Jun 18, 2017

Add bary/heliocentric radial velocity correction #5752

Merged

1 task

adrn force-pushed the coordinates/frame-velocity-support branch from bc3247b to 472f385 Compare June 18, 2017 19:26

adrn mentioned this pull request Jun 18, 2017

Add AffineTransform transform class #6218

Merged

adrn mentioned this pull request Jun 18, 2017

Add velocity support to frame classes adrn/astropy#12

Closed

adrn and others added 10 commits June 20, 2017 00:19

stuck with infinite recursion

76667b6

fix infinite recursion

a1be017

turns out there were *two* problems: self._representation was being used instead of self.representation (which initializes an absent _representation), and self._data.differential was failing when _data was None or differentials were absent

basedifferential has to have a recommended units

6ca9ab3

initial trial implementation of storing a single differential on a Ba…

bd854b4

…seCoordinateFrame

proof of concept using differentials on the ICRS frame

eeb2026

first try at an LSR frame and a very simple sanity test

54e4d26

add default differentials and representation mappings for most frames

4f0e364

changes to Galactocentric frame - use a CoordinateAttribute for posit…

788bf30

…ion and velocity of galactic center

support attribute recall for differential attributes

fcdb50c

update a few docstrings

d64beb8

adrn added 3 commits June 26, 2017 10:08

have to add baseecliptic to __all__

b8cfbe5

move baseradec and final cleanups to frame docstrings [docs only]

db56f22

add module and fix extra newline [docs only]

ef66c12

mhvk reviewed Jun 26, 2017

View reviewed changes

check for obstime adjustments

d202b22

adrn added 3 commits June 26, 2017 16:27

address minor comments by mhvk

86fbe36

more minor changes from mhvk comments [ci skip]

1172aeb

hide test for unwanted behavior

d8c9a8b

eteq approved these changes Jun 26, 2017

View reviewed changes

adrn added 2 commits June 26, 2017 21:15

move lsr test to celestial transformations file

3f63cc1

fix cosalt and add test for expected names [ci skip]

bdddc3e

adrn and others added 2 commits June 26, 2017 22:17

changelog modification [ci skip]

0c3fc49

changelog clarification

0bd7526

[ci skip]

eteq merged commit 2279f1c into astropy:master Jun 27, 2017

adrn mentioned this pull request Jun 27, 2017

Full-space velocity but unit-spherical position doesn't transform properly #6283

Closed

eteq added a commit that referenced this pull request Jun 27, 2017

Merge pull request #6219 from adrn/coordinates/frame-velocity-support

4a29742

Add velocity support to coordinate frames and transformations

adrn mentioned this pull request Jun 27, 2017

Coordinates/velocity bugfix #6286

Merged

adrn mentioned this pull request Jul 2, 2017

Can't specify differential class in frame init #6314

Closed

pllim mentioned this pull request Jun 20, 2019

Remove deprecated functionality for 4.0 release #8888

Closed

16 tasks

ayshih mentioned this pull request Jul 2, 2019

"Must be keyword" in Frame class docstrings? #8950

Open

pllim mentioned this pull request Apr 8, 2020

BUG: Fix leap second table download triggered on import #10125

Closed

Uh oh!

Conversation

adrn commented Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Jun 16, 2017

Uh oh!

mhvk commented Jun 18, 2017

Uh oh!

adrn commented Jun 18, 2017

Uh oh!

StuartLittlefair commented Jun 19, 2017

Uh oh!

mhvk commented Jun 19, 2017

Uh oh!

mhvk commented Jun 19, 2017

Uh oh!

StuartLittlefair commented Jun 19, 2017

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Jun 26, 2017

Uh oh!

eteq commented Jun 26, 2017

Uh oh!

eteq commented Jun 26, 2017

Uh oh!

adrn commented Jun 26, 2017

Uh oh!

eteq commented Jun 26, 2017

Uh oh!

adrn commented Jun 15, 2017 •

edited

Loading

eteq commented Jun 27, 2017 •

edited

Loading