Skip to content

Pr 5891 revised -- fix multidimensional skycoord problems - #5897

Merged
mhvk merged 6 commits into
astropy:masterfrom
mhvk:pr-5891-revised
Mar 24, 2017
Merged

mhvk merged 6 commits into
astropy:masterfrom
mhvk:pr-5891-revised

Conversation

@mhvk

@mhvk mhvk commented Mar 21, 2017

Copy link
Copy Markdown
Contributor

fixes #5889, #5890
replaces #5891 with a simpler implementation.

@mhvk

mhvk commented Mar 21, 2017

Copy link
Copy Markdown
Contributor Author

Note that this just fixes transform_to -- it is possible that it is still possible to get problems with initializing a SkyCoord in way that transform_to used to do: passing it a frame as well as keywords which are exactly the same as the frame attributes. But frankly I think this should not be OK to start with...

Also, I had started on revamping the parsing (which presently is very hard to understand), so maybe it is best to leave that for another time (see #5751 (comment))

@StuartLittlefair

Copy link
Copy Markdown
Contributor

Can't fault this for elegance @mhvk. It doesn't look like there's any significant performance hit for large representations either. :+1 from me.

Comment thread astropy/coordinates/representation.py Outdated
return np.concatenate([coo.reshape(sh).value for coo in coordinates],
axis=-1).view(dtype).squeeze()
# For PY2, convert possible unicode literal to string
coo_items = [(str(c) if six.PY2 else c, getattr(self, c))

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.

Any reason not to move if six.PY2 out of the loop? Besides making the loop (slightly) faster it would be easier to remove it when python2 support is dropped.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It used to do str(c) unconditionally which is guaranteed to work, and really the only reason I put this here was to be sure we would clean it up correctly for astropy 3.0. But on second thought, probably anybody doing that would do a grep for PY2, so then this would be found anyway.
Let me change it back...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

actually, on second thought, I wonder if this was just for numpy 1.6. I'll try without and see if travis signals any problems.

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.

seems to fail, so it's a python 2 thing. :(

@mhvk
mhvk force-pushed the pr-5891-revised branch from b6498c3 to 6091e79 Compare March 22, 2017 01:54
@mhvk

mhvk commented Mar 22, 2017 via email

Copy link
Copy Markdown
Contributor Author

@mhvk

mhvk commented Mar 22, 2017

Copy link
Copy Markdown
Contributor Author

OK, tests now all pass.

@mhvk

mhvk commented Mar 24, 2017

Copy link
Copy Markdown
Contributor Author

Merging since everybody seemed happy with this.

@mhvk
mhvk merged commit 43c64ab into astropy:master Mar 24, 2017
@mhvk
mhvk deleted the pr-5891-revised branch March 24, 2017 19:27
bsipocz pushed a commit that referenced this pull request Mar 30, 2017
Pr 5891 revised -- fix multidimensional skycoord problems
bsipocz pushed a commit that referenced this pull request Mar 30, 2017
Pr 5891 revised -- fix multidimensional skycoord problems
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

__repr__ method of BaseRepresentation can fail

3 participants