Update Simulator Release to check for entanglement #710
Conversation
This change adds a new utility to the full state simulator, `is_classical_or_entangled` that returns a pair of bools indicating when the given qubit is classical with regard to the computational basis and whether the qubit is entangled with any other qubits. The former is needed to ensure the state of the qubit is collapsed before release, while the latter is the correct feedback to return from release to indicate whether releasing that qubit had any side effects on other qubits in the wave function. This updates the logic in the C# infrastructure to use the returned bool as the indication of whether or not the release was valid (as opposed to the old check that used a cache of whether or not the last operation on the qubit was a measurement). THIS IS A CHANGE IN BEHAVIOR. Notably, a qubit that is now considered valid for release as long as it is not entangled with any other qubit, regardless of superposition. As a result, the exception name and text for `ReleasedQubitsAreNotInZeroState` is changed to `ReleaseQubitsAreEntangled`. This also fixes QIR simulator should check result of qubit Release #552 by having the FullstateSimulator wrapper in the QIR Runtime check the return value from release and call `quantum__rt__fail_cstr` if the qubit released was entangled.
swernli
requested review from
DmitryVasilevsky,
ScottCarda-MS,
bamarsha,
bettinaheim,
cesarzc,
cgranade,
dbwz8,
kuzminrobin,
tcNickolas and
thomashaener
dbwz8
left a comment
dbwz8
left a comment
There was a problem hiding this comment.
It's hard to tell if the logic for entanglement is correct by inspection... but since the tests you wrote should catch any of the errors I can think of, I see no reason to block the change.
|
Worth noting with this change: because it asserts that any qubit that is not entangled is valid for release, it assumes that on hardware qubits are either explicitly or implicitly reset on release OR every qubit is explicitly reset on allocate. This would be needed to match the Q# assertion that every freshly allocated qubit shows up in the ground state. While this matches behavior for our currently supported hardware partners, it's worth underscoring in case anyone sees that as a problematic assumption going forward. |
|
Looks like |
|
On second thought, looking at how samples uses the exception, I can simply leave the existing type with a deprecation. I'll go that route instead. |
|
@dbwz8 It turns out the entanglement check can be further simplified! Because of the way we are iterating over the wave function, we are always checking the two states where the only difference is the qubit in question. If both of those have non-zero probability, that means there is a chance of measuring the identical values for every other qubit regardless of the state of the qubit we care about, which by definition means it cannot be entangled with any of the other qubits. If we never see that case, then it must mean that every measurement of the qubit we care about corresponds to a specific measurement of at least one other qubit. The check I was doing before (comparing the two indices for differences other than the target qubit) was redundant, because in the case where the probabilities were non-zero (the case we care about) that check would always be true. |
|
@kuzminrobin thanks for the review and feedback! Especially the observation about the use of |
Sorry for the delay, been a bit head down trying to fix the e2e build for #709, and got behind on this one. Anyway, I'm in the awkward position of thinking that both the old and the new behavior makes sense... effectively, my thinking comes back to that when a user allocates a qubit with Apologies for the incoherent (pun fully intended) wall of text, just my 2¢ from an initial pass. |
|
No worries at all, @cgranade, I definitely wasn't expecting a response over the weekend!
Making it configurable is a really interesting idea, and stands to reason that would allow the simulation to meet the needs of a broader audience. In fact, the C# simulator already has a setting that can disable this check entirely, which QIR simulation infrastructure does not replicate. Just thinking out loud here, the measurement tracking felt like it was a stand in for entanglement (we know a measured qubit couldn't be entangled) but was already weaker than the zero check. If we have |
|
I've converted this to a draft while we work out the questions @cgranade brought up about configurability. That said, #552 should be considered a critical issue for QIR-based simulation, so we should aim to have this included in the next release if at all possible. @bettinaheim @cesarzc FYI. |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
@cgranade Regarding configuration for simulation, there is still an outstanding general question about how we want to support configuring QDK, ranging from settings in the qubit manager to this qubit release behavior and even noise models for the experimental open simulator. I believe both @DmitryVasilevsky and @kuzminrobin have started looking into this and might want to weigh in for this part. |
bettinaheim
left a comment
bettinaheim
left a comment
There was a problem hiding this comment.
I think this change makes sense. I did look at the test cases and think they cover everything.
|
|
||
| namespace Microsoft.Quantum.Simulation.Simulators.Exceptions | ||
| { | ||
| [Obsolete("This class is deprecated and will be removed in a future release.")] |
There was a problem hiding this comment.
Do you want to refer to the ReleasedQubitsAreEntangled exception?
| return true; | ||
| } | ||
|
|
||
| template <class T, class A> |
There was a problem hiding this comment.
A doc comment here would be helpful. The naming "is classical" is a bit confusing to me, since aside from in the simulator, I don't think the choice of basis should matter. It being "in computational basis" seems more accurate to me, but then that would be awfully long as name...
|
@cgranade What is not clear to me is the value of requiring that a qubit is measured (i.e. in the 0,1 basis) rather than in any unentangled state. The whole idea of these checks was that it is completely fine to measure a qubit after it is release (so it can be reused) without that changing the algorithm. What am I missing - why would the stricter condition be beneficial? |
|
After some offline discussion, the concern is captured below. cgranade: bettinaheim: cgranade: |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
This PR is superseded by #796 |
This change fixes what seems to be a subtle bug in the syncrhonization mechanisms for the simulator. In particular, we have seen intermediate failures in the simulator tests that seem to be the result of a simulator thread stepping on another thread. As @kuzminrobin pointed out in #710 (comment), this could be due to the fact that the mutex access method, `mutex()` has the same signature as the standard constructor for mutex. This could mean that if the compilation environment has a `using namespace std;` anwhere in it that the calls to `mutex()` would allocate a fresh mutex rather than access the member mutex, meaning none of the threads are synchronized as expected. This avoid the problem by renaming the accessor to `getmutex()`.
* Fixing use of `mutex()` to avoid subtle bug This change fixes what seems to be a subtle bug in the syncrhonization mechanisms for the simulator. In particular, we have seen intermediate failures in the simulator tests that seem to be the result of a simulator thread stepping on another thread. As @kuzminrobin pointed out in #710 (comment), this could be due to the fact that the mutex access method, `mutex()` has the same signature as the standard constructor for mutex. This could mean that if the compilation environment has a `using namespace std;` anwhere in it that the calls to `mutex()` would allocate a fresh mutex rather than access the member mutex, meaning none of the threads are synchronized as expected. This avoid the problem by renaming the accessor to `getmutex()`. * Update calls in macro
* Check qubit release/measurement status on release This mimics the same behavior in the QIR Runtime wrapper for the fullstate simulator as what we have for C#, namely that a qubit is valid for release if and only if it is either in the ground state or the last operation on that qubit was measure. This fixes #552, and supersedes #710. * CR feedback * Revert mutex change
This change adds a new utility to the full state simulator,
is_classical_or_entangledthat returns a pair of bools indicating when the given qubit is classical with regard to the computational basis and whether the qubit is entangled with any other qubits. The former is needed to ensure the state of the qubit is collapsed before release, while the latter is the correct feedback to return from release to indicate whether releasing that qubit had any side effects on other qubits in the wave function.This updates the logic in the C# infrastructure to use the returned bool as the indication of whether or not the release was valid (as opposed to the old check that used a cache of whether or not the last operation on the qubit was a measurement). THIS IS A CHANGE IN BEHAVIOR. Notably, a qubit is now considered valid for release as long as it is not entangled with any other qubit, regardless of superposition. As a result, the exception name and text for
ReleasedQubitsAreNotInZeroStateis changed toReleaseQubitsAreEntangled.This also fixes #552 by having the FullstateSimulator wrapper in the QIR Runtime check the return value from release and call
quantum__rt__fail_cstrif the qubit released was entangled.