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

Fixed bug in gradients of loaded qiskit circuits #71

Merged
merged 10 commits into from
Feb 28, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,16 @@

### Bug fixes

* Fixed a bug where gradient computations always returned 0 when
loading a parametrized Qiskit circuit as a PennyLane template.
[(#71)](https://github.com/XanaduAI/pennylane-qiskit/pull/71)

### Contributors

This release contains contributions from (in alphabetical order):

Josh Izaac

---

# Release 0.8.0
Expand Down
66 changes: 46 additions & 20 deletions pennylane_qiskit/converter.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@

import numpy as np
from qiskit import QuantumCircuit
from qiskit.circuit import Parameter
from qiskit.circuit import Parameter, ParameterExpression
from qiskit.exceptions import QiskitError

import pennylane as qml
Expand All @@ -36,17 +36,19 @@
inv_map = {v.__name__: k for k, v in QISKIT_OPERATION_MAP.items()}


def _check_parameter_bound(param):
def _check_parameter_bound(param, var_ref_map):
"""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]):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]): ...
    """
    ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I will add that in!

a dictionary mapping qiskit parameters to PennyLane variables

Returns:
param: the parameter after the check
qiskit.circuit.Parameter: the parameter after the check
"""
if isinstance(param, Parameter):
if isinstance(param, Parameter) and param not in var_ref_map:
raise ValueError("The parameter {} was not bound correctly.".format(param))
return param

Expand All @@ -59,19 +61,33 @@ def _check_circuit_and_bind_parameters(quantum_circuit: QuantumCircuit, params:
params (dict): dictionary of the parameters in the circuit

Returns:
qc (QuantumCircuit): quantum circuit with bound parameters
QuantumCircuit: quantum circuit with bound parameters
dict[qiskit.circuit.Parameter, pennylane.variable.VariableRef]: a dictionary mapping
qiskit parameters to PennyLane variables

"""
if not isinstance(quantum_circuit, QuantumCircuit):
raise ValueError("The circuit {} is not a valid Qiskit QuantumCircuit.".format(quantum_circuit))

if params is None:
return quantum_circuit
return quantum_circuit, {}

# map qiskit parameters to PennyLane differentiable VariableRefs.
var_ref_map = {}

for k, v in params.items():
if isinstance(v, qml.variable.Variable):
params.update({k: v.val})
# iterate through the parameters
if isinstance(v, qml.variable.VariableRef):
# If a parameter is a Variable reference, then it represents
# a differentiable parameter.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

var_ref_map[k] = v

for k in var_ref_map:
# Since we cannot bind VariableRefs to Qiskit circuits,
# we must remove them from the binding dictionary before binding.
del params[k]

return quantum_circuit.bind_parameters(params)
return quantum_circuit.bind_parameters(params), var_ref_map
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!



def map_wires(wires: list, qc_wires: list) -> dict:
Expand All @@ -83,7 +99,7 @@ def map_wires(wires: list, qc_wires: list) -> dict:
qc_wires (list): wires from the converted quantum circuit

Returns:
wire_map (dict): map from quantum circuit wires to the user defined wires
dict: map from quantum circuit wires to the user defined wires
josh146 marked this conversation as resolved.
Show resolved Hide resolved
"""
if wires is None:
return dict(zip(qc_wires, range(len(qc_wires))))
Expand All @@ -105,17 +121,12 @@ def execute_supported_operation(operation_name: str, parameters: list, wires: li
"""
operation = getattr(pennylane_ops, operation_name)

parameters = [_check_parameter_bound(param) for param in parameters]

if not parameters:
operation(wires=wires)
elif operation_name == 'QubitStateVector':
operation(np.array(parameters), wires=wires)
elif operation_name == 'QubitUnitary':
operation(*parameters, wires=wires)
else:
float_params = [float(param) for param in parameters]
operation(*float_params, wires=wires)
operation(*parameters, wires=wires)


def load(quantum_circuit: QuantumCircuit):
Expand Down Expand Up @@ -144,8 +155,7 @@ def _function(params: dict = None, wires: list = None):
Returns:
function: the new PennyLane template
"""

qc = _check_circuit_and_bind_parameters(quantum_circuit, params)
qc, var_ref_map = _check_circuit_and_bind_parameters(quantum_circuit, params)

# Wires from a qiskit circuit are unique w.r.t. a register name and a qubit index
qc_wires = [(q.register.name, q.index) for q in quantum_circuit.qubits]
Expand All @@ -160,8 +170,24 @@ def _function(params: dict = None, wires: list = None):
operation_wires = [wire_map[(qubit.register.name, qubit.index)] for qubit in op[1]]

if instruction_name in inv_map and inv_map[instruction_name] in pennylane_ops.ops:
# Extract the bound parameters from the operation. If the bound parameters are a
# Qiskit ParameterExpression, then replace it with the corresponding PennyLane
# variable from the var_ref_map dictionary.

parameters = []
for p in op[0].params:

_check_parameter_bound(p, var_ref_map)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if isinstance(p, ParameterExpression):
if p.parameters:
parameters.append(var_ref_map.get(min(p.parameters)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else:
parameters.append(float(p))
else:
parameters.append(p)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


execute_supported_operation(inv_map[instruction_name], op[0].params, operation_wires)
execute_supported_operation(inv_map[instruction_name], parameters, operation_wires)

elif instruction_name == 'SdgGate':

Expand Down
2 changes: 1 addition & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
qiskit>=0.15
PennyLane>=0.7.0
git+git://github.com/XanaduAI/pennylane.git#egg=pennylane

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why this is needed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

numpy
networkx==2.3
41 changes: 40 additions & 1 deletion tests/test_converter.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@
from pennylane_qiskit.converter import load, load_qasm, load_qasm_from_file, map_wires


THETA = np.linspace(0.11, 3, 5)
PHI = np.linspace(0.32, 3, 5)
VARPHI = np.linspace(0.02, 3, 5)


class TestConverter:
"""Tests the converter function that allows converting QuantumCircuit objects
to Pennylane templates."""
Expand Down Expand Up @@ -551,7 +556,7 @@ def test_quantum_circuit_error_by_calling_wrong_parameters(self, recorder):

quantum_circuit = load(qc)

with pytest.raises(ValueError, match="could not convert string to float: '{}'".format(angle)):
with pytest.raises(TypeError, match="parameter expected, got <class 'str'>"):
with recorder:
quantum_circuit()

Expand Down Expand Up @@ -946,3 +951,37 @@ def circuit_native_pennylane():
return qml.expval(qml.PauliZ(0))

assert circuit_loaded_qiskit_circuit() == circuit_native_pennylane()

@pytest.mark.parametrize("analytic", [True])
@pytest.mark.parametrize("theta,phi,varphi", list(zip(THETA, PHI, VARPHI)))
def test_gradient(self, theta, phi, varphi, tol):
josh146 marked this conversation as resolved.
Show resolved Hide resolved
"""Test that the gradient works correctly"""
qc = QuantumCircuit(3)
qiskit_params = [Parameter("param_{}".format(i)) for i in range(3)]

qc.rx(qiskit_params[0], 0)
qc.rx(qiskit_params[1], 1)
qc.rx(qiskit_params[2], 2)
qc.cnot(0, 1)
qc.cnot(1, 2)

# convert to a PennyLane circuit
qc_pl = qml.from_qiskit(qc)

dev = qml.device("default.qubit", wires=3)
josh146 marked this conversation as resolved.
Show resolved Hide resolved

@qml.qnode(dev)
def circuit(params):
qiskit_param_mapping = dict(map(list, zip(qiskit_params, params)))
qc_pl(qiskit_param_mapping)
return qml.expval(qml.PauliX(0) @ qml.PauliY(2))

dcircuit = qml.grad(circuit, 0)
res = dcircuit([theta, phi, varphi])
expected = [
np.cos(theta) * np.sin(phi) * np.sin(varphi),
np.sin(theta) * np.cos(phi) * np.sin(varphi),
np.sin(theta) * np.sin(phi) * np.cos(varphi)
]

assert np.allclose(res, expected, **tol)