Use native bits and registers for CircuitData.
#13686
Conversation
- Add missing traits on `BitInfo`
- Fix bit display property.
- Some functions in `CircuitData` had become mutable unnecessarily due to erroneous assumptions on the `BitData` `OnceCell` usage. - Fix incorrect serialization to now include registers in `CircuitData::__reduce__`. - Also add `CircuitData::__setstate__` to deal with the new register state.
- Fix bug with `BlueprintCircuit` that discarded most instances of `QuantumRegister`. - Rename `RegisterAsKey::index()` to size, since it was erroneously named.
| pub struct BitInfo { | ||
| added_by_reg: bool, | ||
| registers: Vec<BitLocation>, | ||
| } |
There was a problem hiding this comment.
The BitInfo struct and its registers field may be somewhat ambiguous in conveying their true purpose.It appears that BitInfo primarily tracks how bits are associated with registers.
You might consider renaming this BitInfo to something more indicative of its focus on bit-register relationships or placements.
However, if you assume how to use it in another way, it is not necessary to rename this field.
There was a problem hiding this comment.
You got the intended use correctly. I'm open to renaming this, since it is WiP you can leave any suggestions.
| @@ -230,3 +235,602 @@ where | |||
| self.bits.clear(); | |||
| } | |||
| } | |||
|
|
|||
| #[derive(Clone, Debug)] | |||
| pub struct NewBitData<T: From<BitType>, R: Register + Hash + Eq> { | |||
There was a problem hiding this comment.
[Q] Both BitData<T> and NewBitData<T, R>, which both manage and track bit-related information. However, it’s not entirely clear how they are intended to coexist. Are they meant to be used in parallel for different scenarios(is BitData<T> for dag-circuit?), or is there a plan to eventually replace BitData<T> with NewBitData<T, R>? Any clarification on whether BitData<T> is deprecated or if both are needed for distinct reasons would be greatly appreciated.
There was a problem hiding this comment.
NewBitData here is meant to replace BitData, the only reason why I'm keeping the old one around still is because the new replacement methods will need a bigger rewrite of DAGCircuit's methods. Which I plan on doing on a separate PR due to how big it is.
crates/circuit/src/bit_data.rs
Outdated
| let res = py_reg.unwrap().bind(py).get_item(bit_info.index())?; | ||
| self.bits[index_as_usize] | ||
| .set(res.into()) | ||
| .map_err(|_| PyRuntimeError::new_err("Could not set the OnceCell correctly"))?; |
There was a problem hiding this comment.
Instead of simply saying "Could not set the OnceCell correctly", you might include any relevant internal state or the specific condition that led to the failure. This added detail will greatly aid in diagnosing issues.
There was a problem hiding this comment.
I agree with this.
- Fix `BitLocation` updates when a bit or a register is added to use data directly from `CircuitData`. - Fix incorrect register re-assignment in `QuantumCircuit._from_circuit_data`. - Re-organize `BitData::py_get_bit_location` to behave more closely to python.
Co-authored-by: Kento Ueda <[email protected]>
- Add additional checks to `find_bit()`, to make sure bit exists in the `CircuitData` instance.
- Separate python `add_register` from native method due to conflicting implementation. - Fix register setter in `NewBitData` to discard old `BitLocations`. - Update `BitLocations` when setting a new register in `QuantumCircuit`. - Modify test to include registers when comparing to an equivalency from the `EquivalenceLibrary`.
- Add one test case to test initialization.
- Modify the getter methods for the cached lists to correct the cache if it becomes necessary. This to avoid the uninitialization of bits when rust bits are added after requesting access from Python. These methods are now fallible, so a PyResult will always be returned. - Adapt `CircuitData` and `DAGCircuit` to these changes.
Pull Request Test Coverage Report for Build 13185407140Warning: 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 |
- Remove stale methods `contains` and `get_interned` from `Interner` in favor of the changes introduced by Qiskit#13752.
- Temporarily fix incorrect null assignment of `Bit` object to indices. A different solution can be achieved if more time is spent. - Incorrect python circuit creation in Python due to misusage of the bit's `BitInfo` property inside of `BitData`. - Fix incorrect register and bit creation from `CircuitData::new()` - Add provisional `qubits_mut()` and `clbits_mut()` methods while we address the issue with `indices`.
- Fix error where rust bits wouldn't be initialized in the cache. - Provisionally fix error in which rust bits wouldn't have valid mapping to python bits. Bits need to be generated before being able to map them to a rust native index. The current solution implements said changes during the `find_bit` function, in which there is an extra validation step. However, this is certain to consume more resources. The ideal solution would be to generate said mapping when obtaining the Python bit with something similar to a `RefCell`. However, `RefCells` are not thread safe and could cause several mutable references to coexist.
- Re-purpose `NewBitData.indices` to be modifiable even when if a mutable references is not granted by using `RWLock`. This is done to stay consistent with the behavior of `OnceLock` which allows us to initialize bits upon request. We need to make sure to map a bit once it has been initialized. Otherwise, the circuit will have to regenerate this mapping multiple times during runtime. - Optimize pre-caching of Python bits and registers by using the cache size. Bits and registers are guaranteed to be initialized in ther order they were added/last accessed, so in the case a cache is initialized prematurely, add the missing bits or registers to the back of the cache while following the same index order.
…es of the inner `CircuitData`. - Fix docstrings in `CircuitData` and `BitData`.
- Remove unnecessary mutable reference in `find_bit()`.
- Register constructors will now accept `Cow<'_, [T]>` to prevent ownership issues.
- `CircuitData::{qregs, cregs}` will now return a slice of registers, instead of an iterator.
- Add duplicate tests for when registers are added to any circuit from rust.
- Add two more rust native tests for circuit construction with registers present.
- Add the ability of turning a float into a `Param` by calling `Into::into`.
- Fix incorrect check during `BitData::add_register`.
- Format tests.
|
One or more of the following people are relevant to this code:
|
raynelfss
changed the title
CircuitData.CircuitData.
Summary
The following commits add native bits and register usage to
CircuitData. This is the first public attempt at implementing these structures and is open to feedback.Details and comments
As we continue to move towards having more rust-native components as part of our core data model, we need to continue to promote more functionality to be able to work without needing intervention from Python. #13269 describes the cost paid whenever an instance of
CircuitDataandDAGCircuitneeds to be constructed from Rust. There is a constant need for calling back to Python to create instances ofQubitandClbitas needed.An important point made towards this is to transfer the ownership of
BitsandRegistersto the circuit, but also to keep the current functionality of this API. Meaning that, instead of removing the dependency completely, we had to shift the source of truth to rust. The following commits achieve this by performing the following:Adding representations of
QuantumRegisterandClassicalRegisterin Rust.A simple representation using an
IndexSet, and attributes such asnameandsize.Modifying
BitDatacurrentlyNewBitDatato also store register information and dynamically initialize the Python counter parts as requested:Vec<_>and can be accessed by index or by key (RegisterKey).Qubit(u32)andClbit(u32)and there exists a mapping between PythonBitsand the specified index type.RWLockdue to the dynamic initialization of PythonBits.BitInfokeeps a list, organized by the bit's index, which keeps track of:Bits andRegisters from Python are stored as aOnceLockwhich gets dynamically initialized upon request based on the information stored here.Changes to
CircuitDataA new Python-free constructor
CircuitData::new()was created to allow initialization of Rust instances without any need for interaction with Python unless otherwise requested.CircuitData::with_capacityconstructor to also add the bits dynamically without accessing Python.pytoken due to assignments to theParameterTable.Known issues and drawbacks
copy()can take up to twice as long to perform a copy of the circuit compared to previous iterations. This most likely due re-mapping of the registers.OnceLockandRWLockas these can panic under the wrong circumstances.Bits are fully opaque to the Rust ones and there's no validation other than the mapping and information stored in Rust.DAGCircuitdoes not use any of the new additions.DAGCircuit.QuantumCircuit.Tasks:
BitDatastruct to store registers.BitDatainCircuitData.Questions or comments?
Feel free to leave a review with any questions or concerns you might have.