-
Notifications
You must be signed in to change notification settings - Fork 575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use the new multi_dispatch
decorator in the math module. Add tensordot
tests.
#2096
Conversation
pennylane/math/multi_dispatch.py
Outdated
# Note that diag is not eligible for the multi_dispatch decorator because | ||
# it is used sometimes with iterable `values` that need to be interpreted | ||
# as a list of tensors, and sometimes with a single tensor `values` that | ||
# might not be iterable (for example a TensorFlow `Variable`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dwierichs is it possible to modify the decorator to allow for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. It would have to try
to iterate over the arguments in (via the list method extend
) that are marked via tensor_list
, and if it fails, attempt to just append
the respective argument to the arguments to dispatch over.
I'd consider this an unreasonable overhead given the currently single use case. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I was thinking something simpler, although maybe it is not in the spirit of 'multiple dispatch'.
Basically, check not isinstance(arg, (list, tuple))
, and if this is the case, call qml.math.get_interface(arg)
.
Basically, we assume that a tensor_list
argument must be a built-in Python sequence (either list or tuple), and if this is not the case, we assume it must be a tensor and simply single dispatch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, that sounds cool! I'll give it a go :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented this now, a bit more lightweight, even: If an argument marked as tensor list is not a tuple
or list
, it is simply treated as if it was not marked as a tensor list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, nice call!
…ne into multi_dispatch-usage
…ne into multi_dispatch-usage
Codecov Report
@@ Coverage Diff @@
## master #2096 +/- ##
=======================================
Coverage 99.19% 99.19%
=======================================
Files 228 228
Lines 17534 17534
=======================================
Hits 17393 17393
Misses 141 141
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dwierichs! I must admit I am a bit confused, I can't see the changes as described in the PR comment --- it looks mostly like test changes. Are there commits that haven't been pushed?
@@ -203,7 +203,7 @@ def _get_random_args(args, interface, num, seed, bounds): | |||
if interface == "autograd": | |||
|
|||
# Mark the arguments as trainable with Autograd | |||
rnd_args = pnp.array(rnd_args, requires_grad=True) | |||
rnd_args = [tuple(pnp.array(a, requires_grad=True) for a in arg) for arg in rnd_args] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also curious about the change here 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was producing ragged arrays, raising the corresponding warning. It was very easy to fix and is not
related to the ragged array issues of QNodes, it was just not so nicely written by me back then, producing an array of a list of tuples of different sizes.
@@ -1362,14 +1452,14 @@ def test_array(self): | |||
x = np.array(self.x, requires_grad=True) | |||
y = np.array(self.y, requires_grad=True) | |||
|
|||
def cost(weights): | |||
def cost(*weights): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come this test was changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While x
and y
are markes as trainable above, the list [x, y]
of course is not. This triggered the warning that with Autograd, soon parameters will need to be marked as trainable explicitly. I'm aware that this is in an intermediate state across the codebase currently, but I thought it might be nice to change it here already, removing warnings from the test suite :)
lambda x: qml.math.log(1 + qml.math.exp(100.0 * x)) / 100.0, # Softplus is okay | ||
lambda x: qml.math.log(1 + qml.math.exp(100.0 * x)) / 100.0, # Softplus is okay |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the reason for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the chosen parameters and this massive prefactor of 1000, the exponential function was raising an overflow warning, actually 😅
This change essentially does not affect what is tested.
@@ -1362,14 +1452,14 @@ def test_array(self): | |||
x = np.array(self.x, requires_grad=True) | |||
y = np.array(self.y, requires_grad=True) | |||
|
|||
def cost(weights): | |||
def cost(*weights): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While x
and y
are markes as trainable above, the list [x, y]
of course is not. This triggered the warning that with Autograd, soon parameters will need to be marked as trainable explicitly. I'm aware that this is in an intermediate state across the codebase currently, but I thought it might be nice to change it here already, removing warnings from the test suite :)
@@ -1871,7 +1961,7 @@ def test_autograd(self): | |||
np.array([[x, 1.2 * y], [x ** 2 - y / 3, -x / y]]), | |||
] | |||
f = lambda x, y: fn.block_diag(tensors(x, y)) | |||
x, y = 0.2, 1.5 | |||
x, y = np.array([0.2, 1.5], requires_grad=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to remove the same warning as above (i.e. explicit trainability marking in Autograd)
values = [ | ||
onp.array([0.1, 0.2]), | ||
np.tensor(0.1, dtype=np.float64, requires_grad=True), | ||
np.tensor([0.5, 0.2], requires_grad=True), | ||
] | ||
grad = qml.grad(cost_fn, argnum=[1, 2])(*values) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
lambda x: qml.math.log(1 + qml.math.exp(100.0 * x)) / 100.0, # Softplus is okay | ||
lambda x: qml.math.log(1 + qml.math.exp(100.0 * x)) / 100.0, # Softplus is okay |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the chosen parameters and this massive prefactor of 1000, the exponential function was raising an overflow warning, actually 😅
This change essentially does not affect what is tested.
@@ -167,7 +167,7 @@ def dependent_circuit(x, y, z): | |||
dependent_circuit, | |||
np.array, | |||
lambda x: np.array(x * 0.0), | |||
lambda x: (1 + qml.math.tanh(1000 * x)) / 2, | |||
lambda x: (1 + qml.math.tanh(100 * x)) / 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This again is to avoid overflow warnings.
@@ -519,7 +519,7 @@ def dependent_circuit(x, y, z): | |||
|
|||
dependent_functions = [ | |||
dependent_circuit, | |||
torch.tensor, | |||
torch.as_tensor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applying torch.tensor
to a Torch tensor raises a warning to use an appropriate copying method. We here just want to test some form of identity function, so as_tensor
is as good as tensor
and it does not raise the warning :)
@@ -203,7 +203,7 @@ def _get_random_args(args, interface, num, seed, bounds): | |||
if interface == "autograd": | |||
|
|||
# Mark the arguments as trainable with Autograd | |||
rnd_args = pnp.array(rnd_args, requires_grad=True) | |||
rnd_args = [tuple(pnp.array(a, requires_grad=True) for a in arg) for arg in rnd_args] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was producing ragged arrays, raising the corresponding warning. It was very easy to fix and is not
related to the ragged array issues of QNodes, it was just not so nicely written by me back then, producing an array of a list of tuples of different sizes.
lambda *weights: cost(*weights)[self.indices[0][0], self.indices[1][0]] | ||
+ cost(*weights)[self.indices[0][1], self.indices[1][1]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dwierichs, this looks great to me! Thank you for clarifying some of my questions from earlier
@@ -376,11 +378,12 @@ def tensordot(tensor1, tensor2, axes=None): | |||
Returns: | |||
tensor_like: the tensor product of the two input tensors | |||
""" | |||
interface = _multi_dispatch([tensor1, tensor2]) | |||
return np.tensordot(tensor1, tensor2, axes=axes, like=interface) | |||
tensor1, tensor2 = np.coerce([tensor1, tensor2], like=like) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is minor, but do you think we should include coerce
inside the decorator? Or is the required behaviour to specific to different functions?
The reason I suggest this is we could simplify even more, and just do
tensordot = multi_dispatch(argnum=[0, 1])(np.tensordot)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about this as well 😆 Maybe as an optional kwarg?
tensordot = multi_dispatch(argnum=[0, 1], do_coerce=True)(np.tensordot)
There are multiple functions to which this would apply, e.g. block_diag
, stack
, tensordot
. For other functions, it would just safe a line in the patched function, e.g. in frobenius_inner_product
and dot
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it 😎 Although, I might even just have coerce=True
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed off-PR: Including this in multi_dispatch
is less straight-forward than previously thought, due to unpacking and re-packing of arguments in that are sequences of tensors. Therefore it will not be included.
Context:
This PR makes use of the
multi_dispatch
decorator that was introduced in #2084 throughoutpennylane/math/multi_dispatch.py
. It also introduces tests of differentiability in all interfaces forqml.math.tensordot
, as well as a minimal test oftensordot
with Sequences as inputs.#1865 was partially closed by #1725, but a
qml.math.coerce
is introduced here totensordot
to complete this patch oftensordot
.Description of the Change:
Some more tests; Usage of
multi_dispatch
.Benefits:
Concise codebase and usage examples of the new decorator; Test coverage
Possible Drawbacks:
Tests take longer.
Performance might be reduced minimally by using a higher-level object (the decorator) rather than hard-coding the usage of
_multi_dispatch
to determine the interface.Related GitHub Issues:
closes #1865