added multi_dispatch decorator #2084

Qottmann · 2022-01-12T15:58:07Z

Context:
Added multi_dispatch decorator to help make new function definitions easier in terms of variable interfaces.

Description of the Change:
Added the function multi_dispatch in multi_dispatch.py
Note that this is a reopening of #2082 after encountering some git issues.

Benefits:
Handles arguments for new functions that need to be dispatched for an appropriate interface. Makes interface argument "like" optional as it utilizes _multi_dispatch to automatically recognize the interface based on the tensor objects.

Possible Drawbacks:
Decorators are confusing to some users.

Related GitHub Issues:
closes #1820

Qottmann · 2022-01-12T16:06:32Z

I noticed that in its current state, when I clone the original pennylane repo, install and just run black -l 100 pennylane/ it reformats 31 files. I dont know if this is intended, but to avoid having me submit 31 uninteded changes I just reformatted with black the files I have been working on.

josh146 · 2022-01-12T17:21:28Z

Thanks @Qottmann, the new PR diff looks good :) If you are happy with it, I can begin a review.

I noticed that in its current state, when I clone the original pennylane repo, install and just run black -l 100 pennylane/ it reformats 31 files.

🤔 This makes me wonder if this is due to a version incompatibility. What version of Black do you currently have installed? Locally, I have

$ black --version
black, 21.12b0 (compiled: no)
$ black -l 100 tests pennylane
All done! ✨ 🍰 ✨
431 files left unchanged.

Perhaps this may be solved by upgrading your Black version?

codecov · 2022-01-12T17:42:50Z

Codecov Report

Merging #2084 (ba7cb0d) into master (1275736) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2084   +/-   ##
=======================================
  Coverage   99.18%   99.18%           
=======================================
  Files         226      226           
  Lines       17373    17396   +23     
=======================================
+ Hits        17232    17255   +23     
  Misses        141      141

Impacted Files	Coverage Δ
pennylane/math/__init__.py	`100.00% <ø> (ø)`
pennylane/math/multi_dispatch.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1275736...ba7cb0d. Read the comment docs.

Qottmann · 2022-01-12T18:37:45Z

Indeed, I had an old version of black

$ black --version
black, version 19.10b0

With pip install --upgrade black I am now on

$ black --version
black, 21.12b0 (compiled: no)

If it is okay, I added black >= 21 in requirements.txt

From my side, it is ready for review then :)

josh146 · 2022-01-13T07:39:42Z

doc/requirements.txt

@@ -21,3 +21,4 @@ tensornetwork==0.3
 toml
 torch==1.9.0+cpu
 torchvision==0.10.0+cpu
+black>=21


@Qottmann it's a good idea to include this! Better though to place this in the requirements.txt file, as this one seems to break the documentation from building 🤔 I will remove it for now, just so that the documentation build correctly.

Suggested change

black>=21

Ah right! I added it to this requirement.txt. The remaining error: do I understand it right, that it is complaining that the case that argnum and tensor_list are given as integers, is not tested? I can modify the test to include those tests then!

josh146

Thanks @Qottmann! This is a great contribution, and will make adding framework-agnostic functions much easier to PennyLane going forward.

I've gone through and left some comments and suggestions, but it is looking in great shape --- I think only more 'administrative' work is required to get this merge ready, including fixing up the docstring and adding a few more tests.

do I understand it right, that it is complaining that the case that argnum and tensor_list are given as integers, is not tested? I can modify the test to include those tests then!

Yes, that's correct - we have a coverage check that verifies that all new code is adequately tested 🙂 While your current test is testing part of the new logic, it is likely not testing all logical paths and all input types.

pennylane/math/multi_dispatch.py

josh146 · 2022-01-13T07:56:13Z

doc/releases/changelog-dev.md

+* Added `multi_dispatch` decorator that helps ease the definition of new functions.
+  [(#2082)](https://github.com/PennyLaneAI/pennylane/pull/2084)


It would be good here to add a short description and code example here to help explain to readers the importance/impact of this change 🙂

The reason that this tends to be important is that the readers of the changelog tend to be a very specific demographic --- it includes both regular users, who are likely to be more aware of changes occuring, but also non-regular users who maybe only interact with PennyLane once and a while by reading the changelog.

So when writing the changelog entries, we often try to think --- 'how can we communicate this change to a skim-reader in a succinct manner?'

Note that this approach is a bit unique to PL; quite a few OSS projects tend to have simpler changelogs! The reason we take this approach is that we often use our changelog to advertise the new releases.

In this case, you could likely copy and adapt the intro to the multi_dipatch docstring

josh146 · 2022-01-13T07:59:57Z

pennylane/math/multi_dispatch.py

+def multi_dispatch(argnum=None, tensor_list=None):
+    """Decorater to dispatch arguments handled by the interface.


Here is a link to the rendered version: https://pennylane--2084.org.readthedocs.build/en/2084/code/api/pennylane.math.multi_dispatch.html It looks really nice!

pennylane/math/multi_dispatch.py

josh146 · 2022-01-13T11:59:46Z

tests/math/test_multi_disptach.py

+    ([1.0, 0.0], [2.0, 3.0]),
+    onp.array([[1.0, 0.0], [2.0, 3.0]]),
+    np.array([[1.0, 0.0], [2.0, 3.0]]),
+    # torch.tensor([[1.,0.],[2.,3.]]),


How come this one is commented out?

tests/math/test_multi_disptach.py

Co-authored-by: Josh Izaac <josh146@gmail.com>

Qottmann · 2022-01-13T19:16:23Z

@josh146 I committed your suggestions and

expanded tests in tests/math/test_multi_dispatch.py
added an example of a custom function decorated by multi_dispatch() in the docstring of multi_dispatch. Is this in line with what you had in mind?

…tion to complete tests

josh146

Thanks @Qottmann! Very nice documentation, and great to see the additional tests. I have spotted a potential bug in the implementation, but I believe you might have also noticed it, as I just saw a GitHub notification :)

doc/releases/changelog-dev.md

pennylane/math/multi_dispatch.py

josh146 · 2022-01-14T07:46:08Z

pennylane/math/multi_dispatch.py

+    >>> @math.multi_dispatch(argnum=0,tensor_list=0)
+    >>> def custom_function(values, like, coefficient=10):
+    >>>     # values is a list of vectors
+    >>>     # like can force the interface (optional)
+    >>>     return coefficient * np.sum([math.dot(v,v) for v in values])


Suggested change

>>> @math.multi_dispatch(argnum=0,tensor_list=0)

>>> def custom_function(values, like, coefficient=10):

>>> # values is a list of vectors

>>> # like can force the interface (optional)

>>> return coefficient * np.sum([math.dot(v,v) for v in values])

>>> @qml.math.multi_dispatch(argnum=0, tensor_list=0)

... def custom_function(values, like, coefficient=10):

... # values is a list of vectors

... # like can force the interface (optional)

... return coefficient * np.sum([math.dot(v, v) for v in values])

In Python console ('pycon') syntax,

>>> prepends a new command

... indicates a continuation line of a command

output is not prepended by anything

It might be nice to include in the example an example of branching based on like, e.g.,

@qml.math.multi_dispatch(argnum=0, tensor_list=0) def custom_function(values, like, coefficient=10): squares = sum([qml.math.dot(v, v) for v in values]) if like == "tensorflow": ... return 1j * coefficient * squares

just to show what it looks like for developers. Unfortunately, I cannot think of an example that doesn't look very contrived!

Thanks for the comments and corrections!

Unfortunately, I cannot think of an example that doesn't look very contrived!

I also dont know of an example on top of my head. In math.dot() there are exceptions for torch and tensorflow but I think this example may be a bit too convoluted to put there. For now, I just left it with an empty if clause and a comment that this is a possibility in the doc-string.

Yes, I also had a look at existing functions to see if there were any that would be nice to use as an example, but all were either too complicated or too specific 😆

pennylane/math/multi_dispatch.py

josh146 · 2022-01-14T10:48:22Z

pennylane/math/multi_dispatch.py

+    def decorator(fn):
+        @functools.wraps(fn)
+        def wrapper(*args, **kwargs):
+            argnums = argnum or list(range(len(args)))


@Qottmann I think there might be a bug here (and as well with tensor_lists below!) The issue being that argnum=0 and tensor_list=0 are both valid inputs, but python treats 0 as False, and so the boolean statement will choose the latter argument in both cases!

This is a sign that more tests should be added, to ensure these edge cases work as expected.

josh146 · 2022-01-14T10:49:45Z

tests/math/test_multi_disptach.py

+    def tensordot(x, like, axes=None):
+        return np.tensordot(x[0], x[1], axes=axes)
+
+    assert fn.allequal(tensordot(x, axes=(0, 0)).numpy(), 2)


josh146 · 2022-01-14T10:50:15Z

tests/math/test_multi_disptach.py

+def test_multi_dispatch_decorate2(t1, t2):
+    """Test decorating a standard numpy function for PennyLane, this time using 2 separate inputs"""
+
+    @fn.multi_dispatch(argnum=[0, 1], tensor_list=None)


A good idea to test different values of argnum and tensor_list :) It might be good to also have a test that doesn't set either of these?

Indeed, this is now done in test_multi_dispatch_decorate2 !

Co-authored-by: Josh Izaac <josh146@gmail.com>

doc/releases/changelog-dev.md

pennylane/math/multi_dispatch.py

josh146 · 2022-01-16T16:58:40Z

pennylane/math/multi_dispatch.py

+            if not isinstance(argnums, Sequence):
+                argnums = [argnums]
+            if not isinstance(tensor_lists, Sequence):
+                tensor_lists = [tensor_lists]


tests/math/test_multi_disptach.py

josh146 · 2022-01-16T17:01:54Z

pennylane/math/multi_dispatch.py

+    >>> @math.multi_dispatch(argnum=0,tensor_list=0)
+    >>> def custom_function(values, like, coefficient=10):
+    >>>     # values is a list of vectors
+    >>>     # like can force the interface (optional)
+    >>>     return coefficient * np.sum([math.dot(v,v) for v in values])


Yes, I also had a look at existing functions to see if there were any that would be nice to use as an example, but all were either too complicated or too specific 😆

josh146

Thanks @Qottmann for fixing that final bug, this looks ready from my end now for merging in 💯 💪

Co-authored-by: Josh Izaac <josh146@gmail.com>

Qottmann · 2022-01-17T11:31:11Z

Great! Also from my end it is good to go. I committed your suggestions and all tests seem to pass :)

Qottmann added 3 commits January 12, 2022 16:53

added multi_dispatch decorator

a1e2ea6

added release notes and ran black on only my files

31661c1

typo in PR number, now 2084

96ff418

Qottmann added 2 commits January 12, 2022 19:28

updated black, reran on test_multi_dispatch

c606d34

added black version requirement, optional commit

790e52a

josh146 reviewed Jan 13, 2022

View reviewed changes

josh146 and others added 2 commits January 13, 2022 15:39

Update doc/requirements.txt

c864c48

moved black>=21 requirement out of docs to /requirements.txt

cc7fc37

josh146 requested changes Jan 13, 2022

View reviewed changes

Qottmann and others added 8 commits January 13, 2022 16:24

Update tests/math/test_multi_disptach.py

b4a6e8c

Co-authored-by: Josh Izaac <josh146@gmail.com>

Update pennylane/math/multi_dispatch.py

a25e9b5

Co-authored-by: Josh Izaac <josh146@gmail.com>

Update pennylane/math/multi_dispatch.py

7c97b7f

Co-authored-by: Josh Izaac <josh146@gmail.com>

Update pennylane/math/multi_dispatch.py

ba212d4

Co-authored-by: Josh Izaac <josh146@gmail.com>

Update pennylane/math/multi_dispatch.py

bd3b15e

Co-authored-by: Josh Izaac <josh146@gmail.com>

Update pennylane/math/multi_dispatch.py

ac9a82e

Co-authored-by: Josh Izaac <josh146@gmail.com>

added example in multi_dispatch and expanded tests

48b1834

merged conflict in changelog-dev

9da9a98

changed argument handling to allow for argnum=0 and added custom func…

5907880

…tion to complete tests

josh146 reviewed Jan 14, 2022

View reviewed changes

Qottmann and others added 6 commits January 14, 2022 15:32

Update pennylane/math/multi_dispatch.py

e8cf981

Co-authored-by: Josh Izaac <josh146@gmail.com>

Update doc/releases/changelog-dev.md

a945579

Co-authored-by: Josh Izaac <josh146@gmail.com>

Update doc/releases/changelog-dev.md

a620e13

Co-authored-by: Josh Izaac <josh146@gmail.com>

Update pennylane/math/multi_dispatch.py

65110b1

Co-authored-by: Josh Izaac <josh146@gmail.com>

Update pennylane/math/multi_dispatch.py

421eb7c

Co-authored-by: Josh Izaac <josh146@gmail.com>

small review

3bdf5a7

josh146 reviewed Jan 16, 2022

View reviewed changes

josh146 approved these changes Jan 16, 2022

View reviewed changes

josh146 and others added 6 commits January 17, 2022 01:02

Merge branch 'master' into master

c0f0a8e

Update doc/releases/changelog-dev.md

3ff2742

Co-authored-by: Josh Izaac <josh146@gmail.com>

Update pennylane/math/multi_dispatch.py

7df030b

Co-authored-by: Josh Izaac <josh146@gmail.com>

Update pennylane/math/multi_dispatch.py

cf81b3e

Co-authored-by: Josh Izaac <josh146@gmail.com>

Update tests/math/test_multi_disptach.py

fb79fbe

Co-authored-by: Josh Izaac <josh146@gmail.com>

Update tests/math/test_multi_disptach.py

ba7cb0d

Co-authored-by: Josh Izaac <josh146@gmail.com>

josh146 merged commit 6e817db into PennyLaneAI:master Jan 17, 2022

dwierichs mentioned this pull request Jan 18, 2022

Use the new multi_dispatch decorator in the math module. Add tensordot tests. #2096

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added multi_dispatch decorator #2084

added multi_dispatch decorator #2084

Qottmann commented Jan 12, 2022 •

edited by josh146

Loading

Qottmann commented Jan 12, 2022

josh146 commented Jan 12, 2022 •

edited

Loading

codecov bot commented Jan 12, 2022 •

edited

Loading

Qottmann commented Jan 12, 2022

josh146 Jan 13, 2022

Qottmann Jan 13, 2022

josh146 left a comment

josh146 Jan 13, 2022

josh146 Jan 13, 2022

josh146 Jan 13, 2022

josh146 Jan 13, 2022

Qottmann commented Jan 13, 2022

josh146 left a comment

josh146 Jan 14, 2022

josh146 Jan 14, 2022

Qottmann Jan 14, 2022

josh146 Jan 16, 2022

josh146 Jan 14, 2022

josh146 Jan 14, 2022

josh146 Jan 14, 2022

Qottmann Jan 14, 2022

josh146 Jan 16, 2022

josh146 Jan 16, 2022

josh146 left a comment

Qottmann commented Jan 17, 2022

		* Added `multi_dispatch` decorator that helps ease the definition of new functions.
		[(#2082)](https://github.com/PennyLaneAI/pennylane/pull/2084)

		def multi_dispatch(argnum=None, tensor_list=None):
		"""Decorater to dispatch arguments handled by the interface.

added multi_dispatch decorator #2084

added multi_dispatch decorator #2084

Conversation

Qottmann commented Jan 12, 2022 • edited by josh146 Loading

Qottmann commented Jan 12, 2022

josh146 commented Jan 12, 2022 • edited Loading

codecov bot commented Jan 12, 2022 • edited Loading

Codecov Report

Qottmann commented Jan 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josh146 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Qottmann commented Jan 13, 2022

josh146 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josh146 left a comment

Choose a reason for hiding this comment

Qottmann commented Jan 17, 2022

Qottmann commented Jan 12, 2022 •

edited by josh146

Loading

josh146 commented Jan 12, 2022 •

edited

Loading

codecov bot commented Jan 12, 2022 •

edited

Loading