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 custom pulse instruction conversion to Qobj. #11829

Merged
merged 5 commits into from
Feb 20, 2024

Conversation

TsafrirA
Copy link
Collaborator

Summary

This is a possible fix to #11828.

Details and comments

The behavior of ParametricPulseShapes is restored to raise ValueError when the pulse shape is not supported, and QiskitError when the pulse object is not recognized. The need for the second scenario is not clear to me, but as a quick fix I reverted to the code before #11410.

If we think that the two error types are not needed, we can alternatively change the error type used by the assemble logic.

fixes #11828

@TsafrirA TsafrirA requested a review from a team as a code owner February 19, 2024 11:41
@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 Feb 19, 2024

Pull Request Test Coverage Report for Build 7964805348

Details

  • -1 of 2 (50.0%) changed or added relevant lines in 1 file are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.003%) to 89.298%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/qobj/converters/pulse_instruction.py 1 2 50.0%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 93.81%
crates/qasm2/src/lex.rs 4 91.69%
Totals Coverage Status
Change from base Build 7961815159: 0.003%
Covered Lines: 58906
Relevant Lines: 65966

💛 - Coveralls

@TsafrirA
Copy link
Collaborator Author

As a side note, I think this bug highlights some deficiencies in the tests. We should probably beef up the tests there, but I didn't want to tie it up with the fix.

@TsafrirA TsafrirA changed the title Fix custom pulse instruction converstion to Qobj. Fix custom pulse instruction conversion to Qobj. Feb 19, 2024
wshanks
wshanks previously approved these changes Feb 19, 2024
Copy link
Contributor

@wshanks wshanks left a comment

Choose a reason for hiding this comment

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

Nice fix, @TsafrirA. I just had some wording suggestions for the release notes.

We are trying to move away from Qobj any way, but it might be worth adding a custom pulse shape to this test or one like it:

def test_pulse_gates_single_circ(self):
"""Test that we can add calibrations to circuits."""
theta = Parameter("theta")
circ = QuantumCircuit(2)
circ.h(0)
circ.append(RxGate(3.14), [0])
circ.append(RxGate(theta), [1])
circ = circ.assign_parameters({theta: 3.14})
with pulse.build() as custom_h_schedule:
pulse.play(pulse.library.Drag(50, 0.15, 4, 2), pulse.DriveChannel(0))
with pulse.build() as x180:
pulse.play(pulse.library.Gaussian(50, 0.2, 5), pulse.DriveChannel(1))
circ.add_calibration("h", [0], custom_h_schedule)
circ.add_calibration(RxGate(3.14), [0], x180)
circ.add_calibration(RxGate(3.14), [1], x180)

…56cfd1.yaml

Co-authored-by: Will Shanks <wshaos@posteo.net>
@TsafrirA
Copy link
Collaborator Author

Thanks @wshanks! I added a test to cover something like this.

Copy link
Contributor

@wshanks wshanks left a comment

Choose a reason for hiding this comment

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

Looks good!

@jakelishman jakelishman added this to the 1.0.1 milestone Feb 19, 2024
@jakelishman jakelishman 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 Feb 19, 2024
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 for the quick fix!

@nkanazawa1989 nkanazawa1989 added this pull request to the merge queue Feb 20, 2024
Merged via the queue into Qiskit:main with commit 060cb70 Feb 20, 2024
11 of 12 checks passed
mergify bot pushed a commit that referenced this pull request Feb 20, 2024
* Possible fix.

* Update releasenotes/notes/fix-custom-pulse-qobj-conversion-5d6041b36356cfd1.yaml

Co-authored-by: Will Shanks <wshaos@posteo.net>

* release notes fix

* Add test

---------

Co-authored-by: Will Shanks <wshaos@posteo.net>
(cherry picked from commit 060cb70)
github-merge-queue bot pushed a commit that referenced this pull request Feb 22, 2024
* Possible fix.

* Update releasenotes/notes/fix-custom-pulse-qobj-conversion-5d6041b36356cfd1.yaml

Co-authored-by: Will Shanks <wshaos@posteo.net>

* release notes fix

* Add test

---------

Co-authored-by: Will Shanks <wshaos@posteo.net>
(cherry picked from commit 060cb70)

Co-authored-by: TsafrirA <113579969+TsafrirA@users.noreply.github.com>
@TsafrirA TsafrirA deleted the QobjBugFix branch March 13, 2024 13:13
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: 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
Development

Successfully merging this pull request may close these issues.

Converting custom pulse play instructions to Qobj raises an error
6 participants