-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[DAGCircuit Oxidation] Refactor bit management in CircuitData
#12372
base: main
Are you sure you want to change the base?
Conversation
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 9192883136Warning: 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 |
struct BitAsKey { | ||
/// Python's `hash()` of the wrapped instance. | ||
hash: isize, | ||
/// The wrapped instance. | ||
bit: PyObject, | ||
} |
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'm wondering if we maybe should remove one of the layers of abstraction here. It's something I've been thinking about in #12205 is that the source of truth for bits is still python but for interaction from rust space this isn't super ergonomic. I'm wondering if instead of playing games like this we try flattening the two representations. I guess the complexity is things like register and index (although #11596 will take care of that)
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'm not sure if this is what you were getting at, but I was thinking longer term that we'll want/need to make it possible to construct circuits from Rust space without creating any Python objects at all. We might not be at that point yet, but it might to be thinking about it.
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.
Yeah, that's basically what I was getting at, having a pattern for making a circuit without any python interaction is what we'll want longer term. The example I have for making circuits from rust space right now still needs a py
handle because we need to instantiate Qubit
objects.
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'd agree as well. BitAsKey
isn't new here, though. I've only moved it from CircuitData
.
Also, the source of truth for bits is CircuitData
. It owns its bits, and QuantumCircuit
simply "references" them.
I'm rethinking a bit of this PR (specifically the stuff with packing and interning) so I'm going to put this into draft for now. The |
One or more of the following people are relevant to this code:
|
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 looks fine to me it's mostly moving code around, just a couple of questions inline.
// This really shouldn't fail, but if it does, | ||
// we'll just use 0. | ||
hash: bit.hash().unwrap_or(0), | ||
bit: bit.into_py(bit.py()), |
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.
Is there a reason to not use unbind()
here?
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.
We can, it didn't exist back when I wrote this code originally (I just moved it here).
#[pymethods] | ||
impl CircuitInstruction { | ||
#[new] | ||
pub fn new( | ||
pub fn py_new( |
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, I like this pattern, this is something we need to do a lot and have a dedicated python space constructor like this works well.
operation: PyObject, | ||
qubits: impl IntoIterator<Item = T1, IntoIter = U1>, | ||
clbits: impl IntoIterator<Item = T2, IntoIter = U2>, | ||
) -> PyResult<Py<Self>> |
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.
If this is the rust space constructor why are we putting it in a Py<>
?
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.
It's an interesting point. It'd probably be a better factoring to just return Self
, but the only callers right now stuff it into a Py
so I did it here for less code.
I think there's a much larger conversation we should have around the separation we'd ultimately want between the Python and Rust (model) hierarchies. Especially with the foresight that we'll probably want a pure Rust API at some point, which would likely mean having core Rust-only structs and then PyO3 wrappers around those.
Personally, I'd like the qiskit-circuit
crate to not depend on PyO3 at all, and then to have a new crate qiskit-pyo3
that depends on PyO3 and qiskit-circuit
(and other core Rust-only crates) to bridge everything together. Before we get to that point, I'm not sure it matters too much for new
versus py_new
to be entirely Python versus entirely Rust, since I'd think we'd want to end up with two separate structs for each case, anyway.
#[derive(Copy, Clone, Debug, Hash, Ord, PartialOrd, Eq, PartialEq)] | ||
struct Qubit(BitType); | ||
#[derive(Copy, Clone, Debug, Hash, Ord, PartialOrd, Eq, PartialEq)] | ||
struct Clbit(BitType); |
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.
Should these be public?
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.
At some point, yes. But, they aren't used in the public interface of the crate at the moment.
I also want to call out that I'm not especially keen on us implementing From<BitType>
for Qubit
and Clbit
and From<Qubit|Clbit>
for BitType
, as I've done here. To me, that exposes what could be an implementation detail of BitData
(how the bits are mapped between the native type and Python instances).
I have a separate branch where I explored removing those conversions. I thought it would be nice if Qubit
and Clbit
were defined in bit_data.rs
, and the implementation of BitData
encapsulated the awareness of these conversions to and from a u32
(BitType
), and then we simply exposed Qubit
and Clbit
opaquely in lib.rs
. I decided against this for now, since Python users already expect that bits are contiguous and ordered by insertion in a circuit (and hence convertible to and from an index). But I'd be curious to hear your thoughts, and perhaps what @jakelishman has to say when he's back from holiday.
Summary
This is part of the work we're doing to port
DAGCircuit
to Rust. It's work I've pulled out of a local branch where I'm doing that port, in an effort to reduce the size of that eventual PR.The primary improvement that this brings is the introduction of
BitData
, which encapsulates keeping Python and native bit indices in sync. It provides methods likemap_indices
andmap_bits
which improve the ergonomics of translating between native and Python bits at PyO3 boundaries.Details and comments
Qubit
andClbit
representation.BitData
which encapsulates maintaining a mapping between native Rust bit types and Python bit types. This significantly cleans up the code inCircuitData
and makes it reusable forDAGCircuit
.InternContext
into a more genericIndexedInterner
data structure, which implements the newInterner
trait with slotted interning for anyT
which isHash
andEq
.Interner
trait, for defining interner semantics for containers likeIndexedInterner
.The one caveat to this PR is that we now no longer share interning between
Qubit
andClbit
indices, since this is more difficult from a typing perspective. If we want to share a singleIndexedInterner
for bothQubit
andClbit
operand lists, this would be possible, but we should revisit it. For now I wanted to just focus on keeping things strongly typed so that our Rust code doesn't get overly messy at this early stage.