Skip to content
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

Add __add__ to Wires class. #812

Merged
merged 7 commits into from
Sep 21, 2020
Merged

Conversation

mariaschuld
Copy link
Contributor

@cgogolin had the great request to add _add__ and __radd__ to the wires class as a shortcut to all_wires():

wires1 = Wires([2, 1, 3])
wires2 = Wires([1, 4, 5, 2])
wires3 = Wires([6, 5])

print(wires1 + wires2 + wires3) # <Wires = [2, 1, 3, 4, 5, 6]>

This PR adds the two functions and two tests.

Drawbacks:

Unfortunately, this does not allow native use of sum([wires1, wires2, wires3]), which would be quite elegant in our code base. sum requires a start object which is 0 by default, so the 0 wire would always be added.
One could pass the start object explicitly and use sum([wires1 + wires2 + wires3], Wires([])) which produces the same result, but is arguably more confusing than all_wires([wires1 + wires2 + wires3]).

@@ -161,7 +192,7 @@ def indices(self, wires):
"""
Return the indices of the wires in this Wires object.

For example,
**Example:**

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Snuck those in to match the correct version of the new functions.

@codecov
Copy link

codecov bot commented Sep 20, 2020

Codecov Report

Merging #812 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #812   +/-   ##
=======================================
  Coverage   90.56%   90.57%           
=======================================
  Files         126      126           
  Lines        8213     8219    +6     
=======================================
+ Hits         7438     7444    +6     
  Misses        775      775           
Impacted Files Coverage Δ
pennylane/wires.py 98.41% <100.00%> (+0.07%) ⬆️

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 3dfd0fe...adf4206. Read the comment docs.

Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mariaschuld! This is great. Don't forget to update the changelog as well.

By the way, I'm somewhat glad that sum([wires1, wires2, wires3]) isn't natively supported. To me, this isn't very intuitive --- on first read, I'm not sure what it would do, sum the wire labels?

I guess this is because:

  • I'm so used to NumPy, where sum and concatenate are two separate operations

  • sum doesn't concatenate Python lists, either (it only acts on a single list).

def __add__(self, other):
"""Defines the addition to return a Wires object containing all wires of the two terms.

**Example:**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
**Example:**
**Example**

😆

With Args: and Kwargs:, Sphinx now automatically removes the colon in version 2.0+ (in version 1.X, it didn't).

Also, it might be good to move it below the Args: and Returns:, to match other docstrings

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes!

@mariaschuld mariaschuld merged commit aa2e495 into master Sep 21, 2020
@mariaschuld mariaschuld deleted the add_wires_addition_and_hash branch September 21, 2020 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants