Update parameters to accommodate with RZZ constraints #2126
Conversation
|
I left undone for now:
Other than that it's ready to play with. |
| if is_rz or is_rx: | ||
| # param_exp * 0 to prevent an error complaining that the original parameters, | ||
| # still present in the parameter values, are missing from the circuit | ||
| param_rzz = param_exp * 0 + Parameter(f"{param_prefix}rzz") |
There was a problem hiding this comment.
I am not sure where the error comes in. It might be worth checking with the Qiskit team if this is the best way to do this. I just worry that a future change could make 0 * param still drop param from the circuit.
There was a problem hiding this comment.
@yaelbh do you have an error trace we can look at? I am also not sure what error this is referring to, but I agree that we should take a look.
| rzz_angles[idx] = pi / 2 - abs(mod(angle, pi) - pi / 2) | ||
|
|
||
| rzz_count += 1 | ||
| param_prefix = f"rzz_{rzz_count}_" |
There was a problem hiding this comment.
It might be worth checking if the original circuit has rzz_1_ parameters. I guess that would only happen if function had already been run on the pub and then all the rzz should already be folded and not need new parameters added.
| if all(np.isclose(rz_angle, pi) for rz_angle in rz_angles): | ||
| new_data.append( | ||
| CircuitInstruction( | ||
| RZGate(pi), |
There was a problem hiding this comment.
I wonder if this case and the is_x case come up enough to be relevant vs just always using a parameter for consistency.
| new_circ.data = new_data | ||
|
|
||
| if program_id == "sampler": | ||
| return SamplerPub.coerce((new_circ, val_data), pub.shots) |
There was a problem hiding this comment.
At some point, you need to ravel the new rzz_ parameters back to match the shape of the data in the original pub to support parameter arrays with more than one dimension. For example, this code fails for me on this line:
import numpy as np
from qiskit.circuit import Parameter
from qiskit import QuantumCircuit
from qiskit_ibm_runtime.transpiler.passes.basis.fold_rzz_angle import convert_to_rzz_valid_pub
p1 = Parameter("p1")
p2 = Parameter("p2")
circ = QuantumCircuit(3)
circ.rzz(p1 + p2, 0, 1)
circ.rzz(0.3, 0, 1)
circ.x(0)
circ.rzz(p1 - p2, 2, 1)
convert_to_rzz_valid_pub("sampler", (circ, {"p1": np.ones((3, 2)), "p2": np.ones((3, 2))}))| @@ -244,3 +248,163 @@ def _quad4(angle: float, qubits: Tuple[Qubit, ...]) -> DAGCircuit: | |||
| check=False, | |||
| ) | |||
| return new_dag | |||
|
|
|||
|
|
|||
| def convert_to_rzz_valid_pub( | |||
There was a problem hiding this comment.
Since this is not a transpiler pass that will be run automatically, do you want to expose it in a more public way by including it in the documentation and possibly providing a higher level import path than qiskit_ibm_runtime.transpiler.passes.basis.fold_rzz_angle?
| if is_rz or is_rx: | ||
| # param_exp * 0 to prevent an error complaining that the original parameters, | ||
| # still present in the parameter values, are missing from the circuit | ||
| param_rzz = param_exp * 0 + Parameter(f"{param_prefix}rzz") |
There was a problem hiding this comment.
@yaelbh do you have an error trace we can look at? I am also not sure what error this is referring to, but I agree that we should take a look.
|
|
||
|
|
||
| def convert_to_rzz_valid_pub( | ||
| program_id: str, pub: Union[SamplerPubLike, EstimatorPubLike] |
There was a problem hiding this comment.
If this is intended to be user-facing, I feel like it would be smoother to allow the function to accept a primitive instance instead of a program_id. I don't think that the program_id is an externally understood attribute (and it probably shouldn't be), and you can always fetch it from the primitive.
| program_id: str, pub: Union[SamplerPubLike, EstimatorPubLike] | |
| primitive: Union[SamplerV2, EstimatorV2], pub: Union[SamplerPubLike, EstimatorPubLike] |
| col_indices = [np.where(pub_params == param_name)[0][0] for param_name in param_names] | ||
| # col_indices is the indices of columns in the parameter value array that have to be checked |
There was a problem hiding this comment.
super minor suggestion:
| col_indices = [np.where(pub_params == param_name)[0][0] for param_name in param_names] | |
| # col_indices is the indices of columns in the parameter value array that have to be checked | |
| # col_indices is the indices of columns in the parameter value array that have to be checked | |
| col_indices = [np.where(pub_params == param_name)[0][0] for param_name in param_names] |
| if (angle + pi / 2) % (2 * pi) >= pi: | ||
| rz_angles[idx] = pi | ||
| else: | ||
| rz_angles[idx] = 0 | ||
|
|
||
| if angle % pi >= pi / 2: | ||
| rx_angles[idx] = pi | ||
| else: | ||
| rx_angles[idx] = 0 | ||
|
|
||
| rzz_angles[idx] = pi / 2 - abs(mod(angle, pi) - pi / 2) |
There was a problem hiding this comment.
When the target allows to store angle bound information, this could be extended to read the bounds from the target.
There was a problem hiding this comment.
Let me know when it happens.
ElePT
added
enhancement
Summary
Solves #2125 through a function that translates from pub to pub.
Only stared - work is still in progress.
Details and comments
In fact, in order not to have to bother with the different structures parameter values can have in pubs, at least for now we translate a pair of circuit and parameter values to another pair, and require the parameter values to be just a list of tuples. The order inside each tuple matches the order of appearance of parameters in the circuit, just like one of the options users have to specify parameter values when defining pubs.