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

ParameterVector support for pulse parameter assignment #12045

Merged
merged 24 commits into from
Mar 24, 2024

Conversation

arthurostrauss
Copy link
Contributor

@arthurostrauss arthurostrauss commented Mar 20, 2024

Summary

This PR proposes an extended use of the current method assign_parameters in the ParameterManagerto enable the assignment of a list of values through ParameterVectorclass. This enables an extended usage for the same methods in the Scheduleand ScheduleBlockobjects.

Details and comments

The PR simply takes the already existing unrolling method from QuantumCircuitand 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

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.
@arthurostrauss arthurostrauss requested review from eggerdj, wshanks and a team as code owners March 20, 2024 08:13
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Mar 20, 2024
@qiskit-bot
Copy link
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:

  • @Qiskit/terra-core
  • @nkanazawa1989

@arthurostrauss
Copy link
Contributor Author

This PR would solve issue #5233

@coveralls
Copy link

coveralls commented Mar 20, 2024

Pull Request Test Coverage Report for Build 8398804079

Details

  • 16 of 19 (84.21%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.03%) to 89.33%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/pulse/parameter_manager.py 14 17 82.35%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 1 92.62%
Totals Coverage Status
Change from base Build 8391327735: 0.03%
Covered Lines: 59811
Relevant Lines: 66955

💛 - Coveralls

@TsafrirA TsafrirA added the mod: pulse Related to the Pulse module label Mar 20, 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.

This looks great! I had some feedback on the documentation, but the code looks fine.

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
Copy link
Collaborator

@TsafrirA TsafrirA 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 to me. Just a minor typo and perhaps an extra test.

arthurostrauss and others added 11 commits March 22, 2024 07:13
…019827.yaml

Co-authored-by: TsafrirA <113579969+TsafrirA@users.noreply.github.com>
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 <113579969+TsafrirA@users.noreply.github.com>
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
@TsafrirA
Copy link
Collaborator

Thanks @arthurostrauss for the final fixes! This looks great.
@nkanazawa1989 - Why commits from other PRs are marked as differences here?

@wshanks
Copy link
Contributor

wshanks commented Mar 22, 2024

I think GitHub is confused about the multiple branches/merges. When I look at the diff locally (like git checkout -b tmp Qiskit/main and git merge --squash arthurostrauss/ParameterVector_pulse), it looks right.

…erVector-7d31395fd4019827.yaml

Co-authored-by: Will Shanks <wshaos@posteo.net>
@wshanks
Copy link
Contributor

wshanks commented Mar 22, 2024

@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.

@arthurostrauss
Copy link
Contributor Author

@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.

@wshanks wshanks added this pull request to the merge queue Mar 24, 2024
@wshanks
Copy link
Contributor

wshanks commented Mar 24, 2024

@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.

Merged via the queue into Qiskit:main with commit 88b5193 Mar 24, 2024
12 checks passed
@arthurostrauss arthurostrauss deleted the ParameterVector_pulse branch March 25, 2024 03:48
@sbrandhsn sbrandhsn added the Changelog: New Feature Include in the "Added" section of the changelog label May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo mod: pulse Related to the Pulse module
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

ParameterVector support for Schedules
6 participants