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 representation of "switch" #9833

Merged
merged 8 commits into from
Apr 4, 2023
Merged

Add representation of "switch" #9833

merged 8 commits into from
Apr 4, 2023

Conversation

jakelishman
Copy link
Member

Summary

This simply adds the representation of a switch statement to Terra, without any of the extra supported needed anywhere else in the package. These will come in follow-up commits.

Details and comments

It's possible there are references to a builder interface left in this PR in places - I tried to squash all of those when I rebased my original branch (I'll add the builder in a follow-up, and it still needs tests writing). I can either fix them in this PR, or just leave it to make sense when the builder interface is added again.

There's no support added in the rest of the library for this new operation yet; I'll do that in follow-ups.

@jakelishman jakelishman added the Changelog: New Feature Include in the "Added" section of the changelog label Mar 21, 2023
@jakelishman jakelishman added this to the 0.24.0 milestone Mar 21, 2023
@jakelishman jakelishman requested a review from a team as a code owner March 21, 2023 17:14
@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

This simply adds the representation of a switch statement to Terra,
without any of the extra supported needed anywhere else in the package.
These will come in follow-up commits.
@coveralls
Copy link

coveralls commented Mar 21, 2023

Pull Request Test Coverage Report for Build 4565467629

  • 64 of 76 (84.21%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.001%) to 85.379%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/circuit/controlflow/switch_case.py 59 71 83.1%
Totals Coverage Status
Change from base Build 4564463001: -0.001%
Covered Lines: 67319
Relevant Lines: 78847

💛 - Coveralls

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 overall.

I'm digging this interface—very clean!

qiskit/circuit/controlflow/switch_case.py Outdated Show resolved Hide resolved
qiskit/circuit/controlflow/switch_case.py Outdated Show resolved Hide resolved
qiskit/circuit/quantumcircuit.py Outdated Show resolved Hide resolved
kevinhartman
kevinhartman previously approved these changes Mar 30, 2023
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!

@jakelishman
Copy link
Member Author

Let me poke @kdk for review before as well, since most of the control-flow representation stuff is his.

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.

LGTM

@kdk kdk added this pull request to the merge queue Apr 4, 2023
@jakelishman
Copy link
Member Author

From offline Slack discussions, and just for posterity (it's me typing):

for params: I put the circuit bodies in the params so that QuantumCircuit.assign_parameters could continue to work in its current form (though ideally I’d have that use ControlFlowOp.blocks), but I didn’t use the case labels there because there’s really nothing meaningful you could possibly do with them via params

any attempt to modify them would likely invalidate the data structures, and if they’re mashed into a flat structure in params, they’d be no use to the user

Merged via the queue into Qiskit:main with commit 2b8c093 Apr 4, 2023
@jakelishman jakelishman deleted the switch/repr branch April 4, 2023 23:30
giacomoRanieri pushed a commit to giacomoRanieri/qiskit-terra that referenced this pull request Apr 16, 2023
* Add representation of "switch"

This simply adds the representation of a switch statement to Terra,
without any of the extra supported needed anywhere else in the package.
These will come in follow-up commits.

* Add missing tests

* Fix test naming collision

* Clarify comments on representation

* Remove comment overlooked in rebase

* Fix lint
king-p3nguin pushed a commit to king-p3nguin/qiskit-terra that referenced this pull request May 22, 2023
* Add representation of "switch"

This simply adds the representation of a switch statement to Terra,
without any of the extra supported needed anywhere else in the package.
These will come in follow-up commits.

* Add missing tests

* Fix test naming collision

* Clarify comments on representation

* Remove comment overlooked in rebase

* Fix lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants