-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Add substitute_node_with_dag to c api #15374
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?
Changes from 1 commit
1b10653
591d7e9
0e4c5bf
a09d8b4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,8 +10,10 @@ | |
| // copyright notice, and modified files need to carry a notice indicating | ||
| // that they have been altered from the originals. | ||
|
|
||
| use anyhow::Error; | ||
| use num_complex::Complex64; | ||
| use smallvec::smallvec; | ||
| use std::ffi::{CString, c_char}; | ||
|
|
||
| use qiskit_circuit::Qubit; | ||
| use qiskit_circuit::bit::{ClassicalRegister, QuantumRegister}; | ||
|
|
@@ -23,6 +25,7 @@ use qiskit_circuit::operations::{ | |
| }; | ||
|
|
||
| use crate::circuit::unitary_from_pointer; | ||
| use crate::exit_codes::ExitCode; | ||
| use crate::pointers::{const_ptr_as_ref, mut_ptr_as_ref}; | ||
|
|
||
| /// @ingroup QkDag | ||
|
|
@@ -968,3 +971,96 @@ pub unsafe extern "C" fn qk_dag_topological_op_nodes(dag: *const DAGCircuit, out | |
| unsafe { out_order.add(i).write(node.index() as u32) } | ||
| } | ||
| } | ||
|
|
||
| /// @ingroup QkDag | ||
| /// Replace a node in a `QkDag` with a subcircuit specfied by another `QkDag` | ||
| /// | ||
| /// @param dag A pointer to the DAG. | ||
| /// @param node The node index of the operation to replace with the other `QkDag`. This | ||
| /// must be the node index for an operation node in ``dag`` and the qargs and cargs | ||
| /// count must match the number of qubits and clbits in `replacement`. | ||
| /// @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. | ||
| /// @param error A pointer to a pointer with an nul terminated string with an | ||
| /// error description. If the function fails a pointer to the string with | ||
| /// the error description will be written to this pointer. That pointer | ||
| /// needs to be freed with `qk_str_free`. This can be a null pointer in | ||
| /// which case the error will not be written out. | ||
| /// | ||
| /// @returns The return code for the operation, ``QkExitCode_Success`` means success and all | ||
| /// other values indicate an error. | ||
| /// | ||
| /// # Example | ||
| /// | ||
| /// ```c | ||
| /// QkDag *dag = qk_dag_new(); | ||
| /// QkQuantumRegister *qr = qk_quantum_register_new(1, "my_register"); | ||
| /// qk_dag_add_quantum_register(dag, qr); | ||
| /// | ||
| /// uint32_t qubit[1] = {0}; | ||
| /// uint32_t node_to_replace = qk_dag_apply_gate(dag, QkGate_H, qubit, NULL, false); | ||
| /// qk_dag_apply_gate(dag, QkGate_S, qubit, NULL, false); | ||
| /// | ||
| /// // Build replacement dag for H | ||
| /// QkDag *replacement = qk_dag_new(); | ||
| /// QkQuantumRegister *replacement_qr = qk_quantum_register_new(1, "other"); | ||
| /// qk_dag_add_quantum_register(replacement, replacement_qr); | ||
| /// double pi_param [1] = {3.14159,}; | ||
| /// qk_dag_apply_gate(replacement, QkGate_RZ, qubit, pi_param, false); | ||
| /// qk_dag_apply_gate(replacement, QkGate_SX, qubit, NULL, false); | ||
| /// qk_dag_apply_gate(replacement, QkGate_RZ, qubit, pi_param, false); | ||
| /// | ||
| /// qk_dag_substitute_node_with_dag(dag, node_to_replace, replacement, NULL); | ||
| /// | ||
| /// // Free the replacement dag, register, dag, and register | ||
| /// qk_quantum_register_free(replacement_qr); | ||
| /// qk_dag_free(replacement); | ||
| /// qk_quantum_register_free(qr); | ||
| /// qk_dag_free(dag); | ||
| /// ``` | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// Behavior is undefined if ``dag`` and ``replacement`` are not a valid, non-null pointer to a | ||
| /// ``QkDag``. ``error`` must be a valid pointer to a ``char`` pointer or ``NULL``. | ||
| #[unsafe(no_mangle)] | ||
| #[cfg(feature = "cbinding")] | ||
| pub unsafe extern "C" fn qk_dag_substitute_node_with_dag( | ||
| dag: *mut DAGCircuit, | ||
| node: u32, | ||
| replacement: *const DAGCircuit, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Not a big deal, since the Rust code already has the same interface, but seeing it written out again here made me think.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| error: *mut *mut c_char, | ||
| ) -> ExitCode { | ||
mtreinish marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // SAFETY: Per documentation, ``dag`` is non-null and valid. | ||
| let dag = unsafe { mut_ptr_as_ref(dag) }; | ||
| let replacement = unsafe { const_ptr_as_ref(replacement) }; | ||
|
|
||
| match dag.substitute_node_with_dag(NodeIndex::new(node as usize), replacement, None, None, None) | ||
| { | ||
| Ok(_) => ExitCode::Success, | ||
| Err(e) => { | ||
| if !error.is_null() { | ||
| let err: Error = e.into(); | ||
| // SAFETY: Per the documentation error is either null or a valid and aligned | ||
| // pointer to a pointer of a C string which is safe to write a pointer to the | ||
| // Rust heap into. | ||
| unsafe { | ||
| // Right now we return a backtrace of the error. This at least gives a hint as to | ||
| // which pass failed when we have rust errors normalized we can actually have error | ||
| // messages which are user facing. But most likely this will be a PyErr and panic | ||
| // when trying to extract the string. | ||
| *error = CString::new(format!( | ||
| "Transpilation failed with this backtrace: {}", | ||
| err.backtrace() | ||
| )) | ||
| .unwrap() | ||
| .into_raw(); | ||
mtreinish marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
| ExitCode::DagError | ||
| } | ||
| } | ||
| } | ||
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 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
qargsorcargs, so they don't have a native order that the user can be determined, and so they can't just buildreplacementin order.Also, if there are any
Vars in either circuit, thensubstitute_node_with_dagpanics if you passNonein 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can expose the mapping interfaces as an array of
uint32_twithNULLfor 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline we'll look at adding a new variant function like
substitute_node_with_dag_reorderingthat 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.