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 more classes for 2q synthesis to qiskit synthesis docs #11687

Merged
merged 5 commits into from
Feb 1, 2024

Conversation

ShellyGarion
Copy link
Member

@ShellyGarion ShellyGarion commented Jan 31, 2024

Summary

Following the discussions in #11460, #11635 and #11672, we properly add some more classes for 2q synthesis to qiskit synthesis docs.

Details and comments

@ShellyGarion ShellyGarion added the documentation Something is not clear or an error documentation label Jan 31, 2024
@ShellyGarion ShellyGarion added this to the 1.0.0 milestone Jan 31, 2024
@ShellyGarion ShellyGarion requested review from alexanderivrii and a team as code owners January 31, 2024 12:27
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core
  • @levbishop

@coveralls
Copy link

coveralls commented Jan 31, 2024

Pull Request Test Coverage Report for Build 7746292644

  • 0 of 3 (100.0%) changed or added relevant lines in 3 files are covered.
  • 10 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.01%) to 89.381%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 4 91.94%
crates/qasm2/src/parse.rs 6 97.62%
Totals Coverage Status
Change from base Build 7746232749: 0.01%
Covered Lines: 60155
Relevant Lines: 67302

💛 - Coveralls

@ShellyGarion ShellyGarion requested a review from Cryoris January 31, 2024 14:57
Comment on lines +17 to +18
two_qubit_cnot_decompose,
TwoQubitWeylDecomposition,
Copy link
Contributor

Choose a reason for hiding this comment

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

A search through qiskit-community showed that only these two objects are used. I'd suggest we use this as criterion and only add those classes to the API docs that are really needed 🙂

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine starting small, also looking at the code for all the specialized TwoQubitWeyl* variants it looks like they're all used internally by __new__ when you create a new TwoQubitWeylDecomposition object.

Copy link
Member

Choose a reason for hiding this comment

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

TwoQubitWeylDecomposer kind of implies that all the family classes are public in the sense of going to a lot of trouble to make a repr that you can eval back in (including pickling then b64'ing a Numpy array and making sure __new__ eats keywords that it throws in the repr just to be pretty) (it's a very busy constructor). That said, I don't really think it's necessary to make those public at all - they're largely not meant to be accessed directly, I think, because of the abstract factory class.

Co-authored-by: Julien Gacon <gaconju@gmail.com>
@mtreinish mtreinish changed the title [WIP] Add more classes for 2q synthesis to qiskit synthesis docs Add more classes for 2q synthesis to qiskit synthesis docs Feb 1, 2024
@mtreinish mtreinish added the Changelog: None Do not include in changelog label Feb 1, 2024
@mtreinish mtreinish enabled auto-merge February 1, 2024 19:01
@mtreinish
Copy link
Member

I did a quick search through the code, and confirmed that the only intended public interfaces are probably what @Cryoris pointed out in his review comments. The other classes were either added for internal features (such as QSD) or were magically dispatched for TwoQubitWeylDecomposition. So I restricted the changes in this PR to just the classes which seem to be intended for public use. If we do miss something here we can add the missing things to the public api in 1.1.0

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 is already tagged for merge, but here's a review basically to say I agree.

from .two_qubit.two_qubit_decompose import TwoQubitBasisDecomposer, two_qubit_cnot_decompose
from .two_qubit.two_qubit_decompose import (
TwoQubitBasisDecomposer,
two_qubit_cnot_decompose,
Copy link
Member

Choose a reason for hiding this comment

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

I think this two_qubit_cnot_decompose is missing an .. autofunction:: in the docstring? If it's meant to be there, let's take a note to add it after rc1 - no need to hold up the PR over a documentation thing.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yeah good catch I missed that in the original review. I'll open an issue to track this. It actually will likely require a full docstring addition as it's doesn't have anything there now: https://github.com/Qiskit/qiskit/blob/main/qiskit/synthesis/two_qubit/two_qubit_decompose.py#L1553

it's a lazy loaded instance of TwoQubitBasisDecomposer using cx.

Copy link
Member

Choose a reason for hiding this comment

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

I actually already opened #11709 for it - forgot to comment here.

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 also wasn't sure what to do with this function, since it has no docstring, and also due to the comment here:

# This weird duplicated lazy structure is for backwards compatibility; Qiskit has historically

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for taking care of this PR

Comment on lines +17 to +18
two_qubit_cnot_decompose,
TwoQubitWeylDecomposition,
Copy link
Member

Choose a reason for hiding this comment

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

TwoQubitWeylDecomposer kind of implies that all the family classes are public in the sense of going to a lot of trouble to make a repr that you can eval back in (including pickling then b64'ing a Numpy array and making sure __new__ eats keywords that it throws in the repr just to be pretty) (it's a very busy constructor). That said, I don't really think it's necessary to make those public at all - they're largely not meant to be accessed directly, I think, because of the abstract factory class.

@mtreinish mtreinish added this pull request to the merge queue Feb 1, 2024
Merged via the queue into Qiskit:main with commit 4425460 Feb 1, 2024
12 checks passed
garrison added a commit to Qiskit/qiskit-addon-cutting that referenced this pull request Apr 12, 2024
…ion`

This is a follow-up to the conversation at
Qiskit/qiskit#11635 (comment).

The toplevel package import was made available in
Qiskit/qiskit#11687
in time for Qiskit 1.0.
garrison added a commit to Qiskit/qiskit-addon-cutting that referenced this pull request Apr 12, 2024
…ion` (#547)

This is a follow-up to the conversation at
Qiskit/qiskit#11635 (comment).

The toplevel package import was made available in
Qiskit/qiskit#11687
in time for Qiskit 1.0.
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants