[QENG-716] Fixing DDS that do not produce identities

[leoadec]

Fixes https://q-ctrl.atlassian.net/browse/QENG-716

The solution adopted is the one described in the ticket: we invert the direction of the final pi/2-pulse when necessary. I modified the current tests to expect these inverted pulses. I also added additional tests to check if every kind of DDS produces an identity (most of these tests fail if they are run in the current master branch).

Notice that in some cases the DDS produces sigma_z rather than the identity. This happens when there is an odd number of Y and Z pi-pulses in the DDS, and these situations are identified by expect_sigma_z = True in the tests. Notice that the sigma_z cannot be cancelled by X pulses.

Maybe this is not a problem, as the difference between an identity and a sigma_z is just a matter of reference frame in time. If this a problem, a straightforward solution would be to replace the X pi/2-pulses with Y pi/2-pulses in these situations. I can easily modify the code that.

[leoadec]

As @charmasaur pointed out elsewhere, counting the Y gates is unnecessary because they are equivalent to an X and Z, whose effect in the parity count cancels out each other... I'll simplify the code taking this into account

[leoadec]

@charmasaur Okay, the code to decide whether to invert the pulse should be more clear now

[charmasaur]

This looks pretty good (especially the thoroughness of the tests, nice one), my two concerns are:

• I expect we do always want true identity operations (with no additional z rotation);
• I'm a little worried that the _add_pre_post_rotations function looks like it's very general, in the sense that you can pass it any sequence and it will correctly add the rotations, but in practice only works for specifics kinds of sequences (namely those consisting of X, Y or Z pi pulses). In particular I'm scared of a situation where we add some other kind of DDS that has internal pi/2 pulses or something, but expect _add_pre_post_rotations to still work.

For the second, maybe that's unnecessarily paranoid, since it's a private function after all. Still, the alternative that comes to mind (which would resolve the identity issue too), would be to stop trying to do things automatically, and instead just allow the angle of the pre+post rotations to be configured when calling _add_pre_post_rotations (just as an extra argument to the function, which each caller would set based on number_of_offsets or similar). That's much stupider, but more obviously correct.

What do you think?

[charmasaur]

optional: instead of effectively sum(np.where(..., 1, 0)), you could do np.count_nonzero(...).

[leoadec]

That's interesting, I didn't know True/False would count as 1/0 over there. Thanks for the tip!

[leoadec]

Thanks for the feedback @charmasaur !

I modified the code so that there should be no sigma_z` left over.

About the second point, I kind of share your concern about this not being a universal solution. My only worry is that, if we leave the decision about the parameters to each of the predefined methods, we may end up with a lot of repeated code. As I don't have a very good way of predicting how many pi-pulses a Walsh DDS will produce, for example, I fear I will end up just rewriting routines to count pi-pulses each time. (There's also something mysterious happening when I create an X-concatenated sequence. From the documentation I'd expect to always get an even number of X pi-pulses, but it turns out sometimes I seem to get an odd number, so I have to count them manually. Nevermind, I was misreading the documentation, the number of pulses is not always even. In any case, it doesn't look very straightforward to predict when it's even and when it's odd.)

If the prospect of repeating code doesn't look too bad, I'm okay with modifying the code to take the form of the pi/2-pulses as an argument. I agree that it is indeed a safer option for the future

[charmasaur]

Looking great, just some minor comments

[charmasaur]

How about adding an assert len(x_pi_pulses) + len(y_pi_pulses) + len(z_pi_pulses) == len(offsets)? That will make it pretty obvious if somebody calls this with invalid arguments. If you agree, also update the docstring simply to say that the provided pulses must consist of only X, Y and Z pi pulses (rather than saying that if they're not X, Y or Z pi pulses then you'll get a non-identity).

[leoadec]

That's a very good idea. Instead of using assert I used the exception that was usually thrown in that module, so if someone passes an unacceptable sequence the kind of error will be more consistent with the what they might see elsewhere.

I also made the checks for X/Y/Z pulses more strict to make sure they aren't being misidentified

[charmasaur]

Feel free to object to this, since I imagine we all understand these things in different ways, but...

The way I could convince myself about this was to think about what the sequence does to [1, 0] and [1, 1] (forget the 1/sqrt(2)). Any sequence of X, Y and Z applied to [1, 0] gives [1, 0] or [0, 1], and it's easy to see that which one you get is determined by the parity of nX+nY (since Z doesn't affect either of them). Any sequence applied to [1, 1] gives [1, 1] or [1, -1], and similarly it's easy to see that nY+nZ is what matters (since X doesn't affect either of them).

With that in mind, the other way you could write this would be:

preserves_10 = ((x_pi_pulses + y_pi_pulses)%2 == 0)
preserves_11 = (((y_pi_pulses + z_pi_pulses)%2 == 0)

then, your conditions below would read:

if preserves_10 and preserves_11: # identity
if preserves_10 and not preserves_11: # z
if not preserves_10 and preserves_11: # x
if not preserves_10 and not preserves_11: # y

To me that ends up reading quite intuitively, because I can just think about what the Pauli matrices look like. But as I said, feel free to take it or leave it -- if thinking of things in terms of remaining X and Z rotations is clearer for you then I'm fine with that.

[leoadec]

That might be a difference of whether we think in terms of Schrödinger or Heisenberg pictures. Re-writing the code in terms of the preserves_XX was a fun exercise, though

[charmasaur]

IMO this might be a little simpler if we just listed the cases explicitly:

if remainder_x and not remainder_z:
    # The sequence produces an X gate, so we use initial and final X pi/2 pulses in the same direction.
    rabi_value = np.pi/2
    initial_azimuthal = 0
    final_azimuthal = 0
    detuning_value = 0
else if remainder_x and remainder_z:
    # The sequence produces a Y gate, so we use initial and final Y pi/2 pulses in the same direction.
    rabi_value = np.pi/2
    initial_azimuthal = np.pi/2
    final_azimuthal = np.pi/2
    detuning_value = 0
...

It's a few extra lines, but it means that you only need to look at each block in isolation to understand what it's doing. If you don't like that, I'd at least shift the block where you initialise the values of rabi_value and whatnot to just above here, so they're closer together.

[leoadec]

Agree, it looks better in the way you suggested. I rewrote the code to treat all the cases explicitly

[michaelhush]

Looks like an annoying situation. I think you've done the best you can. Always open to a complete overhaul at some point if need be with this feature for the record. But for now, this is good.