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

[WIP] Encapsulate instruction in args in "Instruction" class #7020

Closed
wants to merge 4 commits into from
Closed
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
2 changes: 2 additions & 0 deletions qiskit/circuit/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@
Clbit
AncillaRegister
AncillaQubit
Operation

Gates and Instructions
----------------------
Expand Down Expand Up @@ -212,6 +213,7 @@
random.random_circuit
"""
from .quantumcircuit import QuantumCircuit
from .operation import Operation
from .classicalregister import ClassicalRegister, Clbit
from .quantumregister import QuantumRegister, Qubit, AncillaRegister, AncillaQubit
from .gate import Gate
Expand Down
81 changes: 81 additions & 0 deletions qiskit/circuit/operation.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
# This code is part of Qiskit.
#
# (C) Copyright IBM 2021.
#
# This code is licensed under the Apache License, Version 2.0. You may
# obtain a copy of this license in the LICENSE.txt file in the root directory
# of this source tree or at http://www.apache.org/licenses/LICENSE-2.0.
#
# Any modifications or derivative works of this code must retain this
# copyright notice, and modified files need to carry a notice indicating
# that they have been altered from the originals.

"""
Operation object
"""

from collections.abc import Sequence


class Operation(Sequence):
Copy link
Member

@kdk kdk Sep 15, 2021

Choose a reason for hiding this comment

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

The terminology we've used here has varied a bit, but it's probably worth thinking over before we nail this down.

Maybe someone with more compiler experience can weigh in, but I thought canonically an Instruction was the combination of an operation and its operands (qargs and cargs), whereas with this PR we'd have an Operation as the combination of an Instruction and its operands. Is there another name we want to use here, or are we set on Operation? (We do already have an InstructionSet which is ~largely unused but very similar in structure to what's here.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for taking this on. Agree this will be much needed as the circuit model evolves going forward.

I'm thinking for performance we'll need to update the access patterns before merging this (which increases the scope of the PR quite a bit). The test execution seems to have slowed down a ton (mostly the shor's algorithm tests).
...
So I think the next step here is to update all the QuantumCircuit methods to use attribute access and see if that fixes the overhead from getitem.

This definitely increases the scope of this PR. (If the performance impact really is that substantial, would we need to deprecate index-based access or otherwise help users know to migrate their code?)

Given that the existing access pattern is entirely index-based, would starting with a NamedTuple be a reasonable first step? (Alternately, using a tuple internally so that we could still have def __getitem__(self, idx): return self._tuple[idx])

Yeah, I was under the impression that slotted attribute access was the fastest return and that's what I was optimizing for and my previous experience with named tuple was it's named attribute access was slower. However, as of py 3.8 that no longer seems to be the case (with this pr: python/cpython#10495 ). I did a quick test with this pr applied on python 3.9:

from qiskit import QuantumCircuit
from collections import namedtuple

qc = QuantumCircuit(2)
qc.h(0)

Op = namedtuple('Op', ['instruction', 'qargs', 'cargs'])

op = qc.data[1]
op_tuple = tuple(op)
op_named_tuple = Op(*op_tuple)

With indexed access:

%timeit op[0]
75.5 ns ± 0.201 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
%timeit op_tuple[0]
22.6 ns ± 0.0474 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
%timeit op_named_tuple[0]
23 ns ± 0.0631 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

with attribute access:

%timeit op.instruction
28.1 ns ± 0.11 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
%timeit op_named_tuple.instruction
26.8 ns ± 0.298 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

So I'm thinking the best choice is a named tuple despite my earlier hesitation (which was clearly wrong). I'll have to see how things behave in py3.7 and 3.6 too (although this is the last release with 3.6 support) but I doubt it will be an issue and even if it causes a regression I think it'll be an ok tradeoff (or we can wait until after 0.19 to do this and then we'll only support 3.7, 3.8, 3.9, and 3.10).

Copy link
Member Author

Choose a reason for hiding this comment

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

It just seemed like the logical choice to me since we're already using instruction to represent the opcode. The terminology here is a bit fuzzy, like I've heard instructions and operation (well really opcode) used interchangeable. If you think operation here would be too overloaded the other term I was thinking of was Statement (ie one line in assembly). But I'll defer to what others like I don't feel too strongly as long as we're not reusing a name already used (since deprecating Instruction as it's used today isn't really a viable path).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think Instruction is the most appropriate name to refer to the tuple, which is like one line of qasm. It's unfortunate it wasn't used like that since the beginning. But there are places in qiskit where it's used like that, e.g. InstructionScheduleMap.

I was thinking in this release we can deprecate Instruction as it is now, and call it something else (actually Operation is a good name for that: https://en.wikipedia.org/wiki/Quantum_operation). And then re-introduce Instruction in the next release to refer to the tuple (Operation, qubits, clbits). This would abide by our 3-month deprecation policy I think.

Copy link
Member

Choose a reason for hiding this comment

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

I also prefer Instruction for this class, and Operation for the base class currently-known-as-Instruction.

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding InstructionScheduleMap, is it possible to wrap instruction to return parameters as a dictionary Dict[str, ParameterValue]? This has been really problematic since parameters in a circuit instruction is order sensitive, and thus management of parameter in the mapper is not straightforward (because parameters in schedules are not order sensitive, i.e. this is always managed by dict). It seems to be good chance to address this issue.
https://github.com/Qiskit/qiskit-terra/blob/66edc9d651f863be7fd195c5c9b40c5f610b3e03/qiskit/scheduler/lowering.py#L159

Copy link
Member

Choose a reason for hiding this comment

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

As a possible candidate to avoid the name collision around the re-use of Instruction, can we maybe re-use the InstructionSet class here (and then, at some point in the future, rename InstructionSet to Instruction)?

"""Class representation of a circuit operation

An operation is an instruction with it's operands.
"""

__slots__ = ("instruction", "qargs", "cargs")

def __init__(self, instruction, qargs=None, cargs=None):
"""Initialize a new instruction object

Args:
instruction (qiskit.circuit.Instruction): The instruction object for
the operation
qargs (list): A list of :class:`~qiskit.circuit.Qubit` objects that
the instruction runs on
cargs (list): A list of :class:`~qiskit.circuit.Clbit` objects that
the instruction runs on
"""
self.instruction = instruction
if qargs is None:
self.qargs = []
else:
self.qargs = qargs
if cargs is None:
self.cargs = []
else:
self.cargs = cargs

def __len__(self):
return 3

def __getitem__(self, index):
if index == 0:
return self.instruction
if index == 1:
return self.qargs
if index == 2:
return self.cargs
if isinstance(index, slice):
out_items = (self.instruction, self.qargs, self.cargs)
return out_items.__getitem__(index)
raise IndexError("Index %s is out of range" % index)

def __eq__(self, other):
if isinstance(other, tuple):
if other[0] == self.instruction and other[1] == self.qargs and other[2] == self.cargs:
return True
return False
elif isinstance(other, Operation):
if (
self.instruction == other.instruction
and self.qargs == other.qargs
and self.cargs == other.cargs
):
return True
return False
else:
return False

def __repr__(self):
return repr((self.instruction, self.qargs, self.cargs))
6 changes: 4 additions & 2 deletions qiskit/circuit/quantumcircuit.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
from qiskit.circuit.instruction import Instruction
from qiskit.circuit.gate import Gate
from qiskit.circuit.parameter import Parameter
from qiskit.circuit.operation import Operation
from qiskit.qasm.qasm import Qasm
from qiskit.qasm.exceptions import QasmError
from qiskit.circuit.exceptions import CircuitError
Expand Down Expand Up @@ -1195,8 +1196,8 @@ def _append(
self._check_cargs(cargs)

# add the instruction onto the given wires
instruction_context = instruction, qargs, cargs
self._data.append(instruction_context)
operation = Operation(instruction, qargs, cargs)
self._data.append(operation)

self._update_parameter_table(instruction)

Expand Down Expand Up @@ -1811,6 +1812,7 @@ def depth(self, filter_function: Optional[callable] = lambda x: not x[0]._direct
# Add to the stacks of the qubits and
# cbits used in the gate.
reg_ints.append(bit_indices[reg])

if (instr, qargs, cargs) in filter(filter_function, self._data):
levels.append(op_stack[reg_ints[ind]] + 1)
else:
Expand Down
5 changes: 4 additions & 1 deletion qiskit/circuit/quantumcircuitdata.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

from qiskit.circuit.exceptions import CircuitError
from qiskit.circuit.instruction import Instruction
from qiskit.circuit.operation import Operation


class QuantumCircuitData(MutableSequence):
Expand Down Expand Up @@ -54,12 +55,14 @@ def __setitem__(self, key, value):
self._circuit._check_qargs(qargs)
self._circuit._check_cargs(cargs)

self._circuit._data[key] = (instruction, qargs, cargs)
self._circuit._data[key] = Operation(instruction, qargs, cargs)

self._circuit._update_parameter_table(instruction)

def insert(self, index, value):
self._circuit._data.insert(index, None)
if isinstance(value, tuple):
self[index] = Operation(*value)
self[index] = value
mtreinish marked this conversation as resolved.
Show resolved Hide resolved

def __delitem__(self, i):
Expand Down