Pauli X, Y, Z measurement instructions#9269
Pauli X, Y, Z measurement instructions#9269pedrorrivero wants to merge 14 commits intoQiskit:mainfrom
Conversation
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
pedrorrivero
force-pushed
the
pauli-measurement
branch
from
230a3ca to
b3fd53e
Compare
| directives = ["measure"] | ||
| if not getattr(operation, "_directive", False) and operation.name not in directives: | ||
| directives = [] | ||
| if ( | ||
| not getattr(operation, "_directive", False) | ||
| and not isinstance(operation, Measure) | ||
| and operation.name not in directives | ||
| ): |
There was a problem hiding this comment.
After introducing the isinstance check to harness subclassing, directives became empty. Should it be removed? If kept, I would suggest turning it into a set instead of a list.
Pull Request Test Coverage Report for Build 3669879986
💛 - Coveralls |
|
@pedrorrivero Thanks for resuming #7716. I recommend to carefully read the discussion there and in #5311. In particular, there has been a question about the |
|
I am also curious how this works with the Also, is there a pass that removes the extra gates after the measurement if they come at the end of a circuit? |
|
Hi @yaelbh and @nonhermitian ! Thanks so much for your comments. I have not implemented that yet, left it for last to have more time to think, but this is what I am planning:
Thoughts anyone? Thanks! |
jakelishman
left a comment
jakelishman
left a comment
There was a problem hiding this comment.
I am keen to see X and Y measurements implemented somehow in Qiskit - it certainly seems like a generally useful feature. Thanks for picking up the thread of work again.
I'm concerned with introducing inheritance to the data model, though - in this situation it could be acceptable, but I'm not certain. It could be that OO hierarchy isn't the best fit possible - it definitely isn't for other quantum operations. Aside from those given in the PR, I can think of a couple of other potential advantages of subclassing though, but it's a bit long for me to type out on my phone.
I'd like to be involved in more discussion on this once I'm back from holiday in January - I can't really give a full review or think things through completely right now.
On Paul's point: in general, we should assume that most non-Z measurements will be decomposed/basis-translated before ResetSimplification and RemoveFinalMeasurements run; it's certainly the case right now, and while that could change, I can imagine other cases where Measure might appear in compound instructions that we'd want to handle well. I think it would be completely acceptable (and clearer) to handle that in a follow-up PR, though.
| \\endxy | ||
| } | ||
| \\POS ="i", | ||
| "i"+UR;"i"+UL **\\dir{-};"i"+DL **\\dir{-};"i"+DR **\\dir{-};"i"+UR **\\dir{-}, |
There was a problem hiding this comment.
(On my phone so I can't select properly.)
This looks like qcircuit code. That's GPL licensed, so we legally cannot include it, or code derived from it, in Qiskit. We need to be really careful when copying code blocks like this - attribution is not generally sufficient. It's legally tricky for us now even if you try and write your own implementation, because it's hard to be certain that it wouldn't be a derived work.
Perhaps we can use their \measure macro somehow?
There was a problem hiding this comment.
Ah! Good catch, I have definitely derived it from their code.
I did try to use their macro but it proved to be very challenging (it builds an image and you lose the reference points after wrapping it up) so I dropped down to base xypic code and recreated part of what they where doing adding the extra pieces. I have reached out to them to see if I can contribute this to their package.
Not sure how to address this otherwise. I'll think about it, thanks for bringing this up.
There was a problem hiding this comment.
I opened a PR to contribute this piece of code to qcircuit
|
|
||
| # Measure class kept for backwards compatibility, and repurposed | ||
| # as a common parent class for all measurement instructions. | ||
| class Measure(Instruction): |
There was a problem hiding this comment.
It's possible that we could accept an inheritance relationship here, but I don't think it's for certain the correct way of doing things, over (for example) a set of flags defined Instruction or Operation. This particular inheritance is definitely not safe, though - Measure is, and needs to remain, a measure in the Z basis, so MeasureX isn't an instance of it. There's also the false abstraction here that the base class is only abstract as far as a single-qubit Pauli-basis measurement, which isn't the most general type.
Aside from those abstraction/false-hierarchy issues, another thing to consider with inheritance is that we would need to work out how external / further subclasses of BaseMeasure (or whatever) would be safely serialised/deserialised by QPY - this PR adds an inheritance structure to instructions that is deliberately avoided in our data model elsewhere. This has some wide-ranging implications that I'm too "on holiday" to think through properly right now (sorry).
There was a problem hiding this comment.
I see 🤔 let's pick this up again once you come back then! Thanks for the detailed explanation
|
I would also add that this code might be hardware specific. Ie there might be hardware that can natively measure in more than one basis. At present, this is a hypothetical and/or I didn't bother to do my homework, but if that situation ever arises, then this code doesn't cover that. |
|
Thanks @nonhermitian, I am not sure I follow though 🤔 If some hardware shows up in the future that can handle (say) |
|
Per @jakelishman's comments, it may be better to follow this data model instead. Open to comments/suggestions!
|
|
Perhaps it does. However, I am not sure what happens to a decomposition over eg measure_x, measure_z, and measure. Also where does the measurement info go? Hopefully not basis gates as we have abused that notion enough. Perhaps it is handled in backend v2, but I have not played with that. |
|
Hi everyone! Thanks for the work on this PR and the insightful discussions. I work at Alice & Bob, a company developing cat qubits. (On top of this PR, we'll also need support for measurements in other bases in qiskit-aer, so that we can attach a specific noise to |
8 participants
Summary
This PR introduces
MeasureX,MeasureY, andMeasureZinstructions with the correspondingmeasure_x,measure_y, andmeasure_zmethods inQuantumCircuit.Details and comments
The
Measureinstruction is used as the parent class for all three new instructions, and an optionalbasisclass attribute is added. Otherwise it is left untouched for backwards compatibility.Ideally, I would suggest progressively moving away from
Measureand towardsMeasureZ(we can always keep theQuantumCircuit.measuremethod as an alias), keeping the former exclusively as a common parent (abstract) class. Suggestions on how to proceed with this —if possible— would be greatly appreciated.