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 transpile() with a Target containing an ideal Measure #8995

Merged
merged 2 commits into from
Oct 28, 2022

Conversation

mtreinish
Copy link
Member

Summary

This commit fixes an oversight in the transpile() function when running with a Target (either directly or via a backend) that contains a Measurement operation that is ideal (either globally or locally) with no properties defined. This was not handled correctly in the function used to convert a Target to a BackendProperties payload for passes that are not yet target aware and this would cause an exception to be raised. This commit fixes this edge case and excludes readout properties from the generated BackendProperties in this case.

Details and comments

Fixes #8969

This commit fixes an oversight in the transpile() function when running
with a Target (either directly or via a backend) that contains a
Measurement operation that is ideal (either globally or locally) with no
properties defined. This was not handled correctly in the function used
to convert a Target to a BackendProperties payload for passes that are
not yet target aware and this would cause an exception to be raised.
This commit fixes this edge case and excludes readout properties from
the generated BackendProperties in this case.

Fixes Qiskit#8969
@mtreinish mtreinish added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: Bugfix Include in the "Fixed" section of the changelog labels Oct 25, 2022
@mtreinish mtreinish added this to the 0.22.1 milestone Oct 25, 2022
@mtreinish mtreinish requested a review from a team as a code owner October 25, 2022 19:23
@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 Oct 25, 2022

Pull Request Test Coverage Report for Build 3342757244

  • 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.07%) to 84.486%

Files with Coverage Reduction New Missed Lines %
qiskit/extensions/quantum_initializer/squ.py 2 79.78%
Totals Coverage Status
Change from base Build 3342084476: 0.07%
Covered Lines: 62157
Relevant Lines: 73571

💛 - Coveralls

Copy link
Contributor

@itoko itoko left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the quick fix. I left one minor comment inline. It's a nitpick, so not worth blocking the merge.

Comment on lines 1065 to 1067
if not props_list:
qubit_props = {}
break
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be a too small nitpick in the original code, but these lines suggest we cannot mix ideal Measures and real Measures (if do that, real Measures will be silently converted to ideal Measures here, right?). Warning here (or allowing the mixture) might be nice. That said, I don't think of any practical use case of such a mixture, so I'm fine with keeping the code as is until we encounter any concrete issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point, I do think it's unlikely in practice for there to be mixed ideal and real measurements but we should try to support that especially since it's possible combination and allowed in the target. The only reason we have to special case measurement here is because it's special in BackendProperties.

@mergify mergify bot merged commit 72ba2ee into Qiskit:main Oct 28, 2022
mergify bot pushed a commit that referenced this pull request Oct 28, 2022
This commit fixes an oversight in the transpile() function when running
with a Target (either directly or via a backend) that contains a
Measurement operation that is ideal (either globally or locally) with no
properties defined. This was not handled correctly in the function used
to convert a Target to a BackendProperties payload for passes that are
not yet target aware and this would cause an exception to be raised.
This commit fixes this edge case and excludes readout properties from
the generated BackendProperties in this case.

Fixes #8969

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 72ba2ee)
mergify bot added a commit that referenced this pull request Oct 28, 2022
)

This commit fixes an oversight in the transpile() function when running
with a Target (either directly or via a backend) that contains a
Measurement operation that is ideal (either globally or locally) with no
properties defined. This was not handled correctly in the function used
to convert a Target to a BackendProperties payload for passes that are
not yet target aware and this would cause an exception to be raised.
This commit fixes this edge case and excludes readout properties from
the generated BackendProperties in this case.

Fixes #8969

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 72ba2ee)

Co-authored-by: Matthew Treinish <[email protected]>
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.

Transpiling fails if the target does not specify properties for measurement instructions
4 participants