CommutativeCancellation performs incorrect commutation

Information

Qiskit Terra version: 0.15.2
Python version: 3.8.2
Operating system: Windows 10 (running through WSL)

What is the current behavior?

The CommutativeCancellation pass modifies some circuits in an incorrect way, which can change both state vector snapshots and measurement outcomes. This happens also when using the transpiler with optimization_level=2 or above, as the pass is then applied.

Steps to reproduce the problem

This circuit seems a bit arbitrary, but it the smallest one that I have found that reproduces the bug.

from qiskit import QuantumRegister,ClassicalRegister,QuantumCircuit,execute,Aer
from qiskit.transpiler import PassManager
from qiskit.transpiler.passes import CommutativeCancellation
import numpy as np
from IPython.core.display import display

circuit = QuantumCircuit(2)
circuit.u1(np.pi/2, 1)
circuit.u2(0, np.pi, 0)
circuit.cz(0, 1)
circuit.iswap(0, 1)
circuit.cz(0, 1)
circuit.u1(np.pi/2, 1)
circuit.snapshot('middle', snapshot_type='statevector')

display(circuit.draw(output='mpl'))

circuit_new = PassManager(CommutativeCancellation()).run(circuit)
display(circuit_new.draw(output='mpl'))

results1 = execute(circuit, Aer.get_backend('qasm_simulator')).result()
results2 = execute(circuit_new, Aer.get_backend('qasm_simulator')).result()

state1 = results1.data()['snapshots']['statevector']['middle'][0] 
state2 = results2.data()['snapshots']['statevector']['middle'][0]
print('state1 ',np.round(state1,decimals = 3))
print('state2 ',np.round(state2,decimals = 3))

Output:

state1  [ 0.707-0.j, -0. +0.j, -0.707+0.j, -0. +0.j]
state2  [ 0.707-0.j, -0. +0.j,  0.+0.707j,  0. +0.j]

The pass gives an unwanted phase in the |10> state. Note that the iSWAP and CZ gates commute, and so the CZs really cancel. If you remove them (or any other gate), the bug stops occurring.

The effect is more pronounced if you add the inverse of the above circuit and measurements. The opposite bug happens to the inverse circuit which causes the state to become |01> instead of |00>. If you replace circuit.snapshot('middle', snapshot_type=snapshot_type) with

inv = circuit.inverse()
circuit.snapshot('middle', snapshot_type='statevector')
circuit += inv
circuit.snapshot('end', snapshot_type='statevector')
circuit.measure_all()

And print the states and measurement counts, you get

middle state1  [ 0.707-0.j, -0.+0.j, -0.707+0.j, -0. +0.j]
middle state2  [ 0.707-0.j, -0.+0.j,  0.+0.707j,  0. +0.j]
end state1  [1.-0.j, 0.+0.j, 0.+0.j, 0.+0.j]
end state2  [0.+0.j, 1.-0.j, 0.-0.j, 0.-0.j]
counts circuit {'00': 1024}
counts transpiled circuit {'01': 1024}

Note how this now even affects the measurement outcomes.

Note also that the CommutativeCancellation pass requires CommutationAnalysis, so the bug may lie in either one. CommutationAnalysis "determines commutativity through matrix multiplication", so if this happens for the iSWAP gate it can probably happen on other ones too.

What is the expected behavior?

Transpilation should not change the behavior of the circuit (except for perhaps commuting the qubit ordering, but that's not relevant here), especially not measurements.

Qiskit / qiskit