Updated non_parametric ops adjoint method and added test

[Jaybsoni]

Context:
The current adjoint() method implementation for non-parametric operations would lead to a bug in which taking multiple adjoints of the operation do not return the correct result (#1968).

Description of the Change:
Updated the adjoint() method such that instead of initializing a new operation (of the same kind) and applying inv(), it would access the internal boolean attribute op.inverse and flip it.

>>> op = qml.S(wires=0)
>>>
>>>
>>> # OLD: 
>>> print(op)
S(wires=[0])
>>>
>>> print(op.adjoint())
S.inv(wires=[0])
>>>
>>> print(op.adjoint().adjoint())
S.inv(wires=[0])
>>>
>>> 
>>> # New: 
>>> print(op)
S(wires=[0])
>>>
>>> print(op.adjoint())
S.inv(wires=[0])
>>>
>>> print(op.adjoint().adjoint())
S(wires=[0])

[github-actions]

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.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]

Codecov Report

❗ No coverage uploaded for pull request base (v0.21.0-rc0@2fbf655). Click here to learn what that means.
The diff coverage is n/a.

@@              Coverage Diff               @@
##             v0.21.0-rc0    #2133   +/-   ##
==============================================
  Coverage               ?   99.20%           
==============================================
  Files                  ?      230           
  Lines                  ?    17665           
  Branches               ?        0           
==============================================
  Hits                   ?    17525           
  Misses                 ?      140           
  Partials               ?        0           

---

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 2fbf655...819f2a3. Read the comment docs.

[Jaybsoni]

[sc-11983]

[josh146]

minor flyby comment, but I'm curious if the op.inverse = not self.inverse can be included in the qml.adjoint() code, to avoid needing to duplicate it in each gate?

[Jaybsoni]

My only concern would be that this logic implicitly implies that the underlying object is Unitary (adjoint = inverse). By implementing it into the adjoint() method of the operation class. Even though all operations which inherit from this class override this method, it might still be better to leave it out. But if we just want to include it from the perspective of reducing code redundancy, then yes I should be able to move the logic there 👍🏼

After thinking about this again, I realized all of the operations we work with will be Unitary. I was worried for the case we would be working with Hermitian observables which inherit from the Observable class (which in turn inherits from Operation). But we can deal with this case by simply overriding the adjoint() method in the Observable class directly.

[josh146]

yep, everything (currently) is unitary :)

[josh146]

I suppose my main reason for originally commenting this is less about the abstraction, and more about making the operator class developer friendly.

Currently, op.inverse is an implementation detail which is not publicly documented. Meanwhile, op.adjoint() is part of the public API that developers should define.

We should try and avoid developers needing to know about implementation details to satisfy the API!

[Jaybsoni]

@josh146, in moving the logic to the Operation class, I also had to update a bunch of the templates which inherit from Operation but don't have an adjoint method implemented. This could be a good opportunity for coding challenges during the interview process (something like complete the adjoint method for the angle embedding template)

[Jaybsoni]

@josh146, after speaking with @albi3ro and the rest of the Core team we concluded that it would be best to go with the simplest option so I am going to revert to simply overwriting the adjoint method in the non-parametric ops since the current change requires updating the adjoint method for all of the templates

[josh146]

no worries! That makes a lot of sense. I believe the templates have already been fixed in op-refactor :)

[albi3ro]

I wonder if we should just get rid of the do_queue keyword in the abstract method, as no child operation seem to either accept the do_queue keyword or do anything with it. Shouldn't break anything if we just get rid of it.

[albi3ro]

It might be easier to move this parametrization inside the test. This way is fine, but we could also:

• take one adjoint
• validate
• take second adjoint
• validate
• take third adjoint
• validate

instead of taking it multiple times in a row then validating.

[albi3ro]

Might want to leave a note about what "idempotent" means.

[albi3ro]

Great to finally have this fixed 🚀