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

Allow shots to be a numpy.int64 #7824

Merged
merged 3 commits into from
Apr 13, 2022
Merged

Allow shots to be a numpy.int64 #7824

merged 3 commits into from
Apr 13, 2022

Conversation

garrison
Copy link
Member

Summary

I've noticed that the Aer backends all work properly if the shots count is passed as a numpy.int64, but the BasicAer and hardware backends both fail in this case. The simplest way to make it work on all backends is by making this change in terra, inspired by #4591.

Details and comments

Basic test program for reproducing the bug:

import numpy as np

from qiskit import QuantumCircuit, execute, Aer, BasicAer, IBMQ
from qiskit.providers.ibmq import least_busy

qc = QuantumCircuit(2, 2)
qc.h(0)
qc.cx(0, 1)
qc.measure([0, 1], [0, 1])

def run(backend):
    sim = execute(qc, backend, shots=np.int64(1024))
    return sim.result().get_counts(qc)

# This works
run(Aer.get_backend("qasm_simulator"))

# This fails, but is fixed by the current change
run(BasicAer.get_backend("qasm_simulator"))

# This also fails, and is also fixed by the current change
provider = IBMQ.load_account()
run(
    least_busy(
        provider.backends(filters=lambda x: x.configuration().n_qubits >= 2, simulator=False)
    )
)

I've noticed that the Aer backends, all work properly if the `shots`
count is passed as a `numpy.int64`, but the BasicAer and hardware
backends both fail in this case.  The simplest way to make it work
on all backends is by making this change in terra, inspired by Qiskit#4591.
@garrison garrison requested a review from a team as a code owner March 28, 2022 00:48
@coveralls
Copy link

coveralls commented Mar 28, 2022

Pull Request Test Coverage Report for Build 2161409796

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 6 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.0002%) to 83.944%

Files with Coverage Reduction New Missed Lines %
qiskit/compiler/assembler.py 6 96.23%
Totals Coverage Status
Change from base Build 2157761883: 0.0002%
Covered Lines: 54202
Relevant Lines: 64569

💛 - Coveralls

@jakelishman jakelishman added this to the 0.20.1 milestone Mar 31, 2022
@@ -321,7 +322,7 @@ def _parse_common_args(
shots = min(1024, max_shots)
else:
shots = 1024
elif not isinstance(shots, int):
elif not isinstance(shots, numbers.Integral):
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to use numpy.integer here instead of the abstract numbers class from stdlib. We've tried to use the abstract numbers classes before and had performance regressions each time (because of how the abstract classes hook into the checks for isinstance). While assemble isn't super performance critical it's still used in job submission path a surprising number of place and this is only just a single isinstance call so the extra overhead isn't as huge as something checked on each instruction in a circuit, I still think it would probably be better to just use numpy.integer even if that means other int types from other external libraries can't be used. In practice I don't think anything besides numpy ints will really come up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds reasonable; I've pushed a fix.

@mtreinish mtreinish added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: Bugfix Include in the "Fixed" section of the changelog automerge labels Apr 13, 2022
@mergify mergify bot merged commit d9a6f0b into Qiskit:main Apr 13, 2022
mergify bot pushed a commit that referenced this pull request Apr 13, 2022
* Allow `shots` to be a `numpy.int64`

I've noticed that the Aer backends, all work properly if the `shots`
count is passed as a `numpy.int64`, but the BasicAer and hardware
backends both fail in this case.  The simplest way to make it work
on all backends is by making this change in terra, inspired by #4591.

* Allow shots to be either an int or a numpy.integer, explicitly

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit d9a6f0b)
@garrison garrison deleted the int64_shots branch April 13, 2022 15:07
mergify bot added a commit that referenced this pull request Apr 13, 2022
* Allow `shots` to be a `numpy.int64`

I've noticed that the Aer backends, all work properly if the `shots`
count is passed as a `numpy.int64`, but the BasicAer and hardware
backends both fail in this case.  The simplest way to make it work
on all backends is by making this change in terra, inspired by #4591.

* Allow shots to be either an int or a numpy.integer, explicitly

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit d9a6f0b)

Co-authored-by: Jim Garrison <garrison@ibm.com>
garrison added a commit to qiskit-community/prototype-qrao that referenced this pull request Jan 10, 2023
which was fixed in Qiskit/qiskit#7824
which made it into Terra 0.20.1
but we now depend on Terra 0.22 or higher
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants