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

Added a How-To guide for transpiler optimization level #10303

Open
wants to merge 34 commits into
base: main
Choose a base branch
from

Conversation

MaldoAlberto
Copy link
Contributor

@MaldoAlberto MaldoAlberto commented Jun 17, 2023

Summary

Documentation of the optimization_level parameter in the transpile method, taking as reference #8574

Details and comments

@MaldoAlberto MaldoAlberto requested a review from a team as a code owner June 17, 2023 02:57
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jun 17, 2023
@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:

@MaldoAlberto MaldoAlberto changed the title Optimization level Added a How-To guide for transpiler optimizaiton level Jun 19, 2023
Copy link
Contributor

@javabster javabster 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 your submission @MaldoAlberto, a few things need to be tidied up then I'll do a proper content review:

Firstly, the file structure needs a bit of a rework. We keep all the how to guides in the docs/how_to folder, so the main .rst file should be moved there. Also the images should be kept in the docs/source_images folder.

Secondly, it looks like CI is failing for the docs build. Taking a look at the logs it says WARNING: document isn't included in any toctree. I think this is probably because you need to move the files into the right folder as I mentioned above. In addition you will need to add the file name for the new how to guide to docs/how_to/index.rst

@MaldoAlberto
Copy link
Contributor Author

Thanks @javabster I did the changes

@coveralls
Copy link

coveralls commented Jun 21, 2023

Pull Request Test Coverage Report for Build 5764174047

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2394 unchanged lines in 144 files lost coverage.
  • Overall coverage decreased (-0.06%) to 85.917%

Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/sabre_layout.rs 1 99.43%
crates/qasm2/src/lex.rs 1 92.15%
qiskit/circuit/commutation_checker.py 1 98.51%
qiskit/circuit/library/n_local/efficient_su2.py 1 94.74%
qiskit/circuit/library/n_local/excitation_preserving.py 1 95.83%
qiskit/circuit/parametertable.py 1 88.19%
qiskit/opflow/gradients/qfi.py 1 95.65%
qiskit/quantum_info/operators/base_operator.py 1 98.08%
qiskit/quantum_info/operators/mixins/adjoint.py 1 92.86%
qiskit/quantum_info/operators/mixins/multiply.py 1 94.44%
Totals Coverage Status
Change from base Build 5520034869: -0.06%
Covered Lines: 73086
Relevant Lines: 85066

💛 - Coveralls

@MaldoAlberto
Copy link
Contributor Author

MaldoAlberto commented Jun 29, 2023

@Eric-Arellano I did new version of the documentation

docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@atharva-satpute atharva-satpute left a comment

Choose a reason for hiding this comment

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

Please fix the typo

docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
@MaldoAlberto MaldoAlberto changed the title Added a How-To guide for transpiler optimizaiton level Added a How-To guide for transpiler optimization level Jul 3, 2023
Copy link
Collaborator

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Nice work so far!

docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

I left a few suggestions and comments inline about some things that I think need to be updated here. A few of the more mechanical suggestions apply to all the instances (like the sphinx refs in particular), but I only commented on a few at the beginning.

My biggest concerns are around using the legacy BackendV1 interface and some accuracy/clarity issue in some of the statements being made about the differences between the optimization levels. Also I'm a bit worried about using hardcoded doc test examples with output in this doc. The transpiler's output changes is non-deterministic because we use a stochastic algorithm for layout and routing, so the output can change depending on seed. But also using hard coded output like this will go stale anytime we make an improvement to the transpiler which changes the output circuit. In general I think it would be better to use the matplotlib plot directive: https://matplotlib.org/stable/api/sphinxext_plot_directive_api.html for all the visualizations so they're generated on the fly. The overhead of the running this code as part of the sphinx build should be minimal as the circuit is very small so the compilation and visualization will be relatively quick.

That being said while I think some of the content here is largely duplicated in the transpiler module documentation, but I realize the how to section is a different target audience and having some duplication between the two documents is ok as there is a different target audience and entry point people, so I'm glad to see this contribution. Thanks for getting it started.

docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@Eric-Arellano Eric-Arellano 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! It sounds like this still needs to migrate to a V2 backend.

#####################################

This guide shows you how to use the ``optimization_level``
parameter with :func:`~qiskit.compiler.transpile`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The ~ results in only showing transpile rather than qiskit.compiler.transpile. Imo it is useful to show the whole import path.

Suggested change
parameter with :func:`~qiskit.compiler.transpile`.
parameter with :func:`qiskit.compiler.transpile`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is ok because just put a link in the method, and the oder tutorials use the same format

docs/how_to/use_optimizationlevel.rst Show resolved Hide resolved
@MaldoAlberto
Copy link
Contributor Author

Hi @mtreinish I did the changes your explanation for the different levels help me more to understand the background of this method, so I added a short sentence for this, as this tutorial is more explain how to use optimization level and no education, but is interesting to learn more how is working the code of optimization_level, I'm thinking if is possible explain more in this or in other kind of tutorial

Copy link
Member

@frankharkins frankharkins left a comment

Choose a reason for hiding this comment

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

Nice work! I've left quite a few comments but most are minor, the only important bits are to do with headings, and a couple of sentences I didn't understand.

docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
@MaldoAlberto
Copy link
Contributor Author

@frankharkins Thank you so much for all the feedback, I did the changes, I hope is now better :D

Copy link
Member

@frankharkins frankharkins left a comment

Choose a reason for hiding this comment

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

Thanks! Sorry just a few more comments from me.

docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
Operation names of your backend: ['id', 'rz', 'sx', 'x', 'cx', 'reset']
Coupling map connection of your backend: [(3, 4), (4, 3), (1, 3), (3, 1), (1, 2), (2, 1), (0, 1), (1, 0)]

When setting the ``optimization_level`` to 0, the resulting quantum circuit is not optimized and simply mapped to the device, considering a trivial layout and stochastic swap.
Copy link
Member

@frankharkins frankharkins Aug 2, 2023

Choose a reason for hiding this comment

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

@MaldoAlberto "considering" is incorrect here, it basically means "think about carefully", so this sentence means "thinking about a trivial layout and stochastic swap".

I'd switch "considering" -> "using".

Suggested change
When setting the ``optimization_level`` to 0, the resulting quantum circuit is not optimized and simply mapped to the device, considering a trivial layout and stochastic swap.
When setting the ``optimization_level`` to 0, the resulting quantum circuit is not optimized and simply mapped to the device using a trivial layout and stochastic swap.

docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
@MaldoAlberto
Copy link
Contributor Author

@frankharkins thank you so much for your nice feedback, explanation and resource 😄 I did the changes

Copy link
Member

@frankharkins frankharkins left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

Copy link
Member

@mtreinish mtreinish 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 great, thanks for all the hard work with all the updates. I think this is basically ready, I just left a couple small inline wording suggestions to make some of the details about the transpiler levels a bit clearer to me.

Other than that I think the only other thing is we can drop most of the testcode blocks that are calling draw. The plot directive has an include-source option that will enable us to display the code side by side with the generated visualization. If the issue is that you want to reuse state between plot calls there is a context option you can use to enable sharing context between calls.

docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
docs/how_to/use_optimizationlevel.rst Outdated Show resolved Hide resolved
@mtreinish mtreinish dismissed their stale review August 9, 2023 20:15

Comments addressed

@MaldoAlberto
Copy link
Contributor Author

@Eric-Arellano Hi! I son{t know iid is ok this version or I need to do some changeS?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PR PRs from contributors that are not 'members' of the Qiskit repo
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

9 participants