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

Adding h, p and u to basic aer #10673

Merged
merged 7 commits into from
Sep 8, 2023
Merged

Adding h, p and u to basic aer #10673

merged 7 commits into from
Sep 8, 2023

Conversation

jaygambetta
Copy link
Member

@jaygambetta jaygambetta commented Aug 20, 2023

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 :-)

@jaygambetta jaygambetta requested review from jyu00 and a team as code owners August 20, 2023 22:39
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

@coveralls
Copy link

Pull Request Test Coverage Report for Build 5919992366

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.03%) to 87.275%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 2 91.14%
qiskit/pulse/library/waveform.py 3 93.75%
Totals Coverage Status
Change from base Build 5902014500: 0.03%
Covered Lines: 74372
Relevant Lines: 85216

💛 - Coveralls

@1ucian0 1ucian0 added this to the 0.45.0 milestone Aug 22, 2023
Copy link
Member

@1ucian0 1ucian0 left a 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.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 5938079564

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • 10 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.004%) to 87.28%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 4 91.39%
crates/qasm2/src/parse.rs 6 97.6%
Totals Coverage Status
Change from base Build 5934004311: 0.004%
Covered Lines: 74378
Relevant Lines: 85218

💛 - Coveralls

@jaygambetta
Copy link
Member Author

I would love to remove u3 but I did not want to force it now and break people code that have used u3

@jakelishman
Copy link
Member

We removed the QuantumCircuit.u3 method (and the other Euler-angle gates), so it's already pretty hidden. Actually removing the U3Gate class would break a huge amount of extant code, since it's a base gate in both the OQ2 and OQ3 standard libraries (in as much as OQ2 has a standard library), and it doesn't cost us anything to maintain the simple gate definition, which makes it much easier for users to on-ramp to Qiskit. Since we're committed to supporting OpenQASM with first-class support in Qiskit, it would be a bit strange for us to remove the native representation of one of their standard gates.

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.

@jaygambetta
Copy link
Member Author

jaygambetta commented Aug 23, 2023

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.

@jakelishman
Copy link
Member

Jay: I'm sorry - I got this discussion confused with other historical discussions about removing u1/u2/u3 in general from Qiskit.

For "which basis gates should be in BasicAer?", I guess to me it depends more on what the purpose of it really is. When I joined, I'd understood it as a sort of "test backend" (i.e. #4443), in which case imo it doesn't hugely matter what the basis is because somebody should always call transpile (and it's a noiseless simulator, so gate-specific error/coupling concerns don't come in). At the time of #4443 and when it was discussed later, this seemed to be the way everyone wanted.

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?

@jaygambetta
Copy link
Member Author

Jake - my understand and I agree with @blakejohnson @levbishop and @chriseclectic is we really need three primitives

  1. Estimator
  2. Sampler
  3. run a single Qasm3 and return output variables.

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

@jaygambetta
Copy link
Member Author

Ie this code should work

from qiskit import QuantumCircuit
from qiskit.providers.basicaer.qasm_simulator import QasmSimulatorPy
from qiskit.compiler import transpile

backend = QasmSimulatorPy()

circuit = QuantumCircuit(1,1)
circuit.h(0)
circuit.measure([0], [0])

result = backend.run(circuit).result()
counts = result.get_counts()
print(counts)

@1ucian0 1ucian0 self-assigned this Sep 6, 2023
Copy link
Member

@1ucian0 1ucian0 left a 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

@1ucian0 1ucian0 added this pull request to the merge queue Sep 8, 2023
Merged via the queue into Qiskit:main with commit cd507ad Sep 8, 2023
13 checks passed
@1ucian0 1ucian0 added Changelog: New Feature Include in the "Added" section of the changelog and removed Changelog: New Feature Include in the "Added" section of the changelog labels Sep 28, 2023
@1ucian0 1ucian0 added the Changelog: API Change Include in the "Changed" section of the changelog label Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the "Changed" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants