Expose `CircuitData` interners and registers to the `qiskit-circuit` crate

[raynelfss]

Summary

Tracked by #13001 This commit adds small changes to the visibility of the internal data of the CircuitData object, which would allow for an easier conversion into a DAGCircuit once #13001 is completed.

Details and comments:

• Make qargs_interner, cargs_interner, qubits and clbits attributes visible to the rest of the qiskit-circuit crate. This could allow for easier transfer of data when converting from CircuitData to DAGCircuuit.
• Make CircuitData::iter() method return an exact-size iterator. Which would allow us to determine the size of the circuit without needing to collect the iterator.

Suggestions

Leave any suggestions in a comment here.

[qiskit-bot]

One or more of the following people are relevant to this code:

• @Qiskit/terra-core
• @kevinhartman
• @mtreinish

[coveralls]

Pull Request Test Coverage Report for Build 10535483243

Details

• 2 of 23 (8.7%) changed or added relevant lines in 1 file are covered.
• 6 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.02%) to 89.166%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/circuit_data.rs	2	23	8.7%
<!--	Total:	2	23	8.7%	-->

Files with Coverage Reduction	New Missed Lines	%
crates/accelerate/src/two_qubit_decompose.rs	1	90.82%
crates/qasm2/src/lex.rs	5	91.73%
<!--	Total:	6		-->

Totals
Change from base Build 10529709209:	-0.02%
Covered Lines:	71504
Relevant Lines:	80192

---

💛 - Coveralls

[raynelfss]

@kevinhartman I agree with making most of the fields in CircuitData publicly visible except for two of them.

• The data field: Although it would be useful, exposing it would allow it to be mutable if the CircuitData instance happens to be owned or mutably borrowed. I'd rather just expose an iterator that returns an immutable reference (like we do with iter()) and/or expose a function that returns an immutable view of the data field.
• The ParameterTable: this attribute is fully private and should not be exposed anywhere else as far as I know based on @jakelishman's assessment.

Thus, that's why I think having both interners and the registers (BitData) be the only attributes to be made public to the crate is the correct approach for now.

[mtreinish]

What about global_phase?

[kevinhartman]

exposing it would allow it to be mutable if the CircuitData instance happens to be owned or mutably borrowed.

That's true, but what I'm getting at is having any of the fields public (e.g. the DAG's bits) makes it possible for things within qiskit-circuit to modify DAGCircuit in ways that make it invalid. For example, with the current state of this PR, we could remove the DAG's bits even if it contains operations that use them. It seems odd to force the implementor of circuit_to_dag (and the rest of this crate) to use some parts of the proper / public DAGCircuit (e.g. DAGCircuit::iter) but also expose a select few of its internal fields. My thinking is that because what we're exposing here already gives qiskit-circuit the power to create bad data, we shouldn't try to pretend that the visibility has been designed to protect anything.

[kevinhartman]

I totally closed that by accident!! I'm so sorry @raynelfss >.<

[raynelfss]

Well in that case do you suggest to have functions that return immutable references instead, or functions that can unpack things. I.e. unpack_qarg() or get_qubits(), these could return immutable references and allow for cloning but never in-place modifications.

[jakelishman]

Since I was mentioned: my feeling is probably mostly with Kevin here, on the side of don't make things pub if you can break data coherence by mutating them (including assigning a new value to the field). That includes global_phase: you can break data coherence if you assign to it without suitably updating the parameter table.

Having immutable-view getters and safe setters is great. There'll definitely be places where we need to go further than that, and that'll involve thinking about what APIs we should expose that'll be both useful and efficient.

For CircuitData in particular, I think it's probably a good idea to not let the pub access get out of hand, even if in other places we've fallen more on the side of permissiveness for now - CircuitData is way more core, and the data-coherence requirements much harder to get right.

[kevinhartman]

Well in that case do you suggest to have functions that return immutable references instead

I would also vote for this option. Since PackedInstruction already exposes interner keys, it feels reasonable to me for CircuitData to expose immutable getters for the interners and bits etc. ^^

[jakelishman]

Fwiw, I do think many places will need to be able to query the interners in one way or another for performance, so either special methods, or making sure that there's a way to get at an immutable reference to an interner will be important, I think.

[raynelfss]

Alright then, I will make those changes promptly later in the day. Thank you for your input, team 🚀