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 object _queue_category property and use it for sorting in tapes #2408

Merged
merged 27 commits into from
Apr 27, 2022

Conversation

albi3ro
Copy link
Contributor

@albi3ro albi3ro commented Apr 1, 2022

Currently, qml.tape.QuantumTape._process_queue uses isinstance checks to sort objects into _prep, _ops, and _measurement lists.

This implementation is reliant on inheritance details and implementations of the various operators and measurements. We change inheritance implementation for operators, we break quantum tapes. This will be especially relevant coming up with Arithmetic Wrapper classes.

So instead we want to rely on polymorphism and duck typing. Objects themselves will decide how they want to be processed by the tape, instead of the tape deciding how to process the objects.

This is accomplished by adding a _queue_category property to relevant objects. The _queue_category can take on the values:

  • "_prep"
  • "_ops"
  • "_measurements"
  • None

This also eliminates the need to maintain the STATE_PREP_OPS list in pennylane/tape/tape.py, reducing the maintenance burden and the amount of code that needs to be modified when adding a single new operation.

@mariaschuld
Copy link
Contributor

mariaschuld commented Apr 1, 2022

Thanks for giving this a shot!

I am wondering if this is the best fix: what if we have an Adjoint of an observable? I don't think there is always a unique mapping between operator class and queue category?

If it is possible to just relax the idea of two queues, I'd much prefer that one - is this worth trying?

@codecov
Copy link

codecov bot commented Apr 1, 2022

Codecov Report

Merging #2408 (b4d0cac) into master (8858306) will increase coverage by 4.27%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2408      +/-   ##
==========================================
+ Coverage   95.20%   99.47%   +4.27%     
==========================================
  Files         244      244              
  Lines       19422    19427       +5     
==========================================
+ Hits        18490    19325     +835     
+ Misses        932      102     -830     
Impacted Files Coverage Δ
pennylane/measurements.py 99.59% <100.00%> (+<0.01%) ⬆️
pennylane/operation.py 96.72% <100.00%> (+0.03%) ⬆️
pennylane/ops/qubit/state_preparation.py 100.00% <100.00%> (ø)
pennylane/tape/tape.py 98.84% <100.00%> (-0.02%) ⬇️
pennylane/transforms/insert_ops.py 100.00% <100.00%> (ø)
pennylane/transforms/qcut.py 100.00% <0.00%> (+0.13%) ⬆️
pennylane/_qubit_device.py 98.73% <0.00%> (+0.31%) ⬆️
pennylane/devices/default_qubit.py 100.00% <0.00%> (+0.38%) ⬆️
pennylane/utils.py 97.97% <0.00%> (+0.67%) ⬆️
...ane/transforms/optimization/single_qubit_fusion.py 100.00% <0.00%> (+1.92%) ⬆️
... and 25 more

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 8858306...b4d0cac. Read the comment docs.

@albi3ro
Copy link
Contributor Author

albi3ro commented Apr 4, 2022

@mariaschuld The adjoint of an observable would still be an observable, which would not be processed. I'm going to play around with getting rid of the differences between the three queues and see how many things that breaks.

@albi3ro albi3ro added the op-arithmetic PR's involved in introducing operator arithmetic label Apr 7, 2022
@albi3ro albi3ro requested a review from Jaybsoni April 22, 2022 13:00
pennylane/operation.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Jaybsoni Jaybsoni 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, this should be enough to run simple workflows with the wrapper classes

pennylane/ops/qubit/non_parametric_ops.py Outdated Show resolved Hide resolved
pennylane/tape/tape.py Show resolved Hide resolved
pennylane/tape/tape.py Show resolved Hide resolved
pennylane/ops/identity.py Outdated Show resolved Hide resolved
pennylane/tape/tape.py Outdated Show resolved Hide resolved
pennylane/tape/tape.py Outdated Show resolved Hide resolved
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.

Thanks Christina! I don't want to drag this out, but there are these two things left:

  1. I strongly suggest having comments everywhere to say that this is a temporary fix (see suggestions). Otherwise I think this is the best solution.

  2. About the checks: there is one serious issue that users try a lot when implementing quantum kernels with AmplitudeEmbedding:

        AmplitudeEmbedding(...)
        qml.adjoint(AmplitudeEmbedding())
        ...

A user does not know that this template, which refers to some routine, is implemented by QubitStatePreparation, must always be the first operation.

Another example is if QubitStatePreparation is first applied to some qubits and then to others - a user cannot guess that this leads to a wrong result (at least I think it should reset the first qubits to zero).

In other words, if we have operators that need to be first in the circuit but this rule is opaque to users I feel we have to throw an error, and this won't change in future either?

pennylane/tape/tape.py Outdated Show resolved Hide resolved
@albi3ro albi3ro merged commit e55f463 into master Apr 27, 2022
@albi3ro albi3ro deleted the ops-know-queue-type branch April 27, 2022 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
op-arithmetic PR's involved in introducing operator arithmetic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants