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

Fix classical-synthesis label ordering with registerless=False #9536

Merged
merged 12 commits into from
Aug 16, 2023

Conversation

1ucian0
Copy link
Member

@1ucian0 1ucian0 commented Feb 4, 2023

Fixes #9363

from qiskit.circuit.classicalfunction import classical_function
from qiskit.circuit.classicalfunction.types import Int1

@classical_function
def grover_oracle(a: Int1, b: Int1, c: Int1) -> Int1:
    return (a and b and not c)

quantum_circuit = grover_oracle.synth(registerless=False)
print(quantum_circuit.draw())
     a: ──■──
          │  
     b: ──■──
          │  
     c: ──o──
        ┌─┴─┐
return: ┤ X ├
        └───┘

@1ucian0 1ucian0 requested a review from a team as a code owner February 4, 2023 20:46
@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 Feb 4, 2023

Pull Request Test Coverage Report for Build 5868913929

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 23 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.03%) to 87.235%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 5 90.63%
crates/qasm2/src/parse.rs 18 96.2%
Totals Coverage Status
Change from base Build 5868812324: -0.03%
Covered Lines: 74277
Relevant Lines: 85146

💛 - Coveralls

@1ucian0 1ucian0 added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Feb 20, 2023
@jakelishman jakelishman changed the title non-registerless synthesis labels were in reverse Fix classical-synthesis label ordering with registerless=False Feb 27, 2023
Comment on lines 51 to 52
from qiskit.circuit.classicalfunction import classical_function
from qiskit.circuit.classicalfunction.types import Int1
Copy link
Member

Choose a reason for hiding this comment

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

Tweedledum isn't a required import, so these lines need to be protected within the function, and it needs decorating with a @unittest.skipUnless(optionals.HAS_TWEEDLEDUM, ...) line.

Copy link
Member Author

@1ucian0 1ucian0 Feb 28, 2023

Choose a reason for hiding this comment

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

oh! I forgot. Thanks! Fixed in 91069ad

Comment on lines 4 to 5
Fixed `#9363 <https://github.com/Qiskit/qiskit-terra/issues/9363>`__.
by labeling the non-registerless synthesis in the order that Tweedledum expects.
Copy link
Member

Choose a reason for hiding this comment

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

This could maybe stand a shade more explanation.

Copy link
Member Author

@1ucian0 1ucian0 Feb 28, 2023

Choose a reason for hiding this comment

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

True. Adding more in 232e4e5

Comment on lines 76 to 77
ClassicalFunctionCompilerError: If there a gate in the Tweedledum circuit has no Qiskit
ClassicalFunctionCompilerError: If there is a gate in the Tweedledum circuit has no Qiskit
equivalent.
Copy link
Member

Choose a reason for hiding this comment

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

It's either "If there is a gate in the Tweedledum circuit that has no ..." or just delete at the start to make it "If a gate in ..."

Copy link
Member Author

@1ucian0 1ucian0 Feb 28, 2023

Choose a reason for hiding this comment

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

fixed in aa04278

@Cryoris Cryoris added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Aug 15, 2023
@mtreinish mtreinish added this to the 0.25.1 milestone Aug 15, 2023
@Cryoris Cryoris added this pull request to the merge queue Aug 16, 2023
Merged via the queue into Qiskit:main with commit 287ac5c Aug 16, 2023
14 checks passed
mergify bot pushed a commit that referenced this pull request Aug 16, 2023
* follow the Tweedledum convention on endianness

* reno

* add test

* optional dep

* docstring

* reno

* attempt to fix sphinx

---------

Co-authored-by: Julien Gacon <gaconju@gmail.com>
(cherry picked from commit 287ac5c)
github-merge-queue bot pushed a commit that referenced this pull request Aug 16, 2023
…) (#10647)

* follow the Tweedledum convention on endianness

* reno

* add test

* optional dep

* docstring

* reno

* attempt to fix sphinx

---------

Co-authored-by: Julien Gacon <gaconju@gmail.com>
(cherry picked from commit 287ac5c)

Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
SamD-1998 pushed a commit to SamD-1998/qiskit-terra that referenced this pull request Sep 7, 2023
…kit#9536)

* follow the Tweedledum convention on endianness

* reno

* add test

* optional dep

* docstring

* reno

* attempt to fix sphinx

---------

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog 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.

Calling synth(registerless=False) on a classical function synth labels the qubits wrongly
6 participants