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

draft: Fix how wire mapping is handled by the QuantumComputer device #147

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
62 changes: 28 additions & 34 deletions pennylane_rigetti/qc.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,14 @@
from abc import ABC, abstractmethod
from typing import Dict
from collections import OrderedDict
from pennylane.utils import Iterable

from pyquil import Program
from pyquil.api import QAMExecutionResult, QuantumComputer, QuantumExecutable
from pyquil.gates import RESET, MEASURE
from pyquil.quil import Pragma

from pennylane import DeviceError, numpy as np

Check notice on line 32 in pennylane_rigetti/qc.py

View check run for this annotation

codefactor.io / CodeFactor

pennylane_rigetti/qc.py#L32

Imports from package pennylane are not grouped (ungrouped-imports)
from pennylane.wires import Wires

from .device import RigettiDevice
Expand All @@ -45,11 +46,9 @@
device (str): the name of the device to initialise.
shots (int): number of circuit evaluations/random samples used
to estimate expectation values of observables.
wires (Iterable[Number, str]): Iterable that contains unique labels for the
qubits as numbers or strings (i.e., ``['q1', ..., 'qN']``).
The number of labels must match the number of qubits accessible on the backend.
If not provided, qubits are addressed as consecutive integers ``[0, 1, ...]``, and their number
is inferred from the backend.
wires (Union[int, Iterable[Number, str]]): Number of qubits represented by the device, or an iterable that contains
MarquessV marked this conversation as resolved.
Show resolved Hide resolved
unique labels for the qubits as numbers or strings (i.e., ``['q1', ..., 'qN']``). When an integer is provided, qubits
are addressed as consecutive integers ``[0, 1, n]`` and mapped to the first available qubits on the device.
active_reset (bool): whether to actively reset qubits instead of waiting for
for qubits to decay to the ground state naturally.
Setting this to ``True`` results in a significantly faster expectation value
MarquessV marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -94,29 +93,30 @@

self.qc = self.get_qc(device, **timeout_args)

self.num_wires = len(self.qc.qubits())

if wires is None:
# infer the number of modes from the device specs
# and use consecutive integer wire labels
wires = range(self.num_wires)
qubits = sorted(
[
qubit.id
for qubit in self.qc.quantum_processor.to_compiler_isa().qubits.values()
if qubit.dead is False
]
)

# Interpret wires as Pennylane would, and use the labels to map to actual qubits on the device.
if isinstance(wires, int):
raise ValueError(
f"Device has a fixed number of {self.num_wires} qubits. The wires argument can only be used "
"to specify an iterable of wire labels."
)

if self.num_wires != len(wires):
raise ValueError(
f"Device has a fixed number of {self.num_wires} qubits and "
f"cannot be created with {len(wires)} wires."
)

self.wiring = dict(enumerate(self.qc.qubits()))
pennylane_wires = Wires(list(range(wires)))
elif isinstance(wires, Iterable):
pennylane_wires = Wires(wires)
else:
raise ValueError("wires must be an integer or an iterable of numbers or strings.")

MarquessV marked this conversation as resolved.
Show resolved Hide resolved
self.wiring = {

Check notice on line 112 in pennylane_rigetti/qc.py

View check run for this annotation

codefactor.io / CodeFactor

pennylane_rigetti/qc.py#L112

Unnecessary use of a comprehension, use dict(zip(pennylane_wires.labels, qubits[:len(pennylane_wires)])) instead. (unnecessary-comprehension)
MarquessV marked this conversation as resolved.
Show resolved Hide resolved
label: qubit
for label, qubit in zip(pennylane_wires.labels, qubits[: len(pennylane_wires)])
}
self.num_wires = len(self.wiring)
self.active_reset = active_reset

super().__init__(wires, shots)
super().__init__(pennylane_wires, shots)

@abstractmethod
def get_qc(self, device, **kwargs) -> QuantumComputer:
Expand Down Expand Up @@ -170,20 +170,14 @@
return timeout_args

def define_wire_map(self, wires):
if hasattr(self, "wiring"):
device_wires = Wires(self.wiring)
else:
# if no wiring given, use consecutive wire labels
device_wires = Wires(range(self.num_wires))

device_wires = Wires(self.wiring)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
device_wires = Wires(self.wiring)
device_wires = Wires(range(self.num_wires))

The purpose of this is to map the Pennylane wires on the operations to the internal wiring. Currently, Wires(self.wiring) just returns the Pennylane wires, since converting a dictionary to a wires object uses the keys of the dictionary, so this just maps custom wire labels to custom wire labels.

I believe we want to preserve the previous behaviour mapping to consecutive integers. I think the if-else clause here may have been a relic - since self.wiring on master is just dict(enumerate(self.qc.qubits())) and self.num_wires is self.num_wires = len(self.qc.qubits()), I can't think of a circumstance where Wires(self.wiring) != Wires(range(self.num_wires)). Correct me if I'm wrong, but seems like we don't want the backend wire labels here, but rather the enumeration of the wires, since its for the operations.

Copy link
Contributor

Choose a reason for hiding this comment

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

The current implementation leads to this behaviour, which I believe is incorrect:

image

With the above suggested change I instead get:

image

Does that look like the expected program for this circuit? Or have I misunderstood the program format for gates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, definitely not what we wanted. However, after refactoring around this idea, I ran into another issue.

When updating as suggested, the correct program gets generated:

image

However, upon processing the samples, pennylane throws an out of bounds exception:

image

After digging into the implementation of expval, this appears to be because pennylane indexes into the sample array using the label of the device qubit. I would expect pennylane to index into the sample array not by using the label literally, but rather by enumerating the device wire labels and doing a reverse lookup to find it's relative position in the sample array. Is my assumption incorrect here? Is there some other way I should be providing the data? Could this be a bug in pennylane?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, okay, I misunderstood how you want the program to look. I assumed that the qubit specification for the RX operator would be the index, not the qubit number. In that case, the issue is that we are trying to use this function for two different and incompatible things.

The version I suggested in my initial comment will work for the part of PennyLane where you are encountering an issue, but will not work for generating the program for Rigetti. You've updated it to work for the Rigetti program, but not for PennyLane post processing 😅

There are two separate types of wire mapping we are interested in here. One is the conversion between the wires on the (software) device and consecutive indexing in PennyLane. The other is the conversion between the wires on the (software) device and the qubit numbers expected by the hardware backend. This function used (and is expected by the PennyLane device interface) to do the first thing, and you've updated it to do the second.

In other words, the dictionary generated by this function IS (or at least should be) the reverse lookup table for mapping wire label to relative position in the sample array. It should not include the qubit numbers at all, but only the (software) device wire labels and the consecutive integers. I.e. a 3 wire device with wires=["a", "b", "c"], this function should return {"a": 0, "b": 1, "c":2}.

I don't think the plugin actually needs a custom implementation of this method, it can inherit from the PennyLane device base class. Where it does need a custom implementation is for mapping to hardware instructions, that will need to be done in a separate function, instead of overwriting this existing PennyLane function.

I'm going to make a branch of off this with a proposed solution. It seems like this same assumption was made in the parent class, i.e. that "custom wire labels" (this is what we call it whenever users don't have consecutive integers starting with 0 as their wire labels) in PennyLane don't currently translate to the Rigetti plugin.

Let's see if my suggestion fixes this, and also if it breaks other things, and then take it from there!

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure you want the operators to be specified by qubit label and not by index? That's not currently the behaviour on master from what I can see.

return OrderedDict(zip(wires, device_wires))

def apply(self, operations, **kwargs):
"""Applies the given quantum operations."""
prag = Program(Pragma("INITIAL_REWIRING", ['"PARTIAL"']))
self.prog = Program(Pragma("INITIAL_REWIRING", ['"PARTIAL"']))
if self.active_reset:
prag += RESET()
self.prog = prag + self.prog
self.prog += RESET()

if self.parametric_compilation:
self.prog += self.apply_parametric_operations(operations)
Expand All @@ -193,7 +187,7 @@
rotations = kwargs.get("rotations", [])
self.prog += self.apply_rotations(rotations)

qubits = sorted(self.wiring.values())
qubits = self.wiring.values()
MarquessV marked this conversation as resolved.
Show resolved Hide resolved
ro = self.prog.declare("ro", "BIT", len(qubits))
for i, q in enumerate(qubits):
self.prog.inst(MEASURE(q, ro[i]))
Expand Down
Loading