Fix/commuting2q block parameter preservation#16459
Conversation
|
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 following people are relevant to this code:
|
|
Thanks @TalKozlovski for finding and fixing the issue! |
Cryoris
left a comment
There was a problem hiding this comment.
Thanks for the contribution -- this needs a bit more testing to ensure the parameters also handled correctly in a circuit context. I left some comments below.
There was a problem hiding this comment.
Can we add tests where
- we check the circuit containing the
Commuting2qBlockalso knows about the parameters, and - we assign parameters to a circuit containing
Commuting2qBlock-- especially with an example where we re-use the same parameter multiple times inside a block
This is to ensure that not only the block knows about the parameters but that they are also handled correctly
There was a problem hiding this comment.
Thank you for the suggestions, added respective tests
| for param in node.op.params: | ||
| if isinstance(param, ParameterExpression): | ||
| params_set.update(param.parameters) | ||
| params_list = sorted(params_set, key=lambda p: p.name) |
There was a problem hiding this comment.
The parameters should support unique sorting already, which will take into account indices of parameters coming from a ParameterVector. Sorting by name will produce unexpected sortings like v[0], v[1], v[10], v[2], v[3], ....
There was a problem hiding this comment.
I drop the sorting and just convert the set into a list
There was a problem hiding this comment.
Dropping the sorting seems correct since the block does not need to know anything about parameter order -- the circuit will figure out the right order when assigning parameters by array. But you have a test which asserts that the order is what you expect, we have to remove this test if we no longer sort.
| """ | ||
| qubits: set[Qubit] = set() | ||
| cbits: set[Clbit] = set() | ||
| params_set: set[ParameterExpression] = set() |
There was a problem hiding this comment.
This contains only Parameters, not expressions
| params_set: set[ParameterExpression] = set() | |
| params_set: set[Parameter] = set() |
This PR fixes parameter loss in
Commuting2qBlockwhereparamswas hardcoded to an empty list, breaking parametric circuits.Fix issue #16456
Details
Problem:
Commuting2qBlock.__init__hardcodedparams=[], losing all parameters from input gates.Solution: Extract parameters from gate operations.
Changes:
__init__to collect parameters fromParameterExpressionobjectstest_swap_strategy_router.pyAI/LLM disclosure