Enable lo frequencies to be passed in circuits #6167
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 6 commits
April 2, 2021 18:31
zachschoenfeld33
requested review from
chriseclectic,
jyu00 and
a team
as code owners
zachschoenfeld33
commented
Apr 6, 2021
zachschoenfeld33
commented
Apr 6, 2021
| if 'qubit_lo_range' in out_dict: | ||
| out_dict['qubit_lo_range'] = [ | ||
| [min_range * 1e9, max_range * 1e9] for |
There was a problem hiding this comment.
This matches the pulse case
zachschoenfeld33
commented
Apr 6, 2021
taalexander
requested changes
Apr 6, 2021
eggerdj
reviewed
Apr 8, 2021
added 7 commits
April 9, 2021 10:54
…valent job and expt level los
|
Updated--can now set los both at the job level, as well as the individual circuit/schedule level (via |
zachschoenfeld33
force-pushed
the
zas-lo-freqs-qasm
branch
from
04e4b5a
to
cbc3cb2
Compare
|
It seems like this one was "almost there" and got hit by the black-conflict tsunami . Any chance to have a look at them @zachschoenfeld33 ? |
|
Updated @1ucian0 |
|
I had to simplify the |
1ucian0
approved these changes
Jun 1, 2021
There was a problem hiding this comment.
Based on the previews review from @taalexander, I'm approving this PR.
1ucian0
added
automerge
Changelog: API Change
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Sep 20, 2021
This commit addresses a performance regression in assemble() introduced in Qiskit#6167. That PR added the ability for users to configure pulse properties via assemble kwargs (such as lo frequencies). While this should really only be exposed via a Backend's options object and terra shouldn't have to deal with it, for legacy/backwards compat reasons we need to expose these options via assemble(). However, that PR was a bit aggressive in when it needed to parse the pulse defaults and properties to determine if input was valid. The intent there was for circuits jobs that users were configuring pulse configuration we'd be providing the same checks as with pulse jobs. However for circuits jobs this additional validation is only needed if the user has specified a pulse configuration options and we need to check that they're valid. If a user doesn't we shouldn't be touching anything pulse, especially as just loading pulse defaults can result in a large performance regression (roughly 2 orders of magnitude over assembly without it). To fix this is commit adjusts the logic when we parse the pulse defaults (and all the subsequent logic that uses them) to be if it's a pulse job, or if it's a circuit job and one of the named pulse paramters has been set. This returns the performance for pure circuits job to what it was prior to PR Qiskit#6167 but still retains the functionality that Qiskit#6167 provided when it's needed.
This was referenced Sep 20, 2021
mtreinish
added a commit
that referenced
this pull request
Sep 20, 2021
…7047) * Avoid configuring pulse defaults in circuit assembly unless needed This commit addresses a performance regression in assemble() introduced in #6167. That PR added the ability for users to configure pulse properties via assemble kwargs (such as lo frequencies). While this should really only be exposed via a Backend's options object and terra shouldn't have to deal with it, for legacy/backwards compat reasons we need to expose these options via assemble(). However, that PR was a bit aggressive in when it needed to parse the pulse defaults and properties to determine if input was valid. The intent there was for circuits jobs that users were configuring pulse configuration we'd be providing the same checks as with pulse jobs. However for circuits jobs this additional validation is only needed if the user has specified a pulse configuration options and we need to check that they're valid. If a user doesn't we shouldn't be touching anything pulse, especially as just loading pulse defaults can result in a large performance regression (roughly 2 orders of magnitude over assembly without it). To fix this is commit adjusts the logic when we parse the pulse defaults (and all the subsequent logic that uses them) to be if it's a pulse job, or if it's a circuit job and one of the named pulse paramters has been set. This returns the performance for pure circuits job to what it was prior to PR #6167 but still retains the functionality that #6167 provided when it's needed. * Make expected logic more explicit
mergify bot
pushed a commit
that referenced
this pull request
Sep 20, 2021
…7047) * Avoid configuring pulse defaults in circuit assembly unless needed This commit addresses a performance regression in assemble() introduced in #6167. That PR added the ability for users to configure pulse properties via assemble kwargs (such as lo frequencies). While this should really only be exposed via a Backend's options object and terra shouldn't have to deal with it, for legacy/backwards compat reasons we need to expose these options via assemble(). However, that PR was a bit aggressive in when it needed to parse the pulse defaults and properties to determine if input was valid. The intent there was for circuits jobs that users were configuring pulse configuration we'd be providing the same checks as with pulse jobs. However for circuits jobs this additional validation is only needed if the user has specified a pulse configuration options and we need to check that they're valid. If a user doesn't we shouldn't be touching anything pulse, especially as just loading pulse defaults can result in a large performance regression (roughly 2 orders of magnitude over assembly without it). To fix this is commit adjusts the logic when we parse the pulse defaults (and all the subsequent logic that uses them) to be if it's a pulse job, or if it's a circuit job and one of the named pulse paramters has been set. This returns the performance for pure circuits job to what it was prior to PR #6167 but still retains the functionality that #6167 provided when it's needed. * Make expected logic more explicit (cherry picked from commit 9ffd69b)
mergify bot
added a commit
that referenced
this pull request
Sep 21, 2021
…7047) (#7049) * Avoid configuring pulse defaults in circuit assembly unless needed This commit addresses a performance regression in assemble() introduced in #6167. That PR added the ability for users to configure pulse properties via assemble kwargs (such as lo frequencies). While this should really only be exposed via a Backend's options object and terra shouldn't have to deal with it, for legacy/backwards compat reasons we need to expose these options via assemble(). However, that PR was a bit aggressive in when it needed to parse the pulse defaults and properties to determine if input was valid. The intent there was for circuits jobs that users were configuring pulse configuration we'd be providing the same checks as with pulse jobs. However for circuits jobs this additional validation is only needed if the user has specified a pulse configuration options and we need to check that they're valid. If a user doesn't we shouldn't be touching anything pulse, especially as just loading pulse defaults can result in a large performance regression (roughly 2 orders of magnitude over assembly without it). To fix this is commit adjusts the logic when we parse the pulse defaults (and all the subsequent logic that uses them) to be if it's a pulse job, or if it's a circuit job and one of the named pulse paramters has been set. This returns the performance for pure circuits job to what it was prior to PR #6167 but still retains the functionality that #6167 provided when it's needed. * Make expected logic more explicit (cherry picked from commit 9ffd69b) Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Closes #5874. This PR allows users to provide drive and measure lo frequencies from
execute/assemblein qasm jobs. It also provides support for returningqubit_lo_rangeandmeas_lo_rangefrom qasm backends so that users know the allowed values of the lo frequencies.Details and comments
default_qubit_los/default_meas_losinexecuteto set job level lo freqsqubit_lo_freq/meas_lo_freqinassembleto set job level lo freqsschedule_losinexecute/assembleto set expt level (circuit or schedule level) lo freqsqubit_lo_rangeandmeas_lo_rangeviabackend.configuration().qubit_lo_range/backend.configuration().meas_lo_rangeA schema update was provided at Qiskit/ibm-quantum-schemas#9