pennylane icon indicating copy to clipboard operation
pennylane copied to clipboard

Directly creating a tape does not initialize `trainable_params` properly

Open dime10 opened this issue 3 years ago • 9 comments

Expected behavior

Creating a tape should initialize the trainable_params property according to the parameters received:

weights = [0.1, 0.2]
with qml.tape.QuantumTape() as tape:
    qml.RX(weights[0], wires=0)
    qml.RY(weights[1], wires=0)
    qml.expval(qml.PauliZ(0) @ qml.PauliZ(1))

should result in tape.trainable_params = []

Actual behavior

All parameters regardless of data type or interface are considered trainable, that is the previous example produces tape.trainable_params = [0, 1].

Note if the tape is executed or constructed via a QNode the attribute is properly set.

Additional information

No response

Source code

No response

Tracebacks

No response

System information

-

Existing GitHub issues

  • [X] I have searched existing GitHub issues to make sure the issue does not already exist.

dime10 avatar Feb 02 '22 17:02 dime10

Copying a potential solution from external conversation for archival properties:

For a while, since we've refactored the interfaces/QNode, I've had at the back of my mind an idea on how we can improve this.

  1. We can add tape.interface, which simply calls

    @property
    def interface(self):
        all_params = self.get_parameters(only_trainable=False)
        return qml.math._multi_dispatch(all_params)
    
    
  2. We can then do

    @property
    def trainable_params(self):
        all_params = self.get_parameters(only_trainable=False)
        return qml.math.get_trainable_indices(all_params)
    

That is, the tape simply uses qml.math and queries its own parameters to work out (a) its interface, and (b) the trainable indices.

I think this could make a lot of the codebase simpler/less complex, and also avoid bugs like this. Unfortunately, I fear it is a very invasive change, that will likely require some surgery of both the codebase and the tests!

josh146 avatar Feb 03 '22 07:02 josh146

TODOs can be removed in the following files once this is fixed: https://github.com/PennyLaneAI/pennylane/blob/ce941f345bee6cd1d89d50753cd68cdb51982ad5/tests/transforms/test_metric_tensor.py#L634 https://github.com/PennyLaneAI/pennylane/blob/ce941f345bee6cd1d89d50753cd68cdb51982ad5/tests/gradients/test_finite_difference.py#L214 https://github.com/PennyLaneAI/pennylane/blob/ce941f345bee6cd1d89d50753cd68cdb51982ad5/tests/gradients/test_param_shift_hessian.py#L535 https://github.com/PennyLaneAI/pennylane/blob/ce941f345bee6cd1d89d50753cd68cdb51982ad5/tests/gradients/test_parameter_shift.py#L231 https://github.com/PennyLaneAI/pennylane/blob/ce941f345bee6cd1d89d50753cd68cdb51982ad5/tests/gradients/test_parameter_shift_cv.py#L305

dime10 avatar Feb 07 '22 21:02 dime10

I realize something else; if we add tape.interface, which is smartly determined by the tape parameters, we no longer require users to pass interface= to the QNode, we should be able to smartly infer the interface.

This should simplify a lot of the QNode logic

josh146 avatar Feb 08 '22 06:02 josh146

This came up for me while working on tape-related stuff, so I just wanted to share some updates/discoveries. The unofficial API here is that tape.trainable_parameters defaults to all parameters, and this makes sense because it might be costly to check every parameter for a tape that isn't even going to go through auto-differentiation. At least, it makes sense to not try and evaluate that immediately on construction. This is a subtle and not-so-public detail about trainable_params, but it doesn't seem to have severe consequences other than looking a little funny if you inspect the trainable_params before executing the tape. So, I'm going to leave this open but mark it as won't-fix

timmysilv avatar Mar 16 '23 16:03 timmysilv

This came up for me while working on tape-related stuff, so I just wanted to share some updates/discoveries. The unofficial API here is that tape.trainable_parameters defaults to all parameters, and this makes sense because it might be costly to check every parameter for a tape that isn't even going to go through auto-differentiation. At least, it makes sense to not try and evaluate that immediately on construction. This is a subtle and not-so-public detail about trainable_params, but it doesn't seem to have severe consequences other than looking a little funny if you inspect the trainable_params before executing the tape. So, I'm going to leave this open but mark it as won't-fix

In that case shouldn't it be either populated with nothing by default, set to None by default until the trainable parameters are determined, or something similar? I guess ideally it would compute them when the member is first accessed.

dime10 avatar Mar 16 '23 17:03 dime10

@dime10 I pretty much proposed that earlier today :D but I don't think the difference is super duper significant. I also think that change would break quite a lot of unit tests. Is this affecting your work?

timmysilv avatar Mar 16 '23 18:03 timmysilv

@dime10 I pretty much proposed that earlier today :D but I don't think the difference is super duper significant. I also think that change would break quite a lot of unit tests. Is this affecting your work?

No not really! I don't remember exactly since this was a year ago, but my guess is that I stumbled upon this bug while writing the tests listed here: https://github.com/PennyLaneAI/pennylane/issues/2155#issuecomment-1031969020

dime10 avatar Mar 16 '23 19:03 dime10

in the past we have tried both:

  • having trainable_params be None by default
  • having the tape 'know' its interface, so that it can set trainable parameters the correct way on initialization (as above).

In both cases, tonnes of unit tests failed, and it became non-trivial to fix.

I think there might be a deeper issue here, in that we are using 'trainable params' to refer to two separate concepts:

  • Parameters a user wants to analyze/transform in their circuit (e.g., metric_tensor uses trainable_params to identify this, although @rmoyard recently added a argnums= argument as a fix)

  • Parameters that are going to be differentiated

If we restrict the definition of trainable_params to just the latter, and have a different UI (e.g., argnums) for the former, it would probably help a lot

josh146 avatar Mar 16 '23 22:03 josh146

Trying out a solution in #5344

albi3ro avatar Mar 08 '24 17:03 albi3ro