-
Notifications
You must be signed in to change notification settings - Fork 632
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
Raise an error for MCMs nested in Conditionals #7027
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
…ne into validate_mcm_in_cond
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7027 +/- ##
=======================================
Coverage 99.60% 99.60%
=======================================
Files 488 488
Lines 46807 46818 +11
=======================================
+ Hits 46622 46634 +12
+ Misses 185 184 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lillian542
If the answer is no to my first question below, then no concerns with the rest, and will be fine to approve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the measure is in a cond
branch that has other operators around it, will this still detect it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing my review based on @mlxd 's observations
Co-authored-by: Andrija Paurevic <[email protected]>
@andrijapau yes! On the tape we have a series of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a bunch @lillian542
One suggestion, but happy to approve otherwise.
Co-authored-by: Lee James O'Riordan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @lillian542
Context:
The current methods for of mid-circuit measurement processing don't support applying a mid-circuit measurement conditionally. This was not within the scope of the implementation for any of these methods, so we don't expect it to work - however, we would prefer to return clear error messages, and we don't want to be returning incorrect results.
Currently,
qml.cond(m, qml.measure)(wire)
either raises a difficult to parse error (in the case of deferred measurements and analytical execution), or returns incorrect results (in the case oftree-traversal
,one-shot
, and deferred measurements withshots
).Description of the Change:
We add validation in the
midcircuit_measurements
transform, checking for conditional mcms and raising an error. Specificmcm_method
s can be made to pass this validation check in the future if support for conditional mcms are added to the method; for now, we disallow it completely.Benefits:
We don't return incorrect results or raise cryptic errors.
Possible Drawbacks:
It adds one more iteration over the operations for validation 🤷♀️
Related GitHub Issues:
None, but sc stories [sc-84177] and [sc-84178]