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

add synthesis plugin docs #11763

Merged
merged 11 commits into from
Feb 15, 2024
Merged

add synthesis plugin docs #11763

merged 11 commits into from
Feb 15, 2024

Conversation

alexanderivrii
Copy link
Contributor

@alexanderivrii alexanderivrii commented Feb 11, 2024

Summary

Adds API docs section for synthesis plugins.
Fixes #11675.

UPDATE:

For each relevant high-level-object (e.g., Clifford, LinearFunction or PermutationGate) we are adding a table comparing different plugins available for this object, and the links to the available plugins (to easy finding the relevant documentation).

@coveralls
Copy link

coveralls commented Feb 11, 2024

Pull Request Test Coverage Report for Build 7914893703

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.

Details

  • 0 of 2 (100.0%) changed or added relevant lines in 2 files are covered.
  • 156 unchanged lines in 35 files lost coverage.
  • Overall coverage increased (+0.006%) to 89.219%

Files with Coverage Reduction New Missed Lines %
qiskit/circuit/library/standard_gates/global_phase.py 1 95.83%
qiskit/circuit/library/standard_gates/rx.py 1 98.55%
qiskit/circuit/library/standard_gates/ry.py 1 98.51%
qiskit/circuit/library/standard_gates/ryy.py 1 97.3%
qiskit/circuit/library/standard_gates/rz.py 1 98.46%
qiskit/circuit/library/standard_gates/rzz.py 1 97.06%
qiskit/circuit/library/standard_gates/swap.py 1 98.08%
qiskit/circuit/library/standard_gates/u3.py 1 98.9%
qiskit/circuit/library/standard_gates/xx_minus_yy.py 1 97.67%
qiskit/circuit/library/standard_gates/xx_plus_yy.py 1 97.67%
Totals Coverage Status
Change from base Build 7837327418: 0.006%
Covered Lines: 58855
Relevant Lines: 65967

💛 - Coveralls

:toctree: ../stubs/

/qiskit/transpiler/passes/synthesis/unitary_synthesis/DefaultUnitarySynthesis
/qiskit/transpiler/passes/synthesis/solovay_kitaev_synthesis/SolovayKitaevSynthesis
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
Member

Choose a reason for hiding this comment

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

should these docs replace the docs here: https://docs.quantum.ibm.com/api/qiskit/dev/transpiler_passes#synthesis
as it seems that there is some overlap between them

Remove duplicate AQC plugin entry in synthesis docs and replace with module link.
Remove duplicate transpiler builtin plugin page.
@ShellyGarion
Copy link
Member

I don't think that the file docs/apidoc/synthesis_aqc.rst should be removed. It contains the entire AQC synthesis code, and not the AQCSynthesisPlugin, so now the following link from qiskit.synthesis does not work:
The Approximate Quantum Compiler is available here: qiskit.synthesis.unitary.aqc

@ElePT
Copy link
Contributor

ElePT commented Feb 14, 2024

I don't think that the file docs/apidoc/synthesis_aqc.rst should be removed. It contains the entire AQC synthesis code, and not the AQCSynthesisPlugin, so now the following link from qiskit.synthesis does not work: The Approximate Quantum Compiler is available here: qiskit.synthesis.unitary.aqc

@ShellyGarion Let me take a look, I removed the file because sphinx was complaining that it wasn't part of any toctree (because the builtin plugin file was the only one using it), but we should be able to make the link work without having an individual file for it. If I can't find the way, let's bring it back. At least, do you think the synthesis plugin page looks like it was intended to?

@ShellyGarion
Copy link
Member

I think that the AQCSynthesisPlugin looks OK, but this page has disappeared:
https://docs.quantum.ibm.com/api/qiskit/synthesis_aqc

@ElePT
Copy link
Contributor

ElePT commented Feb 14, 2024

@ShellyGarion I think that cf11cb7 has fixed the AQC page. I managed to generate it without an additional rst file.

@ShellyGarion
Copy link
Member

@ShellyGarion I think that cf11cb7 has fixed the AQC page. I managed to generate it without an additional rst file.

Indeed, it looks much better now. thanks!

image

Comment on lines +13 to +15
==============================================================================
AQC Synthesis Plugin (in :mod:`qiskit.transpiler.passes.synthesis.aqc_plugin`)
==============================================================================
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried removing these titles for the aqc/solovay-kitaev/unitary synthesis plugins but I think the result looks a bit strange:

Screenshot 2024-02-14 at 15 46 08

So I decided to leave the titles, but either way is possible:

Screenshot 2024-02-14 at 15 47 56

Comment on lines 99 to 133
.. list-table:: Plugins for :class:`.PermutationGate` (key = ``"permutation"``)
:header-rows: 1

* - Plugin name
- Plugin class
- Targeted connectivity
- Description
* - ``"basic"``
- :class:`~.BasicSynthesisPermutation`
- all-to-all
- optimal swap count
* - ``"acg"``
- :class:`~.ACGSynthesisPermutation`
- all-to-all
- swap depth of at most `2`
* - ``"kms"``
- :class:`~.KMSSynthesisPermutation`
- linear
- swap depth of at most `n`
* - ``"token_swapper"``
- :class:`~.TokenSwapperSynthesisPermutation`
- any
-
* - ``"default"``
- :class:`~.BasicSynthesisPermutation`
- all-to-all
- same as ``"basic"``

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

BasicSynthesisPermutation
ACGSynthesisPermutation
KMSSynthesisPermutation
TokenSwapperSynthesisPermutation
Copy link
Contributor

Choose a reason for hiding this comment

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

I also tried integrating these with little success, but I think that having 2 tables is not that bad

Uploading Screenshot 2024-02-14 at 14.44.53.png…

@jakelishman
Copy link
Member

This is marked as closing an issue in the 1.0.0 milestone. Is it ready to review/merge now?

We can always tweak documentation after the release, but it'd be good to have at least the main skeleton in to begin with (and this already looks fairly done?). We could merge this and have more PRs later if that's convenient?

@jakelishman jakelishman added this to the 1.0.0 milestone Feb 15, 2024
@jakelishman jakelishman added 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 Changelog: None Do not include in changelog labels Feb 15, 2024
@alexanderivrii
Copy link
Contributor Author

Sorry for the delay. I would like to make one more quick pass over the text in the tables. And many thanks to @ElePT for turning the initial mess of this PR to the current form.

@alexanderivrii alexanderivrii changed the title [WIP] synthesis plugin docs add synthesis plugin docs Feb 15, 2024
@alexanderivrii alexanderivrii marked this pull request as ready for review February 15, 2024 11:07
@alexanderivrii alexanderivrii requested a review from a team as a code owner February 15, 2024 11:07
@qiskit-bot
Copy link
Collaborator

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

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.

Thanks for this, Sasha, Elena and Shelly - it looks like a really big improvement over what we've (not!) got so far.

@jakelishman jakelishman added this pull request to the merge queue Feb 15, 2024
@ShellyGarion ShellyGarion self-requested a review February 15, 2024 12:04
Merged via the queue into Qiskit:main with commit 9184907 Feb 15, 2024
12 checks passed
mergify bot pushed a commit that referenced this pull request Feb 15, 2024
* very initial commit

* fix?

* temporarily removing

* is this one adds absolute paths?

* partially restoring

* another experiment

* adding the remaining information

* Move docs to corresponding file and link from top.
Remove duplicate AQC plugin entry in synthesis docs and replace with module link.
Remove duplicate transpiler builtin plugin page.

* Generate AQC page

* Remove plugin from synthesis docs, refer to new plugin docs.

* adding short descriptions of available plugins

---------

Co-authored-by: Elena Peña Tapia <epenatap@gmail.com>
(cherry picked from commit 9184907)
github-merge-queue bot pushed a commit that referenced this pull request Feb 15, 2024
* very initial commit

* fix?

* temporarily removing

* is this one adds absolute paths?

* partially restoring

* another experiment

* adding the remaining information

* Move docs to corresponding file and link from top.
Remove duplicate AQC plugin entry in synthesis docs and replace with module link.
Remove duplicate transpiler builtin plugin page.

* Generate AQC page

* Remove plugin from synthesis docs, refer to new plugin docs.

* adding short descriptions of available plugins

---------

Co-authored-by: Elena Peña Tapia <epenatap@gmail.com>
(cherry picked from commit 9184907)

Co-authored-by: Alexander Ivrii <alexi@il.ibm.com>
@alexanderivrii alexanderivrii deleted the plugin-docs branch March 27, 2024 09:17
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.

Add API docs section for synthesis plugins post-1.0.0rc1
6 participants