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

Calibrations involving bound parameters fail to roundtrip through QPY #9764

Closed
jakelishman opened this issue Mar 9, 2023 · 9 comments · Fixed by #10183
Closed

Calibrations involving bound parameters fail to roundtrip through QPY #9764

jakelishman opened this issue Mar 9, 2023 · 9 comments · Fixed by #10183
Labels
bug Something isn't working mod: qpy Related to QPY serialization

Comments

@jakelishman
Copy link
Member

Environment

  • Qiskit Terra version: 0.23.2
  • Python version: 3.10
  • Operating system: macOS

What is happening?

Calibrations that are defined for gates whose parameters are precisely of type float seem to roundtrip through QPY just fine. However, if the calibration is made to a gate whose parameters are fully bound ParameterExpressions, then a QPY-roundtripped circuit doesn't seem to recognise the calibration any more.

How can we reproduce the issue?

import io
from qiskit import QuantumCircuit, transpile, qpy
from qiskit.circuit import ParameterVector
from qiskit.transpiler import PassManager
from qiskit.transpiler.passes import RZXCalibrationBuilder
from qiskit.providers.fake_provider import FakePerth

backend = FakePerth()
inst_sched_map = backend.instruction_schedule_map
channel_map = backend.channels_map

param_bind = [-1.7131982974314244]

qc1 = QuantumCircuit(2)
qc1.rzx(param_bind[0], 0, 1)
qc1.draw()

qc2 = QuantumCircuit(2)
thetas = ParameterVector('θ', 1)
qc2.rzx(thetas[0], 0, 1)

pm = PassManager(RZXCalibrationBuilder(inst_sched_map, channel_map))
qc1_t = transpile(pm.run(qc1), backend)
qc2_t = transpile(pm.run(qc2.assign_parameters(param_bind)), backend)

def roundtrip(qc):
    with io.BytesIO() as fptr:
        qpy.dump(qc, fptr)
        fptr.seek(0)
        return qpy.load(fptr)[0]

def check_calibration(qc):
    return qc.has_calibration_for(qc.data[0])

[check_calibration(x) for x in [qc1_t, qc2_t, roundtrip(qc1_t), roundtrip(qc2_t)]]

gives

[True, True, True, False]

Notably, roundtrip(qc2_t) still does appear to have the requisite calibration, it's just not quite recognised correctly:

>>> roundtrip(qc2_t).calibrations
{'rzx': {((0, 1), (-1.7131982974314244,)): ScheduleBlock(...)}}

What should happen?

The roundtripped circuit should still recognise that it has a calibration for that instruction.

Any suggestions?

I think what's happening is that the floating-point number in the ParameterExpression is changing by 1 ULP, which sounds suspiciously like there might be a dodgy float-to-str conversion happening somewhere in the ParameterExpression handling in QPY?

I mean, the underlying cause is another of these things where fully assigned ParameterExpressions not immediately becoming regular floats is causing problems. But either way, if it happens with bound ParameterExpression, I'm sure there'll be mechanisms to trigger it with bound ones as well, we just don't notice as much if they're like 1ULP differences in a random float coefficient in a mostly symbolic calculation.

See Qiskit/qiskit-ibm-runtime#666.

cc: @kt474, @nbronn.

@jakelishman jakelishman added bug Something isn't working mod: qpy Related to QPY serialization labels Mar 9, 2023
@nkanazawa1989
Copy link
Contributor

nkanazawa1989 commented Mar 13, 2023

Investigated. This is the issue of ParameterExpression roundtrip as you mentioned, which is happening here.

https://github.com/Qiskit/qiskit-terra/blob/6829bb18cf791960896fe72b9be9611aac44155a/qiskit/qpy/binary_io/value.py#L50

repr of obj. _symbol_expr is -1.71319829743142 but float(obj. _symbol_expr) is -1.7131982974314244. Likely sympy srepr command (we still rely on sympy for serialization) takes repr of symbolic expression which likely has smaller significant digit.

My old PR could address this issue if symengine is available in your environement. Basically symengine expression can be directly serialized without making plain text, i.e. obj.__reduce__()[1][0].

(Edit) The PR above has dependency on the next release of symengine, which has not been released for a while.

@nkanazawa1989
Copy link
Contributor

Likely sympy sympify reduces the digit, not srepr. i.e.

from sympy import simplify

qc2_assigned = qc2.assign_parameters(param_bind)
expr = expr = qc2_assigned.data[0][0].params[0]

>>> float(expr._symbol_expr), float(simplify(expr._symbol_expr))
(-1.7131982974314244, -1.71319829743142)

Here simplify is used to typecast symengine Expression to sympy Expression to use srepr.

@jakelishman
Copy link
Member Author

Unfortunately looks like your PR's still on hold, since the necessary Symengine fix is merged to main, but they haven't cut a 0.10.0 release. Other than that, though, yeah, your PR looks like a much more straightforwards way to go than serialisation to string.

@nkanazawa1989
Copy link
Contributor

Thanks for your comment on my PR. As you pointed out, my PR would break portability of QPY data. We need another mechanism to serialize expression without changing significant digits, or we should promote symengine to be required. Alternatively we could use single precision floating point in circuit calibrations dictionary.

@jakelishman
Copy link
Member Author

jakelishman commented Mar 13, 2023

Python doesn't natively support single-precision floats, so I suppose what we'd really be saying is that we round the stored data in QuantumCircuit.calibrations off to a certain number of mantissa digits. That feels a bit gross to do it at that level of round-off, because there'd be cases where certain parameters being 1ULP off each other would need separate calibrations, while others that were $2^{28}$ ULP off will compare equal.

Making the calibration dict fuzzy in any way just to support deficiencies in parameter serialisation via QPY is probably not going to be very satisfying. I wonder if we could accept a performance hit in QPY where we verify on write-out that the roundtrip is exactly equal, and revert to some slower-but-safer serialisation method if it isn't? I'm not very sure what that would be, though.

@nkanazawa1989
Copy link
Contributor

nkanazawa1989 commented Mar 13, 2023

Hmm me neither. Probably we can typecast expression to float before calling sympy simplify.

from qiskit.circuit import Parameter

p = Parameter("p")
v = -1.7131982974314244

p_assigned = p.assign(p, v)

float(p_assigned) == p_assigned  # True

I think this doesn't break any reference.

(edit) but we cannot recover digits of coefficients in unbound expression. this could be an issue for QASM3 with offloaded parameter binding?

@nkanazawa1989
Copy link
Contributor

I found that it's a bug of symengine. See symengine/symengine#1901. It rounds float value with wrong digits, which may affect translation of symengine expression to sympy expression, which is necessary for serialization in QPY. Note that this issue has been already fixed in their master branch but just not released. I tried to install their master branch but it was really complicated and I gave up. I think we just need to wait for symengine 0.10.

@nkanazawa1989
Copy link
Contributor

For temp fix you can uninstall symengine from your env. This allows you to bypass the expression typecast at the price of performance.

@nkanazawa1989
Copy link
Contributor

fyi symengine/symengine.py#433

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working mod: qpy Related to QPY serialization
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants