Adding Qsim confusion matrix runtime exception #630
Conversation
qsimcirq/qsim_simulator.py
Outdated
| param_resolver = param_resolver or cirq.ParamResolver({}) | ||
| solved_circuit = cirq.resolve_parameters(circuit, param_resolver) | ||
|
|
||
| return self._sample_measure_results(solved_circuit, repetitions) | ||
|
|
||
| def _check_for_confusion_matrix(self, circuit: cirq.Circuit): |
There was a problem hiding this comment.
I recommend moving this to qsim_circuit.py and calling it in QSimCircuit.translate_cirq_to_qsim instead. Every Cirq circuit that gets converted to qsim must go through that method, so putting it there would save having to call it from every simulation method in this file.
There was a problem hiding this comment.
I moved the method to QSimCircuit and updated the unit test accordingly. However, I want to note the invocation pattern seen here: quantumlib/Cirq#6305 does not result in an exception being thrown.
There was a problem hiding this comment.
the invocation pattern seen here: quantumlib/Cirq#6305 does not result in an exception being thrown.
That's deeply surprising to me. Could you try adding the check in translate_cirq_to_qtrajectory as well?
Line 438 in 401ebec
There was a problem hiding this comment.
I added the check here as well and the ValueError still wasn't raised. Your surprise makes me want to make sure my environment doesn't have anything odd/wonky. Let me get a summary of my env (python version qsim ver, cirq ver) and follow up
There was a problem hiding this comment.
Python version: 3.9
cirq==1.2.0
qsimcirq==0.16.3
There was a problem hiding this comment.
How was this resolved?
There was a problem hiding this comment.
I confirmed that there was nothing wonky with my setup or library versions and that indeed in neither translate_cirq_to_qsim or translate_cirq_to_qtrajectory is a part of the call stack when the circuit is executed like so
qsimcirq.QSimSimulator(seed=0).run(circuit, repetitions=100)
There was a problem hiding this comment.
Ohhhhhh, I missed that you had moved the check into the constructor. LGTM.
(I'm still confused by the translate_* methods not being called, as these lines should call them, but it's no longer blocking this PR.)
angelo-laskaris
force-pushed
the
qsim_confusion_matrix_exception
branch
from
cf1110e to
401ebec
Compare
|
Adding black formatter to my IDE, rebasing and then posting a commit on top of this one |
…cified in a cirquit and Qsim is the target simulator a ValueError Exception will be thrown until confusion maps for Measurement Gates is supported in QSim.
angelo-laskaris
force-pushed
the
qsim_confusion_matrix_exception
branch
from
401ebec to
0f04453
Compare
|
@95-martin-orion The one test that failed doesn't seem to be related to the change. Is this test ever flaky? |
|
Hmmm. This seems related to this SO answer, as the logs indicate that python 3.12.0 was installed for the test. I'm not quite sure how this happened, though, seeing as both setup.py and the Github Actions workflow specify a maximum python version of 3.11. It's possible we need to pin our python version in the test workflow. |
|
Opened #632 to track the issue. |
|
#632 is fixed - I'm going to go ahead with the v0.17.0 release to unblock some things, but I'm happy to cut a v0.17.1 once this is merged. |
Okay sounds good! |
|
@95-martin-orion Are there any further concerns or can we LGTM? |
Sorry about the delay - this dropped off my radar. I've unresolved one comment above that I'd like clarity on, otherwise this is good to go. I'll follow up with a release shortly after. |
No worries! Followed up on your comment |
QSim currently does not support confusion matrices on measurement gates. See quantumlib/Cirq#6305 for more info. While work is done to support confusion matrices surfacing a runtime exception.