-
Notifications
You must be signed in to change notification settings - Fork 57
Fix qml.prod with autograph #2083
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2083 +/- ##
=======================================
Coverage 97.48% 97.49%
=======================================
Files 91 91
Lines 10594 10601 +7
Branches 990 992 +2
=======================================
+ Hits 10328 10335 +7
Misses 211 211
Partials 55 55 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hello. You may have forgotten to update the changelog!
|
_known_wrapper_functions = ( | ||
catalyst.adjoint, | ||
qml.adjoint, | ||
qml.prod, |
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.
Just wondering, are any of the changes besides this line relevant, or just refactoring?
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.
Yes, it's because I use it to find the decorator and apply it again to the converted call of the fn.__wrapped
.
Actually, there is another way to achieve the same behaviour is through fn.__globals__
(as shown below). It doesn't need to create the _known_wrapper_functions
, and also doesn't need to find which decorator need to be applied as well. With that being said, it didn't seem robust enough for me, so I didn't use this solution 🤔. But if you find this solution is better than the previous one, I'm happy to update it 👍
decorator_name = fn.__module__.split(".")[-1]
decorator = fn.__globals__.get(decorator_name)
catalyst/frontend/catalyst/autograph/ag_primitives.py
Lines 611 to 613 in 67505c5
if decorator := next( | |
(f for f in _known_wrapper_functions if f.__module__ == fn.__module__), None | |
): |
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.
Yes, it's because I use it to find the decorator and apply it again to the converted call of the fn.__wrapped
Right, I am wondering about the robustness, but maybe going via wrapped is actually the best way here? Not sure.
Normally, we grab the decorated function here since it's the first argument to the decorator.
The exception to this is if the decorator was already applied, outside the qjit scope, in which case it's too late to hit this branch. For that we use the CatalystCallable
, or handle the QNode
specially. I guess the difference with qml.prod
and the other qml decorators here is that ctrl and adjoint are dispatched to their catalyst equivalent, so can be handled via CatalystCallable, whereas qml.prod
is not dispatched 🤔
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.
I guess the difference with qml.prod and the other qml decorators here is that ctrl and adjoint are dispatched to their catalyst equivalent, so can be handled via CatalystCallable, whereas qml.prod is not dispatched
Yes, exactly, and prod
aslo doesn't has the catalyst equivalent impl as well.
Context:
Fix qml.prod with autograph
Error message:
Description of the Change:
Handle calls to functions that were decorated with qml.prod, qml.adjoint, etc. These decorators return wrapper functions that call the original function without autograph conversion. We detect these wrappers and unwrap them to convert the original function with autograph.
Benefits:
Possible Drawbacks:
Related GitHub Issues:
[sc-95653]
#1911