Fix missing layout in `Commuting2qGateRouter` #12137

ElePT · 2024-04-04T08:36:39Z

Summary

This PR fixes an oversight in the Commuting2qGateRouter pass where the qreg permutations weren't tracked in the property set, the output circuit layout would be missing, and uses would have to keep track of the mapping from cregs to qregs manually.

Details and comments

Found while working on a PR that used this pass in an independent repository: qiskit-community/qopt-best-practices#30.

qiskit-bot · 2024-04-04T08:36:44Z

One or more of the the following people are requested to review this:

@Qiskit/terra-core

jakelishman

Thanks for catching this. I certainly agree that as a routing pass, this pass should be setting the final_layout attribute. Since it's not a layout pass, though, I'm not sure I understand why it might want to set layout or original_qubit_indices.

jakelishman · 2024-04-04T11:12:15Z

qiskit/transpiler/passes/routing/commuting_2q_gate_routing/commuting_2q_gate_router.py

        current_layout = Layout.generate_trivial_layout(*dag.qregs.values())

+        self.property_set["layout"] = current_layout.copy()
+        self.property_set["original_qubit_indices"] = {
+            bit: index for index, bit in enumerate(dag.qubits)
+        }


This pass seems to rely on the input circuit being physical already, which would imply that self.property_set["layout"] likely is already set, and this then overwrites it.

Is there a reason that this routing pass would want to be overwriting the layout and original qubit indices?

Yes, I get your point, there was a reason but I think it isn't a good one. I had originally tried to only set "final_layout", then realized that in practice we mostly use the pass in isolation (when we transpile qaoa we use it like a pre-layout routing of a sub-circuit that we then use to build the full circuit), and if "layout" wasn't set, the output circuit wouldn't include a layout at all. But if we want to respect the "routing" nature of the pass, I think that we can just guide users to apply a trivial layout to the sub-circuit before running this pass.

I think that your proposal in #9523 (comment) might help in the qaoa use case I mentioned? at least it might avoid requiring a layout if we can just store the permutation.

in practice we mostly use the pass in isolation

This is kind of where the problem is - if you want to use it in isolation, it should be on a circuit that was already physical, so either you need to have pretended that it was (in which case you should already know how to handle the missing layout), or the pass should be running in a pass manager that makes the circuit physical via a embed pipeline. Fwiw, I think TranspileLayout should handle the case of the missing layout fine already when using routing_permutation, which setting final_layout should automatically make available.

If we make this unilaterally set layout (to the trivial layout, no less) it becomes impossible to use correctly in a full pipeline, which I think might be a larger barrier to entry for general use.

jakelishman · 2024-04-04T11:16:38Z

test/python/transpiler/test_swap_strategy_router.py

+        # We use the same scenario as the QAOA test above
+        mixer = QuantumCircuit(4)
+        for idx in range(4):
+            mixer.ry(-idx, idx)
+
+        op = SparsePauliOp.from_list([("IZZI", 1), ("ZIIZ", 2), ("ZIZI", 3)])
+        circ = QAOAAnsatz(op, reps=2, mixer_operator=mixer)
+
+        expected_initial_layout = Layout.generate_trivial_layout(*circ.qregs)
+        expected_final_index_layout = [3, 1, 2, 0]
+        expected_final_layout = Layout.from_intlist(expected_final_index_layout, *circ.qregs)
+
+        swapped = self.pm_.run(circ.decompose())
+
+        self.assertEqual(swapped.layout.initial_layout, expected_initial_layout)
+        self.assertEqual(swapped.layout.final_layout, expected_final_layout)
+        self.assertEqual(swapped.layout.final_index_layout(), expected_final_index_layout)


There's no layout pass in your PassManager here, so I wouldn't expect initial_layout to be set by anything in the pass manager.

fwiw, you don't need to test TranspileLayout.initial_layout or TranspileLayout.final_layout - we recommend not using either of those legacy attributes, but only using the methods on TranspileLayout following #10835. In this case, since the swap-strategy thing is just a router, the only method I'd really expect to return a non-None value is TranspileLayout.routing_permutation.

eggerdj · 2024-04-04T11:44:15Z

Having some mechanism to store the permutation induced by this pass would be really nice. Before, this was left-up to the user e.g. https://github.com/eggerdj/large_scale_qaoa/blob/21a2d929dabb72887c855767514e566b147b9b79/large_scale_qaoa/qaoa.py#L233-L246

jakelishman · 2024-04-04T11:50:12Z

Dan: yeah, this pass is a router, so it's a mistake that it wasn't already setting final_layout, as Elena's PR here makes it. The only bit I'm questioning in this PR is why it's also setting layout and original_qubit_indices, when the pass isn't a layout pass.

coveralls · 2024-04-04T18:30:51Z

Pull Request Test Coverage Report for Build 8988842085

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

6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
4 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.03%) to 89.65%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/expr.rs	1	94.03%
crates/qasm2/src/lex.rs	3	92.62%

Totals
Change from base Build 8983739322:	0.03%
Covered Lines:	62277
Relevant Lines:	69467

💛 - Coveralls

jakelishman

This seems mostly ok to me, but given that the docstring of the class suggests that this doesn't run on physical qubits, I'm thinking it's more inline with what the star-to-line pre-routing does (#11387), and if so, perhaps this pass setting virtual_permutation_layout is more correct?

I don't fully understand the pass, though, so let me just pose this as a question: is this pass supposed to route gates onto physical qubits, or is it supposed to affect the virtual interaction graph, such that a later pass can finalise the routing to physical qubits in the general case?

If the former, it's fine as it is. If the latter (and I think it might be), it probably wants switching to set the pass manager property virtual_permutation_layout. PassManager.run should automatically and correctly handle virtual_permutation_layout in the presence and absence of final_layout, so no worries there, but we also ought to add a test that running a full layout+routing pipeline after this pass handles those circuits correctly.

…ut-prop-set

jakelishman

This looks about right to me, and the test seems good (though I can't immediately see off the top of my head that the expected permutations should be what they're set at, but that's fine).

* Add layout to property set, add test * Apply comments from code review * Add virtual permutation instead of final layout. Test (cherry picked from commit b80885d)

* Add layout to property set, add test * Apply comments from code review * Add virtual permutation instead of final layout. Test (cherry picked from commit b80885d) Co-authored-by: Elena Peña Tapia <[email protected]>

* Add layout to property set, add test * Apply comments from code review * Add virtual permutation instead of final layout. Test

Add layout to property set, add test

50d3f98

ElePT requested a review from a team as a code owner April 4, 2024 08:36

ElePT added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Apr 4, 2024

jakelishman reviewed Apr 4, 2024

View reviewed changes

Apply comments from code review

9661c6d

ElePT requested a review from jakelishman May 6, 2024 08:44

jakelishman reviewed May 7, 2024

View reviewed changes

ElePT added 2 commits May 7, 2024 15:52

Merge branch 'main' of https://github.com/Qiskit/qiskit into add-layo…

63a481b

…ut-prop-set

Add virtual permutation instead of final layout. Test

d57aabf

jakelishman approved these changes May 9, 2024

View reviewed changes

jakelishman added the stable backport potential The bug might be minimal and/or import enough to be port to stable label May 9, 2024

jakelishman added this to the 1.1.0 milestone May 9, 2024

jakelishman added this pull request to the merge queue May 9, 2024

Merged via the queue into Qiskit:main with commit b80885d May 9, 2024
15 checks passed

mergify bot mentioned this pull request May 9, 2024

Fix missing layout in Commuting2qGateRouter (backport #12137) #12383

Merged

ElePT added a commit to ElePT/qiskit that referenced this pull request May 31, 2024

Fix missing layout in Commuting2qGateRouter (Qiskit#12137)

447c420

* Add layout to property set, add test * Apply comments from code review * Add virtual permutation instead of final layout. Test

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix missing layout in `Commuting2qGateRouter` #12137

Fix missing layout in `Commuting2qGateRouter` #12137

ElePT commented Apr 4, 2024

qiskit-bot commented Apr 4, 2024

jakelishman left a comment

jakelishman Apr 4, 2024

ElePT Apr 4, 2024 •

edited

Loading

ElePT Apr 4, 2024 •

edited

Loading

jakelishman Apr 4, 2024

jakelishman Apr 4, 2024

eggerdj commented Apr 4, 2024

jakelishman commented Apr 4, 2024

coveralls commented Apr 4, 2024 •

edited

Loading

jakelishman left a comment

jakelishman left a comment

Fix missing layout in Commuting2qGateRouter #12137

Fix missing layout in Commuting2qGateRouter #12137

Conversation

ElePT commented Apr 4, 2024

Summary

Details and comments

qiskit-bot commented Apr 4, 2024

jakelishman left a comment

Choose a reason for hiding this comment

jakelishman Apr 4, 2024

Choose a reason for hiding this comment

ElePT Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

ElePT Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

jakelishman Apr 4, 2024

Choose a reason for hiding this comment

jakelishman Apr 4, 2024

Choose a reason for hiding this comment

eggerdj commented Apr 4, 2024

jakelishman commented Apr 4, 2024

coveralls commented Apr 4, 2024 • edited Loading

Pull Request Test Coverage Report for Build 8988842085

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

jakelishman left a comment

Choose a reason for hiding this comment

jakelishman left a comment

Choose a reason for hiding this comment

Fix missing layout in `Commuting2qGateRouter` #12137

Fix missing layout in `Commuting2qGateRouter` #12137

ElePT Apr 4, 2024 •

edited

Loading

ElePT Apr 4, 2024 •

edited

Loading

coveralls commented Apr 4, 2024 •

edited

Loading