Remove u.cycle units from phases

[paulray]

OK this was easier than I expected, at least so far.
I change Phases to be u.dimensionless_unscaled (which is a synonym for u.Units("") and Quantity(x)), so Phases are still Quantities, they just are dimensionless. I think this is the right choice rather than trying to make them bare np.arrays.

I removed all the u.cycle references and many dimensionless_cycle equivalencies. In nearly all cases, this was a simplification of the code, which gives me confidence that this is the right way to go.

All the tests pass, but this will need review and people to test it with their downstream code.

[luojing1211]

@mhvk We had a discussion about the u.cycle for pulse phase. I remember you tried to improve the phase object. We would like to hear your comments about this issue.

[mhvk]

Indeed, I constructed a precise Phase class (which holds integer cycle and fractional phase, bit like Time), which gives special treatment to np.sin, etc. (including exp(1j*phase)), using only the fractional part to keep precision. I even started a branch to try to use it, but gave up as at the time PINT was still stuck on using python2 and numpy too old for things to work. If there is interest, I could revive that.

See https://github.com/mhvk/scintillometry/blob/f7907e9abd8330306fffb5deb64a9d5fe8296b6c/scintillometry/phases/phase.py#L205-L219

[paulray]

Just a note that @champagne-cmd is starting to work on a set of unit tests for the Phase() class, which was missing before.

[mhvk]

For the unit tests, you may be able to use quite a bit from https://github.com/mhvk/scintillometry/blob/master/scintillometry/tests/test_phase_class.py

My original intent was to push the phase class up to PINT, and I'm quite happy to help if people are interested. I do think it is the right way in terms of a Quantity-like object.

See also https://scintillometry.readthedocs.io/en/latest/api/scintillometry.phases.Phase.html#scintillometry.phases.Phase

[paulray]

Thanks @mhvk, those are some extensive examples of unit tests!

In the big picture, we need to consider what to do for PINT. At the moment, we have 3 options, I think:
(1) Keep our current Phase()
(2) Keep our Phase() class, but remove u.cycle units (this PR)
(3) Switch to Marten’s Phase(), which uses u.cycle units but is a much more sophisticated implementation.

To decide, we need to consider how Phase() is used and thus how the different choices affect (1) ease of use (including acting in an intuitive manner for new users), (2) performance, and (3) compatibility with astropy<4.0 (only a concern for a short time).

I started looking back at the code I had to change to remove u.cycle, and many of the places we used phases are either wrapped in the dimensionless_cycle equivalency or use phase.value, thus negating any value of having u.cycle units on our phases. One reason is that our frequencies are in u.Hz, not u.cycle/u.s. Thus this fails when you use u.cycle units:

[1]: p1 = Phase(4,0.2)
[2]: F0 = 20.0*u.Hz
[3]: (p1/F0).to(u.s)
UnitConversionError: 'cycle / Hz' and 's' (time) are not convertible

This thus forces you to either use the equivalency or pull out p1.value. This negates any value of the unit. So, if we decide we must use the u.cycle unit, I think we should change our frequencies to be in u.cycle/u.s instead if u.Hz.

For this and some of the other reasons we've talked about, I favor stripping the u.cycle unit from Phases, as done by this PR.

[mhvk]

@paulray - I think there are two separate questions: (i) what unit to use, and (ii) how to keep precision.

On (ii), PINT doesn't seem to use its own Phase class very much, instead relying on long-double Quantity (perhaps not surprisingly, as the Phase class would seem too limited, not being able to interact with other quantities). So, at some level, the question is whether there is a use for a proper Phase class that behaves more like a TimeDelta, guaranteeing not to lose precision. Since phase is the equivalent of time on the pulsar, having a special class does not seem strange!

On the unit, I can see u.cycle being annoying, but it helps if you need to actually use the phase as an angle (say for polarization swing calculations), as for the trig functions and for exp(1j*phase) (indeed, with dimensionless you'll silently get the wrong answer for the latter example; it is special-cased in my Phase class). Also, with a proper Phase class, much of the unit stuff could be avoided by having an equivalency "built in" (this is how logarithmic units work in astropy).

p.s. Hz is really quite annoying! It should have been defined as cy/s, but sadly wasn't, and changing it now would break people being able to do wavelength = c/frequency (or course, wavelengh is really length/cycle too...); see astropy/astropy#6032

[mhvk]

p.s. Are you going to remove the unit from orbital phase too? I ask since that will likely break a few custom scripts that students wrote here (well, @luojing1211 can tell: does this affect things like Ashley's scripts?), and hence you may want to consider also whether phases more generally are used outside PINT.

[luojing1211]

It is only for the rotational phase. The major API should be as same as possible.

[paulray]

It is strange that PINT uses its Phase class so little. The phase functions of a TimingModel don't even return Phases! They are converted to Phase in TimingModel.phase() like this:

            for pf in self.phase_funcs:
                tz_phase += Phase(pf(tz_toa, tz_delay))

Since (at least if we merge this PR) our phases are fractions of a cycle, not angles, I guess we'd have to write exp(2*np.pi*1j*phase).

On orbital phase, I'll have to check on that...

[paulray]

Yeah, it looks like orbital phases are normal quantities with units of u.rad (and not astropy.coordinates.Angle).

[mhvk]

@paulray - thanks! Obviously, in the end it is up to all of you, but I would use at least equivalent units between orbital and rotational phase.

Aside: how should one test these days? I thought I would rebase my phase-class PR and tried tox -e py37 but am getting annoying matplotlib errors - E ImportError: Failed to import any qt binding

[paulray]

@mhvk sorry, that is annoying. As far as I know tox -e py37 should work. I'm testing it now on my machine. It does seem to be working under travis-ci since our tests are still passing. I'm not really a tox expert.

[mhvk]

yes, it may be that somehow some local matplotlib setting manages to get through (I do use KDE which uses qt) ...
...
... yes, that is it, if I remove .config/matplotlib/matplotlibrc, the tests run! (well, they do not error - they do fail but now it is because of code...)

[mhvk]

OK, clearly a different issue, so #694...

[mhvk]

@paulray - see #695 for how my Phase class might fit in. Right now, it is only used where the current phase class is used, but my sense is that it would make things elsewhere a lot simpler.

[paulray]

So, at the telecon, we decided to merge this PR. But, we are very interested in studying @mhvk's Phase class as well. We are wondering if a version of it that kept many of the nice features (that are missing in our simple Phase class) but didn't use the u.cycle unit might be worthwhile?

[mhvk]

@paulray - I think it would not be particularly difficult for the class itself have the equivalency between cycles and dimensionless built-in. Indeed, the example I pushed in #695 already did that for initialization, and that could be extended to the unit-conversion methods .to and .to_value. I don't think I'm the right person to making a PR that extends its use to other parts of tempo, but I'm definitely willing to help trouble-shoot problems and make adjustments as needed.

I do think that the same phase class (or a lower-precision version) should be used for orbital phase as well.

[mhvk]

p.s. I repeated the above in #695 and it is probably good to move further discussion there - merged PRs are not the right place!

[mhvk]

Should have realized this while discussing above, but unsurprisingly this broke our use of PINT phases for folding in scintillometry. Not hard to fix, though annoying since it implies having to support multiple versions for quite a while. And perhaps a useful warning that PINT is being used also as a dependency, not just standalone.

[paulray]

Yeah, sorry! I suspect that others might have the same issue, like @matteobachetti with HENDRICS. Now that we have tagged the version to go with the paper, I hope that we can keep these kind of significant API changes to a minimum!

[matteobachetti]

@paulray thanks for the warning, I'll check