Fixed bug in gradients of loaded qiskit circuits #71

josh146 · 2020-02-24T15:02:31Z

Context: By expliciting calling params.val, the VariableRef classes, which allow us to keep track of which operations are using which parameters so that the parameter shift rule can be applied, are being lost in the conversion.

Description of the changes:

The binding logic in the conversion functions has been modified --- now, a mapping is kept of the differentiable Qiskit parameters to the PennyLane variables. The PennyLane variables are then used for operation execution where needed.
An integration test has been added to ensure that the gradient computation works correctly on converted qiskit circuits.

codecov · 2020-02-24T15:03:20Z

Codecov Report

Merging #71 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #71      +/-   ##
==========================================
+ Coverage   99.08%   99.11%   +0.02%     
==========================================
  Files           7        7              
  Lines         327      338      +11     
==========================================
+ Hits          324      335      +11     
  Misses          3        3

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 803305a...2ce3a9d. Read the comment docs.

pennylane_qiskit/converter.py

antalszava · 2020-02-24T21:59:40Z

pennylane_qiskit/converter.py

+        # iterate through the parameters
+        if isinstance(v, qml.variable.VariableRef):
+            # If a parameter is a Variable reference, then it represents
+            # a differentiable parameter.


This could go as a dictionary comprehension as well (though might be too long of a one-liner):

var_ref_map = {k:v for k, v in params.items() if isinstance(v, qml.variable.VariableRef)}

Will change, I left it as a for loop in an attempt to minimize line changes 😆

antalszava · 2020-02-24T22:09:05Z

pennylane_qiskit/converter.py


-    return quantum_circuit.bind_parameters(params)
+    return quantum_circuit.bind_parameters(params), var_ref_map


It might be slightly unintuitive that here there's a tuple that is returned (but this is also connected to the original implementation which was my doing 😅 ). Given the change there seem to be four parts of logic here inside of this single function:

check_circuit

extract VariableRefs

remove VariableRefs from params

bind parameters

While in theory, this means four separate functions it might be worth having a separate function that solely extracts the VariableRefs and then later the bind_parameters function could delete the needed refs. To me, this latter separation would make this part easier to parse :)

antalszava · 2020-02-24T22:27:14Z

pennylane_qiskit/converter.py

+
+                    if isinstance(p, ParameterExpression):
+                        if p.parameters:
+                            parameters.append(var_ref_map.get(min(p.parameters)))


Not too sure about this line here, in particular min(p.parameters). How come such a call is made? Perhaps naming a certain part of this line (e.g. min_param=min(p.parameters)) and changing this line accordingly could help with providing further clues to the reader :)

Does len(p.parameters) == 1? If not, what happens to p's other parameters?

Oh, this is partly because

Python doesn't provide a way to index into a set, and

Due to a PennyLane restriction, the Qiskit ParameterExpression must always be a set containing a single parameter.

As a result, min(single_element_set) is a useful way of extracting the element. Another approach is single_element_set.pop(), but I tend not to favour pop due to its side effects.

Side note: probably out of scope for this PR, but we should create a PR that raises an error if len(p.parameters) > 1.

Oh, got ya! So a ParameterExpression contains a

Mapping of Parameter instances to the sympy.Symbol serving as their placeholder in expr.

Having seen that, I was also wondering why simply a Parameter could not do it here, but this now allows for ParameterExpression instances that contain only one Parameter as well.

Yep - the issue seems to be that even if you start off with a single Parameter, after binding Qiskit converts everything into a ParameterExpression. So Parameter objects never appear at this point in the logic.

antalszava

This is amazing @josh146, thank you very much for getting that! I've left a couple of comments mainly for improving my own understanding 🙂

tests/test_converter.py

speller26

Thanks for getting to this so quickly!

speller26 · 2020-02-25T08:13:43Z

pennylane_qiskit/converter.py

+                    _check_parameter_bound(p, var_ref_map)
+
+                    if isinstance(p, ParameterExpression):
+                        if p.parameters:
+                            parameters.append(var_ref_map.get(min(p.parameters)))
+                        else:
+                            parameters.append(float(p))
+                    else:
+                        parameters.append(p)


Don't feel particularly strongly about this, but this could be pulled into a helper method, so parameters then gets constructed as a list comprehension.

I had originally had this as a list comprehension, but changed it to a loop for readability (it became quite long due to the ternary expressions)

speller26 · 2020-02-25T08:14:15Z

requirements.txt

@@ -1,4 +1,4 @@
 qiskit>=0.15
-PennyLane>=0.7.0
+git+git://github.com/XanaduAI/pennylane.git#egg=pennylane


Curious why this is needed

A recent PR in PennyLane master renamed the Variable class to VariableRef --- I wanted to double check that this PR passes against the latest master of PennyLane. However, this renaming will likely be reverted due to side effects that occurred, so this change here is temporary and should be reverted before this PR is merged.

speller26 · 2020-02-25T08:18:59Z

pennylane_qiskit/converter.py

    """Utility function determining if a certain parameter in a QuantumCircuit has
    been bound.

    Args:
-        param: the parameter to be checked
+        param (qiskit.circuit.Parameter): the parameter to be checked
+        var_ref_map (dict[qiskit.circuit.Parameter, pennylane.variable.VariableRef]):


At this point, it might be worth considering the typing module:

from typing import Dict def _check_parameter_bound(param: ..., var_ref_map: Dict[Parameter, VariableRef]) -> Parameter: """ ... var_ref_map (Dict[Parameter, VariableRef]): ... """ ...

Good idea, I will add that in!

speller26 · 2020-02-25T08:22:19Z

pennylane_qiskit/converter.py

+                parameters = []
+                for p in op[0].params:
+
+                    _check_parameter_bound(p, var_ref_map)


Can't help but feel we can use the return value of _check_parameter_bound

I've removed the return statement on _check_parameter_bound, since it is never used

speller26 · 2020-02-25T08:33:24Z

pennylane_qiskit/converter.py

+
+                    if isinstance(p, ParameterExpression):
+                        if p.parameters:
+                            parameters.append(var_ref_map.get(min(p.parameters)))


Does len(p.parameters) == 1? If not, what happens to p's other parameters?

Co-Authored-By: antalszava <[email protected]>

…kit into bug/load-grad-fix

antalszava

Looks good to me @josh146! 😊

co9olguy · 2020-02-26T18:01:13Z

Ready to merge @josh146?

josh146 · 2020-02-26T22:16:17Z

I'm holding off, as I would like to revert VariableRef -> Variable in PennyLane master first, to avoid updating all the other plugins as well.

Fixed bug in gradients of loaded qiskit circuits

81dd32a

josh146 added the bug Something isn't working label Feb 24, 2020

josh146 requested a review from antalszava February 24, 2020 15:02

josh146 added 2 commits February 24, 2020 23:06

fix

8710bbc

remove f-string

3183a7e

antalszava reviewed Feb 24, 2020

View reviewed changes

pennylane_qiskit/converter.py Outdated Show resolved Hide resolved

antalszava reviewed Feb 24, 2020

View reviewed changes

antalszava reviewed Feb 25, 2020

View reviewed changes

tests/test_converter.py Outdated Show resolved Hide resolved

antalszava reviewed Feb 25, 2020

View reviewed changes

tests/test_converter.py Outdated Show resolved Hide resolved

antalszava mentioned this pull request Feb 25, 2020

Add gradient test PennyLaneAI/pennylane-rigetti#46

Merged

speller26 reviewed Feb 25, 2020

View reviewed changes

josh146 and others added 4 commits February 25, 2020 21:36

suggested changes

5359ce3

Apply suggestions from code review

ac64638

Co-Authored-By: antalszava <[email protected]>

add type hint

9917ea5

Merge branch 'bug/load-grad-fix' of github.com:XanaduAI/pennylane-qis…

cad1a5c

…kit into bug/load-grad-fix

josh146 requested a review from antalszava February 25, 2020 10:44

added test

2d7cac2

speller26 approved these changes Feb 25, 2020

View reviewed changes

antalszava approved these changes Feb 26, 2020

View reviewed changes

josh146 mentioned this pull request Feb 27, 2020

Temporarily rename VariableRef back to Variable PennyLaneAI/pennylane#523

Merged

josh146 added 2 commits February 27, 2020 12:04

rename variableref to variable

1bc6e5c

remove variableref reference

2ce3a9d

josh146 merged commit 016fbc1 into master Feb 28, 2020

josh146 deleted the bug/load-grad-fix branch February 28, 2020 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed bug in gradients of loaded qiskit circuits #71

Fixed bug in gradients of loaded qiskit circuits #71

josh146 commented Feb 24, 2020

codecov bot commented Feb 24, 2020 •

edited

Loading

antalszava Feb 24, 2020

josh146 Feb 25, 2020

josh146 Feb 25, 2020

antalszava Feb 24, 2020

josh146 Feb 25, 2020

antalszava Feb 24, 2020

speller26 Feb 25, 2020

josh146 Feb 25, 2020

antalszava Feb 26, 2020

josh146 Feb 26, 2020

antalszava left a comment

speller26 left a comment

speller26 Feb 25, 2020

josh146 Feb 25, 2020

speller26 Feb 25, 2020

josh146 Feb 25, 2020

speller26 Feb 25, 2020

josh146 Feb 25, 2020

speller26 Feb 25, 2020

josh146 Feb 25, 2020

speller26 Feb 25, 2020

antalszava left a comment

co9olguy commented Feb 26, 2020

josh146 commented Feb 26, 2020


		return quantum_circuit.bind_parameters(params)
		return quantum_circuit.bind_parameters(params), var_ref_map

Fixed bug in gradients of loaded qiskit circuits #71

Fixed bug in gradients of loaded qiskit circuits #71

Conversation

josh146 commented Feb 24, 2020

codecov bot commented Feb 24, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antalszava left a comment

Choose a reason for hiding this comment

speller26 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antalszava left a comment

Choose a reason for hiding this comment

co9olguy commented Feb 26, 2020

josh146 commented Feb 26, 2020

codecov bot commented Feb 24, 2020 •

edited

Loading