Help device plugin developers deal with the Operation refactor

[cvjjm]

Feature details

As was discussed in #2023 with @josh146 and @mariaschuld in the light of the Operation refactor a smooth transition with useful deprecation warnings should be ensured so that device developers have time to update their devices and custom Operations and support at least a few versions of PennyLane.

Currently this has not been fully achieved.

Assume that some third party code (e.g a device plugin) defines a "old style" custom Operation as follows:

import pennylane as qml
from pennylane.operation import Operation

class MyOp(Operation):
    num_wires = 1
    num_params = 2
    par_domain = "R"
    grad_method = "F"

    @staticmethod
    def decomposition(*params, wires):
        qml.RY(params[0], wires=wires[0])
        qml.PauliZ(wires=wires[0])
        qml.RX(params[1], wires=wires[0])

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

@qml.qnode(dev)
def qnode(*params):
    MyOp(*params, wires=0)
    return qml.state()

print(qml.draw(qnode)(0.1, 0.2))
print(qnode(0.1, 0.2))

This works until v0.21.0 but with master the user gets the unspecific error message:

TypeError: decomposition() missing 1 required keyword-only argument: 'wires'

Converting the implementation to "new style" will break the code with all PL versions prior to and including v0.21.0.

This is not nice.

Due to the smart re-naming of functions in #2023 it is indeed possible to define the above operation in a way that is compatible with both the old and the new style:

    def decomposition(self, *params, wires=None):
        if wires is None:
            wires = self.wires
        if not params:
            params = self.parameters
        qml.RY(params[0], wires=wires[0])
        qml.PauliZ(wires=wires[0])
        qml.RX(params[1], wires=wires[0])

Alternatively one can also directly implement the new compute_decomposition() and patch in a suitable preparation() if one is running on an old PL version. The following also works with both 0.21 and the current master:

class MyOp(Operation):
    num_wires = 1
    num_params = 2
    par_domain = "R"
    grad_method = "F"

    if not hasattr(Operation, 'get_eigvals'): # pennylane <= v0.21
        @classmethod
        def decomposition(cls, *params, wires):
            return cls.compute_decomposition(*params, wires=wires)

    @staticmethod
    def compute_decomposition(*params, wires=None):
        qml.RY(params[0], wires=wires[0])
        qml.PauliZ(wires=wires[0])
        qml.RX(params[1], wires=wires[0])

There are of course a few further variants...

It would be nice to replace the above unspecific error message with a useful error that gives a hint for how the implementations can be made compatible across the 0.21/0.22 cut so that plugin developers can at all times support at least some range of PennyLane versions.

Implementation

No response

How important would you say this feature is?

3: Very important! Blocking work.

Additional information

No response

[josh146]

Thanks @cvjjm for reporting this!

Just double checking, but is Operator.decomposition the only changed method with this issue? Or are there others as well?

From having a look at your code,

def decomposition(self, *params, wires=None):
    if wires is None:
        wires = self.wires
    if not params:
        params = self.parameters
    qml.RY(params[0], wires=wires[0])
    qml.PauliZ(wires=wires[0])
    qml.RX(params[1], wires=wires[0])

is a nice (and relatively simply to read) workaround, I'll see if there is a nice way to catch this (might mean tracking down everywhere that Operator.decomposition() is currently called).

[josh146]

I've created a PR (#2214) to fix this :)

[cvjjm]

I think the similar problems will occur with other refactored method. I also have a few other issues that I will report once I understand them better.

[cvjjm]

Here is another facet of this problem: When using the current mater with an old style Operation that defines its matrix via the old style _matrix() static method and does not implement the new style compute_matrix(), the user gets a reasonable but not terribly helpful MatrixUndefinedError`.

In particular the user is not even told which operation caused this error. As this error is raised in compute_matrix(), which is itself static, this information is no longer available at this point, but two levels up when compute_matrix() is called in get_matrix() would be a good place. Here the MatrixUndefinedError could be called and a useful Exception raised that contains the name of the class Operation that caused the error along with a hint for how to fix things (namely rename _matrix() to the new style compute_matrix() and in case backward compatibility is wanted set _matrix = compute_matrix in the custom Operation).

Side remark: It strikes me as slightly odd that get_matrix() is implemented in a nearly identical fashion in both Operator and Operation.

[cvjjm]

And another issue is that .generator is now no longer a property but a method and it no longer returns the pre-factor and the generator. It is quite ugly to write code that is compatible with both the new and the old style here. Any idea for how this can be made easier?

[josh146]

Side remark: It strikes me as slightly odd that get_matrix() is implemented in a nearly identical fashion in both Operator and Operation.

@cvjjm yep, the only reason for this is the need to take into account for the inverse in the middle of the logic, making it difficult to use super() here:

pennylane/pennylane/operation.py

Lines 1302 to 1303 in fc96309

	if self.inverse:
	canonical_matrix = qml.math.conj(qml.math.T(canonical_matrix))

Having said that, we are still in the process of the operator refactor, unfortunately! There are a couple more changes likely coming down the line in v0.23 (since we are unable to finish them all in time for one release):

• adjoint/inversion will be modified to no longer be in-place, which will resolve a lot of bugs

• support for arbitrary operator arithmetic, including powers and square roots

• likely merging Tensor and Hamiltonian into the base Operator class, which will also resolve a lot of bugs and hotfixes

• Finalizing the user/dev facing API for working with operations.

Regarding this last point, what this means is that Operator.get_matrix() will likely not be the public API which we document and guarantee against breaking changes This is still TBD, but we hope to finalize this ASAP.

One option will be a function along the lines of get_unitary_matrix(), e.g., qml.matrix(op).

One benefit of a function is that (as you've noticed!) it is a lot easier to evolve and deprecate functions that it is with class methods 😆 Another benefit is that the function would be able to act on various inputs, including QNodes, tapes, etc., not just operations.

Here the MatrixUndefinedError could be called and a useful Exception raised that contains the name of the class Operation that caused the error along with a hint for how to fix things (namely rename _matrix() to the new style compute_matrix() and in case backward compatibility is wanted set _matrix = compute_matrix in the custom Operation).

Let me explore this in a PR.

And another issue is that .generator is now no longer a property but a method and it no longer returns the pre-factor and the generator. It is quite ugly to write code that is compatible with both the new and the old style here. Any idea for how this can be made easier?

This is another case where Operator.generator will likely not be the final API - I have a small utility function qml.utils.get_generator() that abstracts away a lot of the 'complexities'. If it is helpful I can take into account the deprecation here?

[cvjjm]

Thanks for the explanations. This sounds all great!

One remark on the scalar multiplication: I may be misunderstanding this, but Operator.__mul__() seems to always return a Hamiltonian, and since you probably want to keep the implementation of .generator implemented in a way that it returns what used to be the "bare" generator and the prefactor "in one thing" I am suspect that .generator will end up returning a Hamiltonian. This may be convenient if one is interested in measuring the Generator, but in some cases one may also be interested (in fact we are in several cases) in applying the "bare" generator as an operation and using the pre-factor elsewhere. Will it remain possible/easy to get the "bare" generator and the pre-factor from the Hamiltonian (probably via .terms()?)? Or do you want to move to an implementation where the pre-factor becomes part of the Operation (I would prefer if you did not do this, so that something of type PauliZ is and remains always a "bare" Pauli and I don't need to inspect some "pre-factor" property to know which operator it actually is.)?

[josh146]

am suspect that .generator will end up returning a Hamiltonian. This may be convenient if one is interested in measuring the Generator, but in some cases one may also be interested (in fact we are in several cases) in applying the "bare" generator as an operation and using the pre-factor elsewhere.

This is the plan yes, but with a caveat: it is likely the Hamiltonian class will be merged with the Operator class.

So Operator.generator will always return another Operator, which itself will always have terms and/or decomposition defined. Would this be okay?

The reason for this is that we ran into too many issues where Operator generators could not be described in terms of a separate pre-factor and operator class. We originally dealt with this by falling back to NumPy arrays, but then we ran into memory problems! Alternatively we could start to return Hamiltonians, but then it becomes confusing what exactly the prefactor means.

A deeper problem was that the fact that Operator.generator could return multiple types, which was bad design and causing bugs throughout the codebase

[cvjjm]

Yes, that sounds perfect. Looking forward to that new functionality!

[cvjjm]

The one last idea: You are currently using a tuple of two lists to represent the terms, which I subjectively find a but "cumbersome". I.e. to determine the number of terms one has to first take, e.g., the first element of that tuple and then call len() and then one has to trust that the two lists are actually the same length... Have you considered using a dictionary mapping the operators to the coefficients like OpenFermion does it?

[cvjjm]

And here is another problem I encountered with the refactor: generator used to be a static class property of Operations, now the same name is used for the instance method generator(). This name clash prevents a implementation of my custom Operations that is compatible with both the old and the new style and thus precludes supporting a range of versions.

As with the num_params property also the generator is a property of the class and not of the instance. So the user should be able to access it from the class without having to make an instance whenever possible.

[josh146]

Have you considered using a dictionary mapping the operators to the coefficients like OpenFermion does it?

We haven't, but good to get this feedback since this is all still TBD!

Would it fit your workflow if this was a simple class containing two lists? We then have the flexibility to add convenience methods such as e.g., to return the nth term (operator + coeff) as well as mappings

[cvjjm]

A simple class would certainly also do the job, but why re-invent the wheel? A dict already has .items(), keys(), ... len(), it is automatically ensured that there are no double entries, and people know how to manipulate dicts. If you want to add functionality on top then maybe subclass dict (or OrderedDict?). I think not sub-classing list but "emulating" list with the Wires class caused a lot of needless headaches :-)

[mariaschuld]

@cvjjm thanks for your valuable feedback!

Just coming in real quick here to clarify something general. As discussed before, unfortunately we cannot consider developers' aim to support multiple versions of PennyLane. This has never been a goal of PennyLane dev, and it is not an industry standard - it was mere chance that this was possible before. It would also require an disproportional amount of resources, especially for refactors, and may often just mean that PennyLane cannot develop as a library, which no one would benefit from in the long run.

But, of course, we will try our very best to allow for at least one deprecation cycle, so there is time to adapt. I hope there was no misunderstanding there.

I think some issues you raise here are still valid (since a deprecation cycle is a kind of "two-version-support"). But just to get the shared goal clear, and to avoid disappointment!

[cvjjm]

Yes, agree. There is no need to make extra efforts to support a broad range of versions, but some sort of deprecation cycle of one or two releases, depending on the magnitude of changes, would be healthy (imho) and much appreciated.

[cvjjm]

Summing up the thread above and the things I have learned while trying to make my Operations and devices compatible across the v0.21.0 / v0.22.0 cut:

• There is one last issue that is making it impossible to implement Operations in a way that is compatible with both v0.21.0 and v0.22.0, namely the name clash of the generator property, which we managed to avoid with decomposition / compute_decoposition and _matrix / compute_matrix naming scheme. If we can do the same with generator and settle on the return format of generator before v0.22.0 is released, plugin developers will still see their devices suddenly break without deprecation warning (which is still not nice imho) but they will at least have a change of updating them in a way that supports at least two consecutive version of PL.
• It would be nice if compute_matrix and compute_decposition (and decomposition) were "upgraded" to @classmethods from @staticmethods because then more complex Operations can call other @classmethods, from those functions which will allow a better structuring of code, especially in more complex Operations that are essentially whole templates with hyper-parameters.

[josh146]

If we can do the same with generator and settle on the return format of generator before v0.22.0 is released

I plan to have a PR merged before 0.22 that:

• has a top-level qml.generator() function as the main UI/public API
• renames generator() to get_generator() temporarily
• supports both return styles via a keyword argument

It would be nice if compute_matrix and compute_decposition (and decomposition) were "upgraded" to @classmethods

I'll have to think if there are any implications here - one reason for preferring staticmethods is that we make use of caching (e.g., via @lrucache()) down the line.

because then more complex Operations can call other @classmethods, from those functions which will allow a better structuring of code

I'm not 100% I follow here - are you wishing to call a class method from inside compute_matrix for example? Would you be able to do something like?

@staticmethod
def compute_matrix(*params, wires, **hyperparameters):
    return SomeOperatorClass.a_class_method()

[cvjjm]

I'm not 100% I follow here - are you wishing to call a class method from inside compute_matrix for example?

Yes.

Would you be able to do something like?

Yes, but repeating the class name in the function is not nice and does not play well with inheritance. But, yes this is a workaround in case caching does clash with @classmethods.

[josh146]

Thanks for clarifying! I suppose one other option is to override matrix() directly in the subclass -- I recall during the operator design this was meant as an option for advanced operator manipulation

[josh146]

@cvjjm in #2256 a top-level qml.generator(op) function is added, which preserves the ability to return a prefactor 🙂 Does this help address the issues discussed here?

[cvjjm]

The top level qml.generator(op) is great!

There remains the problem that .generator has become a function, which makes it impossible to implement custom operations in a way that are compatible with two immediately successive PL versions.

You invested quite a lot of effort renaming other functions such as .compute_matrix() to make this possible. It would be sad if that work would be in vane because of the still existing name clash with .generator.

[josh146]

Hey @cvjjm! Apologies, I think I misunderstood previous discussion. I thought you had code that worked with both styles, it was just ugly? #2210 (comment)

[josh146]

Ah, I missed a later comment: #2210 (comment)

[cvjjm]

Actually my comment #2210 (comment) was written before I talked to @dwierichs . He mentioned that he thinks that if the code using PL consistently only uses the new top level qml.generator(), then PL should be able to fully work with both operations that implement the old style .generator property and such that implement the new style .generator() method. If that is correct, then all my problems (and that of plugin developers who need to supply custom operations supported and provided by their device) should be gone. I haven't managed to test this with the finally released version.

[josh146]

@cvjjm nice, let me know how the testing goes

[cvjjm]

I can confirm that this works! Awesome! So me and other plugin developers actually have a good update path! Thanks for making this possible!

[josh146]

Nice to hear @cvjjm!