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

Remove BasicAer shot limit - QAMP #7801

Merged
merged 11 commits into from
May 3, 2022
Merged

Remove BasicAer shot limit - QAMP #7801

merged 11 commits into from
May 3, 2022

Conversation

alejomonbar
Copy link
Contributor

@alejomonbar alejomonbar commented Mar 22, 2022

Summary

Fixes #7634
Executing the following code gives an error because max_shots for BasicAer has a limit of 65536 shots.

from qiskit import BasicAer, QuantumCircuit

backend = BasicAer.get_backend('qasm_simulator')
circuit = QuantumCircuit(1,1)
circuit.x(0)
circuit.measure(0,0)
job = backend.run(circuit, shots=int(1e6))

Details and comments

The idea is that BasicAer does not have a limit in terms of the number of shots. Instead, the default value equals zero which works as no restriction on the number of shots.

Part of qiskit-advocate/qamp-spring-22#37

At this point, there is an arbitrary number for the maximum shots in the BasicAer simulator "65536". The solution proposed @kevinsung is to work with zero as the default value of max_shots. This solves the problem with the evaluation of max_shots in the assemble function "_parse_common_args" in qiskit/compiler/assembler.py.
@alejomonbar alejomonbar requested review from a team and jyu00 as code owners March 22, 2022 11:51
@jakelishman jakelishman added this to the 0.21 milestone Mar 30, 2022
correcting some errors about where to add the test.
@coveralls
Copy link

coveralls commented Apr 26, 2022

Pull Request Test Coverage Report for Build 2265135965

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 84.349%

Totals Coverage Status
Change from base Build 2263072888: 0.0%
Covered Lines: 54463
Relevant Lines: 64569

💛 - Coveralls

@alejomonbar
Copy link
Contributor Author

@kevinsung now this issue is passing.

@kevinsung
Copy link
Contributor

kevinsung commented Apr 26, 2022

@alejomonbar Please use descriptive titles for your pull requests. For example, "Remove BasicAer shot limit". Also, if you add the sentence "Fixes #7634" somewhere in the description then this PR will automatically close that issue.

@alejomonbar alejomonbar changed the title Fixissue7634 Remove BasicAer shot limit - Fixissue7634 Apr 26, 2022
@kevinsung
Copy link
Contributor

@jakelishman thoughts on this solution?

@jakelishman jakelishman changed the title Remove BasicAer shot limit - Fixissue7634 Remove BasicAer shot limit Apr 27, 2022
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks! Please can you also add an "upgrade" release note that says that the limit has been removed?

Do you know how long the new test takes to run?

@alejomonbar
Copy link
Contributor Author

Looks good to me, thanks! Please can you also add an "upgrade" release note that says that the limit has been removed?

Do you know how long the new test takes to run?

Thank you @jakelishman, I added the release note. About the time, I don't know if there is any command to know it, but, the circuit for the test is simple and only requires 2e6 shots, I don't think this adds much extra time for the tox test.

alejomonbar and others added 3 commits April 28, 2022 11:58
….yaml

Co-authored-by: Kevin J. Sung <kevinjefferysung@gmail.com>
The test previously just ran `BasicAer` with two million shots on the
transpiled circuit to see if it encountered a limit.  This is simple
enough functionality (and unlikely enough to be broken accidentally)
that it can do without a test - the previous one took about 8 seconds,
which is a long time for testing a toggle.
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two million shots is actually quite a lot, especially for a simulator implemented in pure Python. As things stood, the test took about 8 seconds. Taken alone, that's ok, but Terra's test suite has something like ten to fifteen thousand tests in it, and at that scale, 8 seconds is a long time to test on a simple toggle.

While writing the test was definitely the right thing to do, in this very particular circumstance, I think the best course of action is just to not have one. It can be hard to test for the absence of a limit efficiently, and this particular feature is something that's very unlikely to be accidentally changed - if a limit is re-instated in the future, it'll almost certainly be somebody having made a deliberate technical choice.

Thanks for the contribution!

@jakelishman jakelishman added Changelog: API Change Include in the "Changed" section of the changelog automerge labels May 3, 2022
@mergify mergify bot merged commit 1f052be into Qiskit:main May 3, 2022
@alejomonbar alejomonbar changed the title Remove BasicAer shot limit Remove BasicAer shot limit - QAMP May 18, 2022
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.

BasicAer fails when shots requested exceeds a threshold
4 participants