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

Bug in write_ds9 for CircleSkyRegion #41

Merged
merged 1 commit into from
Jul 22, 2016
Merged

Bug in write_ds9 for CircleSkyRegion #41

merged 1 commit into from
Jul 22, 2016

Conversation

cdeil
Copy link
Member

@cdeil cdeil commented Jul 21, 2016

I've run into this bug in write_ds9 for CircleSkyRegion:

>>> from astropy.coordinates import Angle, SkyCoord
>>> import regions
>>> center = SkyCoord(42, 43, unit='deg')
>>> radius = Angle(3, 'deg')
>>> region = regions.CircleSkyRegion(center, radius)
>>> regions.objects_to_ds9_string([region])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/deil/code/regions/regions/io/write_ds9.py", line 46, in objects_to_ds9_string
    x = reg.center.transform_to(frame).spherical.lon.to('deg').value[0]
TypeError: 'float' object is not subscriptable

Removing the TODO and [0] indexing here fixes the issue:
https://github.com/astropy/regions/blame/master/regions/io/write_ds9.py#L44

44        if isinstance(reg, shapes.circle.CircleSkyRegion):
45              # TODO: Why is circle.center a list of SkyCoords?
46              x = reg.center.transform_to(frame).spherical.lon.to('deg').value[0]
47              y = reg.center.transform_to(frame).spherical.lat.to('deg').value[0]

But with this change, the tests that read and write ds9 regions start failing:
https://github.com/astropy/regions/blob/master/regions/io/tests/test_ds9_language.py#L27
because after reading regions, center.shape is (1,), and not center.shape == () scalar like in my code example above.

@keflavich @joleroi - I guess the ds9 reader has to be fixed to return scalar center SkyCoord?
Does one of you have time to have a look and implement a fix?

(If I remember correctly we discussed whether regions with array parameters should be supported and PyAstro16, and I think agreed we only want to do simple scalar regions because vector regions are a nightmare to implement and of limited use, right?)

@cdeil cdeil added the bug label Jun 23, 2016
@cdeil cdeil added this to the 0.1 milestone Jun 23, 2016
@cdeil cdeil mentioned this pull request Jul 16, 2016
@cdeil cdeil self-assigned this Jul 21, 2016
@cdeil
Copy link
Member Author

cdeil commented Jul 21, 2016

Test and fix attached.

@keflavich @joleroi - Do you have time to have a look?

@cdeil
Copy link
Member Author

cdeil commented Jul 21, 2016

travis-ci tests passed.
If I don't hear anything, I'll merge this tomorrow morning and try to finish up the 0.1 release.

(I know I've been saying this for two weeks, this time I really mean it.)

@keflavich
Copy link
Contributor

@cdeil looks fine. Thanks for tracking this down.

@cdeil cdeil merged commit ac831cf into astropy:master Jul 22, 2016
@joleroi
Copy link
Contributor

joleroi commented Jul 25, 2016

@cdeil I left this TODO. Thanks for fixing it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants