Transpiler updates for optional registers. #5515

kdk · 2020-12-11T23:01:10Z

Summary

Following #5504 updates transpiler and in-tree passes to support optional registers, and to prefer inspecting bits over registers where possible.

Details and comments

on-hold until #5504 .

Commits in this PR (without #5504, #5498 and #5486): https://github.com/Qiskit/qiskit-terra/pull/5515/files/51db5270a21a98ca408ad9eca470b715b92639d0..cdfd7ddbbe7542d949426b7c90a7ac723e202569

WIP:

~~Skips test.python.compiler.test_transpiler.TestTranspile.test_wrong_initial_layout which raises a KeyError from Layout instead of the expected DAGCircuitError~~ (Resolved by Better initial_layout validation #5526 .)
~~Some cases where we build qubit_indices might be better as Layout.generate_trivial_layout~~

mtreinish

This LGTM a couple small comments and questions inline. The big takeaway for me though is just like in the previous PR we basically are reconstructing a qubit indices dict for anywhere there was index based access before. I know we're already tracking a follow up where we move that to the QuantumCircuit, but it might make sense to copy that to the dag metadata as well in that follow up so we don't need to reconstruct it in all the passes.

mtreinish · 2021-03-02T20:16:50Z

qiskit/transpiler/passes/routing/cython/stochastic_swap/utils.pxd

@@ -31,7 +31,8 @@ cdef class NLayout:


 cpdef NLayout nlayout_from_layout(object layout,
-                                  object dag, 
+                                  object dag,


Is there a reason this has a different name than the implementation in utils.pyx? I probably would make this also typed as a dict. So something like:

Suggested change

object dag,

dict qubit_indices,

Agree, added in 23a6597 .

mtreinish · 2021-03-02T20:18:05Z

qiskit/transpiler/passes/routing/cython/stochastic_swap/utils.pyx

@@ -162,32 +162,27 @@ cdef class NLayout:
        return out


-cpdef NLayout nlayout_from_layout(object layout, object qregs, 
+cpdef NLayout nlayout_from_layout(object layout,
+                                  object qubit_indices,


I would make this a dict (assuming the signature declaration in the pxd is updated to match:

Suggested change

object qubit_indices,

dict qubit_indices,

I don't think it matters too much, but feels like explicitly typing the argument to always be a dict makes the most sense instead of taking any object. (I expect it was object before because an OrderedDict was used for python 3.5 compat)

Agree, added in 23a6597 .

mtreinish · 2021-03-02T20:23:48Z

qiskit/transpiler/passes/routing/stochastic_swap.py

-        int_layout = nlayout_from_layout(layout, qregs, coupling.size())
+        int_qubit_subset = np.fromiter((self._qubit_indices[bit]
+                                        for bit in qubit_subset),
+                                       dtype=np.int32,


Looking at these I do wonder we don't make these unsigned since we can't have negative qubit indices. But that would require more cython changes because this array gets passed as an arg to the swap_trial cython function.

That's a good idea, maybe an issue as a future improvement (or as a target of opportunity next time we need to dive into the cython code)?

yeah I can push a follow up to do this, it's not crtiical

mtreinish · 2021-03-02T20:25:42Z

qiskit/transpiler/passes/utils/check_cx_direction.py

@@ -47,9 +48,11 @@ def run(self, dag):
        self.property_set['is_direction_mapped'] = True
        edges = self.coupling_map.get_edges()

+        trivial_layout = Layout.generate_trivial_layout(*dag.qregs.values())


I saw the WIP todo in the PR summary about this, but what is the functional difference between using a layout here versus building a qubit indices dict?

Functionally, only that Layout is a bidirectional (instead of unidirectional) mapping from Qubits to ints. Since I wrote that comment though, I've moved away from the idea of using Layouts as a generic mapping (outside the contexts of layout and routing) because the resulting code ends up for me harder to read.

mtreinish · 2021-03-02T20:30:40Z

qiskit/transpiler/passes/optimization/hoare_opt.py

        """ After each gate generate a new unique variable name for each of the
            qubits, using scheme: 'q[id]_[gatenum]', e.g. q1_0 -> q1_1 -> q1_2,
                                                          q2_0 -> q2_1
        Args:
-            qb_id (int): index of qubit to generate new variable for
+            qbt (Qubit): qubit to generate new variable for


Any reason to not use qubit?

Good catch, added in 49865fc .

mtreinish

Thanks for the update I think it LGTM. The one thing I think would be good to have before we merge is some testing of the passes without registers, especially for ones not in the default pipeline (like the hoare optimization pass).

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

…ters.

kdk · 2021-03-11T16:26:46Z

Thanks for the update I think it LGTM. The one thing I think would be good to have before we merge is some testing of the passes without registers, especially for ones not in the default pipeline (like the hoare optimization pass).

Good idea, added in 3d745bb .

mtreinish

LGTM, good to see the extra test coverage and that it caught a bunch of bugs. I think this LGTM now. I still think there are some coverage gaps on some of the non default passes, but we can always fix those in follow ups too.

kdk added on hold Can not fix yet optional-registers labels Dec 11, 2020

kdk added this to the 0.17 milestone Dec 11, 2020

kdk added this to To do in Circuits via automation Dec 11, 2020

kdk requested review from DanPuzzuoli, eggerdj, maddy-tod, nkanazawa1989, nonhermitian and a team as code owners December 11, 2020 23:01

This was referenced Dec 14, 2020

Visualization updates for optional registers. #5519

Merged

Cleanup unneeded Register use in standard library definitions. #5529

Closed

kdk force-pushed the quantumcircuit-optional-registers-transpiler branch from cdfd7dd to 46e8fa7 Compare January 15, 2021 23:47

kdk requested review from chriseclectic, lcapelluto and taalexander as code owners January 15, 2021 23:47

kdk force-pushed the quantumcircuit-optional-registers-transpiler branch from 46e8fa7 to c7ef6fc Compare January 20, 2021 22:48

kdk mentioned this pull request Feb 12, 2021

API changes for optional registers in QuantumCircuit. #5486

Merged

kdk force-pushed the quantumcircuit-optional-registers-transpiler branch from c7ef6fc to 68afca2 Compare March 1, 2021 20:21

kdk removed the on hold Can not fix yet label Mar 2, 2021

mtreinish reviewed Mar 2, 2021

View reviewed changes

kdk force-pushed the quantumcircuit-optional-registers-transpiler branch from 68afca2 to 13a63e6 Compare March 3, 2021 16:54

mtreinish reviewed Mar 4, 2021

View reviewed changes

kdk and others added 7 commits March 10, 2021 17:48

Update faulty qubit remapping to prefer inspecting Bits over Registers.

e952d86

Update transpiler passes to prefer inspecting Bits over Registers.

8ae0e7c

Update FullAncillaAllocation pass to prefer Bits over Registers.

eb4bd61

Update RemoveFinalMeasurements pass to prefer Bits over Registers.

4d37d9d

Update StochasticSwap pass to prefer inspecting Bits over Registers.

975f0c5

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

Update SabreSwap and SabreLayout to prefer inspecting Bits over Regis…

d5cdb7c

…ters.

Update CXDirection passes to prefer inspecting Bits over Registers.

9289355

kdk added 8 commits March 10, 2021 17:49

Update transpiler tests to prefer inspecting Bits over Registers.

1caea82

Update HoareOptimizer pass to prefer inspecting Bits over Registers.

4340f43

Update CrosstalkAdaptiveSchedule pass to prefer Bits over Registers.

18a1fbe

DAGDependency API updates for circuits with optional registers.

98783d6

Update remaining converters for optional registers.

619ffa5

Fix QuantumCircuit allowing duplicate Clbits when adding registers.

35fe301

Add end-to-end test covering transpiling circuits without registers.

3d745bb

Update Layout objects for optional registers.

ffe7ac0

kdk force-pushed the quantumcircuit-optional-registers-transpiler branch from 13a63e6 to ffe7ac0 Compare March 10, 2021 22:49

mtreinish approved these changes Mar 11, 2021

View reviewed changes

mtreinish added the automerge label Mar 11, 2021

Merge branch 'master' into quantumcircuit-optional-registers-transpiler

9b8a8ff

mergify bot merged commit 4d64125 into Qiskit:master Mar 11, 2021

Circuits automation moved this from To do to Done Mar 11, 2021

kdk added the Changelog: None Do not include in changelog label Mar 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transpiler updates for optional registers. #5515

Transpiler updates for optional registers. #5515

kdk commented Dec 11, 2020 •

edited

mtreinish left a comment

mtreinish Mar 2, 2021

kdk Mar 3, 2021

mtreinish Mar 2, 2021

kdk Mar 3, 2021

mtreinish Mar 2, 2021

kdk Mar 3, 2021

mtreinish Mar 4, 2021

mtreinish Mar 2, 2021

kdk Mar 3, 2021

mtreinish Mar 2, 2021

kdk Mar 3, 2021

mtreinish left a comment

kdk commented Mar 11, 2021

mtreinish left a comment

Transpiler updates for optional registers. #5515

Transpiler updates for optional registers. #5515

Conversation

kdk commented Dec 11, 2020 • edited

Summary

Details and comments

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

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

kdk commented Mar 11, 2021

mtreinish left a comment

Choose a reason for hiding this comment

kdk commented Dec 11, 2020 •

edited