-
Notifications
You must be signed in to change notification settings - Fork 90
Change to allow a qubit to be released if it is measured #231
Conversation
swernli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me, though I would wait or one more review to confirm the approach. Thanks for working on this!
| /// <summary> | ||
| /// Makes sure the target qubit of an operation is valid. In particular it checks that the qubit instance is not null. | ||
| /// </summary> | ||
| void CheckQubit(Qubit q1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason that the "if (q == null) throw new ArgumentNullException..." in CheckQubitInUse above is not just replace with a call to CheckQubit? Then all the adaptions in CheckQubits would not be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsure of why it is in the current state. However, before making a change around this, I would let the original author comment on this, as this was as is when I looked at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't here back, please add that change - I will be to blame for any resulting issues. :)
src/Simulation/Simulators.Tests/QuantumSimulatorTests/QubitReleaseTest.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this. This is one is going to make simple intro programs a lot easier to understand!
Changes overall look good. I didn't remember we actually did this check on the QubitManager, I was pretty sure it was on the C++ code. This makes things even simpler.
The idea of using CheckQubit to mark when a Qubit is used is really smart, however there a couple of scenarios that we should consider:
- Releasing a Qubit measured as |1> after calling operations like
DumpRegisterwhich callCheckQubitbut should have no effect. This will throw:
using (q = Qubit()) {
X(q);
if (M(q) == One) {
DumpRegister("", [q]);
}
}
- Similarly using a Qubit measured as |1> as a controlled bit. In this case throws although we know the qubit is not entangled:
using (q = Qubit[2]) { X(q[0]); if (M(q[0]) == One) { CNOT(q[0], q[1]); } } - Releasing a Qubit measured in a different base, the qubit is still in super-position on the Z bases but it doesn't throw:
using (q = Qubit()) { H(q); let _ = Measure([PauliX], [q]); }
I think it might be ok for the last two scenarios to behave as they do but please check with Bettina if this behavior is ok.
Whatever we decide, please make sure you add unittests for all of these to make sure we don't change behavior in the future (or if we do, we do it knowingly).
Great job so far!
Quick comment, but this should be entirely OK, as the qubit is still in a deterministically known state at the point that it is released, such that appropriate instructions can be automatically added to reset it to the |0⟩ state. |
|
For a bit of extra context, Aniket already found that the simulator handles release of qubits properly. See this code from the simulator.hpp elsewhere the repo: When the qubit is released, the simulator will check if it's in a classical value (defined as |0> or |1> in the Z-basis), and if not, it performs a measure. That measure has code to explicitly collapses the state. Then it returns the boolean indicating whether the qubit was originally in a classical state and whether that classical state was |0>, then the higher level code in QubitManager would throw. So the simulator doesn't actually care and will handle the qubit no matter what state it is in, it's just a matter of what the QubitManager will allow, hence the fix that Aniket is building. |
|
@swernli, yes, the simulator works correctly, and there is an option to disable this behavior (is just on by default). |
Agreed, but there's also value in that avoid reset by coherently unpreparing qubits is a common and useful optimization. For example, the target qubit in Deutsch–Jozsa is deterministically in |−⟩ after the query, such that an |
|
Agreed on all points. I was just clarifying the simulator behavior because of some phrasing; as we are discussing this issue, we keep talking about what is "ok" or what "works" but any and all of these approaches can work, it's about what patterns we want to allow and encourage. So the question we should be asking ourselves is: what do we want the simulator to teach people about how to manage their qubits? What usage patterns do we want the simulator to reject so that it forces developers to adjust their programs? The question of whether or not to clear the |
Suppose after a measurement, the qubit Put differently, the word "control" in the context of a controlled quantum operation is at most a mnemonic. A two-qubit operation can in general change the joint state of both qubits, even if one is only a "control." You can even reverse which qubit is a control and which is a target using within {
H(left); H(right);
} apply {
CNOT(right, left);
}
// is equivalent to
CNOT(left, right);From that perspective, I would strongly argue that |
|
That's an excellent explanation, thanks @cgranade ! |
Thanks, happy to help! |
|
I have to say, this is an exciting change from the libraries perspective! We've gotten feedback about the variety of different ways that measurement is used in the libraries, and in particular, that it's not clear when a reset is needed and not without looking at detailed API docs. This change would allow us to adopt the convention that measurements other than MReset* do not reset ever. |
|
After looking at this thread, I am attempting to summarize the required changes:
|
|
That summary looks good to me (plus the renaming feedback which you already mentioned you'd be addressing). |
That's correct; control qubits aren't special, such that the postcondition represented by |
|
@anpaz-msft Could you please take another look whether the changes you requested have been addressed? |
anpaz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great discussion. One final thought:
Can you please add a test that after a Qubit is released and re-allocated, it starts on |0>. I suspect that it might get re-allocated on the state it was when released (i.e. '|1>`).
* Change to allow relese of qubit if measured * added comments to change to allow relese of qubit if measured * Changes to reflect PR review * Changes to add documentation as per PR review * Test renamed to reflect PR review * Updated summary of CheckQubit functions to reflect change in isMeasured flag * Added test for checking state of reallocated qubit Co-authored-by: Aniket Dalvi <t-anikda@microsoft.com> Co-authored-by: bettinaheim <34236215+bettinaheim@users.noreply.github.com>
This PR is for a change I made to allow a qubit to be released if it has been measured and no operation has been made on it after the measurement. This involved adding an isMeasured field to Qubit, and setting its values appropriately. I also added 2 unit tests to confirm the desired behavior.