Skip to content
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

Bug fix in ElidePermutations #13186

Merged
merged 2 commits into from
Sep 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions qiskit/transpiler/passes/optimization/elide_permutations.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,8 @@ def _apply_mapping(qargs):
elif isinstance(node.op, PermutationGate):
starting_indices = [qubit_mapping[dag.find_bit(qarg).index] for qarg in node.qargs]
pattern = node.op.params[0]
pattern_indices = [qubit_mapping[idx] for idx in pattern]
for i, j in zip(starting_indices, pattern_indices):
updated_indices = [starting_indices[idx] for idx in pattern]
for i, j in zip(starting_indices, updated_indices):
qubit_mapping[i] = j
input_qubit_mapping = {qubit: index for index, qubit in enumerate(dag.qubits)}
self.property_set["original_layout"] = Layout(input_qubit_mapping)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
fixes:
- |
Fixed a bug in the transpiler pass :class:`~.ElidePermutations` where the
qubit mapping was not updated correctly in the presence of :class:`.PermutationGate`\s.
31 changes: 31 additions & 0 deletions test/python/transpiler/test_elide_permutations.py
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,37 @@ def test_permutation_in_middle(self):
res = self.swap_pass(qc)
self.assertEqual(res, expected)

def test_partial_permutation_in_middle(self):
"""Test with a permutation gate in the middle of a circuit,
with the permutation gate defined only on a subset of qubits.
"""
qc = QuantumCircuit(5)
qc.cx(0, 1)
qc.append(PermutationGate([1, 2, 0]), [0, 2, 4])
qc.cx(2, 3)

# The permutation corresponding to the permutation gate maps
# 2 -> 0, 4 -> 2, 0 -> 4, and 1 -> 1, 3 -> 3.
# Instead of the permutation gate, we can relabel the qubits
# 0 -> 2, 1 -> 1, 2 -> 4, 3 -> 3, 4 -> 2.
# Hence cx(2, 3) becomes cx(4, 3).
expected = QuantumCircuit(5)
expected.cx(0, 1)
expected.cx(4, 3)

pass_ = ElidePermutations()
res = pass_(qc)

# Make sure that the pass removes the permutation gate and remaps
# the following gate
self.assertEqual(res, expected)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add some assertions on the permutation tracking too to make sure it's correct and reverses to the original circuit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good idea. I have extended the test in 6791002. Do you think it's a good idea to add more "random circuits" tests, with swaps and permutations?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding more tests wouldn't hurt. I think the one you added here provides sufficient coverage to prevent a regression here. But if you wanted to add more that would give us more coverage against potential future issues.


# Make sure that the transpiled circuit *with* the final permutation
# is equivalent to the original circuit
perm = pass_.property_set["virtual_permutation_layout"].to_permutation(qc.qubits)
res.append(PermutationGate(perm), [0, 1, 2, 3, 4])
self.assertEqual(Operator(res), Operator(qc))
Comment on lines +237 to +239
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH, I'm surprised you didn't just use Operator.from_circuit here, but adding a permutation gate like this works fine too.


def test_permutation_at_beginning(self):
"""Test permutation in beginning of bell is elided."""
qc = QuantumCircuit(3, 3)
Expand Down
Loading