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

Assign values directly to fully bound parameters in quantum circuits #10183

Merged
merged 19 commits into from
Jun 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions qiskit/circuit/quantumcircuit.py
Original file line number Diff line number Diff line change
Expand Up @@ -2854,7 +2854,17 @@ def _assign_parameter(self, parameter: Parameter, value: ParameterValueType) ->
new_param = assignee.assign(parameter, value)
# if fully bound, validate
if len(new_param.parameters) == 0:
instr.params[param_index] = instr.validate_parameter(new_param)
Cryoris marked this conversation as resolved.
Show resolved Hide resolved
if new_param._symbol_expr.is_integer and new_param.is_real():
val = int(new_param)
elif new_param.is_real():
# Workaround symengine not supporting float(<ComplexDouble>)
val = complex(new_param).real
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this complex(self).real form be moved into ParameterExpression.__float__ or left here? One reason I had it here was is_real() was already called, and I wasn't sure which behavior was preferred in __float__. float(1+1j) is a TypeError in standard Python.

A couple other things I noticed related to this:

  • symengine/sympy treat complex numbers as floating point, so something involving complex integers will end up here and evaluate to a float rather than an int even if the math involves integers that evaluate to having no imaginary part.
  • The ParameterExpression.__float__ is misleading when the value is complex because it catches the sympy/symengine error and raises an error about unbound parameters instead of the complex vs float error message.

Copy link
Member

Choose a reason for hiding this comment

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

The __float__ method is only meant to be implemented for types for which there is an exact conversion (unlike __int__, which has "casting" semantics), so it sounds like Symengine's behaviour is allowable by the spec. If we want to do something slightly tweaked, it'd be good to unify it in ParameterExpression.__float__ rather than here. I think in the interest of landing this PR for a patch release today, let's look at doing that in a follow-up, though.

Bullet 1: there's no such thing as a complex integral type in Python either, so I don't think we need to worry about that (also, ParameterExpression is not entirely designed to represent integers).

Bullet 2: yeah, I believe it was an oversight when complex-number support was added to ParameterExpression. There's issue #9187 tracking that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, okay. I hadn't seen #9187 which is the exact issue. Regarding ParameterExpression behaving as a float when there is a zero imaginary part, I think following the behavior of Python's float and complex makes sense as an intuitive behavior, even though it goes against what I did here. My motivation for casting complex values with zero imaginary part back to float was that before I did that this test was failing:

https://github.com/Qiskit/qiskit-terra/blob/174b661d57f4b888796c0895ac6b1ba79db11d52/test/python/circuit/test_parameters.py#L1379-L1398

Perhaps that test should be removed and the behavior not supported. It is a bit funny. That test was passing despite #9187 because __eq__ is less strict than __float__ about the types.

Maybe a design should be written up for Parameter. These edge cases like #9187 and #9299 may be arising from different developers making different assumptions.

else:
# complex values may no longer be supported but we
# defer raising an exception to validdate_parameter
# below for now.
val = complex(new_param)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just checking that we expect complex parameters possibly here? I didn't allow them for the calibration keys below. Perhaps I should? The previous code did not allow it and support for complex parameters in pulse has been deprecated in #9897 so I think it is not needed.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think complex parameters are possible here. At least historically complex parameters were only expected in a pulse schedule. But @Cryoris can confirm (since he originally added support for complex parameters).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that's right, so far the only use case was the pulses. If they are deprecated there, I don't think there is any other part that requires supporting binding complex parameter values.

Copy link
Member

Choose a reason for hiding this comment

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

I think/thought that various parts of the algorithms' gradients calculations and fancy custom gates in there used complex parameters (like PauliEvolutionGate maybe?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took the complex case out but put it back just now because a couple tests were failing. The tests were trying to assign a complex value and see that an appropriate exception was raised by the validate_parameter method of the instruction. We can leave it for later whether we want to lift up the exception raising to a higher level of than individual instructions' validate_parameter.

instr.params[param_index] = instr.validate_parameter(val)
else:
instr.params[param_index] = new_param

Expand Down Expand Up @@ -2911,7 +2921,11 @@ def _assign_calibration_parameters(
if isinstance(p, ParameterExpression) and parameter in p.parameters:
new_param = p.assign(parameter, value)
if not new_param.parameters:
new_param = float(new_param)
if new_param._symbol_expr.is_integer:
new_param = int(new_param)
else:
# Workaround symengine not supporting float(<ComplexDouble>)
new_param = complex(new_param).real
new_cal_params.append(new_param)
else:
new_cal_params.append(p)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
---
fixes:
- |
Changed the binding of numeric values with
:meth:`.QuantumCircuit.assign_parameters` to avoid a mismatch between the
values of circuit instruction parameters and corresponding parameter keys
in the circuit's calibration dictionary. Fixed `#9764
<https://github.com/Qiskit/qiskit-terra/issues/9764>`_ and `#10166
<https://github.com/Qiskit/qiskit-terra/issues/10166>`_. See also the
related upgrade note regarding :meth:`.QuantumCircuit.assign_parameters`.
upgrade:
- |
Changed :meth:`.QuantumCircuit.assign_parameters` to bind
assigned integer and float values directly into the parameters of
:class:`~qiskit.circuit.Instruction` instances in the circuit rather than
binding the values wrapped within a
:class:`~qiskit.circuit.ParameterExpression`. This change should have
little user impact as ``float(QuantumCircuit.data[i].operation.params[j])``
still produces a ``float`` (and is the only way to access the value of a
:class:`~qiskit.circuit.ParameterExpression`). Also,
:meth:`~qiskit.circuit.Instruction` parameters could already be ``float``
as well as a :class:`~qiskit.circuit.ParameterExpression`, so code dealing
with instruction parameters should already handle both cases. The most
likely chance for user impact is in code that uses ``isinstance`` to check
for :class:`~qiskit.circuit.ParameterExpression` and behaves differently
depending on the result. Additionally, qpy serializes the numeric value in
a bound :class:`~qiskit.circuit.ParameterExpression` at a different
precision than a ``float`` (see also the related bug fix note about
:meth:`.QuantumCircuit.assign_parameters`).
35 changes: 34 additions & 1 deletion test/python/circuit/test_circuit_load_from_qpy.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

import numpy as np

from qiskit import QuantumCircuit, QuantumRegister, ClassicalRegister
from qiskit import QuantumCircuit, QuantumRegister, ClassicalRegister, pulse
from qiskit.circuit import CASE_DEFAULT
from qiskit.circuit.classicalregister import Clbit
from qiskit.circuit.quantumregister import Qubit
Expand Down Expand Up @@ -274,6 +274,39 @@ def test_bound_parameter(self):
self.assertEqual(qc, new_circ)
self.assertDeprecatedBitProperties(qc, new_circ)

def test_bound_calibration_parameter(self):
"""Test a circuit with a bound calibration parameter is correctly serialized.

In particular, this test ensures that parameters on a circuit
instruction are consistent with the circuit's calibrations dictionary
after serialization.
"""
amp = Parameter("amp")

with pulse.builder.build() as sched:
pulse.builder.play(pulse.Constant(100, amp), pulse.DriveChannel(0))

gate = Gate("custom", 1, [amp])

qc = QuantumCircuit(1)
qc.append(gate, (0,))
qc.add_calibration(gate, (0,), sched)
qc.assign_parameters({amp: 1 / 3}, inplace=True)

qpy_file = io.BytesIO()
dump(qc, qpy_file)
qpy_file.seek(0)
new_circ = load(qpy_file)[0]
self.assertEqual(qc, new_circ)
instruction = new_circ.data[0]
cal_key = (
tuple(new_circ.find_bit(q).index for q in instruction.qubits),
tuple(instruction.operation.params),
)
# Make sure that looking for a calibration based on the instruction's
# parameters succeeds
self.assertIn(cal_key, new_circ.calibrations[gate.name])

def test_parameter_expression(self):
"""Test a circuit with a parameter expression."""
theta = Parameter("theta")
Expand Down
55 changes: 43 additions & 12 deletions test/python/circuit/test_parameters.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
from test import combine

import numpy
from ddt import data, ddt
from ddt import data, ddt, named_data

import qiskit
import qiskit.circuit.library as circlib
Expand Down Expand Up @@ -346,6 +346,23 @@ def test_multiple_named_parameters(self):
self.assertEqual(theta.name, "θ")
self.assertEqual(qc.parameters, {theta, x})

@named_data(
["int", 2, int],
["float", 2.5, float],
["float16", numpy.float16(2.5), float],
["float32", numpy.float32(2.5), float],
["float64", numpy.float64(2.5), float],
)
def test_circuit_assignment_to_numeric(self, value, type_):
"""Test binding a numeric value to a circuit instruction"""
x = Parameter("x")
qc = QuantumCircuit(1)
qc.append(Instruction("inst", 1, 0, [x]), (0,))
qc.assign_parameters({x: value}, inplace=True)
bound = qc.data[0].operation.params[0]
self.assertIsInstance(bound, type_)
self.assertEqual(bound, value)

def test_partial_binding(self):
"""Test that binding a subset of circuit parameters returns a new parameterized circuit."""
theta = Parameter("θ")
Expand Down Expand Up @@ -401,10 +418,10 @@ def test_expression_partial_binding(self):
self.assertTrue(isinstance(pqc.data[0].operation.params[0], ParameterExpression))
self.assertEqual(str(pqc.data[0].operation.params[0]), "phi + 2")

fbqc = getattr(pqc, assign_fun)({phi: 1})
fbqc = getattr(pqc, assign_fun)({phi: 1.0})

self.assertEqual(fbqc.parameters, set())
self.assertTrue(isinstance(fbqc.data[0].operation.params[0], ParameterExpression))
self.assertIsInstance(fbqc.data[0].operation.params[0], float)
self.assertEqual(float(fbqc.data[0].operation.params[0]), 3)

def test_two_parameter_expression_binding(self):
Expand Down Expand Up @@ -448,7 +465,7 @@ def test_expression_partial_binding_zero(self):
fbqc = getattr(pqc, assign_fun)({phi: 1})

self.assertEqual(fbqc.parameters, set())
self.assertTrue(isinstance(fbqc.data[0].operation.params[0], ParameterExpression))
self.assertIsInstance(fbqc.data[0].operation.params[0], int)
self.assertEqual(float(fbqc.data[0].operation.params[0]), 0)

def test_raise_if_assigning_params_not_in_circuit(self):
Expand Down Expand Up @@ -505,8 +522,15 @@ def test_calibration_assignment(self):
circ.add_calibration("rxt", [0], rxt_q0, [theta])
circ = circ.assign_parameters({theta: 3.14})

self.assertTrue(((0,), (3.14,)) in circ.calibrations["rxt"])
sched = circ.calibrations["rxt"][((0,), (3.14,))]
instruction = circ.data[0]
cal_key = (
tuple(circ.find_bit(q).index for q in instruction.qubits),
tuple(instruction.operation.params),
)
self.assertEqual(cal_key, ((0,), (3.14,)))
# Make sure that key from instruction data matches the calibrations dictionary
self.assertIn(cal_key, circ.calibrations["rxt"])
sched = circ.calibrations["rxt"][cal_key]
self.assertEqual(sched.instructions[0][1].pulse.amp, 0.2)

def test_calibration_assignment_doesnt_mutate(self):
Expand All @@ -531,11 +555,11 @@ def test_calibration_assignment_doesnt_mutate(self):
self.assertNotEqual(assigned_circ.calibrations, circ.calibrations)

def test_calibration_assignment_w_expressions(self):
"""That calibrations with multiple parameters and more expressions."""
"""That calibrations with multiple parameters are assigned correctly"""
theta = Parameter("theta")
sigma = Parameter("sigma")
circ = QuantumCircuit(3, 3)
circ.append(Gate("rxt", 1, [theta, sigma]), [0])
circ.append(Gate("rxt", 1, [theta / 2, sigma]), [0])
circ.measure(0, 0)

rxt_q0 = pulse.Schedule(
Expand All @@ -548,8 +572,15 @@ def test_calibration_assignment_w_expressions(self):
circ.add_calibration("rxt", [0], rxt_q0, [theta / 2, sigma])
circ = circ.assign_parameters({theta: 3.14, sigma: 4})

self.assertTrue(((0,), (3.14 / 2, 4)) in circ.calibrations["rxt"])
sched = circ.calibrations["rxt"][((0,), (3.14 / 2, 4))]
instruction = circ.data[0]
cal_key = (
tuple(circ.find_bit(q).index for q in instruction.qubits),
tuple(instruction.operation.params),
)
self.assertEqual(cal_key, ((0,), (3.14 / 2, 4)))
# Make sure that key from instruction data matches the calibrations dictionary
self.assertIn(cal_key, circ.calibrations["rxt"])
sched = circ.calibrations["rxt"][cal_key]
self.assertEqual(sched.instructions[0][1].pulse.amp, 0.2)
self.assertEqual(sched.instructions[0][1].pulse.sigma, 16)

Expand Down Expand Up @@ -789,7 +820,7 @@ def test_binding_parameterized_circuits_built_in_multiproc(self):
for qc in results:
circuit.compose(qc, inplace=True)

parameter_values = [{x: 1 for x in parameters}]
parameter_values = [{x: 1.0 for x in parameters}]

qobj = assemble(
circuit,
Expand All @@ -802,7 +833,7 @@ def test_binding_parameterized_circuits_built_in_multiproc(self):
self.assertTrue(
all(
len(inst.params) == 1
and isinstance(inst.params[0], ParameterExpression)
and isinstance(inst.params[0], float)
and float(inst.params[0]) == 1
for inst in qobj.experiments[0].instructions
)
Expand Down
2 changes: 1 addition & 1 deletion test/python/qasm3/test_export.py
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,7 @@ def test_reused_custom_parameter(self):
" rx(0.5) _gate_q_0;",
"}",
f"gate {circuit_name_1} _gate_q_0 {{",
" rx(1) _gate_q_0;",
" rx(1.0) _gate_q_0;",
"}",
"qubit[1] _all_qubits;",
"let q = _all_qubits[0:0];",
Expand Down