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

Circuit cutting: add mid-circuit measurement integration tests #2234

Merged
merged 24 commits into from
Feb 28, 2022

Conversation

trbromley
Copy link
Contributor

Context:

Adds tests for the cut_circuit transform that ensures circuits resulting in fragments that contain mid-circuit measurements are supported.

@github-actions
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

Comment on lines +364 to +365
copy_ops = [copy.copy(op) for _, op in ordered_ops if not isinstance(op, MeasurementProcess)]
copy_meas = [copy.copy(op) for _, op in ordered_ops if isinstance(op, MeasurementProcess)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously we were treating the ops (gates) and measurements together. However, when isinstance(op, MeasurementProcess), then setting op._wires = ... does not work when the MeasurementProcess is an expectation value of an observable, because the measurement process uses the contained observable to determine op.wires.

In this updated version, we treat the MeasurementProcesses more carefully after applying the gates.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

I'm curious what where the cases that revealed this? Mid circuit measurements and tensor products of obs in measurements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, previously if your observable (or part of it) was on wire i, but wire i had mid-circuit measurements and was relabelled to wire i', then the observable wasn't getting its wire updated from wire i to i'.

pennylane/transforms/qcut.py Show resolved Hide resolved
@@ -424,7 +441,7 @@ def _get_measurements(

obs = measurement.obs

return [expval(obs @ g) for g in group]
return [expval(copy.deepcopy(obs) @ g) for g in group]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am very confused by this. the tensor product works in-place 🤔 Perhaps it is a queuing-related quirk.

obs = qml.PauliZ(0) @ qml.PauliZ(1)
obs2 = obs @ qml.PauliZ(2)
assert obs == obs2
print(obs)

Copy link
Contributor

@anthayes92 anthayes92 Feb 26, 2022

Choose a reason for hiding this comment

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

I see, so the original obs is being overwritten in when we take a tensor product of it with another tensor. This is weird!

But deepcopy seems to solve for our needs, or is there potential for more problems with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also works with copy.copy so I changed to that. I think copy.copy is a sufficient workaround for now, but long run it'd be better for observables not to be altered when taking the tensor product. Issue posted: #2235

@codecov
Copy link

codecov bot commented Feb 25, 2022

Codecov Report

Merging #2234 (bb99ee3) into master (7ad040b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2234   +/-   ##
=======================================
  Coverage   99.26%   99.27%           
=======================================
  Files         231      231           
  Lines       18349    18359   +10     
=======================================
+ Hits        18215    18225   +10     
  Misses        134      134           
Impacted Files Coverage Δ
pennylane/transforms/qcut.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 7ad040b...bb99ee3. Read the comment docs.

@trbromley
Copy link
Contributor Author

[sc-14724]

Copy link
Contributor

@anthayes92 anthayes92 left a comment

Choose a reason for hiding this comment

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

Great spots here! The pipeline is becoming robust 🦾

Would be interested to find out whats going on with the tensor product quirk you've found 🤔

qml.CNOT(wires=[0, 1])
return qml.expval(qml.PauliZ(0) @ qml.PauliZ(1))

spy = mocker.spy(qcut, "qcut_processing_fn")
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps it would be more appropriate to spy on _get_measurements in this test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to change, but why do you think _get_measurements is more appropriate?

@@ -424,7 +441,7 @@ def _get_measurements(

obs = measurement.obs

return [expval(obs @ g) for g in group]
return [expval(copy.deepcopy(obs) @ g) for g in group]
Copy link
Contributor

@anthayes92 anthayes92 Feb 26, 2022

Choose a reason for hiding this comment

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

I see, so the original obs is being overwritten in when we take a tensor product of it with another tensor. This is weird!

But deepcopy seems to solve for our needs, or is there potential for more problems with this?

pennylane/transforms/qcut.py Show resolved Hide resolved
Comment on lines +364 to +365
copy_ops = [copy.copy(op) for _, op in ordered_ops if not isinstance(op, MeasurementProcess)]
copy_meas = [copy.copy(op) for _, op in ordered_ops if isinstance(op, MeasurementProcess)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

I'm curious what where the cases that revealed this? Mid circuit measurements and tensor products of obs in measurements?

Copy link
Contributor Author

@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!

Comment on lines +364 to +365
copy_ops = [copy.copy(op) for _, op in ordered_ops if not isinstance(op, MeasurementProcess)]
copy_meas = [copy.copy(op) for _, op in ordered_ops if isinstance(op, MeasurementProcess)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, previously if your observable (or part of it) was on wire i, but wire i had mid-circuit measurements and was relabelled to wire i', then the observable wasn't getting its wire updated from wire i to i'.

@@ -424,7 +441,7 @@ def _get_measurements(

obs = measurement.obs

return [expval(obs @ g) for g in group]
return [expval(copy.deepcopy(obs) @ g) for g in group]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also works with copy.copy so I changed to that. I think copy.copy is a sufficient workaround for now, but long run it'd be better for observables not to be altered when taking the tensor product. Issue posted: #2235

qml.CNOT(wires=[0, 1])
return qml.expval(qml.PauliZ(0) @ qml.PauliZ(1))

spy = mocker.spy(qcut, "qcut_processing_fn")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to change, but why do you think _get_measurements is more appropriate?

Base automatically changed from qcut_integ_tests to master February 28, 2022 14:02
@trbromley trbromley merged commit 57237b2 into master Feb 28, 2022
@trbromley trbromley deleted the qcut_integ_tests_2 branch February 28, 2022 20:06
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.

2 participants