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

Add function to apply instantiated operations to queuing contexts #1433

Merged
merged 18 commits into from
Jun 30, 2021
Merged

Conversation

josh146
Copy link
Member

@josh146 josh146 commented Jun 24, 2021

Context:

Currently, there is no consistent, user-facing approach for applying (already instantiated) operators to a queuing context. Instead, there is a combination of different approaches:

  • op.queue(): the most common approach. Almost all operators provide their own queuing logic currently, bar Tensor, which inherits a basic implementation from the base class and leads to inconsistent behaviour.

  • qml.QueuingContext.append(): appends the object directly to the currently active queuing context.

  • context._append() appends the object to a specific queuing context.

This situation is not ideal, since op.queue() is not consistent, not documented, and not user-facing.

Description of the Change:

  • Adds Tensor.queue() to make the Tensor class consistent with other Operator subclasses. This results in several accidental bug fixes that have plagued the Hamiltonian class (see Adds Hamiltonian Queueing and Transform to Compute Hamiltonian Expectation Value #1142)

  • Modifies the existing queue() methods, so that they now take an optional context keyword argument. This allows operators to be queued to specific, provided, contexts, rather than just the currently active context.

  • Adds a new function qml.apply, which will be the new user and developer facing method for queuing already instantiated operations.

Benefits:

  • There is now a consistent, documented, and well-tested method for queuing instantiated operators to queuing contexts.

Possible Drawbacks:

  • The existing qml.apply function was an import of qml.collections.apply. This function was infrequently used or advertised, and now must be accessed by importing directly from the collections module.

Related GitHub Issues: n/a

@josh146 josh146 added the WIP 🚧 Work-in-progress label Jun 24, 2021
@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.

@codecov
Copy link

codecov bot commented Jun 24, 2021

Codecov Report

Merging #1433 (0593611) into master (580172a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1433   +/-   ##
=======================================
  Coverage   98.24%   98.24%           
=======================================
  Files         160      160           
  Lines       11991    12016   +25     
=======================================
+ Hits        11780    11805   +25     
  Misses        211      211           
Impacted Files Coverage Δ
pennylane/__init__.py 98.57% <100.00%> (ø)
pennylane/measure.py 91.86% <100.00%> (+0.09%) ⬆️
pennylane/operation.py 95.92% <100.00%> (+0.07%) ⬆️
pennylane/queuing.py 97.64% <100.00%> (+0.54%) ⬆️
pennylane/vqe/vqe.py 92.19% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 580172a...0593611. Read the comment docs.

Comment on lines +813 to +814
qml.PauliZ(0),
qml.PauliZ(2),
Copy link
Member Author

Choose a reason for hiding this comment

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

There are two tests in this test function; one that tests Hamiltonian queuing for a Hamiltonian created outside the context, and one that tests Hamiltonian queuing for a Hamiltonian created inside the context.

This variable queue is the expected result for the Hamiltonian outside the context. If you scroll down, you can see that the addition of these two elements makes it consistent with the expected result for the Hamiltonian inside the context.

@josh146 josh146 requested a review from co9olguy June 24, 2021 13:26
@josh146 josh146 removed the WIP 🚧 Work-in-progress label Jun 24, 2021
@josh146 josh146 changed the title [WIP] Add function to apply instantiated operations to queuing contexts Add function to apply instantiated operations to queuing contexts Jun 24, 2021
@josh146 josh146 requested a review from glassnotes June 24, 2021 13:26
@josh146 josh146 added the review-ready 👌 PRs which are ready for review by someone from the core team. label Jun 24, 2021
Copy link
Contributor

@glassnotes glassnotes left a comment

Choose a reason for hiding this comment

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

Just took a quick look at the code for now, will provide more comments after using this for some local tests 👍

.github/CHANGELOG.md Show resolved Hide resolved
pennylane/queuing.py Outdated Show resolved Hide resolved
pennylane/queuing.py Show resolved Hide resolved
tests/test_queuing.py Show resolved Hide resolved
@josh146
Copy link
Member Author

josh146 commented Jun 29, 2021

@glassnotes @mariaschuld I have renamed the function to qml.apply() 🙂

qml.CNOT(wires=[0, 1])

>>> tape1.operations
[Hadamard(wires=[1]), <QuantumTape: wires=[0], params=1>, PauliX(wires=[0]), CNOT(wires=[0, 1])]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the <QuantumTape: wires=[0], params=1> part there? It looks like only a Hadamard, PauliX, and CNOT should be applied to tape1?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh wait I think I just understood why from the tests later on; the contents of tape2 are also getting added to tape1 because it's within the same queuing context, but using qml.apply allows us to apply something to tape1 without also applying it to tape2?

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 exactly!

qml.Hadamard(wires=1)

with qml.tape.QuantumTape() as tape2:
op1 = qml.PauliX(wires=0)
Copy link
Contributor

Choose a reason for hiding this comment

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

This behaviour actually doesn't feel so intuitive; if I create something and assign it to a variable with the intention of queuing it in a different context, I wouldn't expect it to also be queued in the context that created it. Maybe the fact that operations are queued whenever they are instantiated could just be bolded or otherwise emphasized above at the start of the Example section, and be explicit that this includes cases like this one?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will emphasize it 👍 I think the example here is very much 'advanced' --- day-to-day, I think by the far the most common use-case of this functionality is to queue an operation that was instantiated outside of any queuing context.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great suggestion @glassnotes; I've attempted to make it more clear here: 13aaea2

Copy link
Contributor

Choose a reason for hiding this comment

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

That looks awesome! 💯

pennylane/queuing.py Outdated Show resolved Hide resolved
josh146 and others added 3 commits June 30, 2021 11:31
Co-authored-by: Olivia Di Matteo <2068515+glassnotes@users.noreply.github.com>
self.obs.append(o)
else:
raise ValueError("Can only perform tensor products between observables.")
def queue(self, context=qml.QueuingContext, init=False): # pylint: disable=arguments-differ
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand the init argument?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah - the queuing logic was previous occuring at the exact same time as the Tensor datastructure was being computed.

That is, within __init__, there was a single for loop that did two things:

  • It queued each constituent observable
  • It created the internal obs list of constituent observables.

I originally split it into two separate for loops in order to separate out the logic, but realized that it would result in looping through the observable list twice. Which is fine... but as the number of qubits increases, this could be a bottleneck.

So I combined it back into a single for loop, but use init=True to ensure that building the datastructure only happens on instantiation, while queuing happens every time.

@@ -220,6 +221,9 @@ def _append(self, obj, **kwargs):
def _remove(self, obj):
self.queue.remove(obj)

append = _append
remove = _remove
Copy link
Contributor

Choose a reason for hiding this comment

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

Trying to understand this one?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, this one is weird. QueuingContext.append is a class method, so doesn't take self into account. Here, we are in a subclass, and simply overriding the inherited class methods.

pennylane/queuing.py Outdated Show resolved Hide resolved
"""Test that applying an operation without an active
context raises an error"""
with pytest.raises(RuntimeError, match="No queuing context"):
qml.apply(qml.PauliZ(0))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

op1 = qml.PauliZ(0)
op2 = qml.apply(op1)

assert tape.operations == [op1, op2]
Copy link
Contributor

Choose a reason for hiding this comment

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

also cool, makes total sense!

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.

Very nice Josh! I have to admit I don't follow all changes at the beginning, but the main function, docstring and tests look awesome!

Copy link
Contributor

@glassnotes glassnotes left a comment

Choose a reason for hiding this comment

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

@josh146 thanks for updating the documentation, helps make the examples much clearer! I've incorporated the method into my WIP transforms, and everything works seamlessly 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-ready 👌 PRs which are ready for review by someone from the core team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants