[WIP] Redesign capabilities dictionary #781
Conversation
pennylane/_device.py
Outdated
| 'performs_noisy_computation': False, | ||
| } | ||
| """The capabilities dictionary stores the properties of a device. Devices can add their | ||
| own custom properties and overwrite existing ones by overwriting the ``capabilities`` class method.""" |
There was a problem hiding this comment.
Not sure if one would use docstrings for class attributes?
pennylane/_device.py
Outdated
| _capabilities = {} #: dict[str->*]: plugin capabilities | ||
| _capabilities = { | ||
| 'model': None, | ||
| 'passthru_interface': None, |
There was a problem hiding this comment.
In two minds about this one: we could only define it in the devices that actually have passthru functionality (so that absence of the key signifies that the device does not)...
| def capabilities(cls): | ||
| capabilities = super().capabilities().copy() | ||
| capabilities.update( | ||
| supports_reversible_diff=True, |
There was a problem hiding this comment.
How come this would not be defined among the capabilities in Device?
There was a problem hiding this comment.
It is set to False in Device...
There was a problem hiding this comment.
Oh not sure how I missed that, my bad!
There was a problem hiding this comment.
for my understanding, how do we know if something supports reversible diff?
| ops[operation] | ||
| return qml.expval(qml.Identity(wires=0)) | ||
|
|
||
| assert isinstance(circuit(), (float, np.ndarray)) |
There was a problem hiding this comment.
Just curious, what would be a specific case for this assertion to fail?
There was a problem hiding this comment.
Good question, the assert statement is more of a dummy here. It's one of those tests where I just want to know "does the execution throw an error". Is there a generic way to do this with pytest? One could also not have an assert statement at all, as in some other test, but I thought this is bad practice...
There was a problem hiding this comment.
Looks good to me, great on @mariaschuld! 💯 Had some questions, mostly for my own understanding
Co-authored-by: antalszava <antalszava@gmail.com>
… add 'returns_state' and 'returns_probs' argument
|
After double approval I implemented quite a few more changes:
@trbromley , since the second change is partly requested by you, would you mind having a look at the overall PR too? |
|
After double approval I implemented quite a few more changes:
@trbromley , since the second change is partly requested by you, would you mind having a look at the overall PR too? |
There was a problem hiding this comment.
Thanks @mariaschuld! Just left minor comments. One thing I was wondering is whether we should set return_state=False in the base Device?
| @@ -101,15 +101,31 @@ as well as potential further capabilities, by providing the following class attr | |||
| conversion between the two conventions takes place automatically | |||
| by the plugin device. | |||
|
|
|||
| * :attr:`.Device._capabilities`: a dictionary containing information about the capabilities of | |||
| the device. Keys currently supported include: | |||
| * :func:`.Device.capabilities`: A class method which returns the dictionary of capabilities of a device. A | |||
There was a problem hiding this comment.
Just wondering if it would make sense to update the subsection title to make it more capability focused, e.g., "Defining capabilities"
| supports_analytic_computation=True, | ||
| supports_finite_shots=False, | ||
| supports_tensor_observables=True, | ||
| returns_state=False, |
| def capabilities(cls): | ||
| capabilities = super().capabilities().copy() | ||
| capabilities.update( | ||
| supports_reversible_diff=True, |
There was a problem hiding this comment.
for my understanding, how do we know if something supports reversible diff?
| with pytest.raises(AttributeError): | ||
| dev.state # pylint: disable = pointless-statement |
There was a problem hiding this comment.
Some devices (e.g. qiskit.aer) have the dev.state attribute available, but it is equal to None. I would say that for such devices, cap["returns_state"] = False should be the case. However, I think this test would fail for such a device since it wouldn't raise an AttributeError 🤔
If I understand the layout of the test suite correctly, this test would be applied to qiskit.aer in https://github.com/PennyLaneAI/plugin-test-matrix (I guess it would skip for now, until we define the capabilities for plugins).
Maybe we should have
if cap["returns_state"]:
assert dev.state is not None
else:
try:
state = dev.state
except AttributeError:
state = None
assert state is None| supports_analytic_computation=True, | ||
| supports_finite_shots=True, | ||
| returns_probs=False, | ||
| returns_state=False, |
There was a problem hiding this comment.
Perhaps we could set returns_state=False in the base Device, and then only turn it on when needed.
There was a problem hiding this comment.
The reason why I reduced the default values in the base device is that not having the value is better than misreporting. Also, it would set this value for all devices, and it would be better to go through them one by one...
| @@ -488,6 +500,7 @@ def test_two_qubit_no_parameters(self, device, init_state, op, mat, tol, skip_if | |||
| n_wires = 2 | |||
| dev = device(n_wires) | |||
| skip_if(dev, {"supports_inverse_operations": False}) | |||
| skip_if(dev, {"returns_probs": False}) | |||
There was a problem hiding this comment.
seems to have complaints for theta in this file too 🤔
|
About #781 (comment): I test it by checking if one can use the device in a reversible qnode and can compute a gradient...but maybe there is a better option. Luckily, the device test are due for an upgrade. |
This PR implements two goals:
supports_XYZinstead ofXYZ)In the process, a few bugs were uncovered, and the
default.tensordevice got the attributeanalyticto allow some more tests to run.