[WIP] Use native bits and registers for `CircuitData`. #13686

raynelfss · 2025-01-17T20:48:40Z

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

Tasks:

Implement a new BitData struct to store registers.
Implement the new BitData in CircuitData.
Fix all the broken tests.
Implement rust-native tests to check if this is working properly.
Implement python cross testing.

- 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.

to24toro

Thank you for sharing this pull request. I understand that this is currently marked as a WIP and a draft, so I was initially hesitant to leave comments. However, I do have a few questions and suggestions that I hope will be helpful as the work progresses.

crates/circuit/src/bit.rs

to24toro · 2025-01-25T04:54:13Z

crates/circuit/src/bit.rs

+pub struct BitInfo {
+    added_by_reg: bool,
+    registers: Vec<BitLocation>,
+}


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.

You got the intended use correctly. I'm open to renaming this, since it is WiP you can leave any suggestions.

to24toro · 2025-01-25T05:27:42Z

crates/circuit/src/bit_data.rs

@@ -230,3 +235,602 @@ where
        self.bits.clear();
    }
 }
+
+#[derive(Clone, Debug)]
+pub struct NewBitData<T: From<BitType>, R: Register + Hash + Eq> {


[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.

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.

to24toro · 2025-01-25T05:52:02Z

crates/circuit/src/bit_data.rs

+                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"))?;


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.

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 <38037695+to24toro@users.noreply.github.com>

- 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.

coveralls · 2025-01-29T19:17:11Z

Pull Request Test Coverage Report for Build 13059186759

Details

503 of 865 (58.15%) changed or added relevant lines in 10 files are covered.
47 unchanged lines in 5 files lost coverage.
Overall coverage decreased (-0.4%) to 88.475%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/circuit/quantumcircuit.py	38	39	97.44%
qiskit/circuit/library/blueprintcircuit.py	7	9	77.78%
crates/circuit/src/lib.rs	0	6	0.0%
crates/circuit/src/register.rs	33	98	33.67%
crates/circuit/src/circuit_data.rs	128	207	61.84%
crates/circuit/src/bit_data.rs	244	453	53.86%

Files with Coverage Reduction	New Missed Lines	%
qiskit/circuit/library/blueprintcircuit.py	1	94.12%
crates/circuit/src/circuit_data.rs	1	88.56%
crates/qasm2/src/lex.rs	7	91.23%
crates/circuit/src/bit_data.rs	8	61.58%
crates/qasm2/src/parse.rs	30	95.76%

Totals
Change from base Build 13055798857:	-0.4%
Covered Lines:	80134
Relevant Lines:	90572

💛 - Coveralls

- Remove stale methods `contains` and `get_interned` from `Interner` in favor of the changes introduced by Qiskit#13752.

raynelfss and others added 9 commits January 16, 2025 13:35

Initial: Register Infrastructure in Rust

af13dd2

Add: bits method for register.

cf673db

- Add missing traits on `BitInfo`

Use interners for registry

00eeb7d

Fix: Add functional python Bits

bfa7389

Initial Python registers

db42bab

Finalize Python Registers, prepare for replacements.

20f0ebf

Fix serialization in registers

66bee69

- Fix bit display property.

BitData refactor: Store Registers

3a0c5bb

Add new BitData to CircuitData

f8bdfa6

raynelfss linked an issue Jan 17, 2025 that may be closed by this pull request

Make rust Qubit and Clbit the source of truth in a circuit #13269

Open

raynelfss and others added 6 commits January 21, 2025 15:21

Add more rust native connections to CircuitData and QuantumCircuit.

53a7979

Fix: Do not re-add bits when adding registers.

8b80738

Fix: Incorrect addition of register cache

ff9958f

Merge remote-tracking branch 'upstream/main' into bit_u32_sequel

1744b12

Refactor BitInfo to account for bits added from registers.

e5ebe22

- Fix bug with `BlueprintCircuit` that discarded most instances of `QuantumRegister`. - Rename `RegisterAsKey::index()` to size, since it was erroneously named.

to24toro reviewed Jan 26, 2025

View reviewed changes

jakelishman mentioned this pull request Jan 27, 2025

Create qiskit_circuit::dag submodule #13721

Open

raynelfss and others added 7 commits January 27, 2025 15:32

Update bit.rs

03fbbc2

Co-authored-by: Kento Ueda <38037695+to24toro@users.noreply.github.com>

Merge remote-tracking branch 'upstream/main' into bit_u32_sequel

02c4d1a

Fix: Add registers correctly in circuit_to_instruction.

2cd46a5

- Add additional checks to `find_bit()`, to make sure bit exists in the `CircuitData` instance.

Fix: incorrect circuit initalization on the rust-side.

a353334

- Add one test case to test initialization.

raynelfss added 2 commits January 29, 2025 14:27

Lint: Remove unused Qubit import

5169fc6

Docs: Add header strings in bit.rs and register.rs

c2461ba

raynelfss mentioned this pull request Jan 30, 2025

Add merging of Interners #13752

Merged

Merge branch 'main' into bit_u32_sequel

ae3385e

raynelfss and others added 2 commits January 30, 2025 11:59

Merge branch 'main' into bit_u32_sequel

b18c5ab

Fix: Remove changes to the Interner

f1ffc16

- Remove stale methods `contains` and `get_interned` from `Interner` in favor of the changes introduced by Qiskit#13752.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Use native bits and registers for `CircuitData`. #13686

[WIP] Use native bits and registers for `CircuitData`. #13686

raynelfss commented Jan 17, 2025 •

edited

Loading

to24toro left a comment

to24toro Jan 25, 2025

raynelfss Jan 27, 2025

to24toro Jan 25, 2025

raynelfss Jan 27, 2025

to24toro Jan 25, 2025

raynelfss Jan 27, 2025

coveralls commented Jan 29, 2025 •

edited

Loading

[WIP] Use native bits and registers for CircuitData. #13686

Are you sure you want to change the base?

[WIP] Use native bits and registers for CircuitData. #13686

Conversation

raynelfss commented Jan 17, 2025 • edited Loading

Summary

Details and comments

Tasks:

to24toro left a comment

Choose a reason for hiding this comment

to24toro Jan 25, 2025

Choose a reason for hiding this comment

raynelfss Jan 27, 2025

Choose a reason for hiding this comment

to24toro Jan 25, 2025

Choose a reason for hiding this comment

raynelfss Jan 27, 2025

Choose a reason for hiding this comment

to24toro Jan 25, 2025

Choose a reason for hiding this comment

raynelfss Jan 27, 2025

Choose a reason for hiding this comment

coveralls commented Jan 29, 2025 • edited Loading

Pull Request Test Coverage Report for Build 13059186759

Details

💛 - Coveralls

[WIP] Use native bits and registers for `CircuitData`. #13686

[WIP] Use native bits and registers for `CircuitData`. #13686

raynelfss commented Jan 17, 2025 •

edited

Loading

coveralls commented Jan 29, 2025 •

edited

Loading