-
Notifications
You must be signed in to change notification settings - Fork 66
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
Parameter expressions #139
Conversation
Codecov Report
@@ Coverage Diff @@
## master #139 +/- ##
=======================================
Coverage 99.29% 99.30%
=======================================
Files 7 7
Lines 284 287 +3
=======================================
+ Hits 282 285 +3
Misses 2 2
Continue to review full report at Codecov.
|
@@ -227,37 +250,196 @@ def test_parameter_was_not_bound(self, recorder): | |||
with recorder: | |||
quantum_circuit(params={}) | |||
|
|||
def test_invalid_parameter_expression(self, recorder): |
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.
If you're wondering, I also moved some tests around to try and organize the file better.
tests/test_converter.py
Outdated
@@ -1063,3 +1075,38 @@ def circuit(params): | |||
] | |||
|
|||
assert np.allclose(res, expected, **tol) | |||
|
|||
def test_parameterexpression(self): |
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.
This is a new test.
@@ -144,6 +144,29 @@ def test_quantum_circuit_with_gate_requiring_multiple_parameters(self, recorder) | |||
assert recorder.queue[0].parameters == [0.5, 0.3, 0.1] | |||
assert recorder.queue[0].wires == Wires([0]) | |||
|
|||
def test_longer_parameter_expression(self): |
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.
this is a new test.
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.
Overall looks good! 🎉 This is a nice improvement to have. Had some comments, would just be curious about the performance changes of using lambdify
(if any at all).
@@ -1,5 +1,6 @@ | |||
qiskit>=0.25 | |||
pennylane>=0.15 | |||
numpy | |||
sympy |
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.
Usually, introducing a new dependency would be good to be considered, but in this case, Qiskit already depends on sympy
. Worth noting that we can do this "without a cost" maybe in the PR description.
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.
do you mean in the changelog, or just in the PR?
if p.parameters: # non-empty set = has unbound parameters | ||
ordered_params = tuple(p.parameters) | ||
|
||
f = lambdify(ordered_params, p._symbol_expr, modules=qml.numpy) |
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 wonder if this has any performance implications. 🤔 In Strawberry Fields, there was a case where using lambdify
turned out to not be the best choice:
XanaduAI/strawberryfields#579
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.
Acoording to the sympy docs, lambdify
is more efficient than evalf
. Also, it allows us to use qml.numpy
as the namespace for conversion. Using our wrapped numpy allows the expression to be differentiable.
I don't think this conversion is ever going to be very much of a bother compared to other bottlenecks. Qiskit does not seem very supportive of users creating elaborate expressions.
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.
Could we keep the condition as before and use the solution with lambdify only if len(p.parameters) > 1
? That way we'll make use of it in fewer use cases.
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.
What is the expression is something like ParameterExpression(2*a)
. Then there's only one parameter, a
, but we can't just use a
's value in place of the expression. This is a case that would have fallen through the cracks before. If there's a viable alternative that's faster, I'm all for it, but I think this is the only way to support necessary functionality.
Co-authored-by: antalszava <[email protected]>
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.
Looks good to me 👍 Had a minor question on limiting the use of lambdify
, but that would probably be a minor change to add if accepted.
Closes Issue #129
This improvement allows the plugin to load circuits with more general parameter expressions.
For example,
Currently, the above code would raise an error.
I also add
sympy
to the requirements, as it is explicitly used. Qiskit already has sympy as a dependency, so this does not add any cost to the requirements.