search

Qiskit / qiskit

Qiskit is an open-source SDK for working with quantum computers at the level of extended quantum circuits, operators, and primitives.
https://www.ibm.com/quantum/qiskit
Apache License 2.0
5.26k stars 2.37k forks source link

No in-place dunder methods on `QuantumCircuitData` can lead to slow performance #9555

Open kdk opened 1 year ago

kdk commented 1 year ago

What should we add?

As came up in https://github.com/Qiskit/qiskit-terra/pull/9491 , QuantumCircuitData has no implementations of in-place dunder methods like __iadd__, but does have implementations of corresponding "normal" methods like __add__. (ref. https://docs.python.org/3/reference/datamodel.html#object.__iadd__ ) This can cause unexpected performance problems when writing code that attempts to perform in-place modifications of circuit.data ends up generating a lot of unnecessary copies.

We should either add the corresponding in-place implementations or raise an error re-directing users to the corresponding method on QuantumCircuit.

jakelishman commented 1 year ago

No code outside of QuantumCircuit should be mutating QuantumCircuit.data itself. I think QuantumCircuit.data was originally a hack to support legacy uses that did do this, but now any mutations are just a giant source of bugs, because there's more state in QuantumCircuit that needs to be kept in sync than just that object.

Ideally, we'd move to QuantumCircuit.data being an unsettable view-like object, I think, but I don't know how much code still relies on being able to mutate it.

kdk commented 1 year ago

I don't know how much code still relies on being able to mutate it.

I think this is the rub. QuantumCircuitData is currently modifiable and, for better or for worse, is used in that way. While the QuantumCircuit methods are preferable and guarantee consistency, they don't encapsulate all of what can be done with QuantumCircuitData (mostly on modifications of existing circuits). I wouldn't be against moving QuantumCircuitData to a read-only view, but I don't see that as a reason not to resolve problems with the current implementation.

mustapha-saad commented 1 year ago

Hi, I would love to work on this issue.

yusharth commented 1 year ago

Hey @mustapha-saad-codeStar, are you still working on this? If not I would like to take on this issue. Or we can work together to get a solution?

AngeloDanducci commented 1 year ago

@mustapha-saad @yusharth Hi all, you've expressed interest in this issue previously but it slipped through the cracks. If you are still interested in and available to contribute to this issue please ping me here and I will assign the issue to the first responder.

jakelishman commented 1 year ago

This issue is probably not a good candidate to work on right now, because of other work going on in #10827 that will have serious ramifications for it.

yusharth commented 1 year ago

Hey, @AngeloDanducci I would love to contribute but it seems there was some confusion regarding this issue. Is there any other issue on which I can lend you hands or might be appropriate to work on?