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

Assignment of parameters in pulse Schedule/ScheduleBlock doable through parameter name #12088

Merged

Conversation

arthurostrauss
Copy link
Contributor

Summary

This PR extends the previous usage of the method assign_parameters of Scheduleand ScheduleBlock by enabling the specification of the parameter name instead of the parameter directly. This assignment by name also works when assigning values to a ParameterVector(the length of the list of parameter values should in this case match the length of the vector).

Details and comments

@arthurostrauss arthurostrauss requested review from eggerdj, wshanks and a team as code owners March 27, 2024 04:06
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Mar 27, 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 arthurostrauss force-pushed the ParameterManager_for_string_assignment branch 2 times, most recently from 59ae72f to cbaed51 Compare March 27, 2024 06:06
@wshanks wshanks added the mod: pulse Related to the Pulse module label Mar 27, 2024
@wshanks
Copy link
Contributor

wshanks commented Mar 27, 2024

@arthurostrauss Is there previous discussion of this? It is convenient, but it is not something that QuantumCircuit allows either. I wonder if we want to discuss both contexts and keep behavior consistent. There is also a tradeoff between convenience and the potential for assigning to more parameters than intended (the user can avoid the ambiguity by using parameters but we should consider how much room for ambiguity to allow). We have struggled with string names versus parameters in the Qiskit Experiments calibration module in the past (with blocks like this one).

@arthurostrauss arthurostrauss force-pushed the ParameterManager_for_string_assignment branch from cbaed51 to c9f51e7 Compare March 31, 2024 00:44
@arthurostrauss
Copy link
Contributor Author

The motivation behind such feature lies in the will to integrate in a closer form the primitives V2 to pulse level control, as the former do carry BindingArrays storing the parameter names only. The idea would be to assign parameters to the schedule based solely on the information of the BindingArrays. For instance, one could imagine in the future when Qiskit Pulse will be fully JAX compatible, doing a subroutine using the primitives (with DynamicsBackend) and simulating the pipeline at the pulse level. Instead of binding the parameters before Jit compiling the workflow, one could accelerate the pipeline by compiling over the parametrized schedule, and bind the values using vectorized modelling of Jax, significantly improving performance. I am already working myself on such use case, happy to tell you more if relevant.

@coveralls
Copy link

coveralls commented Mar 31, 2024

Pull Request Test Coverage Report for Build 8855032197

Details

  • 29 of 33 (87.88%) changed or added relevant lines in 2 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.01%) to 89.442%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/pulse/parameter_manager.py 15 16 93.75%
qiskit/pulse/utils.py 14 17 82.35%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 3 92.88%
Totals Coverage Status
Change from base Build 8851171785: 0.01%
Covered Lines: 60950
Relevant Lines: 68145

💛 - Coveralls

@arthurostrauss
Copy link
Contributor Author

By the way after checking, it is possible to use QuantumCircuit.assign_parameters()with strings representing the parameter name. Cf this note in the documentation under the method:
.. note::
When parameters is given as a mapping, it is permissible to have keys that are
strings of the parameter names; these will be looked up using :meth:get_parameter.
You can also have keys that are :class:.ParameterVector instances, and in this case,
the dictionary value should be a sequence of values of the same length as the vector.

        If you use either of these cases, you must leave the setting ``flat_input=False``;
        changing this to ``True`` enables the fast path, where all keys must be
        :class:`.Parameter` instances.

@arthurostrauss arthurostrauss force-pushed the ParameterManager_for_string_assignment branch from c9f51e7 to b01354e Compare April 1, 2024 02:00
@wshanks
Copy link
Contributor

wshanks commented Apr 1, 2024

@arthurostrauss That sounds nice! My main concern was adding something to pulse assign_parameters that was not in circuit assign_parameters but I had missed that circuits supported string parameters. I think this should be fine conceptually then (I haven't looked at it closely enough to approve right now).

@arthurostrauss arthurostrauss force-pushed the ParameterManager_for_string_assignment branch from b01354e to 6d76478 Compare April 8, 2024 03:26
@arthurostrauss
Copy link
Contributor Author

Hi @wshanks, any updates on this?

@arthurostrauss arthurostrauss force-pushed the ParameterManager_for_string_assignment branch from 6d76478 to 665f7a6 Compare April 11, 2024 13:50
@wshanks
Copy link
Contributor

wshanks commented Apr 11, 2024

@arthurostrauss For my part, I had an initial concern about deviating from QuantumCircuit.assign_parameters that you addressed, and then I just have not had time to look at this more closely. I will when I have time. I don't want to block this though if @nkanazawa1989 or @TsafrirA are able to look at it before me.

@arthurostrauss arthurostrauss force-pushed the ParameterManager_for_string_assignment branch from 665f7a6 to 376b54b Compare April 18, 2024 09:42
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.

Although the type annotations do not advertise it, assign_parameters already supports str. What you are adding here is support for assigning a sequence to elements of a ParameterVector using the name of the ParameterVector like:

from qiskit import pulse
from qiskit.circuit import ParameterVector

vec = ParameterVector("test", 2)

with pulse.builder.build() as sched:
    pulse.builder.play(pulse.Gaussian(100, vec[0], vec[1]), pulse.DriveChannel(0))

sched.assign_parameters({"theta": [1, 2]})

Since ParameterVector support was only added recently in #12045, this PR should be viewed as a follow up to it.

Going back to my original concern about deviation from the behavior of QuantumCircuit, I note that QuantumCircuit.assign_parameters does not allow assigning to a parameter vector by name. We should see if there is interest in such support for that case as well.

qiskit/pulse/parameter_manager.py Outdated Show resolved Hide resolved
qiskit/pulse/parameter_manager.py Outdated Show resolved Hide resolved
qiskit/pulse/parameter_manager.py Outdated Show resolved Hide resolved
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.

Sorry for the slightly redundant comments in the previous review. I was shifting comments around and hadn't meant to submit the review yet. Here were a couple other comments.

test/python/pulse/test_parameter_manager.py Outdated Show resolved Hide resolved
if param.name == parameter:
out[param] = value
elif (
isinstance(param, ParameterVectorElement) and param.vector.name == parameter
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this part to be getting a little complicated. Could we lift the string handling to before this loop? So in a preceding loop, loop over parameter_binds and make a new copy that has all Parameter and ParameterVector keys by replacing any strings with the appropriately named matches?

@arthurostrauss arthurostrauss force-pushed the ParameterManager_for_string_assignment branch from 376b54b to 5406185 Compare April 21, 2024 07:47
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.

@arthurostrauss Could you address my comment about lifting up the string handling to earlier in the function and my minor suggestion about the release note? Otherwise, I think this looks good.

@arthurostrauss arthurostrauss force-pushed the ParameterManager_for_string_assignment branch 2 times, most recently from d0a5834 to 6a58f19 Compare April 26, 2024 12:03
@@ -410,21 +411,23 @@ def _unroll_param_dict(
A dictionary from parameter to value.
"""
out = {}
param_name_dict = {param.name: param for param in self.parameters}
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not account for the fact that more than one parameter could have the same name, which pulse allows but circuit does not (it gives an error when trying to add the second parameter).

I think would be best to loop over self.parameters and make one dict shaped like dict[str, list[Parameter | ParameterVector]] mapping each name to the parameters with that name. Then make a new dict and loop over parameter_binds copying in the key and values for when the key is a Parameter or ParameterVector and replacing the key when it is a string with an entry for each value in the first dict for that string (could be more than one parameter). Then loop over that dict like below but without accounting for string keys.

You may want to account for a name being used for a parameter and a parameter vector. There is validation for vectors but not for scalars. If you tried to assign to a list a string name used by both a ParameterVector and a Parameter, it would not catch the assignment of a list to a Parameter here.

Maybe it is worth testing duplicated names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So just to clarify, the expected behaviour is that every Parameter carrying the same name should be assigned to the same value? Or should it be that a list of values could be passed such that all parameters carrying the same name are taken into account? If it's the latter case, then how can we resolve this when using string parameter assignment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Every Parameter carrying the same name should be assigned the same value when a string is assigned a value. Also, every ParameterVector should be assigned the same list of values.

@arthurostrauss arthurostrauss force-pushed the ParameterManager_for_string_assignment branch from a3ec205 to 6c53868 Compare April 26, 2024 22:06
It is now possible to specify in the mapping the names of the parameters instead of the parameters themselves to assign parameters to pulse `Schedule`s and `ScheduleBlock`s. It is even possible to assign all the parameters of a ParameterVector by just specifying its name and setting as a value a list of parameter values.
arthurostrauss and others added 5 commits April 27, 2024 00:34
…b9e.yaml

Co-authored-by: Will Shanks <wshaos@posteo.net>
Corrected mistake in string assignment

Corrected mistake in string assignment
…dules-3a27bbbbf235fb9e.yaml

Co-authored-by: Will Shanks <wshaos@posteo.net>
The check is now based on the value type to infer if assignment should be done on Parameters or ParameterVectors

Removed unnecessary import from utils

Corrected string assignment

Correction part 2

Corrected test

The inplace=True argument was preventing the reuse of a parametrized waveform in the schedule, making the test fail
@arthurostrauss arthurostrauss force-pushed the ParameterManager_for_string_assignment branch from 461aacc to 054022e Compare April 26, 2024 22:34
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 good to me now!

@wshanks wshanks added this pull request to the merge queue Apr 29, 2024
Merged via the queue into Qiskit:main with commit b442b1c Apr 29, 2024
13 checks passed
@arthurostrauss arthurostrauss deleted the ParameterManager_for_string_assignment branch May 7, 2024 02:54
@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.

5 participants