Skip to content
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

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

Open
kdk opened this issue Feb 8, 2023 · 7 comments
Open
Labels

Comments

@kdk
Copy link
Member

kdk commented Feb 8, 2023

What should we add?

As came up in #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.

@kdk kdk added type: feature request New feature or request performance labels Feb 8, 2023
@jakelishman
Copy link
Member

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
Copy link
Member Author

kdk commented Feb 8, 2023

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.

@kdk kdk added good first issue Good for newcomers help wanted community contributions welcome. For filters like http://github-help-wanted.com/ labels Feb 8, 2023
@github-project-automation github-project-automation bot moved this to Tagged but unassigned in Contributor Monitoring Feb 9, 2023
@mustapha-saad
Copy link

Hi, I would love to work on this issue.

@yusharth
Copy link

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
Copy link
Contributor

@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
Copy link
Member

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.

@jakelishman jakelishman removed good first issue Good for newcomers help wanted community contributions welcome. For filters like http://github-help-wanted.com/ labels Sep 28, 2023
@yusharth
Copy link

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Tagged but unassigned
Development

No branches or pull requests

5 participants