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

Remove logic which allows wires to be passed to operations as a positional argument (Issue 187) #188

Merged
merged 14 commits into from Apr 18, 2019

Conversation

@riveSunder
Copy link
Contributor

commented Apr 16, 2019

I've removed the ability to pass wires as a positional argument from classes based on the Operation class. To retain the ability to pass wires as a positional argument to expval calls, I've overloaded the __init__ function in the Expectation class. That can't be the prettiest way to do this as the overloaded initialization function is essentially a copy of the old version, maybe there is another way to only restore the offending part that allows wires as a positional argument, while still requiring the keyword elsewhere.

I tested the changes in examples Q4_quantum-GAN.ipynb and CV1_photon-redirection.ipynb, but there is a pre-existing bug in the unit test suite where I added a unit test.

Thanks for looking at my PR!

riveSunder added 4 commits Apr 16, 2019
Remove logic that allows wires to be passed to operations as a positi…
…onal argument (issue #187)

However, now missing the desired ability to pass wires as a positional argument to expectation value functions
restore ability to pass wires as positional argument in expval calls …
…by overloading __init__ in class Expectation

@riveSunder riveSunder changed the title Issue #187: Remove logic which allows wires to be passed to operations as a positional argument Remove logic which allows wires to be passed to operations as a positional argument (Issue 187) Apr 16, 2019

@co9olguy

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

Hi @riveSunder, thanks for the PR!

I notice that there are a number of tests failing with the latest version. Is this a work-in-progress, or ready for attention on our side (perhaps to help with debugging)?

@ghost ghost assigned josh146 Apr 17, 2019

@ghost ghost added the review label Apr 17, 2019

@josh146

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

Hi @riveSunder, looks great!

I've just pushed a small change that merges in the current master branch, and added in the wires keyword argument to the tests where it was missing.

@codecov

This comment has been minimized.

Copy link

commented Apr 17, 2019

Codecov Report

Merging #188 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #188   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          27     27           
  Lines        1666   1671    +5     
=====================================
+ Hits         1666   1671    +5
Impacted Files Coverage Δ
pennylane/operation.py 100% <100%> (ø) ⬆️

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 efe5822...91fcb79. Read the comment docs.

@riveSunder

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

Thanks! I see now that what I couldn't figure yesterday and was keeping me from getting the test to run was the missing [] in the line:
for t in ([TestExpval]):
It seems to be working fine now.

@@ -29,7 +29,7 @@ Available categories include:
* `PennyLane Help <https://discuss.pennylane.ai/c/pennylane-help>`_: For help and advice using PennyLane
* `PennyLane Development <https://discuss.pennylane.ai/c/pennylane-development>`_: For discussion of PennyLane development
* `PennyLane Plugins <https://discuss.pennylane.ai/c/pennylane-plugins>`_: For discussion of the available PennyLane plugins, and plugin development
* `Xanadu Software <https://discuss.pennylane.ai/c/xanadu-software>`_: For discussion relating to other Xanadu software projects, including `StrawberryFields <https://github.com/xanaduai/strawberryfields>`_, `QMLT <https://github.com/xanaduai/hafnian>`_, `Hafnian <https://github.com/xanaduai/hafnian>`_, and `OpenBoson <https://github.com/xanaduai/sfopenboson>`_.
* `Xanadu Software <https://discuss.pennylane.ai/c/xanadu-software>`_: For discussion relating to other Xanadu software projects, including `StrawberryFields <https://github.com/xanaduai/strawberryfields>`_, `QMLT <https://github.com/xanaduai/QMLT>`_, `Hafnian <https://github.com/xanaduai/hafnian>`_, and `OpenBoson <https://github.com/xanaduai/sfopenboson>`_.

This comment has been minimized.

Copy link
@josh146

josh146 Apr 17, 2019

Member

Good catch!

@josh146
Copy link
Member

left a comment

Looks great to me! I was worried that there may be parts of our documentation or notebook downloads that don't specify wires using keyword arguments, but I just went through everything, and it looks like we were pretty consistent with always using wires=.

Thanks @riveSunder!

@quantshah

This comment has been minimized.

Copy link
Collaborator

commented Apr 18, 2019

Is this ready to be merged? @josh146

I think once this is merged, it gets easier to implement the multi-wire expectation as you suggested.

@josh146

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

From my side it is, just waiting to see if @co9olguy would like any changes.

josh146 added 4 commits Apr 18, 2019

@josh146 josh146 merged commit aeeb45e into XanaduAI:master Apr 18, 2019

4 checks passed

CodeFactor No issues found.
Details
Travis CI - Pull Request Build Passed
Details
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to efe5822
Details

@ghost ghost removed the review label Apr 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.