Skip to content

fix SkyCoord.__init__ edge case with list of one skycoord - #5270

Merged
taldcroft merged 5 commits into
astropy:masterfrom
StuartLittlefair:5209_fix
Oct 8, 2016
Merged

fix SkyCoord.__init__ edge case with list of one skycoord#5270
taldcroft merged 5 commits into
astropy:masterfrom
StuartLittlefair:5209_fix

Conversation

@StuartLittlefair

@StuartLittlefair StuartLittlefair commented Aug 27, 2016

Copy link
Copy Markdown
Contributor

This PR fixes #5209.

There are two bugs in that PR. One is that initialising a SkyCoord from a list of SkyCoords that have distances will raise an exception. This is fixed in the current master.

The second bug is an edge case where the distance goes missing when initialising a SkyCoord from a list which contains a single SkyCoord, as shown below. This PR fixes that edge case.

In [1]: from astropy.coordinates import EarthLocation, SkyCoord, GCRS
In [2]: from astropy import units as u
In [3]: from astropy.time import Time
In [4]: time = Time("2014-01-20 22:00")
In [5]: location = EarthLocation.from_geodetic(-17.88*u.deg, -28.76*u.deg, 2327*u.m)
In [6]: gcrs_loc = u.Quantity([1712675, 5328049, -3053885], unit=u.m)
In [7]: gcrs_vel = u.Quantity([-388, 125, 0], unit=u.m/u.s)
In [8]: moon = SkyCoord(GCRS(172.4*u.deg, 0.51*u.deg, 0.002671*u.au,
                obstime=time, obsgeoloc=gcrs_loc, obsgeovel=gcrs_vel))

In [9]: SkyCoord([moon])  # ODD! Where has the distance gone?
Out[9]: 
<SkyCoord (ICRS): (ra, dec) in deg
    (172.4, 0.51)>

mhvk
mhvk reviewed Aug 30, 2016
View reviewed changes
Comment thread astropy/coordinates/sky_coordinate.py Outdated
# special case of the above logic for edge case where we have a list of one
# SkyCoord
if len(scs) == 1:
allunitsphrepr = isinstance(scs[0].data, UnitSphericalRepresentation)

@mhvk mhvk Aug 30, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would seem simpler to just move this to l.1486 (replacing allunitsphrepr = True with this). That would also make the stanza correct for the case of a list where the first coordinate is not on the unit sphere.

@taldcroft

taldcroft commented Sep 30, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

What about the following further simplification? I can't see a reason to put the allunitsphrepr setting logic inside the for sc in scs[1:] loop.

            # Check that all frames are equivalent
            for sc in scs[1:]:
                if not sc.is_equivalent_frame(scs[0]):
                    raise ValueError("List of inputs don't have equivalent "
                                     "frames: {0} != {1}".format(sc, scs[0]))

            # Now use the first to determine if they are all UnitSpherical
            allunitsphrepr = isinstance(scs[0].data, UnitSphericalRepresentation)

@StuartLittlefair

StuartLittlefair commented Sep 30, 2016

Copy link
Copy Markdown
Contributor Author

Yes - that is better!

@mhvk mhvk added this to the v1.0.11 milestone Sep 30, 2016
@mhvk

mhvk commented Sep 30, 2016

Copy link
Copy Markdown
Contributor

Yes to this version! But still needs a regression test and an entry in CHANGES.rst. I've set the milestone to 1.0.11, assuming this bug is present in 1.0 already.

@mhvk

mhvk commented Oct 3, 2016

Copy link
Copy Markdown
Contributor

@StuartLittlefair - I've been travelling quite a bit, so may be a bit slow, but remain very interested! If I still don't get to this by next week, do feel free to ping again.

@mhvk

mhvk commented Oct 3, 2016

Copy link
Copy Markdown
Contributor

well, tells you the state of my mind if I even reply to the wrong PR. This one I did look at!

@StuartLittlefair

StuartLittlefair commented Oct 8, 2016

Copy link
Copy Markdown
Contributor Author

It looks like this passes apart from one travis build that didn't complete. Can someone with write permission restart that one case? (@mhvk, @taldcroft)

@taldcroft

taldcroft commented Oct 8, 2016

Copy link
Copy Markdown
Member

I restarted the job, fingers crossed. However, this is the second time I restarted a job with this error at the end:

Done. Your build exited with 0.
/Users/travis/build.sh: line 109: shell_session_update: command not found

@astrofrog

astrofrog commented Oct 8, 2016

Copy link
Copy Markdown
Member

@taldcroft - I fixed that error in ci-helpers a couple of days back, so it shouldn't appear anymore now. Let me know if it still does.

@taldcroft taldcroft self-assigned this Oct 8, 2016
@taldcroft
taldcroft merged commit c32aad9 into astropy:master Oct 8, 2016
@taldcroft

taldcroft commented Oct 8, 2016

Copy link
Copy Markdown
Member

Thanks @StuartLittlefair !

eteq pushed a commit that referenced this pull request Dec 13, 2016
@taldcroft @eteq
Merge pull request #5270 from StuartLittlefair/5209_fix
e1fd9bc 
Fix SkyCoord.__init__ edge case with list of one skycoord
@eteq

eteq commented Dec 13, 2016

Copy link
Copy Markdown
Member

Backported to 1.0.x in e1fd9bc although with a subtle change of a "fake" get_moon because get_moon doesn't exist in 1.0.x (but that's not essential to the regression test)

eteq pushed a commit that referenced this pull request Dec 18, 2016
@taldcroft @eteq
Merge pull request #5270 from StuartLittlefair/5209_fix
67639cb 
Fix SkyCoord.__init__ edge case with list of one skycoord
eteq pushed a commit that referenced this pull request Dec 21, 2016
@taldcroft @eteq
Merge pull request #5270 from StuartLittlefair/5209_fix
28af5ee 
Fix SkyCoord.__init__ edge case with list of one skycoord
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@taldcroft taldcroft

Labels

Bug coordinates

Projects

None yet

Milestone

v1.0.11

Development

Successfully merging this pull request may close these issues.

Weird bug initialising SkyCoord from list of SkyCoords that have distances set

6 participants

@StuartLittlefair @taldcroft @mhvk @astrofrog @eteq @pllim