Skip to content

Add GaussianSquareDrag symbolic pulse shape#9329

Merged
mergify[bot] merged 5 commits into
Qiskit:mainfrom
wshanks:gsd
Jan 6, 2023
Merged

Add GaussianSquareDrag symbolic pulse shape#9329
mergify[bot] merged 5 commits into
Qiskit:mainfrom
wshanks:gsd

Conversation

@wshanks
Copy link
Copy Markdown
Contributor

@wshanks wshanks commented Dec 29, 2022

Summary

Add GaussianSquareDrag symbolic pulse shape

Details and comments

Add new qiskit.pulse.GaussianSquareDrag pulse shape. This pulse
shape is similar to qiskit.pulse.GaussianSquare but uses the
qiskit.pulse.Drag shape during its rise and fall. The correction
from the DRAG pulse shape can suppress part of the frequency spectrum of
the rise and fall of the pulse which can help avoid exciting spectator
qubits when they are close in frequency to the drive frequency of the
pulse.

GaussianSquareDrag is sometimes used on IBM backends and its absence from qiskit requires those backends to report calibrations using the pulse with Waveforms instead of SymbolicPulse which obscures the parameters and is less efficient to transfer.

Depending on the timing, either this PR or #9314 will need to be updated to account for the other one as GaussianSquareDrag should also be a ScalableSymbolicPulse.

This pulse shape is similar to the GaussianSquare pulse with Drag
components in the rise and fall of the pulse.
@wshanks wshanks requested review from a team and eggerdj as code owners December 29, 2022 16:01
@qiskit-bot
Copy link
Copy Markdown
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:

@wshanks
Copy link
Copy Markdown
Contributor Author

wshanks commented Dec 29, 2022

Based on #9092, it seems like we need to make sure that this PR is released before allowing IBM backends to report a gaussian_square_drag pulse since terra does not handle it well when an unrecognized pulse is reported. I am interested to know what others think is a reasonable time for backends to start reporting gaussian_square_drag pulses. As soon as the new terra release with GaussianSquareDrag is made? As soon as the following terra release is made? A user will need to upgrade to a version of terra with GaussianSquareDrag in order to load the pulses.

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 29, 2022

Pull Request Test Coverage Report for Build 3857407140

  • 23 of 24 (95.83%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 84.583%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/pulse/library/symbolic_pulses.py 21 22 95.45%
Totals Coverage Status
Change from base Build 3857188091: 0.003%
Covered Lines: 64222
Relevant Lines: 75928

💛 - Coveralls

Copy link
Copy Markdown
Contributor

@taalexander taalexander left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me. I have not manually validated the equation documentation but the symbolic equations look good and the testing strategy seems like a robust comparison.

Comment thread qiskit/pulse/library/symbolic_pulses.py
Comment thread qiskit/pulse/library/symbolic_pulses.py
Copy link
Copy Markdown
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 Will. This looks almost good to me. Just a nitpick.

Comment thread qiskit/pulse/library/symbolic_pulses.py
Comment thread qiskit/pulse/library/symbolic_pulses.py Outdated
Comment thread qiskit/pulse/library/symbolic_pulses.py
Comment thread qiskit/pulse/library/symbolic_pulses.py
Copy link
Copy Markdown
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 @wshanks this looks good to me. Do you want to merge this first or #9314 ?

@wshanks
Copy link
Copy Markdown
Contributor Author

wshanks commented Jan 6, 2023

@nkanazawa1989 I don't mind which between this or #9314 goes first order-wise, but I think we might not have reached a final resolution on what to do about #9314 yet? I would like to get this PR merged in time for the next release, so we can have the backends start supporting this pulse shape.

By the way, did you have a feeling for how soon we should have the backends start reporting gaussian_square_drag pulses?

@nkanazawa1989
Copy link
Copy Markdown
Contributor

Fair enough. I'll add this to auto-merge queue. I don't have any idea about deployment timeline, but I expect we start from upgrading the Qiskit to add new parametric form to the CmdDef.

@jakelishman
Copy link
Copy Markdown
Member

This will bounce off the branch-protection rules until Thomas (@taalexander) removes his "changes requested" review.

@wshanks
Copy link
Copy Markdown
Contributor Author

wshanks commented Jan 6, 2023

I don't have any idea about deployment timeline, but I expect we start from upgrading the Qiskit to add new parametric form to the CmdDef.

I have updated the backend IBM code so that it can report gaussian_square_drag in the CmdDef. I don't think we want backends to start using that code yet because users will have issues like #9092 when using the latest release of terra which doesn't include this PR. Once this PR is in a release, we could let IBM backends start reporting the pulse shape, but then any user trying to load backend schedules with gaussian_square_drag with an older version of terra will run into an error. I am not sure how much time to give users to update.

@mergify mergify Bot merged commit 86634bc into Qiskit:main Jan 6, 2023
@wshanks wshanks deleted the gsd branch January 6, 2023 21:32
@nkanazawa1989
Copy link
Copy Markdown
Contributor

I see you question. I think this problem is really difficult to address without using symbolic pulse. What we could do is to write try-except block to avoid un-supported pulse shape to be loaded or raise more friendly error. I think backend should be able to report new pulse after the terra release otherwise we will never get a chance to start reporting.

king-p3nguin pushed a commit to king-p3nguin/qiskit-terra that referenced this pull request Jan 11, 2023
* Add GaussianSquareDrag symbolic pulse shape

This pulse shape is similar to the GaussianSquare pulse with Drag
components in the rise and fall of the pulse.

* Clarify description of pulse

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Cryoris pushed a commit to Cryoris/qiskit-terra that referenced this pull request Jan 12, 2023
* Add GaussianSquareDrag symbolic pulse shape

This pulse shape is similar to the GaussianSquare pulse with Drag
components in the rise and fall of the pulse.

* Clarify description of pulse

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants