Conversation
|
Looks like the Putting this in every file is part of the Astropy coding guidelines (see here). |
|
This is the error that happens on Python 2 for this branch, where >>> from astropy.coordinates import AstrometricFrame, SkyCoord
>>> origin = SkyCoord(1, 2, unit='deg')
>>> AstrometricFrame(origin=origin)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "astropy/coordinates/builtin_frames/astrometric.py", line 197, in __new__
newcls = make_astrometric_cls(origin_frame.__class__)
File "astropy/coordinates/builtin_frames/astrometric.py", line 108, in make_astrometric_cls
{'__doc__': AstrometricFrame.__doc__})
File "astropy/coordinates/builtin_frames/astrometric.py", line 71, in __new__
res = super(AstrometricMeta, cls).__new__(cls, newname, bases, members)
File "astropy/coordinates/baseframe.py", line 77, in __new__
tmp_cls = super(FrameMeta, mcls).__new__(mcls, name, bases, members)
TypeError: type() argument 1 must be string, not unicodeI don't know how to resolve it. |
|
There were some comments in the code left that talked about adding "d" to the "lon" and "lat" names, which is no longer the case. I tried to fix the comments in 9505058, but @eteq and @alexrudy, could you please check the comments and code in |
|
I've gone ahead and added the rotation option to |
It can be omitted. I think the use of |
| An astrometric frame of the same type as this `SkyCoord` (e.g., if | ||
| this object has an ICRS coordinate, the resulting frame is | ||
| AstrometricFrame with an ICRS origin) | ||
| rotation : Quantity with angle units |
There was a problem hiding this comment.
Maybe just ~astropy.coordinates.Angle? Could still add "or ~astropy.units.Quantity with angle units"
There was a problem hiding this comment.
I copy & pasted this from the AstrometricFrame docstring. Do you want me to change it in both places, just here, or leave as is?
There was a problem hiding this comment.
Definitely both should be the same. And I think it is good practice to link to classes (like ~astropy.units.Quantity). But this is not very important, so I'll leave the decision up to you.
|
Agree to just omit the |
- Add rotation option to SkyCoord.astrometric_frame - Correct some code comments about lon / lat attribute names - Fix some typos and code formatting
|
@mhvk - Thanks for the feedback. I applied your inline comment and also squashed the commits. |
|
@cdeil I tried with 'unicode_literals', and I think that we just need to convert the frame name to a native string type, since it becomes a class name. See alexrudy@09cc02f This issue is one of the reasons people aren't in favor of 'unicode_literals' any more, as it might break if someone ever got a non-ascii character in there. But I don't think we are going to be creating frames with non-ascii names any time soon. So I'm 0 on whether to leave 'unicode_literals' in or take it out. Otherwise LGTM |
Let's leave as-is then. I think this PR is non-controversial and ready to go then. |
|
It all looks good, so I merge this before the name change in promised in #5002 makes this unmergeable! |
Minor cleanup for Astrometric
This PR does some minor cleanup (typos, whitespace) for the Astrometric frame code recently added by @alexrudy and @eteq .
@alexrudy @eteq - How about adding the
rotationoption to SkyCoord.astrometric_frame and pass it along to AstrometricFrame? Would it be OK with you if I add that as a convenience?I think it would be nice, but if it's controversial I'll just leave this PR as-is.