Generalize python-decompositions#2983
Conversation
kipawaa
force-pushed
the
decomp-interface
branch
from
5a65616 to
3812a39
Compare
kipawaa
force-pushed
the
decomp-interface
branch
from
3812a39 to
d46c6af
Compare
dime10
left a comment
dime10
left a comment
There was a problem hiding this comment.
This is great thanks! Moving in a good direction :)
|
Hello. You may have forgotten to update the changelog!
|
kipawaa
changed the title
DecomposableGate interface
maliasadi
left a comment
maliasadi
left a comment
There was a problem hiding this comment.
Happy to see this PR come through 🙌
There was a problem hiding this comment.
We probably need to raise an error to guarantee the biggest constraint with the system which is if any rule for an op is present, all its rules need to be present. in other words, no support for user defined rules via add_decomps with python decomps.
There was a problem hiding this comment.
I think that it will be more appropriate to try to assert this once we have implemented the auto-tracing of decomposition rules with operators which will be future work for Operator2. Does that seem reasonable?
| def circuit(): | ||
| paulirot_subroutine(theta, wires) | ||
| for subroutine in subroutines: | ||
| subroutine(*[0.5 for _ in dynamic_shape], wires=wires) |
There was a problem hiding this comment.
Pls add a TODO for 0.5. It's probably safe for all present ops/rules, but let's bring this up in the Op2 sync meeting to double-check with the rest of the team.
There was a problem hiding this comment.
This is a placeholder for now, but I definitely agree that we should check in about this and what exactly might come through as dynamic data. I'll add the TODO and be sure to discuss this
| ]; | ||
| } | ||
|
|
||
| def DecomposableGate : OpInterface<"DecomposableGate", [QuantumGate]> { |
There was a problem hiding this comment.
Is it also meant to eventually replace the special-casing in getRuleNodes or basically name-handling (gphase -> GlobalPhase, which already has a "TODO: replace this with DecomposableGate interface")? If so, sketching that end state now would clarify whether the current method set is sufficient for use cases.
There was a problem hiding this comment.
Definitely! My end goal is that any op that wants to participate in graph decomp should implement this interface, so that we can standardize how we identify which ops need decomposition and how to gather data from them
There was a problem hiding this comment.
Just to have all of the information in one place, we'll implement this interface for other ops in follow-up PRs. Once all of the core operations have implemented the interface, we can update the graph-decomposition pass to support the interface (and only the interface). I've improved the explanation of this in e3e6736
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2983 +/- ##
==========================================
+ Coverage 96.98% 96.99% +0.01%
==========================================
Files 167 167
Lines 19390 19441 +51
Branches 1813 1817 +4
==========================================
+ Hits 18805 18857 +52
+ Misses 431 430 -1
Partials 154 154 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dime10
left a comment
dime10
left a comment
There was a problem hiding this comment.
Nice work, this is coming together nicely 💯
| return qp.capture.subroutine(decomp_rule) | ||
|
|
||
| # let this fail with the standard error message if the op is not found | ||
| op_class = getattr(qp, op_name) |
There was a problem hiding this comment.
Is there any chance we could process operators that are not in the top-level pennylane namespace?
There was a problem hiding this comment.
Perhaps it's possible to query the decomp rule registry with the name directly instead of the class, that would circumvent this problem (cc @astralcai).
There was a problem hiding this comment.
Good idea, we could probably remove this and just rely on list_decomps(op_name)? @astralcai does this seem like a reasonable approach? Since we intend to let this fail anyways if the op isn't found, that failure can be generated from the frontend graph, which allows a wider set of operators
Co-authored-by: David Ittah <dime10@users.noreply.github.com>
3 participants
Context:
The initial implementation of python-decompositions was specialized to
quantum.paulirot. This was done partly for simplicity, and partly due to a lack of consistency in op interfaces, which would have required bespoke solutions to parse dynamic data, static data etc for each quantum operation.Description of the Change:
Introduces a generic
DecomposableOpInterfaceinterface for interacting with ops in the context of decomposition. This provides consistent methods for accessing dynamic data, static data, op graph IDs and more. This PR introduces the interface as a PoC forquantum.paulirotandquantum.operator, providing tests as a demonstration of the intended functionality. The intended result of this implementation will be to remove any op-specific logic from thegraph-decompositionpass, instead using exclusively methods from the interface to improve generality and improve extensibility to other ops without altering core logic in the graph.Benefits:
Enables consistent interfaces across all op types relevant to decomposition, reduces type-checking boilerplate and code duplication. This allows any op implementing the interface to use python-decompositions, and further simplifies the general graph infrastructure.
Possible Drawbacks:
Related GitHub Issues:
[sc-122013]
[sc-122018]