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

Support binding/assigning parameters by name #7107

Closed
kdk opened this issue Oct 6, 2021 · 9 comments · Fixed by #11432
Closed

Support binding/assigning parameters by name #7107

kdk opened this issue Oct 6, 2021 · 9 comments · Fixed by #11432
Assignees
Labels
priority: high type: enhancement It's working, but needs polishing
Milestone

Comments

@kdk
Copy link
Member

kdk commented Oct 6, 2021

What is the expected enhancement?

From @Cryoris 's comment in #5557 (comment) :

Personally I'd be fine if that'd work with a string as well, since parameter names are unique in circuits.

partially_bound = circuit.assign_parameters({'x': value})

We should consider adding support for binding and assigning parameters via their string name. This was originally omitted for concerns over overlapping parameter names between different circuits. However, by the stage of bind_/assign_, the Parameter s on a given circuit are fixed and guaranteed to be unique, so there isn't really a possibility for the ambiguity that exists when composing two parameterized circuits.

@kdk kdk added the type: enhancement It's working, but needs polishing label Oct 6, 2021
@jakelishman
Copy link
Member

I helped someone in Slack recently who had assumed that this would be possible, and was surprised that it wasn't, so there's an indirect +1 on this from them.

@VicentePerezSoloviev
Copy link
Contributor

VicentePerezSoloviev commented Oct 6, 2021

Yes, I am the one who you helped @jakelishman. Thanks for that!! I was surprised because for example in myQLM you can bind parameters by the string name. You may not have access to the parameter variable name, and only to its string name. I would like to be assigned this issue to fix this problem if possible. Thanks.

@Cryoris
Copy link
Contributor

Cryoris commented Oct 7, 2021

Sure @VicentePerezSoloviev, let us know if you have any questions!

@Cryoris Cryoris added this to the 0.45.0 milestone Aug 18, 2023
@nonhermitian
Copy link
Contributor

This would be a most welcome improvement.

@1ucian0
Copy link
Member

1ucian0 commented Dec 18, 2023

This issue seems to be blocking the high priority #11227 . As such, tagging it as high priority too.

@jakelishman
Copy link
Member

jakelishman commented Dec 18, 2023

At the time I assigned this to myself in October, this was my intended route for this PR:

  • track QuantumCircuit.global_phase in ParameterTable
  • add a QuantumCircuit.get_parameter method that looks up the variable in the parameter table
  • teach QuantumCircuit.assign_parameters to lookup string keys in an input dict using the get_parameter method above.

In service of the first bullet, I found a couple of bugs and problems in implementation that I intended to fix and refactor on the way through, since parameter-handling code was growing into new places. This leads to the current dependency chain, where each PR depends on the one above it:

I'm currently blocked on needing a reviewer for #11107 - this issue wasn't high priority or a blocker for anything at the time, so we didn't race for a review. I can do all the other parts in quick follow-ups if I've got some reviewers.

@jakelishman
Copy link
Member

jakelishman commented Dec 18, 2023

The API change to QuantumCircuit after the last PR would be:

  • a new method QuantumCircuit.get_parameter(self, name: str) -> Parameter that raises KeyError if the parameter is not found. A parameter found by QuantumCircuit.get_parameter is guaranteed to be assignable using QuantumCircuit.assign_parameters.
  • the type of the mapping in the map form of QuantumCircuit.assign_parameters would expand from Mapping[Parameter, ValueT] to Mapping[Parameter | str, ValueT] (ignoring ParameterVector inputs), where strings would be looked up with the semantics of get_parameter.

@jakelishman
Copy link
Member

It's technically possible to skip to the last step of immediately implementing this PR, but if we do skip the dependency chain, then there's a fair amount more code duplication and potential surface for bugs in the parameter handling, because more and more places are having to do special-case lookup between various incompatible places doing partial parameter tracking. It'd be harder for me to write correctly, and more likely to become incorrect in the future if we don't fix the bug tail that I turned up while trying to write the initial version of it.

@jakelishman
Copy link
Member

Alright, all the PRs in the dependency chain are now open (though dependent on their predecessors), so people can build off a concrete proposal if they choose to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high type: enhancement It's working, but needs polishing
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants