Assign values directly to fully bound parameters in quantum circuits

[wshanks]

Summary

Changed QuantumCircuit.assign_parameters so that assigning a concrete value to a parameter inserts that value directly into the circuit's instructions' parameter rather than inserting a ParameterExpression containing only that value.

Details and comments

When QuantumCircuit.assign_parameters is called, parameter substitution occurs in the list of parameters for each instruction, in the definition of each instrution, in the global phase of the circuit, and in the mapping of gates to schedules in the circuit's calibrations dictionary. Before this change, numeric assigned values were inserted into the instructions' parameters lists as ParameterExpression instances with no free parameters and into the calibrations dictionary as floats. This discrepancy in handling of parameters in these two cases was a problem because qpy serializes ParameterExpression differently from float. After a round trip, the numeric content in an instruction's parameters list might no longer match the numeric key in the calibrations dictionary. Code that takes circuits as input and executes them needs the instruction parameters and calibrations keys to match as it needs to look up the calibrations for the circuit's instructions in order to run the circuit.

To address this issue, here QuantumCircuit.assign_parameters is changed to insert the numeric value directly into the instruction's parameter list, so that it matches the key in the calibrations dictionary. There is some precedent for this as the Delay instruction already does type casting in its validate_parameter method which assign_parameters calls:

https://github.com/Qiskit/qiskit-terra/blob/291824a392f131e3b7d7ffca4c6fafdecf72c007/qiskit/circuit/delay.py#L88-L102

This change is not without impact, but its impact needs to be weighed against the impact of not addressing the issue, which prevents parameterized pulse gates from being used with circuits serialized with qpy (such as with qiskit-ibm-provider). The updated unit tests give an indication of the impact:

• Three tests in test_parameters.py were modified to remove an isinstance check for ParameterExpression. int and float are valid in instruction params lists as well as ParameterExpression so I think these changes reflect overly tight test conditions and cases like this would be rare in user code.
• A test in test_export.py was updated to change a 1 to a 1.0. This change was the result of the behavior of pi_check. pi_check(1) gives "1.0" while a = Parameter("a"); pi_check(a.assign(a, 1)) gives "1". Based on this discrepancy, I think likely both float and integer representations need to be allowed in these cases or maybe pi_check needs to be more careful about changing types.
• test_complex_parameter_bound_to_real was still failing. It tests that a complex ParameterExpression that evaluates to a real number gets treated as a real number by code that does not allow an imaginary component. Here is the test code:
  https://github.com/Qiskit/qiskit-terra/blob/291824a392f131e3b7d7ffca4c6fafdecf72c007/test/python/circuit/test_parameters.py#L1382-L1390
  Interestingly, even though the equality check passes, the bound parameter can not be accessed directly as a float. This code:

from qiskit import QuantumCircuit
from qiskit.circuit import Parameter


x = Parameter("x")
  
qc = QuantumCircuit(1)
qc.rx(1j * x, 0)
bound = qc.bind_parameters({x: 1j})
bound.data[0].params

gives

RuntimeError                              Traceback (most recent call last)
File ~/.conda/envs/qiskit/lib/python3.10/site-packages/qiskit/circuit/parameterexpression.py:474, in ParameterExpression.__float__(self)
    473 try:
--> 474     return float(self._symbol_expr)
    475 # TypeError is for sympy, RuntimeError for symengine

File symengine_wrapper.pyx:1151, in symengine.lib.symengine_wrapper.Basic.__float__()

File symengine_wrapper.pyx:976, in symengine.lib.symengine_wrapper.Basic.n()

File symengine_wrapper.pyx:4346, in symengine.lib.symengine_wrapper.evalf()

RuntimeError: Not Implemented

To work around this, this change casts to complex and then takes the real part after checking that the value is real rather than casting to float directly.

Closes #9764 and #10166

[qiskit-bot]

One or more of the the following people are requested to review this:

• @Qiskit/terra-core

[wshanks]

Some other notes:

• This PR implements viable option 3 from qpy roundtrip produces different floating point rounding for parameter values in different locations #10166 (comment).
• Perhaps ParameterExpression.__float__ should be updated to handle the case of a symengine ComplexDouble expression for which the imaginary part evaluates to 0 (which this PR only handles within QuantumCircuit.assign_parameters rather than more generally.
• This PR allows for a Parameter to be assigned to a complex value since Instruction allows this. Note that Gate though does not: https://github.com/Qiskit/qiskit-terra/blob/901e3b89731f9437ccbbe5d9eb5dff657854568f/qiskit/circuit/gate.py#L226-L228
• The code that creates the calibrations dictionary key was updated to allow a parameter to remain an int rather than casting everything to float. A parameterized gate duration for instance would be an int. This code does not allow for complex values since the pulse module assumes real valued parameters.
• Ideally, this PR could be backported to the stable branch. It would be good to get another opinion on how likely this change is to break something where a user is assuming a bound parameter rather than a bare numeric type. I think this is one of the least invasive ways of fixing the issue with parameterized pulse gates. Changes with more impact are possible, like changing ParameterExpression.bind to return an int/float/complex when no free parameters remain rather than doing it as a special case inside QuantumCircuit.assign_parameters.

[qiskit-bot]

One or more of the the following people are requested to review this:

• @Qiskit/terra-core

[Cryoris]

This approach seems good to me, we did have some issues now and then with bound expressions still being ParameterExpression. Since we don't seem to have any use for keeping the object a ParameterExpression, casting it to a number seems like an improvement 👍🏻 maybe @kdk or @jakelishman have further thoughts on this?

[jakelishman]

Thank you so much for all the effort here Will, and for finding what definitely looks like the safest course of action. I'm also on board with this new behaviour of QuantumCircuit.assign_parameters - I think the intended benefits of the old behaviour never really materialised, but it has led to a couple of nuisances in the past.

Please could you add a couple of specific tests for the new behaviour and a regression test on the calibration issue? For example, testing that a fully bound real number produces an instance of float in the parameters, a fully bound complex produces an instance of complex, and that a previously failing roundtrip through QPY for the calibrations now succeeds?

In normal circumstances, I'd say this is too big a behaviour change for a backport, but I think you're right that the fix is sufficiently high priority and you've done as best as is possible to provide the least impactful fix. I'll get a second opinion from Matthew (@mtreinish) since it's against policy, but I think this'll be it.

[mtreinish]

In general I'm in favor of this change, but I do worry about the backwards compatibility here, especially for a backport. The old behavior while inconvenient (an obviously blocking for serializing calibrations) but I always thought it was an intentional choice (I'd expect @kdk, @ewinston, or @Cryoris to know for sure) to enable reassignment of Parameters later if needed. I'm not sure how common a workflow that is in practice, but won't this cause us to lose the context around the parameter on assignment now making that impossible? Or will it still work because we've still got a reference to the original Parameter in the parameter table? If it's the later I think I'm ok with it as a backport as long as we also include an upgrade note documenting the type change (it's unlikely to cause an issue but it's still necessary to document it just in case).

[jakelishman]

The "reassignment of parameters" after a complete binding was (as far as I remember) never actually implemented. Once you've bound something, you can't rebind that same object - the path to that is to keep a reference to the unbound attribute like via QuantumCircuit.assign_parameters(inplace=False).

[wshanks]

I always thought it was an intentional choice to enable reassignment of Parameters later if needed.

To expand on what Jake said, QuantumCircuit.assign_parameters ultimately calls ParameterExpression.assign() here: https://github.com/Qiskit/qiskit-terra/blob/901e3b89731f9437ccbbe5d9eb5dff657854568f/qiskit/circuit/quantumcircuit.py#L2854
and assign() ultimately returns a new ParameterExpression with no parameters (so nothing that can be rebound later) when the last parameter is assigned here: https://github.com/Qiskit/qiskit-terra/blob/901e3b89731f9437ccbbe5d9eb5dff657854568f/qiskit/circuit/parameterexpression.py#L149 so once a value is assigned you are left with a parameter expression wrapping a float/int/complex with nothing to be reassigned. This is why I thought it might be okay to backport the change because I don't think any feature is being taken away. Also, if a user had code that did something like float(circuit.data[0].params[0]) that will not break because float() should give back the same thing whether the parameter is a parameter expression wrapping a float or just the underlying float. From the tests that needed to be changed, the chance for breaking changes is from code that uses isinstance to check for a ParameterExpression. In three tests, I just had to remove that check while still leaving the more important float(param) == ... test. For test_export.py, the 1 vs 1.0 change also came from an isinstance check on ParameterExpression since pi_check handled ParameterExpression differently than float (though I think that is a tiny bug in pi_check and ideally the parameter and numeric paths would produce the same string): https://github.com/Qiskit/qiskit-terra/blob/901e3b89731f9437ccbbe5d9eb5dff657854568f/qiskit/circuit/tools/pi_check.py#L49

[coveralls]

Pull Request Test Coverage Report for Build 5147385330

• 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
• 41 unchanged lines in 9 files lost coverage.
• Overall coverage decreased (-0.03%) to 85.889%

---

Files with Coverage Reduction	New Missed Lines	%
qiskit/circuit/instruction.py	1	95.69%
qiskit/dagcircuit/dagcircuit.py	1	89.59%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	1	90.18%
qiskit/extensions/hamiltonian_gate.py	2	89.39%
crates/accelerate/src/vf2_layout.rs	3	94.74%
crates/qasm2/src/lex.rs	3	91.14%
qiskit/circuit/gate.py	5	90.7%
crates/qasm2/src/parse.rs	12	96.18%
qiskit/circuit/delay.py	13	71.19%

Totals
Change from base Build 5145627724:	-0.03%
Covered Lines:	71199
Relevant Lines:	82897

---

💛 - Coveralls

[wshanks]

I will add tests and an upgrade note. I put a couple questions in line.

One other question is if we stop here or consider a larger change as a follow up, like allowing ParameterExpression.assign to return a float rather than a ParameterExpression.

[wshanks]

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.

[jakelishman]

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.

[wshanks]

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.

[wshanks]

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.

[mtreinish]

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).

[Cryoris]

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.

[jakelishman]

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

[wshanks]

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.

[mtreinish]

The other nice thing that I just realized is that this will be a nice speed boost for QPY serialization of bound parameterized circuits. The biggest bottleneck in QPY serialization right now is calling out to sympy to serialize ParameterExpression objects. So by dropping the wrapping ParameterExpression we should see a noticeable performance improvement in qpy serialization of circuits by avoiding the use of sympy and just writing out the raw float.

[wshanks]

I pushed some new commits just now, mostly related to testing. Here is extra description (though the individual commits should be readable):

1. I put back the isinstance checks on the assign_parameters tests (testing for int/float rather than ParameterExpression) as Jake requested in Assign values directly to fully bound parameters in quantum circuits #10183 (review)
2. I added a test that the simplest circuit with a parameter gets its value assigned to the expected value and type for different assignment types as an additional response to Assign values directly to fully bound parameters in quantum circuits #10183 (review).
3. I added a check that the qubit indices and parameter values in a circuit's instruction can be used to look up the instruction's calibration to the existing tests of parameter assignment with calibrations. This change actually found a bug in one of the tests which I fixed.
4. I added a regression test that currently fails with main but passes with this PR. It binds a parameter used in a gate with a calibration to an irrational number and checks that the instruction's parameters can be used to look up the calibration in the calibrations dict.
5. I split the release note in to bug fix and upgrade notes.
6. I removed support for the complex assignment based on Assign values directly to fully bound parameters in quantum circuits #10183 (comment).

Besides any feedback on the changes here, I am curious about the idea of fixing ParameterExpression.__float__ here or in a separate PR to address the case of a complex expression with 0 imaginary component and also to fix the error (not giving a message about unbound parameters when there are none).

[wshanks]

Interestingly, this passed for me with assertEqual as well and I could not figure out why. I stepped in with a debugger and could see that bound and value had different precision for the float16 case.

[jakelishman]

Yeah, the closest double-precision float to float16(2.1) is different to the closest double-precision float to real(2.1), so the rounding goes differently. Since this test is just testing types, it'd probably be best to test something with an exact representation in half-precision, such as 2.5, then we can use assertEqual to assert that there's no dodgy conversions happening.

[wshanks]

I saw float16 and float32 tested in test_bind_half_single_precision so I tested them here. We could test other types if any are relevant.

[jakelishman]

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.

[jakelishman]

Yeah, the closest double-precision float to float16(2.1) is different to the closest double-precision float to real(2.1), so the rounding goes differently. Since this test is just testing types, it'd probably be best to test something with an exact representation in half-precision, such as 2.5, then we can use assertEqual to assert that there's no dodgy conversions happening.

[jakelishman]

(I'll just push a commit doing that floating-point change I suggested, then tag this for merge)

[jakelishman]

Thanks so much for getting this sorted, Will. It's a super nasty confluence of two Terra components that don't get a whole lot of attention, not to mention we'd somehow got to smash it down into something suitable for a patch release.

Tests passed locally on my machine, so hopefully we're all good here.