Use Operator rather than unitary simulator to convert circuit to unitary matrix #1224

jlapeyre · 2020-08-30T18:02:52Z

Summary

This PR makes (for example) qiskit.aqua.operators.Z.exp_i().to_matrix() return the correct matrix. It does two things:

Z.exp_i() previously returned a circuit containing a Z-rotation gate, but with angle incorrect by a factor of 2. This PR uses the correct factor. The PR corrects the same erroneous angle for X and Y rotations.
The conversion of CircuitOp to a matrix used the Aer unitary simulator. It returns the wrong matrix by a global phase. This is probably due to rough edges in the new global phase support in Terra. qiskit.quantum_info.Operator will also convert a circuit to a matrix, if possible. It does account for the global_phase property.

~~This PR still lacks a test and release note.~~
EDIT: tests have been added.
EDIT: release note has been adde.

The unitary simulator does not account for the global phase in a circuit. qiskit.quantum_info.Operator does account for global phase when converting (when possible) a circuit to a unitary matrix. Closes qiskit-community#1218

jlapeyre · 2020-09-04T15:57:20Z

This should be GTG.

Cryoris

LGTM 👍🏼 about the TODO/NOTE comments, are they still open or could
you check if they can be removed?

jlapeyre · 2020-09-04T18:00:57Z

about the TODO/NOTE comments, are they still open

I'll check. This should probably be resolved before committing.

qiskit/aqua/operators/primitive_ops/circuit_op.py

t-imamichi · 2020-09-09T09:30:01Z

We discussed BasicAer in the past https://github.com/Qiskit/qiskit-aqua/issues/1003. I remember there were some concerns. Is it clear now? I made a PoC for the discussion #1009.

woodsp-ibm · 2020-09-09T11:26:36Z

@t-imamichi Are you asking is it clear about the removal of BasicAer etc as was originally discussed in the issue and PoC PR? To my knowledge nothing further has happened. This PR is just about addressing some internal logic, where the issue as stated is incorrect global phase in the result - I think internal use within Aqua was aspect 2 in your original issue - where it uses BasicAer say for statevector or unitary conversion. Maybe more internal aspects could be changed if it was lower overhead, better performance etc. This was one such internal use where the outcome was just not correct and noticed as part of the phase estimation work and needed to be addressed.

t-imamichi · 2020-09-09T13:35:36Z

Thanks. I got it.

jlapeyre · 2020-09-11T17:35:44Z

Sorry, @t-imamichi I missed your comment till just now.

I didn't make the connection between your #1003 and the present PR until now.
I'm not sure what your question here is. But, maybe this will help make it more clear: The use of BasicAer here resulted in an incorrect matrix. I was looking for the most straightforward way to fix this. Browsing around some comments in Terra, it seemed to me that using quantum_info.Operator is the currently sanctioned way to convert a unitary circuit to a matrix. I tried this, and it also had the happy consequence of returning the correct matrix. I haven't thought about whether replacing BasicAer more broadly, as in your PoC, is a good idea at the moment. But, unless there is something I am overlooking (which might be) it seems that making the change here is a good idea, because it fixes a bug, and fits with direction we want to take Terra and Aqua in any case.

jlapeyre · 2020-09-11T18:04:14Z

LGTM 👍🏼 about the TODO/NOTE comments, are they still open or could
you check if they can be removed?

Yes, they can be removed.
I checked in a case in which BasicAer does give the correct result Z ^ Y. The two methods give the same matrix: $Z \otimes Y$. Printing the circuit shows Y on qubit 0 and Z on qubit 1.

EDIT: The comments have been removed

Using Operator(QuantumCircuit) gives the same qubit ordering that the previous unitary-simulator-based code did. I removed the comment entirely because the behavior depends only on Operator(QuantumCircuit), which is not obscure.

t-imamichi · 2020-09-12T10:01:37Z

@jlapeyre Sorry for confusing you. This PR does not have the problem we discussed in #1003.

…ary matrix (qiskit-community#1224) * Fix factor of 2 error in converting exp_i to rotation gates * Make CircuitOp use Operator rather than unitary simulator to get matrix The unitary simulator does not account for the global phase in a circuit. qiskit.quantum_info.Operator does account for global phase when converting (when possible) a circuit to a unitary matrix. Closes qiskit-community#1218 * Remove note on reversing order of qubits Using Operator(QuantumCircuit) gives the same qubit ordering that the previous unitary-simulator-based code did. I removed the comment entirely because the behavior depends only on Operator(QuantumCircuit), which is not obscure.

…ary matrix (qiskit-community/qiskit-aqua#1224) * Fix factor of 2 error in converting exp_i to rotation gates * Make CircuitOp use Operator rather than unitary simulator to get matrix The unitary simulator does not account for the global phase in a circuit. qiskit.quantum_info.Operator does account for global phase when converting (when possible) a circuit to a unitary matrix. Closes qiskit-community/qiskit-aqua#1218 * Remove note on reversing order of qubits Using Operator(QuantumCircuit) gives the same qubit ordering that the previous unitary-simulator-based code did. I removed the comment entirely because the behavior depends only on Operator(QuantumCircuit), which is not obscure.

Fix factor of 2 error in converting exp_i to rotation gates

384f567

jlapeyre requested review from manoelmarques and woodsp-ibm as code owners August 30, 2020 18:02

jlapeyre requested a review from Cryoris August 30, 2020 18:03

jlapeyre force-pushed the pauli-expi branch 3 times, most recently from 39cda1b to 466bd48 Compare August 31, 2020 16:56

jlapeyre force-pushed the pauli-expi branch from 78c332a to 4ed804e Compare September 4, 2020 15:53

jlapeyre changed the title ~~[WIP] Use Operator rather than unitary simulator to convert circuit to unitary matrix~~ Use Operator rather than unitary simulator to convert circuit to unitary matrix Sep 4, 2020

Merge branch 'master' into pauli-expi

c03c597

Cryoris previously approved these changes Sep 4, 2020

View reviewed changes

woodsp-ibm reviewed Sep 5, 2020

View reviewed changes

qiskit/aqua/operators/primitive_ops/circuit_op.py Outdated Show resolved Hide resolved

woodsp-ibm added the Changelog: Bugfix Include in the Fixed section of the changelog label Sep 5, 2020

woodsp-ibm added this to the 0.8 milestone Sep 5, 2020

Merge branch 'master' into pauli-expi

0c00019

jlapeyre requested a review from levbishop as a code owner September 11, 2020 17:42

Remove note on reversing order of qubits

0bba1eb

Using Operator(QuantumCircuit) gives the same qubit ordering that the previous unitary-simulator-based code did. I removed the comment entirely because the behavior depends only on Operator(QuantumCircuit), which is not obscure.

jlapeyre dismissed Cryoris’s stale review via 0bba1eb September 11, 2020 19:03

jlapeyre requested review from pbark and stefan-woerner as code owners September 11, 2020 19:03

Merge branch 'master' into pauli-expi

50da512

woodsp-ibm approved these changes Sep 11, 2020

View reviewed changes

Merge branch 'master' into pauli-expi

c9e825e

jlapeyre merged commit f059f99 into qiskit-community:master Sep 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Operator rather than unitary simulator to convert circuit to unitary matrix #1224

Use Operator rather than unitary simulator to convert circuit to unitary matrix #1224

jlapeyre commented Aug 30, 2020 •

edited

Loading

jlapeyre commented Sep 4, 2020

Cryoris left a comment

jlapeyre commented Sep 4, 2020

t-imamichi commented Sep 9, 2020 •

edited

Loading

woodsp-ibm commented Sep 9, 2020

t-imamichi commented Sep 9, 2020

jlapeyre commented Sep 11, 2020

jlapeyre commented Sep 11, 2020 •

edited

Loading

t-imamichi commented Sep 12, 2020

Use Operator rather than unitary simulator to convert circuit to unitary matrix #1224

Use Operator rather than unitary simulator to convert circuit to unitary matrix #1224

Conversation

jlapeyre commented Aug 30, 2020 • edited Loading

Summary

jlapeyre commented Sep 4, 2020

Cryoris left a comment

Choose a reason for hiding this comment

jlapeyre commented Sep 4, 2020

t-imamichi commented Sep 9, 2020 • edited Loading

woodsp-ibm commented Sep 9, 2020

t-imamichi commented Sep 9, 2020

jlapeyre commented Sep 11, 2020

jlapeyre commented Sep 11, 2020 • edited Loading

t-imamichi commented Sep 12, 2020

jlapeyre commented Aug 30, 2020 •

edited

Loading

t-imamichi commented Sep 9, 2020 •

edited

Loading

jlapeyre commented Sep 11, 2020 •

edited

Loading