Add qk_dag_compose to the C API
#15329
Conversation
raynelfss
added
Changelog: New Feature
Pull Request Test Coverage Report for Build 19371338481Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
The test is based on the ``test_compose_inorder`` python test, but it is incomplete since we cannot check the order of the operations just yet. However, we can easily check the number of operation nodes as a temporary way of checking the validity of the composition.
|
One or more of the following people are relevant to this code:
|
|
|
||
| // Since we don't yet supposed vars in C, we can skip the include_captures check. | ||
| dag.compose(other_dag, qubits.as_deref(), clbits.as_deref(), false) | ||
| .expect("Error during circuit composition."); |
There was a problem hiding this comment.
quick clarification here, do we need to use the .expect() method here? is it valid considering the rust panic, on failure, may result in undefined behavior across the FFI boundary? I couldn't be sure as I don't know if our C API has a way to handle such panics.
I also wanted to ask why it's otherwise not preferable to handle such cases by unpacking the PyResult type return of :meth:DAGCircuit.compose() and mapping to the newly defined exit codes?
There was a problem hiding this comment.
Your suggestion here is a really good idea, mapping to the newly defined error codes should be the way to go. However, for this case we would have to do a couple of things before we can get there.
- We would need
DAGCircuitto use Rust native errors, the same way that we're doing for CircuitData in Move to rust native errors for rust apis in CircuitData #15279. You may ask why couldn't we just map with the current errors? The problem here is that these errors are aliases for Python exceptions, and in order to get information about them, such as the error message or the type, we would have to go through python first (even if the message originated from Rust). So that means, unless we have our C API currently connected to python, we would panic regardless. - Why not just return the generic exit code that we made for
DAGCircuit(ExitCode::DAGError)? This would work similarly to the current approach except that it is recoverable. However, we get much less information than we do with the current approach since at least we know the error happened during the run of the pass.
We could, however, implement a generic ExitCode::DAGComposeError that may also be salvageable and communicates just as much. This would depend on whether we want this to be the case here, I would be open to it since this method modifies the QkDag directly.
| /// # Safety | ||
| /// | ||
| /// Behavior is undefined if ``dag`` or ``other`` are not valid, non-null pointers to a ``QkDag``. | ||
| /// If ``qubit`` nor ``clbit`` are NULL, it must contains a less or equal amount |
There was a problem hiding this comment.
small typo in the line, should be
/// If neither ``qubit`` nor ``clbit`` are NULL, it must contain a less or equal amount
also, the underlying compose method mandates the use of only an EQUAL amount of wires in both the other circuit and the dag circuit as is in this line in :meth:DAGCircuit.compose:
let qubit_map = match qubits {
None => identity_qubit_map.clone(),
Some(qubits) => {
if qubits.len() != other.qubits.len() {
return Err(DAGCircuitError::new_err(concat!(
"Number of items in qubits parameter does not",
" match number of qubits in the circuit."
)));
}but here we are allowing <= values in code, and not raising the mismatch exit code on the less than condition, am I missing something here?
There was a problem hiding this comment.
Thank you for identifying the typo. As for the second half of the comment, the method DAGCircuit::compose allows for usage of less or equal amount of qubits for the other dag, compared to the main DAG. You can see that the first check in the method does just as much:
if other.qubits.len() > self.qubits.len() || other.clbits.len() > self.clbits.len() {
return Err(DAGCircuitError::new_err(
"Trying to compose with another DAGCircuit which has more 'in' edges.",
));
}Because what this method basically does is to append this dag onto the main dag it should be impossible to do so with more input nodes than the main dag, but a smaller or equally sized DAGCircuit instance should be fine.
Now your confusion here lies with how we compare the qubits and clbits argument. Since these represent the order of the wires in terms of the other dag to explain how we attach them to the main dag, these need to explicitly be of the same length as other when provided (Which is what the code snippet that you referenced explains). So this entry point here does not break that convention and if it did the pass itself would scream at us for not sticking to it.
I hope this explanation clears up any confusion you may have and thank you for reviewing :)
| return result; | ||
| } | ||
|
|
||
| static int test_dag_compose(void) { |
There was a problem hiding this comment.
Do you think here we can also have test cases to test whether the correct exit codes are raised on failure, the new ones added in specific?
There was a problem hiding this comment.
Yes, that is a great idea. I will add a test that does that.
aaryav-3
left a comment
aaryav-3
left a comment
There was a problem hiding this comment.
Thank you so much for your contribution! This is a great, clean FFI implementation for the :meth:DAGCircuit::compose method. I had just some quick clarifications before this PR looks perfect to merge.
4 participants
Summary
The following commits add
qk_dag_composeto the C API, as a counterpart to theDAGCircuit::composemethod.Details and comments
Closes #15196.