Power Rabi conversion

[omrieoqm]

No description provided.

[deanpoulos]

Can you please rename Power_Rabi and any other module names to be lowercase? https://peps.python.org/pep-0008/#package-and-module-names

[deanpoulos]

amps = np.arange(
    node.parameters.min_amp_factor,
    node.parameters.max_amp_factor,
    node.parameters.amp_factor_step,
)
# Number of applied Rabi pulses sweep
if N_pi > 1:
    if operation == "x180":
        N_pi_vec = np.arange(1, N_pi, 2).astype("int")
    elif operation in ["x90", "-x90", "y90", "-y90"]:
        N_pi_vec = np.arange(2, N_pi, 4).astype("int")
    else:
        raise ValueError(f"Unrecognized operation {operation}.")
else:
    N_pi_vec = np.linspace(1, N_pi, N_pi).astype("int")[::2]

The code here fits into the category of defining sweep ranges, and I just like Ramsey, I want these to be put into functions in the parameters.py class for Power Rabi.

There are three main reasons for this:

1. It's not very insightful for the user to see all of the if-else statements which go into constructing the sweep range for different node parameters- they only usually care about one, and even then, don't need to see what it's doing
2. If you separate them into functions like how it's done in Ramsey, it makes it harder for bugs to hide. Each different branch of the if-else statement is put into it's own function (if the name doesn't match the behaviour, there is a bug) and the functions become testable if needed.
3. These functions can be called at any point, using only the node.parameters as input to reliably reproduce the sweep axes, instead of having to worry about passing it around to different functions. So, the sweep axes will be safer from a. being recreated with duplicated code somewhere and b. being modified accidentally somewhere and used again incorrectly.

[deanpoulos]

Please remove redundant comments everywhere

[deanpoulos]

Aside from removing redundant comments, inline comments should be used even more sparingly. https://peps.python.org/pep-0008/#inline-comments

The code itself here is "self-documenting". It is obvious that

n_avg = node.parameters.num_averages  # The number of averages
                                     ^^^^^^^^^^^^^^^^^ unnecessary

is the number of averages.

[deanpoulos]

There is a subtle but important bug here. Before, it was simple enough to do:

    for i, qubit in enumerate(qubits):

because each qubit was played and processed one after the other, and its index in the list, i, was enough to identify it uniquely in the stream processing arrays, i.e., state[i] corresponds to the state of ith qubit in the list.

However, when moving to "batched" processing, qubits can organized arbitrarily into groups of multiplexed qubits which are played sequentially. To be concrete, imagine we had the following multiplexed groups:
Group 1: "q2", "q4"
Group 2: "q1", "q3"

So not only will:

    for i, multiplexed_qubits in qubits.batch():

not work, because qubits.batch() is a list of dictionaries, not a list of tuples, but it would give incorrect behaviour, even if you did

    for i, multiplexed_qubits in enumerate(qubits.batch()):

the variable i doesn't correspond with the qubit being measured, but the index of the multiplexed group.

This is the reason that multiplexed_qubits is actually a dictionary of qubits, each with a key corresponding to it's index in the original qubits list. It In the example above, it would look like this:

>>> qubits.batch()
[
   {1: q2, 3: q4},
   {0: q1, 2: q3}
]

So if you did:

for multiplexed_qubits in qubits.batch()
   for i, qubit in multiplexed_qubits.items():
      ...
      save(state[i], ...)

then now i would once again be a unique identifier for that qubit, and correct stream processing is recovered

[deanpoulos]

Please refactor the outer- and inner-most for loops to be like the Ramsey node so that the behaviour is correct

[deanpoulos]

You added this line twice, please remove one.

And remove the redundant comments

[deanpoulos]

You added this line twice, please remove one.

And remove the redundant comments

[deanpoulos]

I prefer if this node just takes node.parameters as an input, and if amps and N_pi_vec are re-generated using the new functions you implemented, and state_discrimination is taken directly.

[deanpoulos]

FYI I noticed a bug here:

    if node.parameters.load_data_id is None:
        ds = fetch_dataset(job, qubits, node.parameters)
    else:
        node = node.load_from_id(load_data_id)
        ds = node.results["ds"]
...
if node.load_data_id is None:
   # do state updates

This format was meant to protect against having analyzed historical data updating your current state. But notice, that once you re-load the historical data, you also reload the node.parameters.load_data_id which was set to None when the node was executed the first time. So, analyzing historical data was always overwriting your current state.json in the current, incorrect format.

To fix it, we need to do something like this:

 load_data_id = node.parameters.load_data_id
 node = node.load_from_id(load_data_id)
 node.parameters.load_data_id = load_data_id

[deanpoulos]

I would prefer if the plotting function wasn't just called plot, but instead something like:
plot_rabi_oscillations if that's what it's doing.

[deanpoulos]

The indentation here in the docstring is messed up

[deanpoulos]

I don't think the arguments are as important to be documented as the coordinates of the Dataset that will be returned. Consider removing them from the documentation or moving the coordinates to the top of the docstring.

[deanpoulos]

Take a look at the Ramsey Node parameters class and notice the following

I created a separate class called RamseyParameters which gets inherited by the final Parameters class used in the node.

The order of inheritance is actually very important when using dataclasses like these, because it determines which order the arguments will be provided. For Ramsey, I put it here:

class Parameters(
    NodeParameters,
    SimulatableNodeParameters,
    DataLoadableNodeParameters,
    QmSessionNodeParameters,
    RamseyParameters, <------------------
    FluxControlledNodeParameters,
    MultiplexableNodeParameters,
    QubitsExperimentNodeParameters,
):

Since when the order of the input arguments are built, they will be built from bottom-to-top, the RamseyParameters will be added 4th, leading to something that looks like this:

This means qubits are first, then whether you want it multiplexed, then flux control, then RamseyParameters. I like this order, especially since the lesser-used parameters (simulation, data-loading, etc.) are at the bottom, and think it should be kept consistent for all refactored nodes.

Can you do something like this for PowerRabi?

[deanpoulos]

Again, fix indenting