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

[RFC] Generalized unroller and equivalence library RFC. #6

Merged

Conversation

kdk
Copy link
Member

@kdk kdk commented Jan 23, 2020

No description provided.

@dongreenberg
Copy link
Contributor

How do you make these pretty ascii diagrams? Manually?

@kdk
Copy link
Member Author

kdk commented Jan 23, 2020

How do you make these pretty ascii diagrams? Manually?

https://monodraw.helftone.com/

####-rfc-generalized-unroller-and-equivalence-library.md Outdated Show resolved Hide resolved
Storing one circuit entry per gate width in the library is another option, growing the library size proportional to the number of variadic gates and their maximum widths.

The `Decompose` transpile pass and corresponding `QuantumCircuit.decompose` method depend on a single unique path through `Instruction.definition`.
These methods could be deprecated and removed, maintained via a decomposition search targeting `U3` and `CX`, or maintained via additional library entry labeling to support identification of one entry as the legacy `definition`.
Copy link
Member

Choose a reason for hiding this comment

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

The use case for decompose() is when one builds a circuit using multiple composite gates (just because it's easier to reuse a composite gate, not because one is interested in different decompositions of that composite gate). If we don't have decompose then you have to specify a basis to unroll to. How would I select the basis to yield the current decompose behavior? If we can solve this then I'm fine deprecating decompose (i.e. a canonical definition for gates), and going with this more general framework.

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding an optional target basis kwarg to decompose defaulting to ['u3', 'cx'] would preserve the current behavior. Maybe better for the use case you're describing though would be to replace the current behavior with something like 'decompose only gates from outside the standard library'.

Co-Authored-By: Ali Javadi-Abhari <ajavadia@users.noreply.github.com>
@shaimach
Copy link

shaimach commented Feb 3, 2020

Context: I'm Shai Machnes of OpenSuperQ, tasked with adapting the QISKIT stack to our hardware, where we have several two-qubit gates, and none of which are CX (we have CZ, sqrt(iSWAP), SWAP, etc).

I'm confused regarding the timing of unrolling custom gates as depicted in the "envisioned terra diagram":

In the proposed flow, the conversion to the "standard" set of gates takes place before the high-level optimization (i.e. in the top row). I believe this is too soon: When one typically codes an algorithm, one usually makes use of a fairly wide set of gates (not having a specific hardware in mind), and one usually assumes full connectivity (any two-qubit gate can act on any two qubits). So if, for example, one codes the algorithm using an XX gate, the current flow would unroll that into CX and U123 gates (which are the gates in the standard gate set), and then perform high-level optimization. But if the hardware actually supports XX at the native level, it is unlikely that subsequent transformations (specifically the secondary mapping, device unroller and device optimizations appearing on the bottom row of the diagram), will undo the unrolling performed in the first phase, returning from CX to XX in an efficient way. As a result, the final code will be sub-optimal.

If one wishes to truly be architecture-agnostic, there should be no "standard gate set| - all complete (and over-complete) gate sets should have equal standing. The Backend object can specify which gates are supported by the hardware (and which gates can be executed on which qubits), and transpiler adapts accordingly.

The proposed design definitely aims in the right direction, but IMHO does not go far enough, keeping one gate set (the "standard" one), in a privileged position.

To the extent possible, high-level optimizations should be performed using a much wider gate-set than the standard one, and preferably without any unrolling at all. It is true that this may be tricky, but as we are free to require matrix forms from all gates, and can further require "optimization hints" to be added to all gates, I believe this should be possible. It'll also serve to define high-level optimizations in terms of gate attributes, instead of specific gates, which may lead to a better optimizer.

@kdk
Copy link
Member Author

kdk commented Feb 3, 2020

Hi @shaimach , thanks for the feedback. I agree that the goal is to avoid having any preferred basis for the transpiler. There are some terms that are not fully specified by the diagram, that should help clarify its intent.

I'm confused regarding the timing of unrolling custom gates as depicted in the "envisioned terra diagram":

In the proposed flow, the conversion to the "standard" set of gates takes place before the high-level optimization (i.e. in the top row). I believe this is too soon: When one typically codes an algorithm, one usually makes use of a fairly wide set of gates (not having a specific hardware in mind), and one usually assumes full connectivity (any two-qubit gate can act on any two qubits).

The standard/built-in gate set, at present, is not just [U{1,2,3}, CX], but all gates defined in https://github.com/Qiskit/qiskit-terra/blob/592c8987605c988c32bcc922f70071d82c134051/qiskit/extensions/standard/__init__.py (including e.g. the Fredkin gate). Something that is assumed throughout the doc (though is never explicitly stated) is that in parallel with this work will be an extension of qiskit's standard/built-in gate set to include a broader collection of logical and device gates (including e.g. iswap, ...).

So CZ, SWAP, sqrt(iSWAP) would all be standard/built-in gates, not custom gates. The UnrollUser pass would only be responsible for unrolling gates which were defined by the user during their current session into an equivalent series of gates in the standard library.

@shaimach
Copy link

shaimach commented Feb 9, 2020

Hi @kdk,

Thank you for the reply.

It would extremely helpful if the RFC is updated to reflect the clarifications contained in your reply.

In addition, it would be great to address the following:

  • Assume an algorithm has been coded using both CXs and CZs. I wish to run it on a QPU which perform CZs. Does the standard equivalence library contain both the CX-->CZ and CZ-->CX mapping? How about the CX-->SWAP and CZ-->SWAP? This is related to the currently incomplete sections of the RFC and the bramble search algorithm which is mentioned, but not detailed.

  • Which object contains the information regarding the QPU's physical gates? Is it the Backend object ?

  • The same scenario described above, but now we add the assumption that the QPU also supports atomic SWAP gates, negating the need to unroll SWAPs which where generated by the transpilation step of mapping logical qubits to physical ones.

  • The scenario described above, but now the QPU's entangling gate is not one of the "named" gates (e.g. CR gate or a "number preserving gmon gate" that is somewhere between iSWAP and cphase). How and where does one specify the gate matrix, how to construct a SWAP from it, how to translate from the standard gates to it, etc. Both for the case of a fixed gate, and for a parametrized gate. A fully detailed example in the RFC would be extremely helpful.

  • Why are there two Equivalence Library objects? Shouldn't there be only the Session Equivalence Library, having a default value of the Standard Equivalence Library? (in other words, the Standard object should not be readily accessible to prevent using it instead of the Session Equivalence Library)

  • Transpiler optimization passes should probably be defined in terms of gate attributes. For example, the current CXCancellation pass should be applied to any gate which declares that its square is the identity gate. Similarly, CommutativeCancellation should be performed based on the gate matrices and inferred commutation relations. Side note: in the latter case, to avoid floating-point "rounding" issues, the square of the matrix normalization constant to be kept separately.

  • We must ensure the existing simulators are capable of using the enhancements described in this RFC. There is no need to them to support all the new gates - the translation can be done by the standard transpiler steps when they are specified as a Backend. But we do need to verify they work as expected.

I think once this RFC is implemented, QISKIT will become truly universal, which is absolutely great.

@kdk
Copy link
Member Author

kdk commented Feb 18, 2020

Hi @shaimach , some answers to your questions inline. (These will be incorporated into the RFC.)

* Assume an algorithm has been coded using both CXs and CZs. I wish to run it on a QPU which perform CZs. Does the standard equivalence library contain both the CX-->CZ and CZ-->CX mapping? How about the CX-->SWAP and CZ-->SWAP? This is related to the [currently incomplete sections of the RFC](https://github.com/kdk/rfcs/blob/generalized-unroller-and-equivalence-library/%23%23%23%23-rfc-generalized-unroller-and-equivalence-library.md#decomposing-toffolighz-cross-architecture) and the [bramble](https://en.wikipedia.org/wiki/Bramble_(graph_theory)) search algorithm which is mentioned, but not detailed.

Correct that the standard equivalence library will contain entries like CX->{CZ, H}, CZ -> {CX, H},SWAP -> {CX}, SWAP -> {CZ, H}. The unroller will select the set of transformations to apply to map from the initial basis to the desired basis.

* Which object contains the information regarding the QPU's physical gates? Is it the Backend object ?

Correct, right now e.g.

>>> from qiskit.test.mock import FakeVigo
>>> be = FakeVigo()
>>> be.configuration().basis_gates
['u1', 'u2', 'u3', 'cx', 'id']
* The same scenario described above, but now we add the assumption that the QPU also supports atomic SWAP gates, negating the need to unroll SWAPs which where generated by the transpilation step of mapping logical qubits to physical ones.

If swaps are natively implemented (and reported) by the backend, they will not need to be unrolled (though a noise-aware unroller could still choose to do so, if beneficial).

* The scenario described above, but now the QPU's entangling gate is not one of the "named" gates (e.g. CR gate or a "number preserving gmon gate" that is somewhere between iSWAP and cphase). How and where does one specify the gate matrix, how to construct a SWAP from it, how to translate from the standard gates to it, etc. Both for the case of a fixed gate, and for a parametrized gate. A fully detailed example in the RFC would be extremely helpful.

Good question. The assumption has been that the standard gate set would grow to include all families of gates natively implemented by backends (and so have access to matrix definitions, equivalence relations, ...). If this is not the case, or there are cases where this is not possible, this information would need to be supplied by the backend. I do not believe a mechanism currently exists to provide this information, but we can look into this further.

* Why are there two Equivalence Library objects? Shouldn't there be only the Session Equivalence Library, having a default value of the Standard Equivalence Library? (in other words, the Standard object should not be readily accessible to prevent using it instead of the Session Equivalence Library)

The availability of the standard library as distinct from the session library will be useful so that users can easily track where a particular definition originated. The standard tooling will default to referencing the SessionLibrary, so is should be difficult to inadvertently use the StandardLibrary.

* [Transpiler optimization passes](https://qiskit.org/documentation/apidoc/transpiler/passes/passes.html) should probably be defined in terms of gate attributes. For example, the current [CXCancellation](https://qiskit.org/documentation/api/qiskit.transpiler.passes.CXCancellation.html#qiskit.transpiler.passes.CXCancellation) pass should be applied to any gate which declares that its square is the identity gate. Similarly, [CommutativeCancellation](https://qiskit.org/documentation/api/qiskit.transpiler.passes.CommutativeCancellation.html#qiskit.transpiler.passes.CommutativeCancellation) should be performed based on the gate matrices and inferred commutation relations. Side note: in the latter case, to avoid floating-point "rounding" issues, the square of the matrix normalization constant to be kept separately.

I agree. There is follow up work to generalize other transpiler passes (e.g. self-inverse cancellation, commutation analysis, ...) that will come after the implementation of this RFC.

@CLAassistant
Copy link

CLA assistant check
All committers have signed the CLA.

@ajavadia ajavadia changed the title [WIP] Generalized unroller and equivalence library RFC. [RFC] Generalized unroller and equivalence library RFC. May 21, 2020
@ajavadia ajavadia merged commit 344224d into Qiskit:master May 21, 2020
blakejohnson added a commit that referenced this pull request Oct 27, 2023
* Filled out the Summary, Motivation, and User Benefit sections

* Filled out Alternative Approaches section

* Add Tasks section

* Add to Future Extensions section

* Add more minor edits

* Add Sections (#4)

* add to Future Extensions (re Task in BasePrimitive)

* Array broadcasting figure, example (#5)

* Add section references (#6)

* Add "BindingsArray generalizations" subsection

* Add subsection titles

* Add more BindingsArray examples

* Wording change

* Move the example up (no significant changes)

* Add detail to migration plan (#7)

* Light editing of first few sections.

* Move all of the class ideas into Detailed Design

* Edits in the detailed design subsections

* Edits to migration section

* Flesh out examples more

* update date

* update authors

* switch observable array format

* Small fixes here and there---nothing major.

* Update authors

* Address Chris` "Task" comments

* Further response to Chris' "Task" comments

* alphebetize and edit authors

* rename ResultBundle -> TaskResult

* Address Jim's comment

* Rename ObservablesTask -> EstimatorTask

* Update 0015-estimator-interface.md

Co-authored-by: Takashi Imamichi <31178928+t-imamichi@users.noreply.github.com>

* Update 0015-estimator-interface.md

Co-authored-by: Luciano Bello <bel@zurich.ibm.com>

* Update 0015-estimator-interface.md

Co-authored-by: Takashi Imamichi <31178928+t-imamichi@users.noreply.github.com>

* Update 0015-estimator-interface.md

Co-authored-by: Luciano Bello <bel@zurich.ibm.com>

* Update 0015-estimator-interface.md

Co-authored-by: Blake Johnson <blakejohnson04@gmail.com>

* fix typos in broadcasting examples

* Address Blake's comment about exponential memory overhead for BaseOperator

* Added Migration Examples section

This is following a request to see some simpler examples of what the new
interface would look like.

* Update 0015-estimator-interface.md

Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>

* Update 0015-estimator-interface.md

Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>

* Fix shapes

* Update 0015-estimator-interface.md

Co-authored-by: Luciano Bello <bel@zurich.ibm.com>

* Removed 'Inner' from figure

* Update 0015-estimator-interface.md

Co-authored-by: Jim Garrison <jim@garrison.cc>

* Update 0015-estimator-interface.md

Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>

* Update 0015-estimator-interface.md

Co-authored-by: Toshinari Itoko <15028342+itoko@users.noreply.github.com>

* Update 0015-estimator-interface.md

Co-authored-by: Toshinari Itoko <15028342+itoko@users.noreply.github.com>

* Update 0015-estimator-interface.md

Co-authored-by: Toshinari Itoko <15028342+itoko@users.noreply.github.com>

* change migration path to versioning

* clarify output type

---------

Co-authored-by: Samantha Barron <sam@sambarron.me>
Co-authored-by: Takashi Imamichi <31178928+t-imamichi@users.noreply.github.com>
Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
Co-authored-by: Blake Johnson <blakejohnson04@gmail.com>
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
Co-authored-by: Jim Garrison <jim@garrison.cc>
Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
Co-authored-by: Toshinari Itoko <15028342+itoko@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants