-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Move interners from trait to generic structs #13033
Conversation
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 10577094637Details
💛 - Coveralls |
b5e06e4
to
a34578e
Compare
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 love this.
Very cool Rust going on here that gives the interner a much nicer interface. Super curious to hear the answers to some of the questions I've left in line.
Thanks for doing this, @jakelishman!
)?; | ||
let qubits_id = self | ||
.qargs_cache | ||
.insert_owned(self.qubits.map_bits(qargs.iter().flatten())?.collect()); |
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.
So in this case, we can use collect
here which will collect into a native array? This seems very cool, but I think I'm not understanding how this works. Can you explain?
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.
This might be more straightforward than you had in mind - <[T] as ToOwned>::Owned
is Vec<T>
, so this is basically identical to what it was before. It just looks shorter because it's easier to access the insert
method, and the Vec<T>
is a bit more obfuscated (unfortunately - I actually wanted it to be more obvious, but the only way I could think to do that was to introduce a second type parameter that had no real effect).
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.
Oh wow, I would not have guessed that. Thanks for clarifying!
I agree with your choice to avoid a redundant type parameter.
Looks good, I'll approve once conflicts are resolved. |
This rewrites the interner mechanisms be defined in terms of two generic structs (`Interner` and `Interned`) that formalise the idea that the interned values are owned versions of a given reference type, and move the logically separate intern-related actions (get a key from the value, and get the value from the key) into separate functions (`get` and `insert`, respectively). This has a few advantages: 1. we now have both `insert` and `insert_owned` methods, which was awkward to add within the trait-based structure. This allows a more efficient path when the owned variant has already necessarily been constructed. 2. additionally, the standard `insert` path now takes only a reference type. For large circuits, most intern lookups will, in general, find a pre-existing key, so in situations where the interned value is sufficiently small that it can be within a static allocation (which is the case for almost all `qargs` and `cargs`), it's more efficient not to construct the owned type on the heap. 3. the type of the values retrieved from an interner are no longer indirected through the owned type that's stored. For example, where `IndexedInterner<Vec<Qubit>>` gave out `&Vec<Qubit>`s as its lookups, `Interner<[Qubit]>` returns the more standard `&[Qubit]`, which is only singly indirect rather than double. The following replacements are made: 1. The `IndexedInterner` struct from before is now just called `Interner` (and internally, it uses an `IndexSet` rather than manually tracking the indices). Its generic type is related to the references it returns, rather than the owned value it stores, so `IndexedInterner<Vec<Qubit>>` becomes `Interner<[Qubit]>`. 2. The `Interner` trait is now gone. Everything is just accessed as concrete methods on the `Interner` struct. 3. `<&IndexedInterner as Interner>::intern` (lookup of the value from an interner key) is now called `Interner::get`. 4. `<&mut IndexedInterner as Interner>::intern` (conversion of an owned value to an interner key) is now called `Interner::insert_owned`. 5. A new method, `Interner::insert`, can now be used when one need not have an owned allocation of the storage type; the correct value will be allocated if required (which is expected to be less frequent). 6. The intern key is no longer called `interner::Index`, but `Interned<T>`, where the generic parameter `T` matches the generic of the `Interner<T>` that gave out the key.
a34578e
to
fb71771
Compare
…12975) * Initial: implement fixed capacity constructor for DAGCircuit - Implement `DAGCircuit` with `with_capacity` to create an initially allocated instance, for rust only. - Implement `with_capacity` for `BitData` and `IndexInterner` that creates an initally allocated instance of each type. - Other small tweaks and fixes. - Add num_edges optional argument. If known, use `num_edges` argument to create a `DAGCircuit` with the exact number of edges. - Leverage usage of new method in `copy_empty_like`. - Use `..Default::default()` for `op_names` and `calibrations` fields in `DAGCircuit::with_capacity()` * Fix: Adapt to #13033 * Fix: Re-arrange the function arguments as per @mtreinish's review. - The order now follows: qubits, clbits, vars, num_ops, edges. As per [Matthew's comment](#12975 (comment)).
…iskit#12975) * Initial: implement fixed capacity constructor for DAGCircuit - Implement `DAGCircuit` with `with_capacity` to create an initially allocated instance, for rust only. - Implement `with_capacity` for `BitData` and `IndexInterner` that creates an initally allocated instance of each type. - Other small tweaks and fixes. - Add num_edges optional argument. If known, use `num_edges` argument to create a `DAGCircuit` with the exact number of edges. - Leverage usage of new method in `copy_empty_like`. - Use `..Default::default()` for `op_names` and `calibrations` fields in `DAGCircuit::with_capacity()` * Fix: Adapt to Qiskit#13033 * Fix: Re-arrange the function arguments as per @mtreinish's review. - The order now follows: qubits, clbits, vars, num_ops, edges. As per [Matthew's comment](Qiskit#12975 (comment)).
…ckedInstruction (#13032) * Initial: Add add_from_iter method to DAGCircuit - Introduce a method that adds a chain of `PackedInstruction` continuously avoiding the re-linking of each bit's output-node until the very end of the iterator. - TODO: Add handling of vars - Add header for a `from_iter` function that will create a `DAGCircuit` based on a chain of `PackedInstruction`. * Fix: leverage new methods in layers - Fix incorrect re-insertion of last_node. * Fix: Keep track of Vars for add_from_iter - Remove `from_iter` * Fix: Incorrect modification of last nodes in `add_from_iter`. - Use `entry` api to either modify or insert a value if missing. * Fix: Cycling edges in when adding vars. - A bug that adds duplicate edges to vars has been temporarily fixed. However, the root of this problem hasn't been found yet. A proper fix is pending. For now skip those instances. * Fix: Remove set collecting all nodes to be connected. - A set collecting all the new nodes to connect with a new node was preventing additional wires to connect to subsequent nodes. * Fix: Adapt to #13033 * Refactor: `add_from_iter` is now called `extend` to stick with `Rust` nomenclature. * Fix docstring - Caught by @ElePT Co-authored-by: Elena Peña Tapia <[email protected]> * Fix: Remove duplicate vars check * Fix: Corrections from code review. - Use Entry API to modify last nodes in the var. - Build new_nodes with an allocated vec. - Add comment explaining the removal of the edge between the output node and its predecessor. * Fix: Improper use of `Entry API`. - Use `or_insert_with` instead of `or_insert` to perform actions before inserting a value. --------- Co-authored-by: Elena Peña Tapia <[email protected]>
* Initial: Add add_from_iter method to DAGCircuit - Introduce a method that adds a chain of `PackedInstruction` continuously avoiding the re-linking of each bit's output-node until the very end of the iterator. - TODO: Add handling of vars - Add header for a `from_iter` function that will create a `DAGCircuit` based on a chain of `PackedInstruction`. * Fix: leverage new methods in layers - Fix incorrect re-insertion of last_node. * Fix: Keep track of Vars for add_from_iter - Remove `from_iter` * Fix: Incorrect modification of last nodes in `add_from_iter`. - Use `entry` api to either modify or insert a value if missing. * Fix: Cycling edges in when adding vars. - A bug that adds duplicate edges to vars has been temporarily fixed. However, the root of this problem hasn't been found yet. A proper fix is pending. For now skip those instances. * Fix: Remove set collecting all nodes to be connected. - A set collecting all the new nodes to connect with a new node was preventing additional wires to connect to subsequent nodes. * Fix: Adapt to #13033 * Refactor: `add_from_iter` is now called `extend` to stick with `Rust` nomenclature. * Fix: Remove duplicate vars check * Fix: Corrections from code review. - Use Entry API to modify last nodes in the var. - Build new_nodes with an allocated vec. - Add comment explaining the removal of the edge between the output node and its predecessor. * Fix: Improper use of `Entry API`. - Use `or_insert_with` instead of `or_insert` to perform actions before inserting a value. * Initial: Add add_from_iter method to DAGCircuit - Introduce a method that adds a chain of `PackedInstruction` continuously avoiding the re-linking of each bit's output-node until the very end of the iterator. - TODO: Add handling of vars - Add header for a `from_iter` function that will create a `DAGCircuit` based on a chain of `PackedInstruction`. * Initial: Expose `CircuitData` interners and registers to the `qiskit-circuit` crate. - Make the `CircuitData` iter method be an exact-size iterator. * FIx: Expose immutable views of interners, registers and global phase. - Revert the changes making the interners and registers visible to the crate `qiskit-circuit`. - Create methods to expose immutable borrowed views of the interners, registers and global_phase to prevent from mutating the DAGCircuit. - Add `get_qargs` and `get_cargs` to unpack interned qargs ans cargs. - Other tweaks and fixes. * Refactor: Use naming convention for getters. * Docs: Apply suggestions from code review - Correct incorrect docstrings for `qubits()` and `clbits()` Co-authored-by: Eli Arbel <[email protected]> * Initial: Add `circuit_to_dag` in rust. - Add new method `DAGCircuit::from_quantum_circuit` which uses the data from a `QuantumCircuit` instance to build a dag_circuit. - Expose the method through a `Python` interface with `circuit_to_dag` which goes by the alias of `core_circuit_to_dag` and is called by the original method. - Add an arbitrary structure `QuantumCircuitData` that successfully extract attributes from the python `QuantumCircuit` instance and makes it easier to access in rust. - This structure is for attribute extraction only and is not clonable/copyable. - Expose a new module `converters` which should store all of the rust-side converters whenever they get brought into rust. - Other small tweaks and fixes. * Fix: Move qubit ordering check to python - Add more accurate estimate of num_edges. * Fix: Regenerate `BitData` instances dynamically instead of cloning. - When converting from `QuantumCircuit` to `DAGCircuit`, we were previously copying the instances of `BitData` by cloning. This would result in instances that had extra qubits that went unused. This commit fixes this oversight and reduced the amount of failing times from 160 to 12. * Fix: Make use of `copy_operations` - Use `copy_operations` to copy all of the operations obtained from `CircuitData` by calling deepcopy. - Initialize `._data` manually for instances of `BlueprintCircuit` by calling `._build()`. - Other small fixes. * FIx: Adapt to 13033 * Fix: Correctly map qubits and clbits to the `DAGCircuit`. - Previously, the binding of qubits and clbits into bitdata was done based on the `push_back()` function behavior. This manual mapping was removed in favor of just inserting the qubits/clbits using the `add_qubits()` and `add_clbits()` methods and keeping track of the new indices. - Remove cloning of interners, use the re-mapped entries instead. - Remove manual re-mapping of qubits and clbits. - Remove cloning of BitData, insert qubits directly instead. - Other small tweaks and fixes. * Add: `DAGCircuit` from `CircuitData` - Make `QuantumCircuitData` extraction struct private. - Rename `DAGCircuit::from_quantum_circuit` into `DAGCircuit::from_circuit` to make more generalized. - Pass all attributes of `QuantumCircuit` that are passed from python as arguments to the `DAGCircuit::from_circuit` function. - Add `DAGCircuit::from_circuit_data` as a wrapper to `from_circuit` to create an instance solely from the properties available in `CircuitData`. - Use `DAGCircuit::from_circuit` as base for `DAGCircuit::from_circuit_data`. * Fix: Make `QuantumCircuitData` public. - `QuantumCircuitData` is an extractor helper built only to extract attributes from a Python-based `Quantumircuit` that are not yet available in rust + the qualities that already are through `CircuitData`. - Add `Clone` trait `QuantumCircuitData` as all its properties are clonable. - Make `circuit_to_dag` public in rust in case it needs to be used for a transpiler pass. - Adapt to renaming of `DAGCircuit::add_from_iter` into `DAGCircuit::extend`. * Fix: Use `intern!` when extracting attributes from Python. * Fix: Copy instructions in place instead of copying circuit data. - Use `QuantumCircuitData` as argument for `DAGCircuit::from_circuit`. - Create instance of `QuantumCircuitData` in `DAGCircuit::from_circuit_data`. - Re-add duplicate skip in extend. - Other tweaks and fixes. * Fix: Remove second re-mapping of bits - Remove second re-mapping of bits, perform this mapping at insertion of qubits instead. - Modify insertion of qubits to re-map the indices and store them after insertion should a custom ordering be provided. - Remove extra `.and_modify()` for `clbits_last_node` from `DAGCircuit::extend`. - Remove submodule addition to `circuit` module, submodule is now under `_accelerate.converters`. - Other tweaks and fixes. * Update crates/circuit/src/dag_circuit.rs Co-authored-by: Matthew Treinish <[email protected]> --------- Co-authored-by: Eli Arbel <[email protected]> Co-authored-by: Matthew Treinish <[email protected]>
Summary
This rewrites the interner mechanisms be defined in terms of two generic structs (
Interner
andInterned
) that formalise the idea that the interned values are owned versions of a given reference type, and move the logically separate intern-related actions (get a key from the value, and get the value from the key) into separate functions (get
andinsert
, respectively).This has a few advantages:
we now have both
insert
andinsert_owned
methods, which was awkward to add within the trait-based structure. This allows a more efficient path when the owned variant has already necessarily been constructed.additionally, the standard
insert
path now takes only a reference type. For large circuits, most intern lookups will, in general, find a pre-existing key, so in situations where the interned value is sufficiently small that it can be within a static allocation (which is the case for almost allqargs
andcargs
), it's more efficient not to construct the owned type on the heap.the type of the values retrieved from an interner are no longer indirected through the owned type that's stored. For example, where
IndexedInterner<Vec<Qubit>>
gave out&Vec<Qubit>
s as its lookups,Interner<[Qubit]>
returns the more standard&[Qubit]
, which is only singly indirect rather than double.The following replacements are made:
The
IndexedInterner
struct from before is now just calledInterner
(and internally, it uses anIndexSet
rather than manually tracking the indices). Its generic type is related to the references it returns, rather than the owned value it stores, soIndexedInterner<Vec<Qubit>>
becomesInterner<[Qubit]>
.The
Interner
trait is now gone. Everything is just accessed as concrete methods on theInterner
struct.<&IndexedInterner as Interner>::intern
(lookup of the value from an interner key) is now calledInterner::get
.<&mut IndexedInterner as Interner>::intern
(conversion of an owned value to an interner key) is now calledInterner::insert_owned
.A new method,
Interner::insert
, can now be used when one need not have an owned allocation of the storage type; the correct value will be allocated if required (which is expected to be less frequent).The intern key is no longer called
interner::Index
, butInterned<T>
, where the generic parameterT
matches the generic of theInterner<T>
that gave out the key.Details and comments
This is sufficiently generic now that it can probably be used directly for the purposes of #12917, though I didn't make it generic over the key width. It probably wouldn't be too hard to do that too, if we want it very much.
I'm intending to follow this PR with one that makes the key corresponding to the "empty" state constructible without any hash lookup, since we end up needing that quite a lot (think the cargs key when pushing a standard gate). (edit: now #13035.)