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 the output macros.measure with backendV2 #10135

Merged
merged 8 commits into from
Jun 7, 2023
Merged

Fix the output macros.measure with backendV2 #10135

merged 8 commits into from
Jun 7, 2023

Conversation

to24toro
Copy link
Contributor

Summary

In the process of working on #9501, the output of measure_v1 and measure_v2 don't match.
This is the PR for this fix.

Details and comments

  • modify some codes about _measure_v2
  • add some test codes that confirms the consistency of outputs between measure_v1 and measure_v2

@to24toro to24toro requested review from a team, eggerdj and wshanks as code owners May 22, 2023 03:08
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core
  • @nkanazawa1989

@coveralls
Copy link

coveralls commented May 22, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 5173873465

  • 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.03%) to 85.935%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 1 91.39%
qiskit/pulse/macros.py 1 94.67%
Totals Coverage Status
Change from base Build 5148601299: 0.03%
Covered Lines: 71315
Relevant Lines: 82987
💛 - Coveralls

@to24toro to24toro mentioned this pull request May 24, 2023
Merged
@nkanazawa1989 nkanazawa1989 self-assigned this May 24, 2023
@nkanazawa1989 nkanazawa1989 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 mod: pulse Related to the Pulse module labels May 24, 2023
nkanazawa1989
nkanazawa1989 reviewed May 24, 2023
View reviewed changes
Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

This looks almost good to go. Please also add a release note with fix section. @mtreinish do we have plan for 0.24.2?

qiskit/pulse/macros.py Outdated

return _measure_v2(
qubits=qubits,
target=backend.target,
meas_map=meas_map,
meas_map=meas_map or getattr(backend, "meas_map", [list(range(backend.num_qubits))]),
Copy link
Contributor

Choose a reason for hiding this comment

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

This attribute is defined in the base class of V2 backend, so it's fair to assume backend has meas_map. On the other hand, it is not implemented and raises NotImplementedError by default, so

try:
    meas_map = backend.meas_map
except NotImplementedError:
    meas_map = [list(range(backend.num_qubits))]

is more appropriate. Do you have any reason to use getattr?
https://github.com/Qiskit/qiskit-terra/blob/02502b5d98796dcc2c8b80ac5d0f2af85e978e5c/qiskit/providers/backend.py#L452-L466

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed at 4ae58e7.

@jakelishman
Copy link
Member

We don't have any date set for a 0.24.2 release, but if we've got patches to go into one then we'll make one. While we won't hold up the 0.24.1 release planned for tomorrow on this, if it merges before then, we could just fold it in.

@nkanazawa1989
Copy link
Contributor

Thanks Jake. I don't think we can add this to 0.24.1 because of the tight deadline. 0.24.2 sounds good to me :)

nkanazawa1989
nkanazawa1989 reviewed Jun 2, 2023
View reviewed changes
Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

LGTM. Minor fix for the release note.

releasenotes/notes/fix-outputs-of-measure_v2-8959ebbbf5f87294.yaml Outdated Show resolved Hide resolved
to24toro and others added 2 commits June 5, 2023 10:09
@to24toro @nkanazawa1989
Update reno.
367ed91 
Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
@to24toro
fix reno again
0616555
@nkanazawa1989 nkanazawa1989 enabled auto-merge June 5, 2023 02:30
nkanazawa1989
nkanazawa1989 approved these changes Jun 7, 2023
View reviewed changes
Copy link
Contributor

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

Thanks @to24toro for the bugfix.

@nkanazawa1989 nkanazawa1989 added this pull request to the merge queue Jun 7, 2023
@nkanazawa1989 nkanazawa1989 added this to the 0.24.2 milestone Jun 7, 2023
Merged via the queue into Qiskit:main with commit f911366 Jun 7, 2023
mergify bot pushed a commit that referenced this pull request Jun 7, 2023
@to24toro @mergify
Fix the output macros.measure with backendV2 (#10135)
320866a 
* fix measure_v2

* modify measure_all

* fix meas_map input in measure_v2

* add reno

* fix reno

* Update reno.

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

* fix reno again

---------

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
(cherry picked from commit f911366)
@mergify mergify bot mentioned this pull request Jun 7, 2023
Merged
jakelishman pushed a commit that referenced this pull request Jun 7, 2023
@mergify @to24toro
Fix the output macros.measure with backendV2 (#10135) (#10241)
c3617e8 
* fix measure_v2

* modify measure_all

* fix meas_map input in measure_v2

* add reno

* fix reno

* Update reno.

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

* fix reno again

---------

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
(cherry picked from commit f911366)

Co-authored-by: Kento Ueda <38037695+to24toro@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nkanazawa1989 nkanazawa1989 nkanazawa1989 approved these changes

@eggerdj eggerdj Awaiting requested review from eggerdj eggerdj is a code owner

@wshanks wshanks Awaiting requested review from wshanks wshanks is a code owner

Assignees

@nkanazawa1989 nkanazawa1989

Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog mod: pulse Related to the Pulse module stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Milestone
0.24.2
Development

Successfully merging this pull request may close these issues.
5 participants
@to24toro @qiskit-bot @coveralls @jakelishman @nkanazawa1989