Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Assign values directly to fully bound parameters in quantum circuits #10183
Changes from 16 commits
c5ab217
80a1fe0
d6f9ee6
27681c4
84cc952
c58e094
3901e76
d4eb3ca
9d1a9ae
7552949
56a9955
715deca
03189be
92a0b80
061cc25
c37770e
f00fb40
1c18825
d2cfc14
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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 intoParameterExpression.__float__
or left here? One reason I had it here wasis_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:
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.There was a problem hiding this comment.
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 inParameterExpression.__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.There was a problem hiding this comment.
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'sfloat
andcomplex
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?).There was a problem hiding this comment.
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
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thatbound
andvalue
had different precision for the float16 case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the closest double-precision float to
float16(2.1)
is different to the closest double-precision float toreal(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 useassertEqual
to assert that there's no dodgy conversions happening.