Update Split2QUnitaries to handle SWAP unitary gates #13531

gadial · 2024-12-05T15:28:05Z

Summary

Updates the Split2QUnitaries transpiler pass to handle 2-qubit unitary gates which can be decomposed as a swap gate and 1-qubit gates.

Closes #13476

Details and comments

This PR extends the Split2QUnitaries in the following manner:

During the initial pass Split2QUnitaries performs, it checks whether any unitary encountered is a SWAP gate.
If SWAP gates were found, another pass is performed on the DAG, replacing SWAP gates by the 1-qubit unitary gates in their decomposition (as is currently done in Split2QUnitaries) and instead of adding SWAP gates, performs a virtual swap by dynamically changing the qubit layout (affecting subsequent gates and the final layout) similar to what the ElidePermutations does.

Since step 2 requires generating a new DAG (as opposed to what Split2QUnitaries does to identity-equivalent unitary gates), it is not performed during the initial pass, only if SWAP gates were detected.

qiskit-bot · 2024-12-05T15:28:10Z

One or more of the following people are relevant to this code:

@Qiskit/terra-core

coveralls · 2024-12-05T15:52:49Z

Pull Request Test Coverage Report for Build 13158018594

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

133 of 139 (95.68%) changed or added relevant lines in 3 files are covered.
823 unchanged lines in 35 files lost coverage.
Overall coverage decreased (-0.3%) to 88.66%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/transpiler/passes/optimization/split_2q_unitaries.py	15	16	93.75%
qiskit/transpiler/preset_passmanagers/builtin_plugins.py	4	5	80.0%
crates/accelerate/src/split_2q_unitaries.rs	114	118	96.61%

Files with Coverage Reduction	New Missed Lines	%
qiskit/synthesis/linear/linear_depth_lnn.py	1	90.91%
qiskit/providers/fake_provider/fake_backend.py	1	85.51%
qiskit/providers/basic_provider/basic_provider_tools.py	1	83.33%
crates/circuit/src/operations.rs	1	89.61%
crates/accelerate/src/split_2q_unitaries.rs	1	96.27%
qiskit/pulse/transforms/canonicalization.py	1	93.44%
qiskit/pulse/transforms/base_transforms.py	2	86.96%
qiskit/dagcircuit/collect_blocks.py	2	98.34%
qiskit/transpiler/passes/optimization/collect_cliffords.py	2	92.0%
qiskit/pulse/schedule.py	3	88.6%

Totals
Change from base Build 12905478077:	-0.3%
Covered Lines:	79062
Relevant Lines:	89174

💛 - Coveralls

mtreinish

Thanks for doing this, from a quick glance the code looks good (but I only quickly skimmed it on my phone, I'll do a deeper reviewer later). But I had some quick comments on the tests to start.

mtreinish · 2024-12-05T23:18:26Z

test/python/transpiler/test_split_2q_unitaries.py

+    def test_2q_swap_with_1_qubit_gates(self):
+        """Test that a 2q unitary matching a swap gate with 1-qubit gates before and after is correctly processed."""
+        qc = QuantumCircuit(2)
+        qc.h(0)
+        qc.x(1)
+        qc.swap(0,1)
+        qc.sx(0)
+        qc.sdg(1)
+        qc.global_phase += 1.2345
+        qc_split = QuantumCircuit(2)
+        qc_split.append(UnitaryGate(Operator(qc)), [0, 1])
+
+        pm = PassManager()
+        pm.append(Collect2qBlocks())
+        pm.append(ConsolidateBlocks())
+        pm.append(Split2QUnitaries())
+        res = pm.run(qc_split)
+        res_op = Operator.from_circuit(res, final_layout=res.layout.final_layout)


Can you add a test that has this in it as part of a larger circuit that runs through the full transpilation pipeline? I want to make sure that we validating that we're tracking and composing the final permutations correctly.

Done. Also added a smaller test which does non-trivial swapping (with swap gates removed using the elide permutation pass) before the unitary test which surfaced a possible bug in the layout composition phase. We need to discuss whether in this phase we should compose on the existing layout or (as was the case before my change) on its inverse.

mtreinish · 2024-12-05T23:18:55Z

test/python/transpiler/test_split_2q_unitaries.py

+        self.assertTrue(expected_op.equiv(res_op))
+        self.assertTrue(
+            matrix_equal(expected_op.data, res_op.data, ignore_phase=False)
+        )


Can we add an assertion here that swap has been removed from the output?

I've added self.assertNotIn("swap", res.count_ops()) but I don't think it's enough - there's also the possibility that we didn't split the unitary at all (note that it's also not being checked in the tests relating to the original split unitary code).

Any idea how to easily verify that the unitary was removed? Note that just using count_ops is tricky since we have a unitary both before (as the 2-qubit unitary) and after (as two individual 1-qubit unitaries). I can compare the count itself (1 before vs 2 after) but it feel less robust than it should.

test/python/transpiler/test_split_2q_unitaries.py

alexanderivrii · 2024-12-06T07:58:27Z

If I recall correctly, we don't run the ElidePermutations pass if the user explicitly sets the initial layout (to prevent the transpiler from messing up circuits that are already routed). Is this something we should worry about for this pass as well? Would it make sense to have an additional flow where the pass would replace a 2-qubit unitary by a SWAP + 1-qubit unitaries without merging the SWAP into the final permutation?

ShellyGarion

Thanks for your contribution. I have a few minor comments.

crates/accelerate/src/split_2q_unitaries.rs

test/python/transpiler/test_split_2q_unitaries.py

qiskit/transpiler/passes/optimization/split_2q_unitaries.py

mtreinish

Overall this looks excellent thanks for doing this, this looks about ready now. I just have a few small comments and questions inline. The biggest one I think is over how/when you're thinking we should integrate this new feature into the preset pass managers. It's fine if you'd like to do that in a separate follow-up PR to keep this PR to a single logical change (that's probably preferable actually). I just wanted to understand your thinking on that.

mtreinish · 2025-01-31T12:10:03Z

crates/accelerate/src/split_2q_unitaries.rs

+        params: (!new_op.params.is_empty()).then(|| Box::new(new_op.params)),
+        extra_attrs: new_op.extra_attrs,
+        #[cfg(feature = "cache_pygates")]
+        py_op: std::sync::OnceLock::new(),


You do have the py op available for caching here since you are generating this via python. But in this case it doesn't actually matter too much because this is always going to be a PyGate and if the cache is None the python object reference will just be taken from that.

So, do you think something needs to change here?

I think this is fine as is. This was just more my idle musings.

crates/accelerate/src/split_2q_unitaries.rs

qiskit/transpiler/passes/optimization/split_2q_unitaries.py

mtreinish · 2025-01-31T12:23:46Z

qiskit/transpiler/passes/optimization/split_2q_unitaries.py

+
+        (new_dag, qubit_mapping) = result
+        input_qubit_mapping = {qubit: index for index, qubit in enumerate(dag.qubits)}
+        self.property_set["original_layout"] = Layout(input_qubit_mapping)


Does this behave correctly if we run elide permutations first? I realize this is just a straight copy of what elide permutations is doing, but I'm wondering if we need to check that we're not doing multiple permutations before applying the layout or do some other storing if we run these together.

It should work - in theory. in split_2q_unitaries.py we have

if current_layout := self.property_set["virtual_permutation_layout"]: self.property_set["virtual_permutation_layout"] = new_layout.compose( current_layout, dag.qubits )

And compose works, if I recall correctly. However, this is not covered by the current test suite, and that's one of the things I wanted to ask about - should the "joint" test be placed in TestSplit2QUnitaries or perhaps a different location? What flows should we check, other than adding Split2QUnitaries before and after ElidePermutations?

You can put it in TestSplit2QUnitaries that's fine if the intent is to validate the Split2QUnitaries logic is correct that's fine. But having the coverage in general is more important than where it's located specifically.

releasenotes/notes/update-split-2q-unitaries-with-swap-557a1252e3208257.yaml

mtreinish

I think the PR looks good now, thanks for all the work on it. I'll hold off on adding it to the merge queue if you want to push the extra test case in this PR. If you'd rather add it to a follow up PR though that's fine too since I think we have indirect coverage of it now that we're running with the new option in the preset pass manager. Having a direct test of the combination of two permutation passes back to back would be good to have direct validation though.

gadial · 2025-02-12T12:59:03Z

I think the PR looks good now, thanks for all the work on it. I'll hold off on adding it to the merge queue if you want to push the extra test case in this PR. If you'd rather add it to a follow up PR though that's fine too since I think we have indirect coverage of it now that we're running with the new option in the preset pass manager. Having a direct test of the combination of two permutation passes back to back would be good to have direct validation though.

Thanks Matthew. I'll try adding something useful tomorrow and see if I succeed.

crates/accelerate/src/split_2q_unitaries.rs

This also reworks the logic of the function to be consistent with the new structure for UnitaryGate defined in rust.

mtreinish

I pushed up a fix for the merge conflicts caused by using the rust native UnitaryGate in the pass. As part of that process I caught a bug in the indexing in the pass. So I'm going to merge this without that one test. I'll open a tracking issue to add the test coverage so you can push a new PR when you have the test ready.

* Update Split2QUnitaries to handle SWAP unitaries as well * Linting, additional test, bugfix in layout composition * Docstring updates * Add a flag for toggling swap split * Changes according to PR review * Fix indexing in pass --------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

Update Split2QUnitaries to handle SWAP unitaries as well

53817b5

gadial requested a review from a team as a code owner December 5, 2024 15:28

ShellyGarion added this to the 2.0.0 milestone Dec 5, 2024

mtreinish reviewed Dec 5, 2024

View reviewed changes

Linting, additional test, bugfix in layout composition

0bdba2a

ShellyGarion reviewed Jan 1, 2025

View reviewed changes

gadial added 4 commits January 2, 2025 11:14

Docstring updates

1d7b4f6

Merge branch 'main' into split_2q_swap_unitaries

ea5e164

Merge branch 'main' into split_2q_swap_unitaries

e30b617

Add a flag for toggling swap split

93b59f9

1ucian0 assigned mtreinish Jan 30, 2025

mtreinish reviewed Jan 31, 2025

View reviewed changes

Changes according to PR review

a9bc3e1

1ucian0 assigned gadial Feb 6, 2025

mtreinish previously approved these changes Feb 12, 2025

View reviewed changes

mtreinish reviewed Feb 13, 2025

View reviewed changes

crates/accelerate/src/split_2q_unitaries.rs Outdated Show resolved Hide resolved

mtreinish added 2 commits February 13, 2025 14:09

Merge remote-tracking branch 'origin/main' into split_2q_swap_unitaries

0f84fc4

This also reworks the logic of the function to be consistent with the new structure for UnitaryGate defined in rust.

Fix indexing in pass

6e00055

mtreinish dismissed their stale review via 6e00055 February 13, 2025 21:42

mtreinish approved these changes Feb 13, 2025

View reviewed changes

mtreinish enabled auto-merge February 13, 2025 22:03

mtreinish mentioned this pull request Feb 13, 2025

Add test case for Split2QUnitaries with swap equiv combined with ElidePermutations #13842

Open

mtreinish added this pull request to the merge queue Feb 13, 2025

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 14, 2025

mtreinish added this pull request to the merge queue Feb 14, 2025

Merged via the queue into Qiskit:main with commit 8a7803e Feb 14, 2025
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Split2QUnitaries to handle SWAP unitary gates #13531

Update Split2QUnitaries to handle SWAP unitary gates #13531

gadial commented Dec 5, 2024

qiskit-bot commented Dec 5, 2024

coveralls commented Dec 5, 2024 •

edited

Loading

mtreinish left a comment

mtreinish Dec 5, 2024

gadial Dec 11, 2024

mtreinish Dec 5, 2024

gadial Dec 9, 2024

alexanderivrii commented Dec 6, 2024

ShellyGarion left a comment

mtreinish left a comment

mtreinish Jan 31, 2025

gadial Feb 4, 2025

mtreinish Feb 5, 2025

mtreinish Jan 31, 2025

gadial Feb 5, 2025

mtreinish Feb 12, 2025

mtreinish left a comment

gadial commented Feb 12, 2025

mtreinish left a comment

Update Split2QUnitaries to handle SWAP unitary gates #13531

Update Split2QUnitaries to handle SWAP unitary gates #13531

Conversation

gadial commented Dec 5, 2024

Summary

Details and comments

qiskit-bot commented Dec 5, 2024

coveralls commented Dec 5, 2024 • edited Loading

Pull Request Test Coverage Report for Build 13158018594

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

mtreinish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderivrii commented Dec 6, 2024

ShellyGarion left a comment

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

gadial commented Feb 12, 2025

mtreinish left a comment

Choose a reason for hiding this comment

coveralls commented Dec 5, 2024 •

edited

Loading