Add substitute_node_with_dag to c api #15374
Conversation
mtreinish
added
Changelog: New Feature
|
One or more of the following people are relevant to this code:
|
This commit adds a new function qk_dag_substitute_node_with_dag to the C api. This facilitates in place substitute from the new dag api. This is fairly mechanical addition to the api as `DAGCircuit::substitute_node_with_dag()` is already rust native. So we only need to expose that inteface to C. The only difference between the C and existing internal rust interface is that the qubit, clbit, and var mapping arguments are not exposed to C. These options aren't commonly used, and if the user needs to map the ordering they can do that during replacement dag construction rather that at substitution time. Fixes Qiskit#15190
mtreinish
force-pushed
the
substituting_nodes_with_dags_at_sea
branch
from
5e09540 to
1b10653
Compare
| /// @param replacement The other `QkDag` to replace `node` with. This dag must have | ||
| /// the same number of qubits as the operation for ``node``. The node | ||
| /// bit ordering will be ordering will be handled in order, so `qargs[0]` for | ||
| /// `node` will be mapped to `qubits[0]` in `replacement`, `qargs[1]` to | ||
| /// `qubits[0]`, etc. The same pattern applies to classical bits too. |
There was a problem hiding this comment.
I didn't think about it in the meeting, but the reason that the map needs to exist is for nodes that have "extra" wires, like control-flow or classical expressions - those don't appear in qargs or cargs, so they don't have a native order that the user can be determined, and so they can't just build replacement in order.
Also, if there are any Vars in either circuit, then substitute_node_with_dag panics if you pass None in that argument (??).
If we merge this PR without it, we'll likely have to break the API later to add it back in. That's not necessarily a deal-breaker, but it is more than we thought of at the time.
There was a problem hiding this comment.
I can expose the mapping interfaces as an array of uint32_t with NULL for no mapping (we don't even need a length because it has to match the qargs length). That should work for Vars too (although for vars it's a bit weird to expose that and not have it in C yet) right?
There was a problem hiding this comment.
As discussed offline we'll look at adding a new variant function like substitute_node_with_dag_reordering that takes the mapping for the control flow use case when we add that to the C API. But we will keep this function as the simpler entrypoint for the common case.
| pub unsafe extern "C" fn qk_dag_substitute_node_with_dag( | ||
| dag: *mut DAGCircuit, | ||
| node: u32, | ||
| replacement: *const DAGCircuit, |
There was a problem hiding this comment.
We maybe don't want to meaningfully change things in this PR since it stems from Rust, but is "borrow" semantics definitely the right choice for replacement? From C it makes things awkward, because we have to manually free each replacement DAG, whereas if it was "move" semantics, then I think most cases would be more natural, and we'd be able to inline qk_dag_copy calls for those that weren't.
Not a big deal, since the Rust code already has the same interface, but seeing it written out again here made me think.
There was a problem hiding this comment.
I can make it take ownership of the dag and free it as part of the function. It's easy to do if you think that's a better interface for this. I just did it this way because it was how the rust interface worked. But there isn't a reason we can't take ownership with this function.
There was a problem hiding this comment.
I'm uncertain. This does feel like it wants to be a "move" to me, but given we never really had that option in Python, I don't really trust my intuition around it yet.
Pull Request Test Coverage Report for Build 19945790991Details
💛 - Coveralls |
The failure modes for calling the inner rust method are all caused by invalid input or aren't possible via the C API (come from python) so there isn't a reason to have a subpar error experience dealing with backtraces when it's not something recoverable.
4 participants
Summary
This commit adds a new function qk_dag_substitute_node_with_dag to the C api. This facilitates in place substitute from the new dag api. This is fairly mechanical addition to the api as
DAGCircuit::substitute_node_with_dag()is already rust native. So we only need to expose that inteface to C. The only difference between the C and existing internal rust interface is that the qubit, clbit, and var mapping arguments are not exposed to C. These options aren't commonly used, and if the user needs to map the ordering they can do that during replacement dag construction rather that at substitution time.Details and comments
Fixes #15190