Skip to content
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

Write DAGCircuit.compose with Rust-native types #13024

Open
jakelishman opened this issue Aug 23, 2024 · 0 comments
Open

Write DAGCircuit.compose with Rust-native types #13024

jakelishman opened this issue Aug 23, 2024 · 0 comments

Comments

@jakelishman
Copy link
Member

          This seems like an unusual choice to do quite as literal translation of the Python source as this. Feels like we're making life a lot harder for ourselves - it seems like the flow of this function is converting Rust-native objects in `other` into their Python equivalents, using a Python-space mapping to convert them to `self`-based objects, then having to extract everything back out of Python-space again to get the Rust-native types in `self`.

I'd have thought we'd be able to build up qubit_map: Vec<Qubit> where the indices are the qubits in other and the values are the corresponding Qubit in self, and never need to involve Python space beyond converting the function arguments into self's Qubit objects (like qubits.iter()?.map(|bit| self.qubits.get(bit?)).collect::<PyResult<Vec<Qubit>>>()? or something).

Let's not change it now, but we definitely should bring this more Rust-native in follow-ups; if nothing else, it'll make the file easier to read.

Originally posted by @jakelishman in #12550 (comment)

Following #12550, DAGCircuit.compose is a fairly literal translation of the original Python code into Rust. This includes using Python objects for the bit comparisons, argument lists and hashmaps, in places where we already have Rust-native types that can be used. We should rewrite the function to use the Rust-native types, and overhead the Python-space overhead of the conversions.

Also, as mentioned in #12550 (comment), the version at the merge point was not a translation of the optimised version made in #12826. Moving to the Rust-native types should largely fix that concern automatically, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant