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

TypeError: ParameterExpression with unbound parameters (set()) cannot be cast to a float. #9187

Closed
mafaldaramoa opened this issue Nov 23, 2022 · 5 comments · Fixed by #10244
Labels
bug Something isn't working

Comments

@mafaldaramoa
Copy link

Environment

  • Qiskit Terra version: 0.21.0
  • Python version: 3.9.12
  • Operating system: Windows 11 Home

What is happening?

Creating and evaluating a Trotter evolution circuit for certain operators raises an error. The error message indicates that the ParameterExpression has unbound parameters, even though this is not the case.

This issue has similarities with #7507, but it's not the same. The proposed solution in there (forcing the parameter to a float in PauliEvolutionGate.validate_parameter) does not work in this case.

How can we reproduce the issue?

from qiskit.circuit import Parameter
from qiskit.opflow import PauliTrotterEvolution, Suzuki
from qiskit.opflow import X, Y

operator = (0.5j * X^X^Y^X) 
param = Parameter('θ')

ev_op = (1j*operator*param).exp_i()

exp_operator = (PauliTrotterEvolution().convert(ev_op))
circuit = exp_operator

bound_circuit = circuit.bind_parameters({param: -0.1})
bound_circuit.eval()

What should happen?

  • The error message should be accurate. As is, it doesn't allow one to identify the problem, because the set of parameters is in fact empty (as the message itself shows).
  • bound_circuit should be evaluated without errors.

Any suggestions?

The problem seems to arise in the _apply_operation method of ParameterExpression. When our (yet unbound) param is multiplied by the numeric coefficient (1j * 0.5j, simplified to 0.5 + 0j), the values of the relevant local variables inside that method are as follows:

> other_expr # Numeric coefficient (class: complex)
(-0.5+0j)
> self_expr # Coefficient of the ParameterExpression (class: symengine.lib.symengine_wrapper.Symbol)
θ
> expr
(-0.5 + 0.0*I)*θ

Later when the parameter θ is bound to -0.1, the attribute _symbol_expr associated with the ParameterExpression is -0.1*(-0.5 + 0.0*I). Casting this to a float is what gives an error (RuntimeError: Not Implemented). The error doesn't occur if _symbol_expr is -0.1*(-0.5).

Naively, doing

if numpy.isreal(other_expr):
    other_expr = numpy.real(other_expr)

in _apply_operation fixes the issue.

Alternatively, the error doesn't occur if we change the code the reproduces the problem into

operator = (0.5 * X^X^Y^X) 
param = Parameter('θ')

ev_op = (operator*param).exp_i()

i.e., if the coefficient is real to begin with. However, this seems a bit rigid, as we might want to define operators with imaginary coefficients.

@mafaldaramoa mafaldaramoa added the bug Something isn't working label Nov 23, 2022
@jakelishman
Copy link
Member

jakelishman commented Nov 23, 2022

I'm agreed that the error message from ParameterExpression could be better. For handling of complex numbers, note that in raw Python float(1 + 0j) is a TypeError as well - it's always forbidden to pass things that have complex type, even if they're representing reals, to float converters, so Symengine is being consistent with Python here.

In general, if you introduce complex numbers into your parameters as a user, you're the one responsible for handling the potentially lossy cast back to reals. In your case there's no floating-point fuzziness, but if you use your parameters more, you can easily end up in the case of having something like 1 + 1e-17j, and only the user has the context to know how small epsilons should be handled, really.

@mafaldaramoa
Copy link
Author

Hello,

Thanks for the answer.

I understand your point. However, as a user I feel like this could be handled better, and it doesn't seem very consistent with other parts of Qiskit.

  1. If the same operator is wrapped inside a PauliSumOp, no error is raised. The following executes without a problem:
operator = (0.5j *  X^X^Y^X) + (0 * I^I^I^I) 
param = Parameter('θ')

ev_op = (1j*operator*param).exp_i()

exp_operator = (PauliTrotterEvolution().convert(ev_op))
circuit = exp_operator

bound_circuit = circuit.bind_parameters({param: -0.1})
bound_circuit.eval()

Here, Qiskit has no problem discarding the null imaginary part and effectively treating the coefficient as a real number. The only difference is I added (0j * I^I^I^I) to the operator, which does nothing. It just results in the operator being a PauliSumOp, the coefficient of which is treated differently (when I see no reason why it should).

  1. Complex coefficients with null imaginary parts are converted to floats for PauliOp:
> operator = 0.5j * X^X^Y^X
> param = Parameter('θ')
> mult_op = 1j * operator * param
> mult_op = mult_op.bind_parameters({param:-0.1})
> mult_op.coeff
0.05

The coefficient is exactly the same as in the code that raises the error, yet here it was converted to a float. In this case, the user doesn't get to decide how the complex number is handled. I don't see why the same shouldn't happen for the exponentiated version.

  1. The time evolution should be unitary, so it doesn't make sense for the coefficient of operator to have a real part. Actually, if we try that, we get an error when binding the parameter value. This code
operator = (0.5j + 0.5) * X^X^Y^X
param = Parameter('θ')
ev_op = (1j*operator*param).exp_i()
circuit = PauliTrotterEvolution().convert(ev_op)
bound_circuit = circuit.bind_parameters({param: -0.1})

raises CircuitError: 'Bound parameter expression is complex in gate PauliEvolution'.

In my view, it doesn't make sense to raise an error when the imaginary part of the coefficient of the Hamiltonian is non-null (thereby recognizing that the imaginary part shouldn't exist), but expect the user to handle it when the imaginary part is zero (or very close to it). If the fact that the coefficient is complex (even with null imaginary part) will cause problems later on, then maybe the error should be raised here, as happens for coefficients with non-null imaginary parts.

  1. Qiskit does indeed decide for the user what is or isn't regarded as complex in the method bind_parameters of class CircuitOp.

E.g., this works:

operator = (0.5j + 10**-323) * (X^X^Y^X) 
param = Parameter('θ')

ev_op = (1j*operator*param).exp_i()

circuit = (PauliTrotterEvolution().convert(ev_op))
bound_circuit = circuit.bind_parameters({param: -0.1})

But if we change the first line into

operator = (0.5j + 10**-322) * (X^X^Y^X) 

we get CircuitError: 'Bound parameter expression is complex in gate PauliEvolution'

github-merge-queue bot pushed a commit to qiskit-community/qiskit-experiments that referenced this issue May 9, 2023
### Summary
This PR aims to remove the use of complex amplitude for `SymbolicPulse`
in calibration experiments. Most notable changes are to the
`HalfAngleCal` experiment and the `FixedFrquencyTransmon` library. With
these changes, support of complex values in general raises a
`PendingDeprecationWarning`.

### Details and comments
Qiskit Terra recently changed the representation of `SymbolicPulse` from
complex amplitude to (`amp`,`angle`). Once the deprecation is completed,
some calibration experiments will fail. Additionally, assignment of
complex parameters in general has caused problems recently (See
Qiskit-Terra issue
[9187](Qiskit/qiskit#9187)), and is also
being phased out (See Qiskit-Terra PR
[9735](Qiskit/qiskit#9735)).

Most calibration experiments are oblivious to these changes, with the
exception of `HalfAngleCal` and `RoughAmplitudeCal`. The library
`FixedFrequencyTransmon` also has to conform with the new
representation.

To create as little breaking changes as possible, the following were
changed:

- `FixedFrequencyTransmon` library was converted to the new
representation. All experiments will work as they have been with it.
- `RoughAmplitudeCal` was changed such that it will work for both real
or complex `amp`, without changing the type of the value.
- `HalfAngleCal` was changed to calibrate 'angle' instead of the complex
amplitude. A user which uses the `FixedFrequencyTransmon` library will
experience no change (except for the added parameters). A user which
uses custom built schedules will experience an error. To simplify the
transition, most likely scenarios (schedule with no `angle` parameter,
`cal_parameter_name="amp"`) will raise an informative error with
explanation about the needed changes.

A `PendingDeprecationWarning` is raised with every initialization of
`ParameterValue` with complex type value (which also covers addition of
parameter value to a calibration). Note that Qiskit-Terra PR
[9897](Qiskit/qiskit#9897) will also raise
a warning from the Terra side, for all assignment of complex parameters.

Handling of loaded calibrations which do not conform to the new
representation will be sorted out once PR #1120 is merged, as it
introduces a major change in calibration loading.

---------

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
Co-authored-by: Will Shanks <wshaos@posteo.net>
Co-authored-by: Daniel J. Egger <38065505+eggerdj@users.noreply.github.com>
@wshanks
Copy link
Contributor

wshanks commented Jun 1, 2023

I inadvertently fixed this in #10183. There I changed QuantumCircuit._assign_parameter to replace a fully bound ParameterExpression with its underlying data during assignment. Initially, I did this by calling int, float or complex on the parameter as appropriate, but that caused this test to fail:

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

because ParameterExpression.is_real() would evaluate to True and cause my change to call float on the parameter and trigger the error described here. Based on the test description ("Test a complex parameter expression can be real if bound correctly") and the behavior of ParameterExpression.is_real returning True for complex values with zero imaginary component, I interpreted casting complex values with zero imaginary part to float to be the desired behavior and switched my float(param) to complex(param).real following a param.is_real() check.

I did not intend to fix this and I think there are still some outstanding issues (__float__ will still raise an exception when called directly rather than handled through circuit assignment), so I don't know that this issue should be closed yet. I think it would be good to review Parameter current usage and intended usage and see if it could be made more consistent in general.

@jlapeyre
Copy link
Contributor

jlapeyre commented Jun 1, 2023

I accidentally opened a dup. It might lay things out clearly though: #10191

It may be inadvertently fixed. But it should be fixed at a lower level. Like ParameterExpression itself.

@waelitani
Copy link

The issue still persists when building a controlled-version of a parameterized gate.

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

Successfully merging a pull request may close this issue.

5 participants