Make hms/dms namedtuples and add `absdms` #1988

eteq · 2014-01-22T19:27:51Z

This small change adjusts Angle so that the hms and dms attributes yield namedtuples instead of regular tuples. That enables usage along these lines:

'{0.h} {0.m} {0.s:.3f}'.format(coordinate.hms)

It also adds an absdms attribute that gives the absolute value of the dms elements. This is valuable for string formatting purposes (this was prompted by an on-list discussion about how to make IAU-style formatted strings.) Note that there is no abshms because I know of know convention where hour angle is negative. But it could be easily added in.

… regular tuple

mdboom · 2014-01-22T21:50:17Z

👍

cdeil · 2014-01-23T05:02:48Z

astropy/coordinates/angles.py

@@ -217,7 +222,7 @@ def hms(self):
        The angle's value in hours, as a ``(h, m, s)`` tuple


You could be more precise in the description, e.g. "tuple" -> "named tuple" or even "tuple" -> "(h, m, s) tuple" -> "named tuple with (h, m, s) members".

cdeil · 2014-01-23T05:05:53Z

👍

cdeil · 2014-01-23T10:44:22Z

docs/coordinates/angles.rst

    >>> a.dms
-    (57.0, 17.0, 44.806247...)
+    DMS_tuple(d=57.0, m=17.0, s=44.806247...)


Maybe add an example with a negative declination here or to the Angle.dms docstring?
I was surprised that m and s are negative in addition to d in that case.

an excellent idea! done in the version I'll push up soon

cdeil · 2014-01-23T10:50:45Z

I've written a function to create IAU designation strings here that covers my needs:
https://github.com/gammapy/gammapy/pull/64/files

Comments on how to improve the implementation are absolutely welcome.

If you want this for astropy now (as a coordinate member function?) let me know and I'll give it a try and make it more general / add more tests / docs.

I did not find the Angle.absdms method useful in the implementation, because I need d with sign, but m and s without sign. So I'm not sure it's a useful addition to Angle.

mhvk · 2014-01-23T13:50:49Z

astropy/coordinates/angles.py

+        This is primarily intented for use with `dms` to generate string
+        representations of coordinates that are correct for negative angles.
+        """
+        d, m, s = util.degrees_to_dms(self.degree)


I would take the absolute value of self.degree rather than the absolute value of what is returned by the function.

good point - I was concerned some implementations might come out subtly different, but right now it comes out the same, which is what really matters (fixed in a version I'm about to psuh up)

mhvk · 2014-01-23T13:58:11Z

Like the idea, but wonder about absdms, as I feel it is better to always return something that represents the actual value (people can do abs(angle).dms already). How about something like sdms or signed_dms, which returns a 4-part tuple in which the first item is the sign. This would also cover @cdeil's usage. (Actually, I would even consider to have dms return that, but perhaps that it just because I cannot see how I would use three negative numbers.)

mdboom · 2014-01-23T19:54:54Z

👍 to the idea of a function that returns (sign, d, m, s) (where d, m and s are all positive).

The current behavior is so that the obvious mathematical expression works:

d + (m / 60.0) + (s / 360.0)

If, for example, only d were signed, you'd have to do something like:

d + (m / 60.0) * sign(d) + (s / 360.0) * sign(d)

In short, dms is mathematically-convenient, not formatting-convenient.

cdeil · 2014-01-27T07:43:23Z

Here's the coordinate_iau_format utility function I came up with:
https://gammapy.readthedocs.org/en/latest/api/gammapy.catalog.utils.coordinate_iau_format.html
https://github.com/gammapy/gammapy/blob/master/gammapy/catalog/utils.py
https://github.com/gammapy/gammapy/blob/master/gammapy/catalog/tests/test_utils.py

For the declination I ended up using this code, which could be made a bit more readable (by using tuple names instead of indices) and maybe simpler (if sign becomes available as an extra field or maybe even d and m are returned as int instead of float) if this PR gets merged.

dec_sign = '+' if dec.deg >= 0 else '-'
dec_d = int(abs(dec.dms[0]))
dec_m = int(abs(dec.dms[1]))
dec_s = abs(dec.dms[2])

@cdeil

…ested by @cdeil

eteq · 2014-01-27T23:13:02Z

Good idea, @mhvk - I like the signed_dms approach better, tool. I know there was debate at one point suggesting this be how dms works, but having two in the way your suggesting nicely works around that. So I just pushed up a new version that implements it that way. (and @cdeil, does that change look good to you, too?)

cdeil · 2014-01-28T06:12:16Z

CHANGES.rst

@@ -12,6 +12,10 @@ New Features

 - ``astropy.coordinates``

+  - Updated `Angle.dms` and `Angle.hms` to return `namedtuple`s instead of
+    regular tuples, and added `Angle.absdms` attribute that gives the absolute


Change absdms -> signed_dms

mhvk · 2014-01-28T14:15:41Z

Apart from my minor comment about the documentation (and @cdell's corrections), this looks good!

@mhvk

…uggested by @mhvk)

astrofrog · 2014-01-28T16:00:55Z

astropy/coordinates/angles.py

@@ -23,6 +24,11 @@

 TWOPI = math.pi * 2.0  # no need to calculate this all the time

+# these are used by the `hms` and `dms` attributes
+HMS_tuple = namedtuple('HMS_tuple', ('h', 'm', 's'))


Just a minor question - is there a reason why not use hms_tuple, dms_tuple, and signed_dms_tuple for the names of the tuple types?

Not really. I was thinking about them as something sort of like classes, and hence the PEP8 first-letter-cap convention led to those. It's pretty ambiguous, though, so if you think lower-case is better, I'm happy to do that instead, @astrofrog .

I think the reason I felt unsure is because currently it seems like a mix of the two conventions. I'd either go the full way and treat it as a class (HMSTuple) or use the normal variable convention (hms_tuple). I have no strong preference :)

Ah, I see your point. In that case I mildly prefer hms_tuple so I'll switch it all to the attribute/variable-style convention.

@astrofrog

…estion of @astrofrog

eteq · 2014-01-28T17:02:18Z

Alright, I think I've implemented all the suggestions here, so if the tests pass I'll go ahead and merge this later today.

eteq · 2014-01-28T17:03:34Z

Oh, and @cdeil - The coordinate_iau_format is definitely something we'll want in Astropy, but I think it belongs in the SkyCoordinate that's discussed in APE5. So once APE5 is sorted out and we start implementing it, that's a great place to put it.

eteq · 2014-01-28T21:05:25Z

Tests passing, so I'll go ahead and merge. Thanks all of you for the valuable feedback!

Make hms/dms namedtuples and add `signed_dms`

eteq added 5 commits January 22, 2014 11:42

changed .hms and .dms attributes to output a named tuple instead of a…

5ab6661

… regular tuple

fixed tests to match new dms/hms

d80fef7

fix docstrings to reflect namedtuple dms/hms

32f4f43

added absdms attribute to Angle

c99e1c2

Update changelog

cec334b

cdeil reviewed Jan 23, 2014
View reviewed changes

cdeil mentioned this pull request Jan 23, 2014

Add coordinate string IAU designation format gammapy/gammapy#64

Merged

mhvk reviewed Jan 23, 2014
View reviewed changes

eteq added 4 commits January 27, 2014 17:51

Clarified in docstrings that named tuples were being returned as sugg…

b0570ee

…ested by @cdeil

Add a negative examples case to the docs as suggested by @cdeil

885be1e

Simply absolute value-taking for dms as suggested by @mhvk

daa4881

Changed absdms to signed_dms as suggested by @mhvk

8231873

cdeil reviewed Jan 28, 2014
View reviewed changes

eteq added 2 commits January 28, 2014 10:41

Removed remaining mentions of absdms as pointed out by @cdeil

d897f9b

Adjusted examples to only do negative angles where there's a point (s…

f27e35b

…uggested by @mhvk)

astrofrog reviewed Jan 28, 2014
View reviewed changes

changed named tuples for hms/dms to lower-case convention at the sugg…

10c39ca

…estion of @astrofrog

eteq added a commit that referenced this pull request Jan 28, 2014

Merge pull request #1988 from eteq/hmsdmsnamedtuple

96f4dd4

Make hms/dms namedtuples and add `signed_dms`

eteq merged commit 96f4dd4 into astropy:master Jan 28, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make hms/dms namedtuples and add `absdms` #1988

Make hms/dms namedtuples and add `absdms` #1988

eteq commented Jan 22, 2014

mdboom commented Jan 22, 2014

cdeil Jan 23, 2014

cdeil commented Jan 23, 2014

cdeil Jan 23, 2014

eteq Jan 27, 2014

cdeil commented Jan 23, 2014

mhvk Jan 23, 2014

eteq Jan 27, 2014

mhvk commented Jan 23, 2014

mdboom commented Jan 23, 2014

cdeil commented Jan 27, 2014

eteq commented Jan 27, 2014

cdeil Jan 28, 2014

mhvk commented Jan 28, 2014

astrofrog Jan 28, 2014

eteq Jan 28, 2014

astrofrog Jan 28, 2014

eteq Jan 28, 2014

eteq commented Jan 28, 2014

eteq commented Jan 28, 2014

eteq commented Jan 28, 2014

		@@ -217,7 +222,7 @@ def hms(self):
		The angle's value in hours, as a ``(h, m, s)`` tuple

Make hms/dms namedtuples and add absdms #1988

Make hms/dms namedtuples and add absdms #1988

Conversation

eteq commented Jan 22, 2014

mdboom commented Jan 22, 2014

Choose a reason for hiding this comment

cdeil commented Jan 23, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdeil commented Jan 23, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhvk commented Jan 23, 2014

mdboom commented Jan 23, 2014

cdeil commented Jan 27, 2014

eteq commented Jan 27, 2014

Choose a reason for hiding this comment

mhvk commented Jan 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eteq commented Jan 28, 2014

eteq commented Jan 28, 2014

eteq commented Jan 28, 2014

Make hms/dms namedtuples and add `absdms` #1988

Make hms/dms namedtuples and add `absdms` #1988