Fix barrier instructions in tomography experiments

[wshanks]

Summary

Fix barrier instructions in tomography experiments

Details and comments

QuantumCircuit.barrier() accepts many forms of input, but tuple is not one of them. The prep and measurement indices passed as tuples to the barrier method were causing qpy serialization/deserialization to fail. It is possible that there were other unnoticed consequences to the type of the barrier argument being incorrect.

Closes #1060.

See also this Slack thread.

[wshanks]

I will add a release note. #1060 should probably be moved to this repo, though maybe it's not worth the trouble.

[wshanks]

I added a release note. mtreinish moved the issue into the repo.

[wshanks]

Also, I fixed the reset() invocation as well. I am a little confused how that one was not noticed before? It looks like reset() expects a single qubit argument but all the indices were being given as a tuple.

[coruscating]

Thanks for the fix! Perhaps we should add a test that tomography circuits can be serialized and deserialized?

[wshanks]

I added some simple tests. In principle a test like these could be added for every experiment.

Also, I fixed the reset() invocation as well. I am a little confused how that one was not noticed before? It looks like reset() expects a single qubit argument but all the indices were being given as a tuple.

I stepped through in the debugger and followed what happened for reset. Although the type hint suggests that reset() accepts only a single qubit index, it actually works for a sequence of qubits through the QuantumCircuit.append call that calls the instruction's broadcast_arguments method which replaces a list of qubit indices with a set of instructions, one per qubit argument (here for Reset).

Barrier is a special case because you do not want it to be split out into multiple instructions -- you want to collect all the qubit arguments and make a barrier across all of them. The barrier method (here) flattens out the qubit arguments for some cases but not for tuple. Barrier.broadcast_arguments does additional flattening. The problem is that the barrier method sets the num_qubits for the instruction. When passed a tuple, it just appends it to the list of qargs and then sees a list with one item and assigns num_qubits to 1. My guess of what happens beyond this is that nothing else in the old provider flow checks num_qubits and the additional flattening in broadcast_arguments produces an instruction that works well enough for the old provider. With the new provider, qpy looks at num_qubits and uses it to specify how much data will be written to the qpy binary. Then it writes data for each flattened qarg. So it says it will write 1 qubit but then writes 2. When it goes to read the data, this mismatch leads to it reading the data incorrectly.

[chriseclectic]

Barrier thing seems like a straightforward fix, but can you change the reset one back to keep code cleaner unless there is a specific issue with it since this is expected behaviour of that instruction.

[chriseclectic]

Is this one actually causing an issue? All 1-qubit instructions in terra are setup to broadcast over the arguments so shouldn't have any issues with qpy, and unless that changes I dont think we need to change this (And if there is specifically an issue with tuple, then it can be converted to a list)

eg reset doc string indicates it works with more than 1-qubit:

reset(qubit: 'QubitSpecifier') -> 'InstructionSet' method of qiskit.circuit.quantumcircuit.QuantumCircuit instance
    Reset the quantum bit(s) to their default state.
    
    Args:
        qubit: qubit(s) to reset.
    
    Returns:
        qiskit.circuit.InstructionSet: handle to the added instruction.

[wshanks]

I will change it back. You and I put more weight on different parts of the quoted text 🙂 The qubit: 'QubitSpecifier' type annotation says to me that it accepts only a single qubit.

[chriseclectic]

I just went with qubit(s) :D

[chriseclectic]

Beyond scope of this PR, but we should probably add similar tests for serialization of generated circuits for all experiments.

[wshanks]

I opened #1063 to keep track of this.

[chriseclectic]

I think you can probably use the round trip json serialization test function (like we use for experiment data), since json serializer should serialize a list of circuits via qpy.

[wshanks]

Since I addressed @chriseclectic's feedback and the PR was pretty straightforward, I added it to auto-merge since we just enabled it and #1062 was already in the queue and we wanted to see how it auto-merging handles multiple queued PR's.

[mtreinish]

I'd expect this to be prevent from enqueued because of @chriseclectic's changes requested review is sticky. But it'll be interesting to watch this all play out. It's a new feature in github and I've not used it yet.

[wshanks]

Ah, that makes sense. It shows me now "Auto-merge enabled" but I don't see it in the merge queue so far. It is still running checks. The documentation made it sound like I wouldn't be able to enable auto-merge until all the checks had passed. I am glad that is not the case because part of the motivation is not having to wait for all the tests to pass and the branch be up to date before queuing something for merging.

Do you happen to know how the commit message works? It didn't prompt me for one and we are using squash merging, so it seems like we might get stuck with the automated message that combines all the messages.

[wshanks]

I found this post about the commit message. It looks like you can choose from a few options (the concatenated commit messages, the PR title and description, or just the PR title).

[mtreinish]

Ah cool, that's good to know that's there now, it's a relatively recent (well 6 months ago) addition from that post. Personally I prefer the default combination of the branch's commit messages. But I also tend to write more detailed commit messages for each intermediate commit on a PR branch, and I know others typically don't bother.

[wshanks]

Hmm, actually, that post is about the default commit message, which is what I thought auto-merging might have to use, but I see here that the documentation for auto-merging says that it should prompt for a commit message if you are using the merge or squash methods, but I didn't get that prompt.

Personally I prefer the default combination of the branch's commit messages. But I also tend to write more detailed commit messages for each intermediate commit on a PR branch, and I know others typically don't bother.

I like that style as well, but it doesn't work very well with GitHub PR reviews. With that style, when the reviewer requests changes, ideally you could go back and rebase/squash fixes into the individual commits and force push, but then GitHub gives up and shows you the full diff again. If you just do little clean up commits, GitHub will give you the option to see just the diff of those cleanups which makes it easy for a reviewer to do a follow up review. Ideally, there would be a workflow that makes it easy for the reviewer to review the responses to their feedback with the multiple commit style.

[wshanks]

It seems like that documentation I screenshotted is not correct (yet?). The workaround is to choose the default commit message to be the PR title and description as suggested here like I had mentioned in my post before last.