pennylane
pennylane copied to clipboard
Directly creating a tape does not initialize `trainable_params` properly
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.
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.
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)
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!
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
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
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
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 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?
@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
in the past we have tried both:
- having
trainable_params
beNone
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
usestrainable_params
to identify this, although @rmoyard recently added aargnums=
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
Trying out a solution in #5344