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 conversion transpiler pass from c_if to IfElseOp #8764

Merged
merged 7 commits into from
Sep 29, 2022

Conversation

jakelishman
Copy link
Member

@jakelishman jakelishman commented Sep 15, 2022

Summary

This is intended to be used to ease the transition from the old-style conditions to the new-style form. Backends may use it in their custom pipelines, so they can guarantee they only need to deal with one form.

A further classical optimisation may be to group IfElseOp blocks that share the same condition into a single operation, but this would likely be a separate pass, if added, or potentially be the domain of a separate part of the circuit-description/compilation/running pipeline.

Details

Fixes #8601.

Will need rebasing over #8627 and #8615 once they merge, as this depends on them, and currently includes their commits. I just can't look at my screen any more tonight to wait til it merges.

edit: This is now rebased.

cc: @taalexander (this is the pass I wrote for you)

@jakelishman jakelishman requested a review from a team as a code owner September 15, 2022 23:02
@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 is intended to be used to ease the transition from the old-style
conditions to the new-style form.  Backends may use it in their custom
pipelines, so they can guarantee they only need to deal with one form.

A further classical optimisation may be to group `IfElseOp` blocks that
share the same condition into a single operation, but this would likely
be a separate pass, if added, or potentially be the domain of a separate
part of the circuit-description/compilation/running pipeline.
@jakelishman jakelishman added the Changelog: New Feature Include in the "Added" section of the changelog label Sep 16, 2022
@coveralls
Copy link

coveralls commented Sep 20, 2022

Pull Request Test Coverage Report for Build 3147692364

  • 47 of 47 (100.0%) changed or added relevant lines in 3 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage remained the same at 84.502%

Files with Coverage Reduction New Missed Lines %
qiskit/extensions/quantum_initializer/squ.py 2 79.78%
Totals Coverage Status
Change from base Build 3147690270: 0%
Covered Lines: 60254
Relevant Lines: 71305

💛 - Coveralls

@jakelishman jakelishman added this to the 0.22 milestone Sep 21, 2022
@mtreinish mtreinish self-assigned this Sep 27, 2022
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.

Overall the code LGTM, this is a pretty straightforward replacement and you're more thorough than I would have been as I never would have thought about a user using c_if inside a control flow block (but that could be because I'm not really used to dealing with recursive pass calling yet to work with control flow).

The only thing that I think would be good before we merge is some negative (as in not conditional instructions) test coverage. I think just a couple test to test a dag with no conditional operations to validate that it is unchanged on the output and also a test checking a control flow block with no conditional operations inside it (which was my one inline comment). They're not super important but just asserting this as part of the api will hopefully prevent bugs in the future just in case.

@mergify mergify bot merged commit fca3864 into Qiskit:main Sep 29, 2022
@jakelishman jakelishman deleted the c_if-to-if_else-pass branch September 29, 2022 12:15
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 priority: medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transpiler pass for converting c_if to if_test
4 participants