RandomLayers does not work with the Qiskit plugin as NumPy native integers are passed as wires instead of Python ints

antalszava commented 4 years ago

Issue description

Expected behavior:

No problem with using RandomLayers with the Qiskit plugin.

Actual behavior:

As RandomLayers uses numpy specific functions to generate wires, numpy native integers are returned as wires instead of values of type int. This results in a Qiskit error when using devices from the Qiskit plugin as per the following known bug and open issue: https://github.com/Qiskit/qiskit-terra/issues/3929

Reproduces how often:

Each time RandomLayers is used with the Qiskit plugin.

System information:

Name: PennyLane
Version: 0.9.0.dev0
Summary: PennyLane is a Python quantum machine learning library by Xanadu Inc.
Home-page: https://github.com/XanaduAI/pennylane
Author: None
Author-email: None
License: Apache License 2.0
Location: /xanadu/pennylane
Requires: numpy, scipy, networkx, autograd, toml, appdirs, semantic-version
Required-by: 
Platform info:           Linux-4.19.11-041911-generic-x86_64-with-debian-buster-sid
Python version:          3.7.6
Numpy version:           1.17.4
Scipy version:           1.3.2
Installed devices:
- default.gaussian (PennyLane-0.9.0.dev0)
- default.qubit (PennyLane-0.9.0.dev0)
- default.tensor (PennyLane-0.9.0.dev0)
- default.tensor.tf (PennyLane-0.9.0.dev0)
- strawberryfields.fock (PennyLane-SF-0.8.0)
- strawberryfields.gaussian (PennyLane-SF-0.8.0)
- qiskit.aer (PennyLane-qiskit-0.8.0)
- qiskit.basicaer (PennyLane-qiskit-0.8.0)
- qiskit.ibmq (PennyLane-qiskit-0.8.0)
- forest.numpy_wavefunction (PennyLane-Forest-0.8.0)
- forest.qpu (PennyLane-Forest-0.8.0)
- forest.qvm (PennyLane-Forest-0.8.0)
- forest.wavefunction (PennyLane-Forest-0.8.0)
- projectq.classical (PennyLane-PQ-0.7.0.dev0)
- projectq.ibm (PennyLane-PQ-0.7.0.dev0)
- projectq.simulator (PennyLane-PQ-0.7.0.dev0)
- microsoft.QuantumSimulator (PennyLane-qsharp-0.6.0)
- cirq.simulator (PennyLane-Cirq-0.8.0)

Source code and tracebacks

Code to reproduce this problem:

import pennylane as qml
from pennylane.templates.layers import RandomLayers
from pennylane.init import random_layers_uniform
import numpy as np

num_qubits = 5

dev = qml.device('qiskit.aer',wires=num_qubits)
conv_params = random_layers_uniform(n_layers=3, n_wires=num_qubits, seed=2)

@qml.qnode(dev)
def circuit(weights=None):
    RandomLayers(weights=weights, wires=list(range(num_qubits)))
    return qml.expval(qml.PauliZ(0))

circuit(weights=conv_params)

Traceback:

~/anaconda3/lib/python3.7/site-packages/qiskit/circuit/register.py in __getitem__(self, key)
     88         """
     89         if not isinstance(key, (int, slice, list)):
---> 90             raise CircuitError("expected integer or slice index into register")
     91         if isinstance(key, slice):
     92             return self._bits[key]
CircuitError: 'expected integer or slice index into register'

Note: a quick fix is available on the wires_to_int branch.

Additional information

Things to consider:

this particular case might be solved in Qiskit with a next release;
further templates might include similar logic for wires.

Fruit for thought in the long run: it could be a good idea in general, to place a sort of "wire type" modifying logic in PennyLane, such that "from a given point" in the line of execution, it would be guaranteed, that all wires are of type int under the hood.

This would allow:

validation for wire types and a possibly needed conversion of wires to take place only at one place in the entire codebase
any operation (such as RandomLayers) to not need to contain logic on conversion, checking validity, etc.

co9olguy commented 4 years ago

I think we should be able to fix this issue, #456, and #423 at the same time. I'm thinking a single bookkeeping Wires or WireMap class (name provisional) which is responsible for all this, and which is called by all other parts of the software as needed

johannesjmeyer commented 4 years ago

Maybe this should be an ADR though. I think it should be done at the level of the device, i.e. like

dev = qml.device("qiskit.aer", wires=[3,4,5,6,7])

We can just overload the wires argument in the following way

if isinstance(wires, Sequence):
    wires = list(wires)
elsif isinstance(wires, numbers.Integral):
    wires = range(wires)

num_wires = len(wires)

I don't even think we have to change so much in the rest of the code. Only the device itself will have some kind of WireMap that maps the wires given to the physical qubits that are available.

josh146 commented 4 years ago

dev = qml.device("qiskit.aer", wires=[3,4,5,6,7])

I agree! This is what I have been envisioning as well. Simpler for the user (in that they don't need to learn a new object/new methods), and fits the existing paradigm really well.

josh146 commented 4 years ago

@antalszava, Qiskit version 0.16.2 was released on March 21st. Is this bug now closed?

antalszava commented 4 years ago

Yes :crying_cat_face:

Qiskit versions (pip install qiskit):

qiskit==0.16.2
qiskit-aer==0.4.1
qiskit-aqua==0.6.5
qiskit-ibmq-provider==0.5.0
qiskit-ignis==0.2.0
qiskit-terra==0.12.0

After that, had to pip install python-constraint (due an error from qiskit-terra) and then received the original error.

antalszava commented 4 years ago

Still relevant with

qiskit==0.18.0
qiskit-aer==0.5.0
qiskit-aqua==0.6.5
qiskit-ibmq-provider==0.6.0
qiskit-ignis==0.3.0
qiskit-terra==0.13.0

PennyLaneAI / pennylane