Changes to Phase objects

[dlakaplan]

As part of #1371, I am proposing a change to the Phase objects. They were defined as a named tuple, so that the integer & fraction parts could be accessed as p.int and p.frac, or as p[0] and p[1]. However, this meant that if the object is an array you need to first get the integer/fractional parts, do any slicing, and then repack. So I changed the interface by implementing __getitem__. Now you can do:

from pint.phase import Phase
import numpy as np
p = Phase(np.arange(10),np.random.random(10))
print(p.int)
print(p.frac[:5])
print(p[::2])
i,f = p

so the integer and fraction parts can still be accessed through .int and .frac, which return quantity arrays. But you can also slice directly. And tuple unpacking still works, which means that iteration through the object returns the integer/fractional parts rather than individual elements. So:

for x in p:
    print(x)

prints the integer and fraction parts, not each element (this could be changed).

Does anybody have opinions about this? Ways to make the interface deprecation clear?

[paulray]

Note you get Phase objects from model.phase(ts) so any external code that computes phases would now get an object that behaves differently. External authors who might care are me, @matteobachetti @kerrm

[paulray]

In NICERsoft, photon_phase.py code does this:

    phsi, phsf = modelin.phase(toas, abs_phase=True)

I think that should still work because it uses tuple unpacking, right? phsi will be an array of integer parts and phsf will be an array of fractional parts.

Elsewhere it does this:

phss = modelin.phase(ts, abs_phase=True)[1].value  # discard units

This should get all the fractional parts and make a regular array instead of a Quantity array. But the new change will break this.

And yet another code does this:

    phss = modelin.phase(ts, abs_phase=True)[1]
    # Strip the units, because PINT may return u.cycle
    phss = np.array(phss)
    # ensure all postive
    phases = np.where(phss < 0.0, phss + 1.0, phss)

This was to work around the crazy u.cycle issue. But again this will break because it uses [1] to get the fractional part.

[dlakaplan]

Yes, those will break. is it possible to update NICERsoft?

[paulray]

Yes, it is easy for me to update NICERsoft on github. The tricky bit is that it means all the NICERsoft uses must update NICERsoft and PINT at the same time. Unfortunately, I never packaged up NICERsoft so it has no setup.py and isn't pip-installable, so there isn't a good mechanism for enforcing version on dependencies like PINT

[paulray]

I think if I change [1] to .frac in NICERsoft, it will work with both old and new PINT? If so, that isn't so bad since I can just try to get everyone to update NICERsoft now and then they can update PINT whenever without a problem.

[paulray]

OK, I made that change to NICERsoft

[dlakaplan]

Great, that should work with both versions.

[kerrm]

This change will also break my Fermi pulsar timing code. It's not a big deal for me to fix it. The code published along with the "Fermi PTA" paper is referenced to a specific commit of PINT, but I can see some user in the future being bitten if they have a fresh copy of PINT. Probably also not a big deal.

[aarchiba]

I'm a little concerned that tuple unpacking does not agree with indexing. People expect a, b = thing to mean a == thing[0] and b == thing[1]. They also expect that iteratiion will work the same as indexing, and that array-like things can be iterated over. Users may write for phase in phases[::2]: and be very surprised to get exactly two iterations. I think we should aim to surprise users as little as possible.

It seems the existing interface has us somewhat painted into a corner. I'm not quite sure what the best way forward is.

[aarchiba]

I do think we should encourage people to use .int/.frac rather that [0] and [1], but it seems hard to avoid the conclusion that tuple unpacking is an extremely sensible approach as well.

I don't know if it's a useful analogy, but in recent versions np.loadtxt has acquired the option unpack, which transposes the returned array so that you can do col1, col2 = np.loadtxt(..., unpack=True), instead of having to do annoying column indexing or a manual transpose.

[dlakaplan]

I agree the tuple vs. array interfaces conflict a bit. Options would be:

• redefine Phase as a separate class, not a named tuple. This would break some other uses like Paul's example above:

phsi, phsf = modelin.phase(toas, abs_phase=True)

but it would mean iteration goes over the elements.

• Go back to the way it was. Clumsier for accessing multiple elements, but keeps backward compatibility. We can still encourage explicit calls to .int and .frac just in case we change in the future.

[aarchiba]

How we implement it is a detail. But yes, the problem regardless of implementation is that we want:

• .int/.frac to access the parts. (No problem.)
• indexing to index into the array elements.
• iteration to iterate over the array elements. (That is, behave the same way arrays do, so that iteration and indexing agree.)
• tuple unpacking to split the parts. (But tuple unpacking is iteration.)

I don't really see how to build a consistent interface meeting all these criteria. The current interface ditches easy indexing; the proposed interface makes iteration and indexing differ from each other.

Is there a possible alternative syntax for indexing? phase.array[::2] maybe? Or can we make Phase objects 2D arrays (phase[:,::2]) with accessors so that phase.int gave you phase[0]? (This is complicated because one might have Phase objects whose integer and fractional parts are n-dimensional.)

[dlakaplan]

What about adding a split() method that will unpack the integer and fraction parts?

[aarchiba]

What about adding a split() method that will unpack the integer and fraction parts?

Doesn't this still break backward compatibility? Or maybe I haven't understood your suggestion? Can you elaborate?

One suggestion I had would be to make Phase objects look like (be?) 2-D Quantity objects. Then phase[0] and phase[1] would still return integer and fractional parts, and iteration and tuple unpacking would work as expected. But phase[:,::-1] would allow indexing the object in all sorts of numpy ways, returning a perfectly satisfactory Phase object.

[dlakaplan]

I am wary of assuming that the phases will always be at most 1D, since there could be cases (e.g., simulations) where 2D or higher might be needed.

For the moment in the PR (which was merged) I removed the __getitem__, so I think it won't break things.