Introduce units.cycle (2 pi radians) [note: initially considered u.pi] - #1160

I'm not sure how I feel about this. In principle I like the idea, but in practice it leads to some kind of odd results. Like what if you have u.pi in a Quantity but you just want to drop the radians part? It's not possible:

In [8]: 5 * u.pi * u.m
Out[8]: <Quantity 5 m pi>

In [12]: (5 * u.pi * u.m).to(u.m * u.rad)
Out[12]: <Quantity 15.7079632679 m rad>

In [13]: (5 * u.pi * u.m).to(u.m)
ERROR: UnitsException: 'm pi' and 'm' (length) are not convertible [astropy.units.core]

unless maybe you defined some sort of equivalency. I think it almost makes more sense to define pi as a constant, but even then I'd leave it unitless; I don't think pi naturally has "units" of radians.

I think if one uses u.pi, the above behaviour would be expected, i.e., one should not do 2*u.pi*r to calculate a circumference. Given that, I understand the hesitation. Indeed, there generally is something to be said for treating angular units as dimensionless (in radians). E.g., it would not be bad if 0.5*I*Omega**2 would have units consistent with energy, which currently it only has if one ensures Omega has units of frequency, rather than rad/s. But this goes also directly to allowing trigonometric functions on dimensionless numbers, which I'm still not sure is a good idea. Possibly, this could indeed be solved with equivalencies, though it doesn't seem trivial.

I did also consider putting pi under constants, but I think that is even more likely to lead to confusion (like you, I concluded it would have to be dimensionless, which beats the point). Under units, my sense was that it hurts no-one, but provides some convenience.

Added si.cycle as well, so that, e.g.,

import astropy.units as u
import numpy as np
omega = 2.*u.pi/(3.5*u.hr)
t = np.linspace(0.,0.2,10)*u.day
phase = omega*t
phase.to(u.cycle)

yields

<Quantity [ 0.          0.15238095  0.3047619   0.45714286  0.60952381  0.76190476
  0.91428571  1.06666667  1.21904762  1.37142857] cycle>

I'm a little concerned about the potential for confusion between units.pi and constants.pi. I realize they are quite different things (one's a unit, the other a quantity), but I think that subtlety will be lost on some people, and will also lead to some confusing code. After all, one can always reproduce your usage of units.pi with constants.pi * u.radians.

I do like cycle though, as it seems less ambiguous as a "unit". And it's actually even better for your use case, because there's no need for the factor of two.

So I would be in favor of dropping the pi unit, but leaving in cycle.

No reason dimensionless quantities can't be in constants. I think we have several dimensionless constants already (fine-structure comes to mind).

Huh. Turns out we don't have the fine-structure constant, actually. There are several others I think we need to add but that's a separate issue.

This issue of having pi in expressions does bring up an interesting point regarding constants in general though. Since Astropy defines constants as a subclass of Quantity, when you combine them with other quantities in an expression any reference to the constant is lost. It would definitely be desirable to have a way to use constants in an expression as though they are irreducible constants. So if I multiply by c for example it might look something like:

>>> from astropy.constants import c
>>> 0.5 * c
<Quantity 0.5 c>
>>> (0.5 * c).decompose()
<Quantity 149896229.0 m / s>

It's possible that the combined units of all constants in a quantity could still be displayed which would definitely help with bookkeeping:

>>> 0.5 * c
<Quantity 0.5 c (m / s)>

Something like that. Having behavior like this would be ideal for pi as well because you could leave pis in expressions until you need a value. We could also give all constants LaTeX formats for display purposes, where relevant.

This would all be really great to see but it would take a bit of work to overhaul what we already have and to get it right. It's also getting dangerously close to being a symbolic manipulation system and reinventing at least parts of SymPy's wheel.

With pi as a constant already in available both via math.pi and numpy.pi, I don't think we should add it to constants as well. In my example, the case of interest really is where pi carries a unit of angle. But it does risk confusion and maybe should addressed more generally in the context of when angle units should be considered to have a dimension in the first place; I started discssion in #1161, and @iguananaut suggested to bring this up on the mailing list -- I'll do that.

Returning to this request, the right thing may be @eteq's suggestion of having units.cycle only, as it is less likely to lead to confusion (and covers the uses I can most easily think of). Shall I just do that, so this can be merged and closed? If so, I'd probably expand the definition slightly to ['cycle', 'cy', 'tau', 'twopi'].

@mhvk - actually, you can just update this PR by removing the pi stuff - although you're right it might be cleaner to open a new PR. I actually slightly prefer using this one, because then this history gets incorporated in master in case someone wants to bring it back. No big deal, though.

I would say the aliases shouldn't include tau or twopi, though. tau is a pretty generic variable (it certainly doesn't leap to mind for me as "all the way around a circle"), and twopi begs the question of "two pi of what?" basically the same as my point before - as in angle it's actually two pi radians). But perhaps something slightly more explicit that still makes the meaning clear? Like fullcircle or twopirads?

@eteq - OK, I changed the pull request (unfortunately, I messed up and hence decided to rebase). I added units.cycle to astrophys.py instead of si.py, since in the SI system it is never used, but it is explicitly recognised by the IAU [1]. I could not bring myself to use the symbol c for it, though, that just seems asking for trouble. I also removed the twopi and tau options -- as really those are not units, and u.cycle covers the use case. I'll also change the title of this pull request, so it is clearer what this is about.
[1] http://www.iau.org/science/publications/proceedings_rules/units/

When #1006 is merged, this will mean that Angle classes will also grow a cycle member and be instantiated by Angle('3 cycle'). I think that's good behavior, but I just wanted to point that out in case that side effect of this was not obvious.

@mdboom - agree that being able to do Angle('3 cycle') is, if anything, an improvement.

This looks great to me in terms of functionality, @mhvk - I totally agree u.c seems very dangerous. I didn't realize this is technically an SI unit, though - thanks for looking into that.

Perhaps you can also add a quick test that exercises this? (Maybe just something that does assert u.cycle.to(u.radians).value == 2*pi or similar - although == might not work, you might need assert_almost_equal) Once that's in I think this is good to merge.

@eteq - OK, test added.

Looks great - merging! Thanks @mhvk