Correctly account for height in transformations between intermediate and AltAz frames by eteq · Pull Request #5591 · astropy/astropy

eteq · 2016-12-12T07:29:50Z

This addresses the problem pointed out in #5133, where the exact observer location (particularly the height above the other's surface) seemed to be getting ignored when converting to AltAz.

The root of the problem is that the ERFA transformations from CIRS to AltAz behave somewhat unexpectedly and do not properly account for these distance effects (in essence they assume the target is at infinite distance). Because everything goes through CIRS to get to AltAz, this showed up even in #5133 (where the starting point is ITRS). This PR fixes that by computing the "astrometric" ra/dec - i.e., the ra/dec of the angle from the observer to the target - and passes that into the ERFA transformations. (Then it reverses the transformation on the way back out into CIRS when going the other way.)

This PR also adds a set of regression tests to ensure this doesn't happen, inspired by #5133 (including @mhvk's #5133 (comment)).

This closes #5133. Tentatively milestoning it as 1.3.0, as it is a bugfix.

eteq · 2016-12-12T18:04:39Z

I'm pretty sure this is ready to go, but would be good to get another pair of eyes... @adrn @StuartLittlefair @mhvk, or @astrofrog?

astrofrog

This looks reasonable but I'm not the most familiar with this code

astrofrog · 2016-12-12T18:24:58Z

+        # compute an "astrometric" ra/dec -i.e., the direction of the
+        # displacement vector from the observer to the target in CIRS
+        loccirs = altaz_frame.location.get_itrs(cirs_coo.obstime).transform_to(cirs_coo)
+        diffrepr = (cirs_coo.cartesian-loccirs.cartesian).represent_as(SphericalRepresentation)


Would UnitSphericalRepresentation be enough? (not sure if it makes a difference)

Performance wise I think it's identical, but you're right conceptually it makes more sense (and uses slightly less memory). Will fix.

astrofrog · 2016-12-12T18:25:15Z

                         altaz_frame.obswl.value)

-    az, zen, ha, obs_dec, obs_ra = erfa.atioq(cirs_ra, cirs_dec, astrom)
+    az, zen, _, _, _ = erfa.atioq(cirs_ra, cirs_dec, astrom)


In principle, could also just put [:2] at the end but up to you

Obviously this is totally in the noise for essentially any real scenario, but...

>>> timeit a, b = f()[:2] 10000000 loops, best of 3: 177 ns per loop >>> timeit a, b = f()[:2] 10000000 loops, best of 3: 177 ns per loop >>> timeit a, b, _, _, _ = f() 10000000 loops, best of 3: 99.8 ns per loop >>> timeit a, b, _, _, _ = f() 10000000 loops, best of 3: 105 ns per loop

So apparently the current case is actually faster (first time I checked this...)

mhvk · 2016-12-12T18:31:00Z

-    usrepr = cirs_coo.represent_as(UnitSphericalRepresentation)
-    cirs_ra = usrepr.lon.to(u.radian).value
-    cirs_dec = usrepr.lat.to(u.radian).value
+    if cirs_coo.distance.unit is u.one:


Is distance guaranteed to exist? I ask because below the way to test this seems to be something like dat.get_name() == 'unitspherical' or dat.to_cartesian().x.unit == u.one (I never quite understood these hoops; we probably should define a property that gives this information... but that's besides this PR).

Good point. In nearly all practical cases it should exist because the default representation is Spherical, but the following produces an error:

c = CIRS(ra=..., dec=...) c.representation = UnitSphericalRepresentation c.transform_to(AltAz(...))

because forcing the representation to unit removes the distance. Will at least make sure it's internally consistent here, but you're right that we should make that all more consistent (either a method or a consistent one-liner idiom that's recommended in the docs)

mhvk · 2016-12-12T18:32:52Z

+        # compute an "astrometric" ra/dec -i.e., the direction of the
+        # displacement vector from the observer to the target in CIRS
+        loccirs = altaz_frame.location.get_itrs(cirs_coo.obstime).transform_to(cirs_coo)
+        diffrepr = (cirs_coo.cartesian-loccirs.cartesian).represent_as(SphericalRepresentation)


Might as well represent as UnitSphericalRepresentation (saves calculating the distance which is not needed anyway).

@astrofrog noticed this too.. turns out it's the same calculations cost, but less memory intensive. (so will change)

OK, makes sense I guess: squaring a few numbers and taking the square root is less work than arctan's.

mhvk · 2016-12-12T18:35:17Z

@@ -97,30 +106,22 @@ def altaz_to_cirs(altaz_coo, cirs_frame):
    cirs_ra, cirs_dec = erfa.atoiq('A', az, zen, astrom)*u.radian

    if isinstance(altaz_coo.data, UnitSphericalRepresentation):


I guess a similar question here: is it possible here to have altaz data that have unit-less distances?

same as above: unlikely but possible, so I will switch them all to the same idiom

mhvk · 2016-12-12T18:37:19Z


+@pytest.mark.parametrize('testframe', totest_frames)
+def test_cirs_altaz_nodist(testframe):
+    """


Is this an object resembling the moon?

oops, left over docstring from copy-and-paste - will fix

mhvk · 2016-12-12T18:40:49Z

@eteq - this looks good modulo my queries about how to identify a frame as having no distance.

p.s. I'm also wondering about light-travel-time effects for sources that move, but that is outside of this PR anyway...

eteq · 2016-12-12T21:28:21Z

p.s. I'm also wondering about light-travel-time effects for sources that move, but that is outside of this PR anyway...

@mhvk - good point - light travel time is not accounted for here, but that requires deciding more clearly what distance means, whether there should be a way to turn it off, etc. So definitely a separate PR (possibly even with some breaking side-effects), but at least the foundation for making that will be good once this is in...

eteq · 2016-12-12T21:29:57Z

Thanks for the review @astrofrog and @mhvk - assuming the tests pass I think the current version is set to merge.

Correctly account for height in transformations between intermediate and AltAz frames

eteq · 2016-12-12T23:07:36Z

Tests passed, so merged. backported to v1.3.x in ff653f3

eteq added 7 commits December 10, 2016 17:46

add regression test for 5133

61319b0

experimental attempt at astrometric fixing

d4eae9b

add another regression test as suggested by @mhvk

3836544

and distance-less cirs<->altaz test

bd178ea

make altaz<->CIRS transform correctly reversible

103921f

make sure GCRS<->AltAz is reversible

f9b857b

add changelog entry

dc49f79

eteq added the coordinates label Dec 12, 2016

eteq added this to the v1.3.0 milestone Dec 12, 2016

put in correct changelog PR number

7391173

eteq added the Ready-for-final-review label Dec 12, 2016

astrofrog approved these changes Dec 12, 2016

View reviewed changes

mhvk reviewed Dec 12, 2016

View reviewed changes

eteq added 3 commits December 12, 2016 16:11

address @astrofrog's suggestions

eb970ec

make unitspherical/dimensionless checking more consistent

1612954

fix incorrect docstring pointed out by @mhvk

f87428a

eteq merged commit f7f70d3 into astropy:master Dec 12, 2016

eteq deleted the fix-5133 branch December 12, 2016 23:06

eteq added a commit that referenced this pull request Dec 12, 2016

Merge pull request #5591 from eteq/fix-5133

ff653f3

Correctly account for height in transformations between intermediate and AltAz frames

		@@ -97,30 +106,22 @@ def altaz_to_cirs(altaz_coo, cirs_frame):
		cirs_ra, cirs_dec = erfa.atoiq('A', az, zen, astrom)*u.radian

		if isinstance(altaz_coo.data, UnitSphericalRepresentation):

Conversation

eteq commented Dec 12, 2016

Uh oh!

eteq commented Dec 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astrofrog 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!

eteq Dec 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Dec 12, 2016

Uh oh!

eteq commented Dec 12, 2016

Uh oh!

eteq commented Dec 12, 2016

Uh oh!

eteq commented Dec 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eteq commented Dec 12, 2016 •

edited

Loading

eteq Dec 12, 2016 •

edited

Loading

eteq commented Dec 12, 2016 •

edited

Loading