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

Make tape mode default #1040

Merged
merged 43 commits into from
Jan 28, 2021
Merged

Make tape mode default #1040

merged 43 commits into from
Jan 28, 2021

Conversation

josh146
Copy link
Member

@josh146 josh146 commented Jan 26, 2021

Context:

  • Makes tape mode default when importing PL

Description of the Change:

  • qml.enable_tape is placed at the bottom of the __init__.py file

  • Test fixtures are updated to take this into account

  • Various tests are updated to take this into account

  • The device method is removed from default.tensor.tf

  • Various bug fixes

Benefits:

Possible Drawbacks: The old core remains present!

Related GitHub Issues:

@@ -230,3 +230,6 @@ def circuit():
def version():
"""Returns the PennyLane version number."""
return __version__


enable_tape()
Copy link
Member Author

Choose a reason for hiding this comment

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

This is placed at the bottom of the __init__ file, to allow the import logic to remain in non-tape mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is that necessary so that non-tape mode can still be accessible through qml.disable_tape()?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this is because a lot of the logic that is executed above this isn't tape-mode compatible yet 😆

I initially added qml.enable_tape to the top of the __init__.py file, but it requires a lot of changes throughout the code base to get it to work. I figured it would be too hard to code-review, and might introduce bugs.

Comment on lines 573 to 576
if isinstance(o, qml.tape.MeasurementProcess) and o.obs is not None:
observable_name = o.obs.name
else:
observable_name = o.name
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 bug uncovered

@github-actions
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit .github/CHANGELOG.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

Comment on lines +319 to +321
if isinstance(op, qml.tape.MeasurementProcess) and op.obs is not None:
op = op.obs

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 bug uncovered

@@ -253,7 +253,7 @@ def hash(self):
"""
return hash(self.serialize())

def to_openqasm(self, rotations=True):
def to_openqasm(self, wires=None, rotations=True):
Copy link
Member Author

Choose a reason for hiding this comment

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

This method now has the same issue re: device wires as the circuit drawer :)

@@ -40,6 +41,128 @@
_mock_stack = []


class TapeOperationRecorder(QuantumTape):
Copy link
Member Author

@josh146 josh146 Jan 26, 2021

Choose a reason for hiding this comment

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

The OperationRecorder and @template decorator were never ported to work with the new queuing refactor.

As a result, in tape mode, they no longer work. They do not error, but instead just return empty lists, as they are not accessing the new AnnotatedQueue and QueuingContext. The versions below are written to work in tape mode, and dynamically patch the existing versions when enable_tape is called.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if these classes could be lost in the __init__.py file 🤔 Could it be worth having them in a separate file?

def __repr__(self):
"""Representation of this class."""
if self.obs is None:
return "{}(wires={})".format(self.return_type.value, self.wires)
return "{}(wires={})".format(self.return_type.value, self.wires.tolist())
Copy link
Member Author

Choose a reason for hiding this comment

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

Discovered a bug :)

Comment on lines 329 to 343
if qml.tape_mode_active():
for op in operation_list:
try:
qml.tape.QueuingContext.remove(op)
except KeyError:
pass

with qml.tape.QuantumTape() as tape:
for o in operation_list:
o.queue()
if o.inverse:
o.inv()

tape.inv()
return tape
Copy link
Member Author

@josh146 josh146 Jan 26, 2021

Choose a reason for hiding this comment

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

This is too allow qml.inv() to work in tape mode. It was not previously re-written to support the new queuing system.

Here, we simply convert the list of operations into a sub/nested tape, remove the operations from the existing/parent tape, and invert the nested tape.

Copy link
Member

Choose a reason for hiding this comment

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

isn't qml.inv() more of a transform (take a circuit and return the inverted circuit), rather than something we want to do "in-place" 🤔

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, agree, however this existing function was already an in-place function.

I heavily considered just deleting this function (not sure if anyone ever uses it, I had completely forgotten it existed 😆 ) but realised I could make it work with existing logic with these few added lines.

Copy link
Member Author

Choose a reason for hiding this comment

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

But replacing this with a transform is definitely on the to-do list

Copy link
Contributor

Choose a reason for hiding this comment

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

So this is now the only operation except from decompositions that introduces nesting, right? So if I use inv together with decompositions, the resulting tape can have nesting depth 3? And that is accounted for everywhere by the tape.expand function?

@@ -174,16 +174,16 @@ def test_layers(self, parameterized_circuit, wires):
assert layers[1].ops == [ops[3]]
assert layers[1].param_inds == [3]
assert layers[2].ops == [ops[x] for x in [5, 6]]
assert layers[2].param_inds == [4, 5]
assert layers[2].param_inds == [6, 7]
Copy link
Member Author

Choose a reason for hiding this comment

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

This is because tape mode numbers parameters differently to non-tape mode.

Comment on lines +699 to +700
if "State" not in g:
qml.Displacement(a, 0, wires=[0])
Copy link
Member Author

Choose a reason for hiding this comment

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

In tape mode, state preparations are now not allowed to be placed after unitary operations.

Comment on lines -523 to -526
# Test that the gate parameter is not a PennyLane tensor, but a
# float
assert not isinstance(rec.queue[0].parameters[0], np.tensor)
assert isinstance(rec.queue[0].parameters[0], float)
Copy link
Member Author

Choose a reason for hiding this comment

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

This was a feature specifically of BaseQNode, and is now no longer the case.

def test_operation_inside_context_do_queue_false(self, qnode):
"""Test that an operation does not get added to the QNode queue when do_queue=False"""
Copy link
Member Author

Choose a reason for hiding this comment

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

do_queue has been removed from tape mode

@@ -47,7 +47,6 @@
# function arguments in various formats
mixed_list = [(0.2, 0.3), np.array([0.4, 0.2, 0.4]), 0.1]
mixed_tuple = (np.array([0.2, 0.3]), [0.4, 0.2, 0.4], 0.1)
nested_list = [[[0.2], 0.3], [0.1, [0.4]], -0.1]
Copy link
Member Author

Choose a reason for hiding this comment

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

ragged arrays as input is no longer allowed in tape mode

@@ -72,16 +72,24 @@ def __init__(self, return_type, obs=None, wires=None, eigvals=None):
# Below, we imitate an identity observable, so that the
# device undertakes no action upon recieving this observable.
self.name = "Identity"
self.diagonalizing_gates = lambda: []
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit surprised this 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.

This was just so that the TestCircuitGraphDiagonalizingGates test passes 🤔

Alternatively, I could just delete that test class?

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know the circuit graph has functionality for diagonalisation in the first place...

Copy link
Contributor

@mariaschuld mariaschuld left a comment

Choose a reason for hiding this comment

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

I think another approval would be necessary, but all changes make sense to me, and you addressed all concerns/questions I had.

The only thing one might want to consider is to delete non-tape tests. They have basically no meaning any more if we know we reproduce them in the new tape tests. And the delete will obviously not introduce bugs :)

@josh146
Copy link
Member Author

josh146 commented Jan 27, 2021

Thanks @mariaschuld!

The only thing one might want to consider is to delete non-tape tests. They have basically no meaning any more if we know we reproduce them in the new tape tests. And the delete will obviously not introduce bugs :)

I thought about this, but I figured it doesn't make sense to delete the old tests until we delete the old core. Plus, it will lead to a massive (temporary) decrease in our coverage rating.

if qml.tape_mode_active():
recorder_class = qml.tape.TapeOperationRecorder

with recorder_class() as rec:
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to understand, why is it not

     if qml.tape_mode_active():
            import pennylane as qml
            recorder_class = qml.tape.TapeOperationRecorder
     else: 
        recorder_class = OperationRecorder

Copy link
Contributor

Choose a reason for hiding this comment

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

If tape mode is active to reimport, else you use the old one?

Copy link
Member Author

@josh146 josh146 Jan 28, 2021

Choose a reason for hiding this comment

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

If PennyLane is already imported in the Python session, import pennylane as qml won't actually do anything (e.g., the __init__.py won't be re-executed).

The import pennylane as qml is just to make the qml namespace available in this file, so that we can access the tape_mode_active and TapeOperationRecorder functions/classes.

I couldn't include this import at the top of the file due to circular imports, so that's the only reason it is inside the decorator :(

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. I never understood circular imports, maybe we should solve the issue once and for all...

pennylane/_qubit_device.py Outdated Show resolved Hide resolved
if res_type_namespace == "jax":
return __import__(res_type_namespace).numpy.squeeze(res)
return __import__(res_type_namespace).squeeze(res)
return qml.math.squeeze(res)
Copy link
Contributor

Choose a reason for hiding this comment

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

🙌


@wraps(func)
def wrapper(*args, **kwargs):
with OperationRecorder() as rec:
import pennylane as qml
Copy link
Contributor

Choose a reason for hiding this comment

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

This import feels hacky and cyclical. Can't we import the needed modules at the top of this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

hahaha I tried so many combinations (including having it at the top of the file), and constantly ran into cyclical import errors. This was the only combination that didn't result in a cyclical import error.

I'm okay with this for now, since

  • we can remove it next week when we rip out the old core
  • PennyLane has already been imported by this point, so there is no overhead here, it simply makes the namespace available to the function.

for o in operation_list:
o.queue()
if o.inverse:
o.inv()
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this feels fragile. We'll definitely want to rework how to do this in a nonmutable way.

Copy link
Contributor

@antalszava antalszava left a comment

Choose a reason for hiding this comment

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

Looks good to me! 💯 🥇 🍾

@@ -173,4 +173,4 @@ For more details and examples, please see the tape documentation.
.. automodapi:: pennylane.tape
:no-main-docstr:
:include-all-objects:
:skip: enable_tape, disable_tape
:skip: QNode, qnode, enable_tape, disable_tape
Copy link
Contributor

Choose a reason for hiding this comment

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

How come that we are skipping these two? There seems to be other new objects too like TapeCircuitGraph, would we like to skip to show info about them once we deprecate the old core?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because of all the mocking done by qml.enable_tape(), qml.qnode, qml.QNode, and qml.CircuitGraph all actually point to the pennylane/tape versions 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

So I am skipping them from showing at the tape module level, as they will instead be displayed at the qml level

pennylane/circuit_graph.py Outdated Show resolved Hide resolved
tests/beta/test_default_tensor_tf.py Outdated Show resolved Hide resolved
antalszava and others added 6 commits January 28, 2021 11:38
* get hash if tape in execute

* Remove hash def for tape; it is accessible through tape.graph.hash

* hash move around

* format

* hash

* test

* update qubit_device file

* Move updating the wires for the state measurement to the vanilla circuit graph class

* format

* same hash test

* comment update
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: Chase Roberts <chase@xanadu.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants