Properly substitute symbolic parameters #152
Conversation
| new_gate_params = [] | ||
| for param_expression, value in zip(gate_instance.params, gate_params): | ||
| # Assumes that each Qiskit gate has one free parameter per expression |
There was a problem hiding this comment.
To be more accurate, a gate can have many parameters (gate_instance.params), but these expressions contains a single Parameter. Here we are only looking at the gates in _GATE_NAME_TO_QISKIT_GATE where it is the case (at worst gate parameters are proportial to the used Parameter, e.g GPi). It is very unlikely that we will define a gate in this dict as Gate(Parameter("a")+Parameter("b")).
There was a problem hiding this comment.
Yep, that's what I was trying to say with "per expression"; it felt weird to say "each gate's parameter expressions" because the expressions themselves are referred to as the gate's parameters.
| gate_cls = gate_instance.__class__ | ||
| else: | ||
| raise TypeError(f'Braket gate "{gate_name}" not supported in Qiskit') | ||
| assigned = param_expression.assign(param, value) |
There was a problem hiding this comment.
assign is the function I was looking for. So glad you bring this up. This is much better that the fix I was implementing on my side.
Side-effect, it works when passing a ParameterExpression
| param_uuids = { | ||
| param.name: param._uuid for param in qiskit_circuit.parameters | ||
| } | ||
| params_qiskit = [ |
There was a problem hiding this comment.
Why not just passing qiskit_circuit.parameters? We could assert that qiskit.parameters names are the same as those in params if we want to check the parameters.
tests/providers/test_adapter.py
Outdated
| expected_qiskit_circuit = expected_qiskit_circuit.assign_parameters( | ||
| dict( | ||
| zip( | ||
| # Need to use gate parameters because |
There was a problem hiding this comment.
Would this be simpler if we use qiskit_circuit.parameters?
There was a problem hiding this comment.
The problem is qiskit_circuit.parameters orders the parameters alphabetically, and the parameters in the _GATE_NAME_TO_BRAKET_GATE translations aren't. For example, the order of u parameters is theta, phi, lambda, but the alphabetical order is theta, lambda, phi.
|
|
||
| expected_qiskit_circuit = QuantumCircuit(op.qubit_count) | ||
| qiskit_gate = _GATE_NAME_TO_QISKIT_GATE.get(gate_name.lower()) | ||
| expected_qiskit_circuit.append(qiskit_gate, target) |
There was a problem hiding this comment.
for IonQ, I think this test would still miss the 2pi scaling factor since we are using directly the gate from the dict. We could maybe try to test the unitaries or a shot=0 sim.
There was a problem hiding this comment.
That would make sense; the ideal way would be to compare the parametrized unitaries.
Summary
to_qiskitnow substitutes the parameter in a Qiskit gate's parameter expression instead of replacing the entire expression wholesale.Details and comments