-
Notifications
You must be signed in to change notification settings - Fork 584
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds support for CCZ and CCX/TOF gates. #487
base: master
Are you sure you want to change the base?
Conversation
@jaeyoo Would you be able to take a look at this issue ? It seems like the ParameterShift differentiator is breaking with adding the new gates. I did some investigation and I think it may have actually been broken by one of the recent PRs we did (since it still breaks even without the new gates sometimes if I increase the depth of the circuits). Do you think you would be able to take a look and see if you can spot the fix ? |
Let me look at it during weekend, and let's talk about it at our next sync. |
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.
Good news! I found out the fix.
@@ -75,6 +75,20 @@ void CreateGradientCircuit( | |||
grad_gates->push_back(grad); | |||
} | |||
|
|||
// Three qubit Eigen. | |||
else if (circuit.gates[i].kind == qsim::Cirq::GateKind::kCCZPowGate || | |||
circuit.gates[i].kind == qsim::Cirq::GateKind::kCCXPowGate) { |
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.
Good news! I fixed it Mike. The culprit was qsim::Cirq::GateKind::kCCX for Toffoli gate. It has its own enum value. :)
Please add the following line:
else if (circuit.gates[i].kind == qsim::Cirq::GateKind::kCCZPowGate ||
circuit.gates[i].kind == qsim::Cirq::GateKind::kCCXPowGate ||
circuit.gates[i].kind == qsim::Cirq::GateKind::kCCX) {
and then I saw the following test passed.
bazel test tensorflow_quantum/python/differentiators:gradient_test
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.
Unfortunately I don't think that is the fix :(. Like we talked about on our sync it is the parametershift differentiator that is having issues, not the adjoint differentiator. The tests fail somewhat randomly and unpredictably so If you can get the parametershift test to pass with --runs_per_test=50
and with many more circuits then I think that would be enough to say that the bug is fixed. When I run with this change I can still get the error if I change the number of circuits and ops.
Oops, right, thank you for the catch. I am working on it, and found out the broken case in ParameterShift. |
Hi Mike, I want to ask you some questions. I am trying to figure out the culprit circuit, but it seems not yet supported for multiple controlled qubit ops.
and got this error:
Any help please? |
Hmmm is |
Is there any update on this? |
…ulators Update unitary calculators.
This PR adds serialization and C++ implementation support for three qubit eigen gates. A lot of this code is copy pasted from the two qubit cases with tweaks to work for the three qubit cases. Currently tests will fail on our unitary op since the latest qsim (https://github.com/quantumlib/qsim/blob/278b217f94bca179ac5fafd85498ee28bbd3e236/lib/unitary_calculator_basic.h#L52) unitary calculator doesn't support three qubit gates. This is being worked on by @sergeisakov so we should be able to merge very soon once we can upgrade our qsim dependency. Fixes #480