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

Integration testing for the cut_circuit_mc transform #2407

Merged
merged 75 commits into from
Apr 8, 2022

Conversation

anthayes92
Copy link
Contributor

Context:
Integration testing for the cut_circuit_mc transform.

Description of the Change:
Additional unit tests for mid circuit measurements, circuits with disconnected components, circuits with trivial wire cuts, and
circuits with complicated cuts

anthayes92 and others added 30 commits March 11, 2022 18:51
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
…ithub.com:PennyLaneAI/pennylane into qcut-sample-subgraphs-to-fragment-tape-conversion
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Apr 6, 2022

Codecov Report

Merging #2407 (1ffe8da) into master (e8e6faa) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2407   +/-   ##
=======================================
  Coverage   99.45%   99.45%           
=======================================
  Files         244      244           
  Lines       18978    18978           
=======================================
  Hits        18874    18874           
  Misses        104      104           
Impacted Files Coverage Δ
pennylane/transforms/qcut.py 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 e8e6faa...1ffe8da. Read the comment docs.

@anthayes92 anthayes92 marked this pull request as ready for review April 6, 2022 18:57
@anthayes92
Copy link
Contributor Author

[sc-16860]

Base automatically changed from qcut-sampling-template-expansion to master April 6, 2022 21:33
Copy link
Contributor

@trbromley trbromley left a comment

Choose a reason for hiding this comment

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

Thanks @anthayes92! Approved, with a few suggestions 🟢

tests/transforms/test_qcut.py Outdated Show resolved Hide resolved
tests/transforms/test_qcut.py Outdated Show resolved Hide resolved
shots = 10
dev = qml.device("default.qubit", wires=2, shots=shots)

@qml.cut_circuit_mc(classical_processing_fn=fn)
Copy link
Contributor

Choose a reason for hiding this comment

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

How could we make it so that @qml.cut_circuit_mc(fn) is valid?
Perhaps we could make classical_processing_fn and max_depth (the two arguments we expect users to maybe set at QNode level) be the first and second arguments (after tape), respectively.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to make sure I've understood, do you mean like this?:

def cut_circuit_mc(
    tape: QuantumTape,
    classical_processing_fn: Optional[callable], 
    max_depth: int,
    shots: Optional[int] = None,
    device_wires: Optional[Wires] = None,
) 

Copy link
Contributor Author

@anthayes92 anthayes92 Apr 8, 2022

Choose a reason for hiding this comment

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

I've made the change here a1adee8 however, I'm not totally conviced it's doing what we want. When passing a classical processing function it's happy, but if no function is passed an error is given e.g:

@qml.cut_circuit_mc(fn, 1)

works well, but

@qml.cut_circuit_mc(None, 1)

causes an error in _cut_circuit_mc_expand() which has also been updated to accommodate the new order. In the tests this is solved by using

@qml.cut_circuit_mc(classical_processing_fn=None, max_depth=1)

which actually seems worse than the original.

If we want to keep these changes I'll update the docstrings. But maybe there's something I missed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @anthayes92! Left a comment here.

tests/transforms/test_qcut.py Outdated Show resolved Hide resolved
tests/transforms/test_qcut.py Outdated Show resolved Hide resolved
tests/transforms/test_qcut.py Outdated Show resolved Hide resolved
tests/transforms/test_qcut.py Show resolved Hide resolved
tests/transforms/test_qcut.py Outdated Show resolved Hide resolved
anthayes92 and others added 9 commits April 7, 2022 16:47
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
…/pennylane into qcut-extend-mc-cutting-tests
* Add summary of sample-based cutting additions to changelog, add example

* remove draw and draw_mpl from transforms __init__

* add draw imports back to trasnforms __init__ file

* Update

* Increase shot number

* Apply suggestions from code review

Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>

* fix indentation

Co-authored-by: trbromley <brotho02@gmail.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
@anthayes92 anthayes92 merged commit 0b055a5 into master Apr 8, 2022
@anthayes92 anthayes92 deleted the qcut-extend-mc-cutting-tests branch April 8, 2022 23:39
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