-
Notifications
You must be signed in to change notification settings - Fork 591
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
Fix bug where unexpected queuing occurs in qml.ctrl
among other functions
#6284
base: master
Are you sure you want to change the base?
Conversation
Hello. You may have forgotten to update the changelog!
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6284 +/- ##
=======================================
Coverage 99.70% 99.70%
=======================================
Files 444 444
Lines 42133 42151 +18
=======================================
+ Hits 42008 42026 +18
Misses 125 125 ☔ View full report in Codecov by Sentry. |
qml.ctrl
among other functionsqml.ctrl
among other functions
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.
qml.cond
also contains similar logic. Can you also update that to use this recursive method?
@@ -199,11 +199,29 @@ def create_controlled_op(op, control, control_values=None, work_wires=None): | |||
return _ctrl_transform(op, control, control_values, work_wires) | |||
|
|||
|
|||
def remove_from_queue_args_and_kwargs(item): |
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.
Maybe move this helper method to the tape
module and just call it something like recursively_remove_operators_from_queue
.
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.
Sounds good, is okey in tape/operation_recorder.py
?
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.
Might be more suitable in pennylane/queueing.py
actually.
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.
We can use pytrees and simply do:
leaves, _ = qml.pytrees.flatten((args, kwargs), lambda obj: isinstance(obj, Operator))
_ = [qml.QueuingManager.remove(l) for l in leaves if isinstance(l, Operator)]
Given this is just two lines, I'd be worried that adding a public function adds more complexity and coupling that it solves. Duplicate code is not a bad thing when it helps keep modules simple and loosely coupled.
I'd prefer to just copy those two lines into adjoint
, controlled
, and TransformDispatcher._qfunc_transform
.
I tried this but it seems |
This PR seeks to fix this bug.
The issue was that unwanted operators were being added when applying the
qml.ctrl
,qml.prod
,qml.cond
andqml.adjoint
functions if they were located as arguments or kwargs.To solve it I do a recursive search, for operators inside args and kwargs and I remove them.
Drawback:
This will delete it always. I can't think of a case where we want to add an operator to the queue by passing it as an argument.