Fix inversion of templates

[josh146]

Context:

In v.0.15 of PennyLane, two conflicting changes were made:

• The templates were re-written as operations with expand methods
• The qml.adjoint() transform was introduced, which recursively inverts the decomposition method

As a result, qml.adjoint() no longer works with templates.

In addition, the template expand() methods did not take into account the self.inverse flag, which is used by the deprecated qml.inv() function. As a result, the qml.inv() function also stopped working with templates.

Description of the Change:

The two main changes are:

• qml.adjoint() now uses the Operation.expand method, rather than Operation.decomposition, for better compatibility and allowing qml.adjoint() to again remain working with templates.

• qml.inv() was modified to use qml.adjoint() internally, allowing it to continue to work.

Two additional bugs were fixed at the same time

• tape.adjoint() has been added, mirroring Operation.adjoint(), so that qml.adjoint() will now work on nested tape structures.

• tape.inv() now uses the op.adjoint() method to invert operations, rather than the deprecated Operation.inverse flag.

Benefits:

• qml.inv() and qml.adjoint() now work again in all cases.

• Tests have been added to test qml.inv() and qml.adjoint() on the templates, to ensure we catch this in future.

Possible Drawbacks:

Not a drawback per se, but the following functionality is due to be removed in v0.16:

• qml.inv()
• The tape.inv() method
• The Operation.inverse flag

This will help simplify the code base, all uses of these logical blocks can be deleted.

Related GitHub Issues: n/a

[josh146]

We need to add an adjoint() method to the tape, so that the qml.adjoint() function knows what to do when it recursively hits a tape.

[mariaschuld]

Much cleaner, because we do not need to know a signature of decomposition here.

[codecov]

Codecov Report

Merging #1243 (ffa9b20) into master (fd60d39) will decrease coverage by 0.02%.
The diff coverage is 88.46%.

@@            Coverage Diff             @@
##           master    #1243      +/-   ##
==========================================
- Coverage   98.14%   98.11%   -0.03%     
==========================================
  Files         146      146              
  Lines       11035    11050      +15     
==========================================
+ Hits        10830    10842      +12     
- Misses        205      208       +3     

Impacted Files	Coverage Δ
pennylane/tape/tape.py	97.72% <81.25%> (-0.80%)	⬇️
pennylane/templates/embeddings/amplitude.py	100.00% <100.00%> (ø)
...nnylane/templates/layers/particle_conserving_u1.py	100.00% <100.00%> (ø)
...nnylane/templates/layers/particle_conserving_u2.py	100.00% <100.00%> (ø)
pennylane/transforms/adjoint.py	100.00% <100.00%> (ø)
pennylane/utils.py	95.94% <100.00%> (-0.03%)	⬇️

---

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 fd60d39...ffa9b20. Read the comment docs.

[josh146]

This was an idea by @mariaschuld: if inverting AmplitudeEmbedding, we should force it to go via the Mottonen route

[mariaschuld]

I was actually thinking that even without this explicit adjoint function this would be done, since the template calls QubitStateVector which should not have an adjoint, and hence its decomposition is used? But great like that too!

[josh146]

This ensures that this layer structure is invertible

[mariaschuld]

Any reason to change the assert statement from the previous tests?

[josh146]

I think I decided it was safer to check for the ground state explicitly, since any other basis state would also satisfy assert len(np.nonzero(res)) == 1

[mariaschuld]

Great @josh146, you are a star. Shall I add the other tests?

Alternatively, we could just add a test that compares the tape returned by the new inv method in the templates? Is there any way to check if a tape is the inverse of another? Because if that is checked, and the rest of the logic tested in the inverse/adjoint functions works, things should be solid, no?

[josh146]

Is there any way to check if a tape is the inverse of another?

Currently there is no way of doing this I believe 🤔

It might also be safer just to manually test every template? Even though it is a lot of extra work... But yeah I don't really have a good solution for parametrizing it, especially since a lot of the templates have different signatures.

[josh146]

@Thenerdstation I updated this docstring after discussion with a user that was having trouble

[josh146]

@Thenerdstation @mariaschuld: I had an epiphany today! If we re-write qml.inv() to simply use qml.adjoint internally, then:

• it reduces the complexity of the code
• it avoids maintainability issues; qml.inv() will likely continue to work as long as qml.adjoint() is working
• It ensures that they are both using the same logical pathway. That is, both will now simply end up using the Operation.adjoint() method, and all the extra tape.inv()'s that I placed in the templates to get qml.inv() working can be removed.

I'd like to take a second to thank codecov, as I was inspired to this approach due to all the missing lines covered due to the many tape.inv()'s lol.

[antalszava]

Looks good to me @josh146! 🥇 (popped in for the review, hope that's okay :) )

Was wondering: is it worth using qml.state in the tests?

There could be cases, where we get the expected probabilities with an unexpected state, e.g.,:

import pennylane as qml

dev = qml.device('default.qubit', wires=1)

@qml.qnode(dev)
def circuit_with_state():
    qml.PauliX(0)
    qml.S(wires=[0])
    return qml.state()

print(circuit_with_state())

dev = qml.device('default.qubit', wires=1)

@qml.qnode(dev)
def circuit_with_probs():
    qml.PauliX(0)
    qml.S(wires=[0])
    return qml.probs(wires=[0])

print(circuit_with_probs())

[0.+0.j 0.+1.j]
[0. 1.]

We have a complex amplitude in the state vector, and yet we'd sample the |1> basis state with 100% theoretically. So checking [0, 1] for the probabilities could give a false positive if we'd like to check that our state is in

[antalszava]

For my understanding: do we now have tape.inv as the in-place operation and qml.adjoint as the tape transform?

And qml.inv is good for everything including tapes.

[josh146]

Yep! qml.adjoint and qml.inv are currently interchangeable, but qml.inv will be removed in the next release.

[mariaschuld]

Interesting, invisible makes sure it is not queued? so it is a transform transform :)

[josh146]

Yep. Under the hood, it looks like this:

with tape.stop_recording():
    # things you don't want queued go here

However, @Thenerdstation pointed out that with PL's functional UI, it also makes sense to have a functional 'entry point' to this functionality. Nathan is not wild about the name though, so I'm trying to brainstorm better names 😆

How about qml.transforms.ignore? Although I feel that invisible is a bit more clear in its intention --- the function is still executed, it is just invisible to the queue.

[josh146]

Note that qml.transforms.invisible is not meant to be user-facing; it is more for developers (and potentially advanced users).

[antalszava]

How come this was changed? (and line 2056 stayed as is)

[josh146]

With this change, qml.inv() now uses qml.adjoint() internally. As a result. gates that PL knows how to invert are automatically inverted; PhaseShift(x) becomes PhaseShift(-x) (rather than PhaseShift(x).inv).

The only exception are gates where the inverse is not known.

[antalszava]

Oh! That makes sense. thank you! 🙂

[mariaschuld]

Just to understand, .inv() still works but will be deprecated?

[josh146]

qml.inv(), op.inv(), and qml.adjoint() should all work identically.

• qml.inv() is marked as deprecated (in my head, need to formalize this).
• op.inv() is not marked for deprecation, we still need to work out what the future of this is.

[mariaschuld]

Thanks for sorting the inversion mess out!

Looks good to me. In the long run I wonder if there is a better option than testing every template? I guess when all the Operations follow the same design, adjoint() could be tested with only a few examples?

[josh146]

Looks good to me. In the long run I wonder if there is a better option than testing every template? I guess when all the Operations follow the same design, adjoint() could be tested with only a few examples?

Yep, I think long-term (once templates are sorted out) this is something to be moved to the template tests. I.e., every template should have class TestMyTemplateAdjoint.