Skip to content

Make limit on waveform amplitude optional#6345

Merged
mergify[bot] merged 29 commits intoQiskit:mainfrom
peendebak:feat/waveform_amplitude_limit
Jun 10, 2021
Merged

Make limit on waveform amplitude optional#6345
mergify[bot] merged 29 commits intoQiskit:mainfrom
peendebak:feat/waveform_amplitude_limit

Conversation

@peendebak
Copy link
Copy Markdown
Contributor

@peendebak peendebak commented May 3, 2021

Summary

Remove limit of waveform amplitude.

Details and comments

Fixes #6012

Comment thread qiskit/pulse/library/waveform.py Outdated
@taalexander taalexander self-assigned this May 4, 2021
@peendebak peendebak changed the title [WIP] remove limit on waveform amplitude Make limit on waveform amplitude optional May 4, 2021
@peendebak peendebak force-pushed the feat/waveform_amplitude_limit branch from 2f562aa to 6940100 Compare May 6, 2021 08:58
@peendebak
Copy link
Copy Markdown
Contributor Author

Rebase to main

@peendebak peendebak force-pushed the feat/waveform_amplitude_limit branch from 6940100 to 8c0bd9c Compare May 6, 2021 10:16
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.

I believe the problem with failing linting is due to the recent incorporation of Black in #6361. It should be sufficient to run the black formatter over the files touched in this PR.

Comment thread qiskit/pulse/library/waveform.py Outdated
Comment thread qiskit/pulse/library/waveform.py Outdated
@nkanazawa1989
Copy link
Copy Markdown
Contributor

Simple question: What happens if we send pulse with amplitude > 1. to our backend? Will it return 9999 error?

@eendebakpt
Copy link
Copy Markdown
Contributor

Simple question: What happens if we send pulse with amplitude > 1. to our backend? Will it return 9999 error?

For the openpulse backend of https://qiskit.org/textbook/ch-quantum-hardware/calibrating-qubits-pulse.html there is an error:

IBMQJobFailureError: 'Unable to retrieve result for job 6096b1d254112347133f3ea6. Job has failed: Internal Error. Error code: 9372.'

@peendebak peendebak force-pushed the feat/waveform_amplitude_limit branch from b69727a to d98f165 Compare May 8, 2021 17:54
@taalexander
Copy link
Copy Markdown
Contributor

IBMQJobFailureError: 'Unable to retrieve result for job 6096b1d254112347133f3ea6. Job has failed: Internal Error. Error code: 9372.'

Thanks for highlighting this, I've created an internal ticket to add a more informative error in a future release.

Comment thread qiskit/pulse/library/waveform.py Outdated
Comment thread qiskit/pulse/library/waveform.py Outdated
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.

:py:attr:~some_property

Comment thread qiskit/pulse/library/waveform.py Outdated
Co-authored-by: Thomas Alexander <thomasalexander2718@gmail.com>
@peendebak peendebak requested a review from taalexander May 17, 2021 20:03
@peendebak
Copy link
Copy Markdown
Contributor Author

@taalexander Added release notes to the PR. Let me know whether we need to add something more.

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.

Thank you for additional changes, my only follow-up question is should we consider extending the limit_amplitude kwarg to the existing parametric pulse constructors?

Comment thread test/python/pulse/test_pulse_lib.py
peendebak and others added 4 commits May 24, 2021 20:30
…mit-c58e2ec61f6789e6.yaml

Co-authored-by: Thomas Alexander <thomasalexander2718@gmail.com>
…mit-c58e2ec61f6789e6.yaml

Co-authored-by: Thomas Alexander <thomasalexander2718@gmail.com>
…mit-c58e2ec61f6789e6.yaml

Co-authored-by: Thomas Alexander <thomasalexander2718@gmail.com>
@peendebak
Copy link
Copy Markdown
Contributor Author

Thank you for additional changes, my only follow-up question is should we consider extending the limit_amplitude kwarg to the existing parametric pulse constructors?

I would suggest creating a new issue/PR for this since it will be backwards incompatible. (if we follow the same convention as for the other parametric pulses, e.g. limit_amplitude defaults to True)

@peendebak peendebak requested a review from taalexander May 25, 2021 11:13
@eendebakpt
Copy link
Copy Markdown
Contributor

@taalexander I rebased to main. Let me know whether I should make any additional changes to the PR.

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.

Almost ready for merger.

Comment thread test/python/pulse/test_pulse_lib.py Outdated
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.

LGTM!

@taalexander
Copy link
Copy Markdown
Contributor

Created a follow up issue #6544 for propagating limit_amplitude to parametric pulse constructors.

@mergify mergify Bot merged commit ce2f2e7 into Qiskit:main Jun 10, 2021
@peendebak peendebak deleted the feat/waveform_amplitude_limit branch June 10, 2021 13:33
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.

[RFC] Limit on amplitude of Gaussian (and other parametric pulses)

4 participants