-
Notifications
You must be signed in to change notification settings - Fork 46
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
[WIP] Allow catalyst.cond
to take in branch functions with arguments
#1531
base: main
Are you sure you want to change the base?
Conversation
Thanks @paul0403 for the quick turnaround. Just some stuff that was brought up: let's hold off on merging this for now, as we have a few things to resolve elsewhere in the ecosystem first, such as:
We can keep the PR, but maybe only in a WIP state. |
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.
As commented above
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1531 +/- ##
==========================================
- Coverage 95.99% 95.96% -0.03%
==========================================
Files 80 80
Lines 8411 8411
Branches 780 780
==========================================
- Hits 8074 8072 -2
- Misses 281 282 +1
- Partials 56 57 +1 ☔ View full report in Codecov by Sentry. |
catalyst.cond
to take in branch functions with argumentscatalyst.cond
to take in branch functions with arguments
Hey @paul0403, yep I have the same questions as Lee, but perhaps one additional one:
We need to also make sure we also update the documentation in this PR, as there are a few places (autograph guide, sharp bits, cond docstring) etc. that does not specify this behaviour or says this is not possible. |
Hi @josh146 , this PR is in essence a hot fix for @mlxd to experiment FTQC on Catalyst. It is not tied to any product feature for now (hence no shortcut story). I can look into those if we decide that this is worth spending time polishing. But just a quick thought, I don't think this breaks anything in Raul's cond capture because this PR is after his capture PR #1468 went in and none of the tests broke. |
I have added a test for PLxPR capture of conditional branches taking arguments: it seems to work out of the box :) So, it does not affect the PLxPR work so far. |
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.
LGTM
Thanks! Does this new test fail without this PR and give the error message "catalyst.cond branch functions cannot have arguments"? Just for my peace of mind |
It does fail: |
Context:
There is a restriction on
catalyst.cond
, that the conditional branch functions can never have arguments. This feature should be allowed, especially as we move towards FTQC.Description of the Change:
The strategy used in #1232 to allow
cond
to take in pennylane gates is extended to apply to all callables with arguments.Benefits:
catalyst.cond
can take in branch functions with arguments, e.g