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 :)
| "Return the name of the operation.", "std::string", "getOpName" | ||
| >, | ||
| InterfaceMethod< | ||
| "Return the number of wires the operation acts on.", "size_t", "getNumWires" |
There was a problem hiding this comment.
I guess this won't generalize to multiple wire args 🤔
There was a problem hiding this comment.
Can you explain this? I think that even when an op has multiple wire args, from a decomposition perspective we only need to know the total number of wires to lower the rule/properly assess resources.
There was a problem hiding this comment.
I don't think that's true. If there was an operator that had multiple wire args that could be varied independently, the decomp rule would likely be specific the number of wires for each arg, not the total number (i.e. Op(Wires[3], Wires[2]) and Op(Wires[2], Wires[3]) are most likely different operators for decomposition purposes, or at least we cannot assert that they would always be the same).
The best approach is to simply treat the wires as part of the signature (which they are). The full spec for an operator ID for decomp purposes should look something like this (dyn arg sig, wire sig, static data OR uid). But this is why I think it would be good to align with the Python decomp folks on the exact form of the ID.
| "mlir::DictionaryAttr", "getStaticData" | ||
| >, | ||
| InterfaceMethod< | ||
| "Return the operations ID for use with the graph.", "std::string", "getDecompId" |
There was a problem hiding this comment.
What is the ID? Is it the same as UID or some other concept?
There was a problem hiding this comment.
Oh I see, for paulirot it's currently "paulirot" + getPauliWord(). Yeah I think that's the format that we should standardize on, but as a concept makes sense 👍
Essentially we want a standard recipe to build a unique identifier for every op that enters the graph as a unique node.
There was a problem hiding this comment.
Exactly, it's effectively a generalization of UID that includes static data, since the graph needs to distinguish all instantiations of an operator. I think that this is only relevant to the compiler graph, so standardization should be as simple as requiring the use of the interface when using/checking the decomp IDs!
There was a problem hiding this comment.
Exactly, it's effectively a generalization of UID that includes static data, since the graph needs to distinguish all instantiations of an operator.
Yeah I think this is a good idea, one could just hash this tuple (dyn arg sig, wire sig, static data OR uid) to generate a single distinguishable operator ID to attach to the graph nodes.
|
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.
Could we return a more structured format (to be used/parsed later at MLIR) rather than a pretty-printed IR string? Can we use Bytecode? it is versioned and far more robust than textual round-tripping here. It also avoids the name-prefix conventions in the string-version of rules.
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.
| 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.
| ]; | ||
| } | ||
|
|
||
| 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.
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.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]