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

Improve transpiler documentation #9087

Merged
merged 33 commits into from
Jan 26, 2023
Merged

Conversation

mtreinish
Copy link
Member

Summary

This commit expands the module documentation for the qiskit.transpiler module to explain details of working with preset pass managers and how to modify stages for custom workflows. It also updates some of the details about the stages which were a bit stale on the exact behavior of the transpiler.

Details and comments

This commit expands the module documentation for the qiskit.transpiler
module to explain details of working with preset pass managers and how
to modify stages for custom workflows. It also updates some of the
details about the stages which were a bit stale on the exact behavior of
the transpiler.
@mtreinish mtreinish added documentation Something is not clear or an error documentation Changelog: None Do not include in changelog labels Nov 7, 2022
@mtreinish mtreinish requested a review from a team as a code owner November 7, 2022 15:55
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Nov 7, 2022

Pull Request Test Coverage Report for Build 4016054745

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.03%) to 84.935%

Files with Coverage Reduction New Missed Lines %
src/sabre_swap/layer.rs 2 98.95%
Totals Coverage Status
Change from base Build 4010423670: 0.03%
Covered Lines: 66703
Relevant Lines: 78534

💛 - Coveralls

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

This looks like a good (and probably a bit overdue!) change to the documentation to me. I've left some typo fixes and stuff like that.

I was commenting more on some of the stuff in the scheduling stage, then I realised that it was just unchanged from the previous version, so we don't necessarily try to fix everything about it in a single PR right now.

Comment on lines +46 to +61
.. _stage_generators:

Stage Generator Functions
-------------------------

.. autosummary::
:toctree: ../stubs/

~qiskit.transpiler.preset_passmanagers.common.generate_control_flow_options_check
~qiskit.transpiler.preset_passmanagers.common.generate_error_on_control_flow
~qiskit.transpiler.preset_passmanagers.common.generate_unroll_3q
~qiskit.transpiler.preset_passmanagers.common.generate_embed_passmanager
~qiskit.transpiler.preset_passmanagers.common.generate_routing_passmanager
~qiskit.transpiler.preset_passmanagers.common.generate_pre_op_passmanager
~qiskit.transpiler.preset_passmanagers.common.generate_translation_passmanager
~qiskit.transpiler.preset_passmanagers.common.generate_scheduling
Copy link
Member

Choose a reason for hiding this comment

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

This documentation is a bit misleading - the currentmodule is qiskit.transpiler.preset_passmanagers, but these functions aren't actually available from that namespace. We either need to change the module for the documentation of these, or import them into the top-level namespace (my preferred choice - I like packages to properly define a public API, and not require users to go searching in the code for the module stuff happens to be defined it).

Copy link
Member Author

Choose a reason for hiding this comment

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

I was on the fence about re-exporting it form the root here. I opted to not do it in this PR since I wanted to leave this as purely documentation changes. But I can add it if you think it's necessary here. Otherwise I think we can do this in a follow up PR. TBH, I'm not really sure why I didn't re-export these functions when I added these in #6403.

qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
Comment on lines +630 to +642
The scheduling of a circuit involves two parts, analysis and constraint mapping followed by a
padding pass. The first part requires running a scheduling analysis pass such as
:class:`~.ALAPSchedulingAnalysis` or :class:`~.ASAPSchedulingAnalysis` which analyzes the circuit
and records the start time of each instruction in the circuit using a scheduling algorithm ("as late
as possible" for :class:`~.ALAPSchedulingAnalysis` and "as soon as possible" for
:class:`~.ASAPSchedulingAnalysis`) in the property set. Once the circuit has an initial scheduling
additional passes can be run to account for any timing constraints on the target backend, such
as alignment constraints. This is typically done with the
:class:`~.ConstrainedReschedule` pass which will adjust the scheduling
set in the property set to the constraints of the target backend. Once all
the scheduling and adjustments/rescheduling are finished a padding pass,
such as :class:`~.PadDelay` or :class:`~.PadDynamicalDecoupling` is run
to insert the instructions into the circuit, which completes the scheduling.
Copy link
Member

Choose a reason for hiding this comment

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

The first sentence says "two parts", but the rest of the paragraph makes it sound like a minimum of three components to me.

qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
@des137
Copy link
Contributor

des137 commented Nov 9, 2022

Hi @mtreinish and @jakelishman,

Related to this, in the transpiler documentation page, under "Mapping circuits to hardware topology", the "bad" initial_layout fails to create a wider distribution of depths.

Essentially, all the different iterations lead to depth=16. Given that the point of that section is to demonstrate a wider distribution in the depths of the transpired circuits, I believe it needs to corrected.

Please let me know if I should create a separate issue for this. Thanks.

@mtreinish
Copy link
Member Author

Thanks for pointing that out, I hadn't realized that I expect it maybe an artifact of changing the default algorithm to SabreSwap and it now converging on the same solution regardless of seed. I need to fix lint on this anyway I'll modify the example to have it better show the stochastic nature of the routing algorithm.

@mtreinish
Copy link
Member Author

I updated the examples in: 2b7e998 and it is showing more of a distribution now:

Screenshot_2022-11-17_15-38-42

@mtreinish mtreinish added this to the 0.23.0 milestone Jan 11, 2023
Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

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

Up to the Routing section, these sections looks great.

qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
Comment on lines 187 to 188
To be able to compile a :class:`~.QuantumCircuit` for a target backend the compiler needs to
be able to represent that backend for the compiler. There are a few data structures for doing
Copy link
Member

Choose a reason for hiding this comment

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

It seems like we're using backend, target and target backend interchangeably throughout. Is there a distinction to be made between them?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this context not so much, there is a distinction between a Target and a Backend (as in the classes) but for the purposes of the transpiler module, pass managers, and pass managers the target, backend, and target backend I was referring to the the same thing (the compilation target backend).

Comment on lines +681 to +682
optimization level 1 if there are control flow operations (such as
:class:`~.IfElseOp`) present in the circuit.
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, this will likely be outdated following #9417 .

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, after this merges we should make a note to update this doc after when #9417 is fixed

qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
minimum number of swap gates needed to map a circuit onto a given device, is an important
step (if not the most important) in the whole execution process.

However, as with many important things in life, finding the optimal swap mapping is hard.
Copy link
Member

Choose a reason for hiding this comment

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

❤️

Copy link
Contributor

Choose a reason for hiding this comment

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

#MyLifeIsNPHard

qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
Comment on lines +746 to +747
keyword argument, where the index labels the virtual qubit in the circuit and the
corresponding value is the label for the physical qubit to map onto:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
keyword argument, where the index labels the virtual qubit in the circuit and the
corresponding value is the label for the physical qubit to map onto:
keyword argument where the index labels the virtual qubit in the circuit and the
corresponding value is the label for the physical qubit to map onto:

This has to be the hardest-to-describe kwarg input format in all Qiskit. Maybe something like "where each integer specifies a physical qubit index onto which the corresponding virtual qubit (following the order in circuit.qubits will be mapped"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Heh, I know the array of physical bits is Paul's preferred input format (I'm also fond of it too). But initial_layout supports like every way imaginable to describe the mapping. It might make more sense to use the dict format:

{ghz.qubits[0]: 3, ghz.qubits[1]: 4. ghz.qubits[2]: 2}

which is a bit easier to describe.

Copy link
Member

Choose a reason for hiding this comment

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

Agree, and I think the example below does a good job of clarifying the format should be used. I'm just wondering if there's a clearer text explanation we can give (but I wouldn't be surprised if not, there isn't one in e.g. the transpile docstring).

mtreinish and others added 2 commits January 25, 2023 15:50
Co-authored-by: Kevin Krsulich <kevin@krsulich.net>
Copy link
Contributor

@kevinhartman kevinhartman left a comment

Choose a reason for hiding this comment

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

Partially reviewed, more to come. Adding comments so far to reduce conflicts with other reviewers.

qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
Comment on lines 507 to 514
When writing a quantum circuit you are free to use any quantum gate (unitary operator) that
you like, along with a collection of non-gate operations such as qubit measurements and
reset operations. However, when running a circuit on a real quantum device one no longer
has this flexibility. Due to limitations in, for example, the physical interactions
between qubits, difficulty in implementing multi-qubit gates, control electronics etc,
a quantum computing device can only natively support a handful of quantum gates and non-gate
operations. The allowed instructions for a given backend can be found by querying the
:class:`~.Target` for the devices:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When writing a quantum circuit you are free to use any quantum gate (unitary operator) that
you like, along with a collection of non-gate operations such as qubit measurements and
reset operations. However, when running a circuit on a real quantum device one no longer
has this flexibility. Due to limitations in, for example, the physical interactions
between qubits, difficulty in implementing multi-qubit gates, control electronics etc,
a quantum computing device can only natively support a handful of quantum gates and non-gate
operations. The allowed instructions for a given backend can be found by querying the
:class:`~.Target` for the devices:
Users are free to write quantum circuits using any quantum gate (unitary operator) in addition
to non-gate operations such as qubit measurements and reset operations. However, when
running a circuit on real quantum hardware, these operations must be translated to the native
set of operations supported by the device, which is often limited. The allowed instructions for a
given backend can be found by querying the :class:`~.Target` for the devices:

qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
Comment on lines 544 to 549
We have :math:`H`, :math:`X`, and controlled-:math:`P` gates, all of which are
not in our devices basis gate set, and must be expanded. This expansion is taken
care of for us in the :func:`qiskit.execute` function. However, we can
decompose the circuit to show what it would look like in the native gate set of
the IBM Quantum devices (the :class:`~.FakeVigoV2` backend is a fake backend that
models the historical IBM Vigo 5 qubit device for test purposes):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
We have :math:`H`, :math:`X`, and controlled-:math:`P` gates, all of which are
not in our devices basis gate set, and must be expanded. This expansion is taken
care of for us in the :func:`qiskit.execute` function. However, we can
decompose the circuit to show what it would look like in the native gate set of
the IBM Quantum devices (the :class:`~.FakeVigoV2` backend is a fake backend that
models the historical IBM Vigo 5 qubit device for test purposes):
We have :math:`H`, :math:`X`, and controlled-:math:`P` gates, none of which are
in our device's basis gate set, and thus must be expanded. This expansion is taken
care of for us in the :func:`qiskit.execute` function. However, we can
decompose the circuit to show what it would look like in the native gate set of
the IBM Quantum device (the :class:`~.FakeVigoV2` backend is a fake backend that
models the historical IBM Vigo 5 qubit device for test purposes):

qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
Comment on lines 610 to 614
As a product of three CNOT gates, swap gates are expensive operations to perform on a
noisy quantum devices. However, such operations are usually necessary for embedding a
circuit into the limited entangling gate connectivities of actual devices. Thus,
minimizing the number of swap gates in a circuit is a primary goal in the
transpilation process.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
As a product of three CNOT gates, swap gates are expensive operations to perform on a
noisy quantum devices. However, such operations are usually necessary for embedding a
circuit into the limited entangling gate connectivities of actual devices. Thus,
minimizing the number of swap gates in a circuit is a primary goal in the
transpilation process.
Swap gates are expensive operations to perform on a noisy quantum devices because
each one is decomposed to three CNOT gates. However, swaps are usually necessary
for embedding a circuit to match the limited connectivity of actual devices.
Thus, a primary goal of the transpilation process is to minimize the number of swap gates
required to make the circuit compatible with the target device.

Copy link
Contributor

@kevinhartman kevinhartman left a comment

Choose a reason for hiding this comment

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

LGTM

qiskit/transpiler/preset_passmanagers/__init__.py Outdated Show resolved Hide resolved
@itoko
Copy link
Contributor

itoko commented Jan 26, 2023

Overall LGTM. I have one comment on Overview section. When I first read it, I was a bit confused about the misalignment between "steps" in the first figure and "stages" defined in preset pass manager. Some reader would expect one-on-one mapping between the six steps in the figure and the six stages in the main text but in fact the first two steps will be implemented in the init stage and there is no step corresponding to scheduling stage in the figure. The "steps" seems a classification of transpiler tasks (often seen in the transpiler research literature) and the "stages" is a grouping of their implementations (passes) in Qiskit. I think a little more explanation something like above things might be helpful for novice readers.

Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

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

Last few comments, then looks good to me! Didn't review through the Scheduling section as I think it was unchanged.

qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
Comment on lines +746 to +747
keyword argument, where the index labels the virtual qubit in the circuit and the
corresponding value is the label for the physical qubit to map onto:
Copy link
Member

Choose a reason for hiding this comment

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

Agree, and I think the example below does a good job of clarifying the format should be used. I'm just wondering if there's a clearer text explanation we can give (but I wouldn't be surprised if not, there isn't one in e.g. the transpile docstring).

qiskit/transpiler/__init__.py Outdated Show resolved Hide resolved
try a few different methods to find the best layout. Typically this involves 2 stages, first
trying to find a "perfect" layout (a layout which does not require any swap operations) and
a heuristic pass that tries to find the best layout to use if a perfect layout can not be found.
For the first stage there are 2 passes typically used for this:
Copy link
Member

Choose a reason for hiding this comment

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

True, but even with translation, there are the unroll and synthesis methods which we could highlight here, likewise routing has stochastic and lookahead. I suppose I'm saying if we're going to document the pass behaviors and available *_method options for layout here, we should do the same for translation and routing.

As an alternative, the documentation here could be at the level of "here are the default stages and the problems they solve" and then for folks to understand the implementation of those stages at each optimization level, we could move the descriptions of the passes being run (and corresponding *_method options) to the documentation of the preset pass manager generators.

@mtreinish mtreinish requested a review from kdk January 26, 2023 14:33
Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, LGTM.

@kdk kdk added the automerge label Jan 26, 2023
@mergify mergify bot merged commit 925f9f3 into Qiskit:main Jan 26, 2023
mergify bot pushed a commit that referenced this pull request Jan 26, 2023
* Improve transpiler documentation

This commit expands the module documentation for the qiskit.transpiler
module to explain details of working with preset pass managers and how
to modify stages for custom workflows. It also updates some of the
details about the stages which were a bit stale on the exact behavior of
the transpiler.

* Apply suggestions from code review

Co-authored-by: Jake Lishman <jake@binhbar.com>

* More refactors from code review

* Update reference label for stages section

* Add section headers to scheduling stage docs

* Fix lint

* Fix typo

* Update examples to show stochastic passes

* Fix rebase typos and example issues

* Update qiskit/transpiler/__init__.py

* Apply suggestions from code review

Co-authored-by: Kevin Krsulich <kevin@krsulich.net>

* Fix trailing whitespace

* Expand init stage definition

* Apply suggestions from code review

Co-authored-by: Kevin Krsulich <kevin@krsulich.net>

* Simplify target section intro

* Highlight legacy views are for backwards compatibility

* Updating layout and routing for extra details

* Apply suggestions from code review

Co-authored-by: Kevin Hartman <kevin@hart.mn>

* Update qiskit/transpiler/preset_passmanagers/__init__.py

Co-authored-by: Kevin Hartman <kevin@hart.mn>

* More updates from code review

* Fix line length lint failure

* Apply suggestions from code review

Co-authored-by: Kevin Krsulich <kevin@krsulich.net>

* Expand sabre layout explanation

* Explain pass description in stage details header

Co-authored-by: Jake Lishman <jake@binhbar.com>
Co-authored-by: Kevin Krsulich <kevin@krsulich.net>
Co-authored-by: Kevin Hartman <kevin@hart.mn>
(cherry picked from commit 925f9f3)
mergify bot added a commit that referenced this pull request Jan 26, 2023
* Improve transpiler documentation

This commit expands the module documentation for the qiskit.transpiler
module to explain details of working with preset pass managers and how
to modify stages for custom workflows. It also updates some of the
details about the stages which were a bit stale on the exact behavior of
the transpiler.

* Apply suggestions from code review

Co-authored-by: Jake Lishman <jake@binhbar.com>

* More refactors from code review

* Update reference label for stages section

* Add section headers to scheduling stage docs

* Fix lint

* Fix typo

* Update examples to show stochastic passes

* Fix rebase typos and example issues

* Update qiskit/transpiler/__init__.py

* Apply suggestions from code review

Co-authored-by: Kevin Krsulich <kevin@krsulich.net>

* Fix trailing whitespace

* Expand init stage definition

* Apply suggestions from code review

Co-authored-by: Kevin Krsulich <kevin@krsulich.net>

* Simplify target section intro

* Highlight legacy views are for backwards compatibility

* Updating layout and routing for extra details

* Apply suggestions from code review

Co-authored-by: Kevin Hartman <kevin@hart.mn>

* Update qiskit/transpiler/preset_passmanagers/__init__.py

Co-authored-by: Kevin Hartman <kevin@hart.mn>

* More updates from code review

* Fix line length lint failure

* Apply suggestions from code review

Co-authored-by: Kevin Krsulich <kevin@krsulich.net>

* Expand sabre layout explanation

* Explain pass description in stage details header

Co-authored-by: Jake Lishman <jake@binhbar.com>
Co-authored-by: Kevin Krsulich <kevin@krsulich.net>
Co-authored-by: Kevin Hartman <kevin@hart.mn>
(cherry picked from commit 925f9f3)

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@mtreinish mtreinish deleted the improve-transpiler-docs branch January 27, 2023 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog documentation Something is not clear or an error documentation stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants