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.

Sign up for GitHub

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

[RFC] Improve handling of nested control structures #602

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

[RFC] Improve handling of nested control structures #602

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
abb4e21
Add general control support to lightning.qubit
dime10 Jan 25, 2024
49e3a28
Auto update version
github-actions[bot] Jan 26, 2024
b8d91c4
trigger ci
vincentmr Jan 26, 2024
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: 1 addition & 1 deletion pennylane_lightning/core/_version.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,4 @@
Version number (major.minor.patch[-label])
"""

__version__ = "0.35.0-dev7"
__version__ = "0.35.0-dev8"
66 changes: 36 additions & 30 deletions pennylane_lightning/lightning_qubit/lightning_qubit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,27 +117,6 @@
"CRX",
"CRY",
"CRZ",
"C(PauliX)",
Copy link
Contributor

Choose a reason for hiding this comment

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

(This is more a question, I am not asking for immediate changes in this PR)

The set of supported gates (in Lighthing at least) is defined here and in C++ (and I would say that the complete spec would also include threading model and the number of qubits). Is the Python list below - a duplicate of these C++ enums?
Could we use the device API to read the supported gates somehow? Maybe, we can autogenerate Python sources from the C++ ones if we want to have this information in compile time without running C funcitons?

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is we do not have a tight and robust mechanism to communicate natively supported gates. The enums are just that, enums. So they are not lists of the supported gates, just gates that might be supported on some device. What dictates which gates are decomposed are the allowed_operations dictionaries, but these are manually maintained and might not include gates that are natively supported (usually goes uncaught), and vice versa (usually fails some test). Some gates like OrbitalRotation are listed in allowed_operations but are not natively supported and do not crash any test. This is because gates that do not have a specialized implementation are applied via matrices. In the case of OrbitalRotation, this is fine because the number of wire is fixed at 4, a reasonable number. For other gates however, like QFT, this can cause issues since the number of wire is unlimited, possibly requiring the generation of a large dense matrix at the Python layer (filling out the memory).

"C(PauliY)",
"C(PauliZ)",
"C(Hadamard)",
"C(S)",
"C(T)",
"C(PhaseShift)",
"C(RX)",
"C(RY)",
"C(RZ)",
"C(SWAP)",
"C(IsingXX)",
"C(IsingXY)",
"C(IsingYY)",
"C(IsingZZ)",
"C(SingleExcitation)",
"C(SingleExcitationMinus)",
"C(SingleExcitationPlus)",
"C(DoubleExcitation)",
"C(DoubleExcitationMinus)",
"C(DoubleExcitationPlus)",
"CRot",
"IsingXX",
"IsingYY",
Expand Down Expand Up @@ -367,20 +346,36 @@
Returns:
array[complex]: the output state tensor
"""
basename = "PauliX" if operation.name == "MultiControlledX" else operation.base.name
if basename == "Identity":
return
method = getattr(sim, f"{basename}", None)
control_wires = self.wires.indices(operation.control_wires)
control_values = (
[bool(int(i)) for i in operation.hyperparameters["control_values"]]
if operation.name == "MultiControlledX"
else operation.control_values
)

if operation.name == "MultiControlledX":
basename = "PauliX"
control_values = [bool(int(i)) for i in operation.hyperparameters["control_values"]]

Check warning on line 353 in pennylane_lightning/lightning_qubit/lightning_qubit.py

View check run for this annotation

Codecov / codecov/patch

pennylane_lightning/lightning_qubit/lightning_qubit.py#L352-L353 

Added lines #L352 - L353 were not covered by tests
target_wires = list(set(self.wires.indices(operation.wires)) - set(control_wires))
Comment on lines 351 to 354
Copy link
Contributor

Choose a reason for hiding this comment

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

This special case is not necessary anymore. The MultiControlledX now inherits from Controlled, you can access its base like operation.base, its control values like operation.control_values just like you would with any other operator.

else:
basename = operation.base.name
control_values = operation.control_values

Check warning on line 357 in pennylane_lightning/lightning_qubit/lightning_qubit.py

View check run for this annotation

Codecov / codecov/patch

pennylane_lightning/lightning_qubit/lightning_qubit.py#L356-L357 

Added lines #L356 - L357 were not covered by tests
target_wires = self.wires.indices(operation.target_wires)

if basename == "Identity":

Check notice on line 360 in pennylane_lightning/lightning_qubit/lightning_qubit.py

View check run for this annotation

codefactor.io / CodeFactor

pennylane_lightning/lightning_qubit/lightning_qubit.py#L360 

Unnecessary "elif" after "return", remove the leading "el" from "elif" (no-else-return)
return

Check warning on line 361 in pennylane_lightning/lightning_qubit/lightning_qubit.py

View check run for this annotation

Codecov / codecov/patch

pennylane_lightning/lightning_qubit/lightning_qubit.py#L360-L361 

Added lines #L360 - L361 were not covered by tests
# This special handling is required because PennyLane doesn't canonicalize
# C(CNOT/Toffoli) in qml.simplify or qml.ctrl/Controlled, and we now allow `C(...)`
# instances through for any supported base gate. However, the C++ simulator `method`
# for CNOT/Toffoli does not accept additional control wires.
elif basename == "CNOT":
basename = "PauliX"
control_values += [True]
control_wires += operation.base.wires[0]
target_wires = [operation.base.wires[1]]
elif basename == "Toffoli":
basename = "PauliX"
control_values += [True, True]
control_wires += operation.base.wires[0:2]
target_wires = [operation.base.wires[2]]

Check warning on line 375 in pennylane_lightning/lightning_qubit/lightning_qubit.py

View check run for this annotation

Codecov / codecov/patch

pennylane_lightning/lightning_qubit/lightning_qubit.py#L366-L375 

Added lines #L366 - L375 were not covered by tests
Comment on lines +366 to +375
Copy link
Contributor

@astralcai astralcai Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I believe after my PR, in the current master of pennylane, if you call operation.simplify(), it would flatten any nested controlled structures to a multi-controlled structure. You can test it out and let me know if anything doesn't work. This would allow you to get rid of these special treatment.

Copy link
Contributor

Choose a reason for hiding this comment

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

>>> import pennylane as qml
>>> from pennylane.ops import Controlled
>>> op = Controlled(qml.CNOT([0,1],2))
>>> op.simplify()
Toffoli(wires=[2, 0, 1])


method = getattr(sim, f"{basename}", None)

Check warning on line 377 in pennylane_lightning/lightning_qubit/lightning_qubit.py

View check run for this annotation

Codecov / codecov/patch

pennylane_lightning/lightning_qubit/lightning_qubit.py#L377 

Added line #L377 was not covered by tests

if method is not None: # apply n-controlled specialized gate
inv = False
param = operation.parameters
Expand All @@ -402,6 +397,15 @@
operation.base.matrix, control_wires, control_values, target_wires, False
)

def supports_operation(self, operation):
"""Overwrite base class implementation to allow arbitrarily nested Controlled instances
to be applied by Lightning."""

while operation[0:2] == "C(":
operation = operation[2:-1]

Check warning on line 405 in pennylane_lightning/lightning_qubit/lightning_qubit.py

View check run for this annotation

Codecov / codecov/patch

pennylane_lightning/lightning_qubit/lightning_qubit.py#L404-L405 

Added lines #L404 - L405 were not covered by tests

return super().supports_operation(operation)

Check warning on line 407 in pennylane_lightning/lightning_qubit/lightning_qubit.py

View check run for this annotation

Codecov / codecov/patch

pennylane_lightning/lightning_qubit/lightning_qubit.py#L407 

Added line #L407 was not covered by tests
Copy link
Contributor

@sergei-mironov sergei-mironov Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@dime10 could you please clarify how e.g. C(CX) gate would be handled? From my understanding, Lighting does support CX but does not natively support the controlled version of this gate.

UPD: I understand that PL would decompose gates like C(CX), but can we rely on this fact here, in the PL-lightning?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems PennyLane will spit out C(CX) as a Toffoli, if it has a single control, and MultiControlledX otherwise. This is handled by the Python module and the C++ layer. The latter treats CNOT and Toffoli gates separately, but all MultiControlledX are understood as C(PauliX), which are natively supported.

Copy link
Contributor

Choose a reason for hiding this comment

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

A slight caveat here: a qml.ctrl(op, wire) where op is a CNOT will spit out Toffoli, but if the operation is instantiated using the constructor of Controlled like Controlled(op, wire), then it doesn't magically becomes a Toffoli, in which case you will need to call op.simplify() to flatten the nested control structure.


def apply_lightning(self, state, operations):
"""Apply a list of operations to the state tensor.

Expand All @@ -420,8 +424,10 @@
# Skip over identity operations instead of performing
# matrix multiplication with it.
for operation in operations:
if operation.name == "Identity":

Check notice on line 427 in pennylane_lightning/lightning_qubit/lightning_qubit.py

View check run for this annotation

codefactor.io / CodeFactor

pennylane_lightning/lightning_qubit/lightning_qubit.py#L427 

Unnecessary "elif" after "continue", remove the leading "el" from "elif" (no-else-continue)
continue
elif operation.name[0:2] == "C(":
operation = qml.simplify(operation)

Check warning on line 430 in pennylane_lightning/lightning_qubit/lightning_qubit.py

View check run for this annotation

Codecov / codecov/patch

pennylane_lightning/lightning_qubit/lightning_qubit.py#L429-L430 

Added lines #L429 - L430 were not covered by tests
method = getattr(sim, operation.name, None)
wires = self.wires.indices(operation.wires)

Expand Down
Loading