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

Support dynamic circuit in BackendEstimator #9700

Merged
merged 17 commits into from
Oct 6, 2023

Conversation

ikkoham
Copy link
Contributor

@ikkoham ikkoham commented Mar 1, 2023

Summary

This PR makes BackendEstimator supports dynamic circuit.

Details and comments

@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:

@ikkoham ikkoham changed the title Support dynamic circuit in Estimator Support dynamic circuit in BackendEstimator Mar 1, 2023
@ikkoham ikkoham added mod: primitives Related to the Primitives module Changelog: New Feature Include in the "Added" section of the changelog labels Mar 1, 2023
@coveralls
Copy link

coveralls commented Mar 1, 2023

Pull Request Test Coverage Report for Build 5821920454

  • 10 of 11 (90.91%) changed or added relevant lines in 1 file are covered.
  • 288 unchanged lines in 18 files lost coverage.
  • Overall coverage increased (+1.4%) to 87.256%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/primitives/backend_estimator.py 10 11 90.91%
Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/sabre_swap/mod.rs 1 97.53%
qiskit/visualization/circuit/_utils.py 1 93.79%
qiskit/qpy/binary_io/circuits.py 2 93.05%
qiskit/pulse/library/waveform.py 3 93.75%
qiskit/transpiler/passes/utils/block_to_matrix.py 3 83.33%
crates/qasm2/src/lex.rs 4 90.89%
qiskit/transpiler/preset_passmanagers/builtin_plugins.py 4 97.04%
qiskit/transpiler/preset_passmanagers/level0.py 4 93.75%
qiskit/transpiler/preset_passmanagers/level1.py 4 96.49%
qiskit/transpiler/passes/optimization/consolidate_blocks.py 5 95.04%
Totals Coverage Status
Change from base Build 5684162640: 1.4%
Covered Lines: 74290
Relevant Lines: 85140

💛 - Coveralls

@garrison
Copy link
Member

garrison commented Mar 6, 2023

Actually, it is not clear to me why circuit.num_qubits == observable.num_qubits should be relaxed to circuit.num_qubits >= observable.num_qubits in order to support dynamic circuits. Can you clarify?

@ikkoham
Copy link
Contributor Author

ikkoham commented Mar 7, 2023

@garrison Thanks. The intent of this relaxed condition is to support circuits with auxiliary qubits like the test example https://github.com/Qiskit/qiskit-terra/pull/9700/files#diff-ec629567417753086ce09aba87c8dc9f34ab946d903fe809200ba692a7ed8e2dR364

@garrison
Copy link
Member

garrison commented Mar 7, 2023

I see. But in that example, why not just specify the observable as "ZI", i.e., such that it is over all the qubits?

Otherwise, you presumably need some logic to determine what is an "auxiliary" qubit or not. For instance, is it considered an auxiliary qubit if it contains a Measure instruction somewhere? If so, what if the Measure is immediately followed by a Reset? Would one be prevented from taking expectation values over such qubits?

Another way of phrasing my question: With this PR, you could pass the same circuit multiple times along with observables of different numbers of qubits. How are these different observables expected to be handled within an Estimator implementation?

@ikkoham
Copy link
Contributor Author

ikkoham commented Mar 9, 2023

You are right. Certainly there is no way to distinguish if they are auxiliary qubits or not, so it may be a good idea to have the user explicitly enter I.

@ikkoham ikkoham marked this pull request as ready for review July 4, 2023 04:42
@ikkoham ikkoham requested review from a team and t-imamichi as code owners July 4, 2023 04:42
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core
  • @ajavadia
  • @ikkoham
  • @levbishop
  • @t-imamichi

@ikkoham ikkoham added this to the 0.25.0 milestone Jul 4, 2023
@t-imamichi
Copy link
Member

I come up with an idea to make a separate classical register for measurements. Could you take a look at it, @ikkoham?
ikkoham#78

@ikkoham ikkoham added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Jul 27, 2023
@ikkoham ikkoham removed this from the 0.45.0 milestone Jul 27, 2023
@ikkoham
Copy link
Contributor Author

ikkoham commented Jul 27, 2023

I initially made this PR with the intention of supporting dynamic circuits, but I noticed that it is bug for circuits with classical registers, even if they are not dynamic. I hope it will be merged as soon as possible.

@ikkoham ikkoham added this to the 0.25.1 milestone Jul 27, 2023
@t-imamichi
Copy link
Member

OK. I review it asap

@t-imamichi
Copy link
Member

t-imamichi commented Jul 28, 2023

When I ran the test locally, I found the following warning in test_job_size_limit_v1 and test_job_size_limit_v2. The denom value gets 0. It would be better to check zero division before expvals /= denom.

./Users/ima/tasks/3_2023/qiskit/terra/qiskit/primitives/backend_estimator.py:454: RuntimeWarning: invalid value encountered in divide
  expvals /= denom

@t-imamichi
Copy link
Member

How is the final measurement treated?

  1. Should the final measurement order be same as the observable order?
  2. What if final measurements are applied to only q0 and q2, and q1 is left not measured? Should two-qubit observables be passed with the circuit?

@t-imamichi
Copy link
Member

minor update of tests. ikkoham#79

@ikkoham
Copy link
Contributor Author

ikkoham commented Aug 10, 2023

Discussing internally.

@ikkoham ikkoham removed this from the 0.25.1 milestone Aug 16, 2023
@ikkoham ikkoham removed the on hold Can not fix yet label Oct 5, 2023
@ikkoham ikkoham modified the milestones: 0.45.0, 0.25.3 Oct 5, 2023
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

@ikkoham ikkoham added this pull request to the merge queue Oct 6, 2023
Merged via the queue into Qiskit:main with commit 660448c Oct 6, 2023
14 checks passed
mergify bot pushed a commit that referenced this pull request Oct 6, 2023
* Support dynamic circuit in Estimator

* fix lint

* revert the validation condition

* fix test

* add reno

* separate measurements

* lint

* add test and bug fix reno

* fix reno

* revise tests and fix typo

---------

Co-authored-by: Takashi Imamichi <imamichi@jp.ibm.com>
(cherry picked from commit 660448c)
rupeshknn pushed a commit to rupeshknn/qiskit that referenced this pull request Oct 9, 2023
* Support dynamic circuit in Estimator

* fix lint

* revert the validation condition

* fix test

* add reno

* separate measurements

* lint

* add test and bug fix reno

* fix reno

* revise tests and fix typo

---------

Co-authored-by: Takashi Imamichi <imamichi@jp.ibm.com>
github-merge-queue bot pushed a commit that referenced this pull request Oct 11, 2023
* Support dynamic circuit in Estimator

* fix lint

* revert the validation condition

* fix test

* add reno

* separate measurements

* lint

* add test and bug fix reno

* fix reno

* revise tests and fix typo

---------

Co-authored-by: Takashi Imamichi <imamichi@jp.ibm.com>
(cherry picked from commit 660448c)

Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
Co-authored-by: Takashi Imamichi <31178928+t-imamichi@users.noreply.github.com>
Co-authored-by: Jake Lishman <jake.lishman@ibm.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 mod: primitives Related to the Primitives module priority: high 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.

7 participants