qiboteam / qibojit-benchmarks

Benchmark code for qibojit performance accessment
Apache License 2.0
2 stars 3 forks source link

Fix CU3 gate #21

Closed andrea-pasquale closed 2 years ago

andrea-pasquale commented 2 years ago

In this PR I've fixed the CU3 test in #20 which was failing for the qulacs and qiskit libraries. Regarding the qulacs library the problem was a small typo which was causing the error. Instead for qiskit, as I suspected in #20, the problem is related to the different definitions of the U3 gate, and consequently of the CU3 gate, between qibo and qiskit. I've fixed this issue by using the CUGate in qiskit, which consists in a generalization of the CU3 gate, with an additional parameter gamma, which introduces an overall phase in the target qubit. By choosing an appropriate value for gamma, depending on the parameters of the CU3 gate, I was able to recover the definition of the U3 gate of qibo in qiskit. Maybe the implementation could be improved, but at least now we know what was causing the tests to fail. Let me know what you think.

stavros11 commented 2 years ago

Thank you for the fixes, looks that both work.

Concerning qiskit, thanks for finding the problem, however I'm not sure that fixing the test by changing the gate (only in the test) is the best option, given that it doesn't change Qiskit's behavior during benchmarks. By doing so, we would have a test that passes only because we make a ad-hoc change in the test. Don't know how to fix, though. @stavros11 what do you think?

I fully agree with this, it would be better to incorporate the fix inside the qiskit backend instead of the tests, so that it is propagated in the benchmarks. A potential, not very clean but hopefully working, way to do this would be to modify the libraries.qiskit.QiskitDefault.from_qasm method to something like the following:

def from_qasm(self, qasm):
    if "cu3" in qasm:
        qasm = qasm.replace("cu3(...)", "cu(...)")
    return self.QuantumCircuit.from_qasm_str(qasm)

The ... part will require some string manipulations in order to extract the parameter values from cu3, apply the fix, and pass the new parameters to cu. @andrea-pasquale let me know if you like to give this a try, otherwise I can also do so.

andrea-pasquale commented 2 years ago

Thank you @mlazzarin and @stavros11 . I agree that this fix should be moved inside the qiskit backend. My first attempt was just to see if it was possible to fix the problem. Also I wasn't sure on how to implement the change directly in the qiskit backend. I can try to implement @stavros11 's suggestion and I will let you know if it works.

andrea-pasquale commented 2 years ago

I've implemented the suggestion. Let me know if you agree @stavros11 @mlazzarin .

andrea-pasquale commented 2 years ago

Thank you. I've removed the unused lines. I've also resolved the conflicts we are ready to merge now.

stavros11 commented 2 years ago

Thank you. I've removed the unused lines. I've also resolved the conflicts we are ready to merge now.

Great, thanks. All looks good to me, feel free to merge.