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.

Sign up for GitHub

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

Jump to bottom

Fix bug in backend primitives with bound_pass_manager #9629

Merged
merged 8 commits into from
Feb 24, 2023
Merged

Fix bug in backend primitives with bound_pass_manager #9629

merged 8 commits into from
Feb 24, 2023

Conversation

ElePT
Copy link
Contributor

@ElePT ElePT commented Feb 21, 2023 •
edited
Loading

Summary

While working on the quantum instance migration guide, I noticed that setting a custom bound_pass_manager on the BackendSampler would make it fail with ValueError: Inconsistent number of experiments across data fields, only when running single circuits (with circuit batches, there would be no error).

I traced this back to the _bound_pass_manager_run , which was not casting its output to a list in the case of single circuits.

This PR fixes this issue in both BackendSampler and BackendEstimator and adds the corresponding unit tests to check that the transpiler passes are running and no errors are raised.

Details and comments

To reproduce the original error, revert the changes on BackendSampler and run the proposed unit test.

@ElePT ElePT added Changelog: Bugfix Include in the "Fixed" section of the changelog mod: primitives Related to the Primitives module labels Feb 21, 2023
@ElePT ElePT requested review from a team, ikkoham and t-imamichi as code owners February 21, 2023 15:42
@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:

@coveralls
Copy link

coveralls commented Feb 21, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 4263458829

  • 12 of 12 (100.0%) changed or added relevant lines in 4 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.002%) to 85.342%
Files with Coverage Reduction New Missed Lines %
src/vf2_layout.rs 1 86.44%
Totals Coverage Status
Change from base Build 4263404154: 0.002%
Covered Lines: 67520
Relevant Lines: 79117
💛 - Coveralls

ikkoham
ikkoham reviewed Feb 22, 2023
View reviewed changes
Copy link
Contributor

@ikkoham ikkoham left a comment

Choose a reason for hiding this comment

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

Great catch. I did not know this spec of pass manager. LGTM if a test is passed.

qiskit/primitives/backend_sampler.py Show resolved Hide resolved
test/python/primitives/test_backend_sampler.py Outdated Show resolved Hide resolved
@ElePT ElePT requested a review from ikkoham February 22, 2023 15:20
@ElePT ElePT added this to the 0.23.2 milestone Feb 22, 2023
@Cryoris Cryoris removed this from the 0.23.2 milestone Feb 22, 2023
@Cryoris Cryoris added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Feb 22, 2023
@Cryoris Cryoris added this to the 0.23.3 milestone Feb 22, 2023
t-imamichi
t-imamichi reviewed Feb 23, 2023
View reviewed changes
test/python/primitives/test_backend_estimator.py Outdated
with self.assertLogs(logger, level="INFO") as cm:
_ = estimator.run(qc, op).result()
expected = ["bound_pass_manager"]
self.assertEqual([record.message for record in cm.records], expected)
Copy link
Member

Choose a reason for hiding this comment

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

Is it better to access cm within the with block?

Copy link
Contributor Author

@ElePT ElePT Feb 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure of what the implications are, but just in case I moved it inside the with block :)

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 wondering whether cm is valid after the with-statement because cm.__exit__ is called. If you are sure that cm is valid after the with-statement, I'm fine with the original code.

ikkoham
ikkoham approved these changes Feb 24, 2023
View reviewed changes
Copy link
Contributor

@ikkoham ikkoham left a comment

Choose a reason for hiding this comment

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

Thank you. LGTM

t-imamichi
t-imamichi approved these changes Feb 24, 2023
View reviewed changes
Copy link
Member

@t-imamichi t-imamichi left a comment

Choose a reason for hiding this comment

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

LGTM

@Cryoris Cryoris mentioned this pull request Feb 24, 2023
Closed
@mergify mergify bot merged commit 05310bd into Qiskit:main Feb 24, 2023
mergify bot pushed a commit that referenced this pull request Feb 24, 2023
@ElePT @mergify
Fix bug in backend primitives with bound_pass_manager (#9629)
d5c6040 
* Fix pass, add unit test

* Fix black

* Fix lint

* Fix tests

* Add reno

* Move assert to within cm

* black

---------

Co-authored-by: ikkoham <ikkoham@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 05310bd)
@mergify mergify bot mentioned this pull request Feb 24, 2023
Merged
mergify bot added a commit that referenced this pull request Feb 24, 2023
@mergify @ElePT
Fix bug in backend primitives with bound_pass_manager (#9629) (#9652)
d705295 
* Fix pass, add unit test

* Fix black

* Fix lint

* Fix tests

* Add reno

* Move assert to within cm

* black

---------

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

Co-authored-by: ElePT <57907331+ElePT@users.noreply.github.com>
@jakelishman jakelishman mentioned this pull request Feb 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t-imamichi t-imamichi t-imamichi approved these changes

@ikkoham ikkoham ikkoham approved these changes

Assignees
No one assigned
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog mod: primitives Related to the Primitives module stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Milestone
0.23.3
Development

Successfully merging this pull request may close these issues.
6 participants
@ElePT @qiskit-bot @coveralls @ikkoham @t-imamichi @Cryoris