Open 1ucian0 opened 5 years ago
Is the question here about the comparison of a bound parameter to a float, i.e.
ParameterExpression(0.5) == 0.5
or comparing two unbound parameter expressions
0.5 * x == x / 2
?
If it's the former, I think this would be addressed by #6812.
Need clarification on this discussion. Is it already addressed in #6812 as mentioned by @Cryoris. if not , more information is required.
To go back to the title of this PR: if it does exist I think we have to support
0.5 * x == x / 2
otherwise it would be better to raise an error saying we cannot compare parameterized circuits.
I haven't really looked too deeply at ParameterExpression
in Terra, but if it's anything like a typical expression tree, I would think that they should only be equal if they've got the same structure and nodes. The reason being that the semantics of operators like *
and /
aren't defined by the expression, but rather by a specific visitor / resolver.
If that's the case here as well, we should instead define this comparison external to ParameterExpression
.
Maybe it depends on what we want to check here. An expression-tree type check would possibly put more focus on how the parameters are constructed. On the other hand, if you want to check whether circuits are the same I would think we mainly care about whether they perform the same operation. I might construct my circuit as circuit.rx(theta / 2, 0)
or circuit(0.5 * theta, 0)
but I would think that's the same circuit 😄
We could also think of having an equals
(expression-tree check) and equiv
(same operation) like quantum info has for some objects.
Whether two ParameterExpression
s have the same structure and nodes is somewhat up to Sympy; it's not necessarily the same type of expression tree that you'd expect in a classical programming language, because it assumes the general mathematical rules of manipulation with infinite-precision complex numbers, not with floating point, and performs simple transformations into a canonical form on expression creation. a + b - c
and a - c + b
are represented by the same tree in Sympy, but that would be an invalid floating-point transformation without fast-math
or equivalent.
ParameterExpression
doesn't manipulate the expression tree itself, it delegates to Sympy/Symengine, so we're using their semantics.
Julien, about the 0.5 * x == x / 2
thing: we don't have to support that, not because of the *
and /
thing, but because of the floating-point 0.5
. In this particular case, 0.5
exactly represents 1 / 2
, but there's no way to tell if that's exactly what the programmer intended. You can see that Sympy considers *
and /
as dual operations when it can be sure it's dealing with perfect precision, because a * (3 / 4) == a / (4 / 3)
, without any simplifying operations. It's one more example of Sympy doing expression-tree manipulations that we don't specifically ask it to, but that's exactly why we use it - your applications want to be able to do maths on the parameters as if they're real values, not floating-point.
At the moment, ParameterExpression.__eq__
violates the Python data model, because it uses simplify
, which mutates the expression tree, and consequently changes the hash of the objects compared. The current __eq__
should probably become the equiv
that Julien's talking about, and __eq__
should become the expression-tree comparison that Kevin's talking about (albeit with Sympy's canonicalisation). I imagine that's going to cause some issues downstream, but really the current behaviour isn't correct.
When a parameter is bound (
bqc = qc.bind_parameters({theta: 0.5})
) the parameter is not the value, thereforebqc.data[0][0].params[0] != 0.5
(see the change here).I suggested this method for
ParameterExpression
:However, I reverted the change after talking with @kdk. He raised a valid concern about "'objects that are
__eq__
must have the same__hash__
' issues we had with DAGNode".I think the intuition (and the reason for so many casts in https://github.com/Qiskit/qiskit-terra/blob/master/test/python/circuit/test_parameters.py) should be address. But I'm not sure how...