-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Adding h, p and u to basic aer #10673
Conversation
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 5919992366
💛 - Coveralls |
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 added a release note.
In general, LGTM. I'm not sure about u
and its redundancy with u3
(should it be removed from the basisgates?) and U
.
Pull Request Test Coverage Report for Build 5938079564
💛 - Coveralls |
I would love to remove u3 but I did not want to force it now and break people code that have used u3 |
We removed the We could reduce our internal dependence on it a little more by rewriting more of the standard equivalence libraries to avoid it, but even there, it's only really used as an intermediary in the graph-search algorithm, so it never gets output unless a user's backend explicitly supports it. |
Jake I agree and It’s why I left it. My comment was more if we wanted to have a min set in basic air I would put [h p], [sx rz] and [u] and [U] then we could just say for anything more complicated use transpile. Today the h p breaks so why I added this. I am also totally ok leaving u1 u2 and u3 as it does not require much work to maintain it and the performance of a longer list to search I doubt is that much. |
Jay: I'm sorry - I got this discussion confused with other historical discussions about removing For "which basis gates should be in If it is going to be kept and expanded, then it may be more appropriate to do something like #7670 to let it handle almost all Qiskit instructions, not just a restricted set of basis gates? |
Jake - my understand and I agree with @blakejohnson @levbishop and @chriseclectic is we really need three primitives
I believe the plan is the 3 is how backend.run should evolve and what that means is we will need at least basic aer for a QASM simulator. I have no problem with it using statevetor inside but I would like a simple backend.run to run code that supports simple gates sets h, p and rz and sx cx Thanks |
Ie this code should work
|
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.
Trying to leave deeper discussions out for avoiding blocking, as we agree that "which basis gates should be in BasicAer" can include gates we currently have in https://github.com/Qiskit/qiskit/tree/main/qiskit/circuit/library/standard_gates, let's merge this PR
Summary
While statevector in quantum-info replaces most of the need of basic aer it does not replace QasmSimulator nor should it. I personally think we should remove the unitary and statevector from basicaer but that is a longer term issue. I know that we can use the transpile to rewrite into u1, u2, and u3 these simulations but I am giving some beginner demonstrators and not having h, p and u seems a little strange to require an extra line for using these (transpile). Furthermore since we want to depreciate u1,u2, and u3 it would be good to include all basis [h, p], u and [sx, rz] in basic aer.
Details and comments
@blakejohnson I can here you while I'm in the plane see this is why we need simulators :-)