-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
ParameterVector support for pulse parameter assignment #12045
Conversation
It is now possible to assign to a pulse schedule parameters in the form of a list of values that can be directly binded to ParameterVector. This PR is based on the current functioning of the analogous method for the QuantumCircuit class.
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:
|
This PR would solve issue #5233 |
Pull Request Test Coverage Report for Build 8398804079Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! I had some feedback on the documentation, but the code looks fine.
releasenotes/notes/pulse_parameter_manager_compat_with_ParameterVector-7d31395fd4019827.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/pulse_parameter_manager_compat_with_ParameterVector-7d31395fd4019827.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/pulse_parameter_manager_compat_with_ParameterVector-7d31395fd4019827.yaml
Outdated
Show resolved
Hide resolved
Type for submitting a list of parameters has been set to Sequence for the case of ParameterVector. This enables the user to also pass a tuple of values/ParameterExpressions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Just a minor typo and perhaps an extra test.
releasenotes/notes/pulse_parameter_manager_compat_with_ParameterVector-7d31395fd4019827.yaml
Outdated
Show resolved
Hide resolved
…019827.yaml Co-authored-by: TsafrirA <[email protected]>
Complementary tests have been added for checking the pulse.Schedule.assign_parameters() method, as well as the functioning of binding to a ParameterVector a collection of numeric values and new ParameterExpression (through Parameter).
An error was occurring when trying to get access to an instruction parameter within schedule.instructions
It is now possible to assign to a pulse schedule parameters in the form of a list of values that can be directly binded to ParameterVector. This PR is based on the current functioning of the analogous method for the QuantumCircuit class.
Type for submitting a list of parameters has been set to Sequence for the case of ParameterVector. This enables the user to also pass a tuple of values/ParameterExpressions
…019827.yaml Co-authored-by: TsafrirA <[email protected]>
Complementary tests have been added for checking the pulse.Schedule.assign_parameters() method, as well as the functioning of binding to a ParameterVector a collection of numeric values and new ParameterExpression (through Parameter).
An error was occurring when trying to get access to an instruction parameter within schedule.instructions
…auss/qiskit into ParameterVector_pulse
Thanks @arthurostrauss for the final fixes! This looks great. |
releasenotes/notes/pulse_parameter_manager_compat_with_ParameterVector-7d31395fd4019827.yaml
Outdated
Show resolved
Hide resolved
I think GitHub is confused about the multiple branches/merges. When I look at the diff locally (like |
…erVector-7d31395fd4019827.yaml Co-authored-by: Will Shanks <[email protected]>
@arthurostrauss This looks good to me now! Do you want to try to make it look clean in GitHub, so we know it will merge the way we want? I can squash things down and force push to your branch if you want. |
@wshanks Yes if you can do such force push (if necessary) I'd be happy to let you do it, as I'm not sure how to proceed on my side for this. |
@arthurostrauss Actually, the diff looks good in GitHub now after the last merge, so I think it is fine to merge as is. I am not sure what messed up the diff view before. |
Summary
This PR proposes an extended use of the current method
assign_parameters
in theParameterManager
to enable the assignment of a list of values throughParameterVector
class. This enables an extended usage for the same methods in theSchedule
andScheduleBlock
objects.Details and comments
The PR simply takes the already existing unrolling method from
QuantumCircuit
and applies it for the pulse level case, to enable a simple extension of the existing methods for parameter assignment.This can be practical when mixing pulse level calibrations with many parameters.
This PR closes #5233