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

Performance improvement of _DiagonalEstimator #9229

Merged
merged 4 commits into from
Dec 5, 2022

Conversation

t-imamichi
Copy link
Member

@t-imamichi t-imamichi commented Dec 2, 2022

Summary

I notice that SamplingVQE is slower than the former VQE when I update a tutorial of Qiskit optimization.
The bottleneck is _DiagonalEstimator._evaluate_sparsepuali. See qiskit-community/qiskit-optimization#448 (comment) for details.
I optimized that part with numpy.

Details and comments

microbenchmark

from timeit import timeit
from qiskit.algorithms.minimum_eigensolvers.diagonal_estimator import _evaluate_sparsepauli
from qiskit.quantum_info import SparsePauliOp
from qiskit.quantum_info.random import random_pauli_list

seed = 123
n = 100
m = 100

op = SparsePauliOp(random_pauli_list(num_qubits=n, size=m, seed=seed))

print(_evaluate_sparsepauli(2<<n-1, op))
print(f'{timeit(lambda: _evaluate_sparsepauli(2<<n-1, op), number=100)} sec')

main

(-2+4j)
0.5635605140000001 sec

this PR

(-2+4j)
0.0018880719999999629 sec

benchmark with QAOA

from qiskit_optimization.algorithms import MinimumEigenOptimizer
from qiskit.utils import algorithm_globals
from qiskit.algorithms.minimum_eigensolvers import QAOA
from qiskit.algorithms.optimizers import COBYLA
from qiskit.primitives import Sampler
from qiskit_optimization.applications import Knapsack

algorithm_globals.random_seed = 123

prob = Knapsack(values=[3, 4, 5, 6, 7], weights=[2, 3, 4, 5, 6], max_weight=10)
qp = prob.to_quadratic_program()

# QAOA
meo = MinimumEigenOptimizer(min_eigen_solver=QAOA(reps=1, sampler=Sampler(), optimizer=COBYLA(maxiter=100)))
result = meo.solve(qp)
print(result.prettyprint())
print("\nsolution:", prob.interpret(result))
print("\ntime:", result.min_eigen_solver_result.optimizer_time)

main

objective function value: 13.0
variable values: x_0=1.0, x_1=1.0, x_2=0.0, x_3=1.0, x_4=0.0
status: SUCCESS

solution: [0, 1, 3]

time: 13.972561120986938

this PR

objective function value: 13.0
variable values: x_0=1.0, x_1=1.0, x_2=0.0, x_3=1.0, x_4=0.0
status: SUCCESS

solution: [0, 1, 3]

time: 0.8536498546600342

@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

@coveralls
Copy link

coveralls commented Dec 2, 2022

Pull Request Test Coverage Report for Build 3618715227

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

Files with Coverage Reduction New Missed Lines %
src/sabre_swap/layer.rs 2 98.95%
Totals Coverage Status
Change from base Build 3618561726: 0.04%
Covered Lines: 63148
Relevant Lines: 74645

💛 - Coveralls

@t-imamichi
Copy link
Member Author

It would be great if this PR is backport stable because Qiskit optimization relies on SamplingVQE.

@Cryoris
Copy link
Contributor

Cryoris commented Dec 2, 2022

LGTM, but I'll let @mtreinish or @jakelishman comment on the backporting part 🙂 From my side backporting would be fine considering this a "performance bug".... 😁

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.

I think we can include this in a backport safely enough, as long as there's direct test coverage that the change produces correct results.

Every time I get tagged in one of these I always end up getting performance nerd-sniped. Here's my offering:

_PARITY = np.array([-1 if bin(i).count("1") % 2 else 1 for i in range(256)], dtype=np.complex128)

def _evaluate_sparsepauli(state: int, observable: SparsePauliOp) -> complex:
    packed_uint8 = np.packbits(observable.paulis.z, axis=1, bitorder="little")
    state_bytes = np.frombuffer(state.to_bytes(packed_uint8.shape[1], "little"), dtype=np.uint8)
    reduced = np.bitwise_xor.reduce(packed_uint8 & state_bytes, axis=1)
    return np.sum(observable.coeffs * _PARITY[reduced])

For small numbers of qubits and operators in the list, it's about ~10-20% faster, but say for an observable with 100q and 100 Paulis, it's nearly 10x faster due to avoiding the Python object conversions. For the optimisation code block given in the top comment, on my machine the evaluation went from 11.5s on main to 1.1s with the version in this PR, and about 0.8s with my version of this function (I didn't time very accurately).

@woodsp-ibm woodsp-ibm mentioned this pull request Dec 4, 2022
t-imamichi and others added 2 commits December 5, 2022 17:06
Co-authored-by: Jake Lishman <[email protected]>
@t-imamichi t-imamichi added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Dec 5, 2022
@t-imamichi
Copy link
Member Author

Thank you, Jake. Your code is much faster than mine. Cool!
I added a short reno too.

Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

LGTM thanks for the improvement (and nerd-sniping 😉)!

@Cryoris Cryoris added stable backport potential The bug might be minimal and/or import enough to be port to stable automerge labels Dec 5, 2022
@mergify mergify bot merged commit 0df0d29 into Qiskit:main Dec 5, 2022
mergify bot pushed a commit that referenced this pull request Dec 5, 2022
* performance improvement of _DiagonalEstimator

* optimize

Co-authored-by: Jake Lishman <[email protected]>

* add reno

Co-authored-by: Jake Lishman <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 0df0d29)
@t-imamichi t-imamichi deleted the fast-diag branch December 5, 2022 13:02
Cryoris pushed a commit to Cryoris/qiskit-terra that referenced this pull request Jan 12, 2023
* performance improvement of _DiagonalEstimator

* optimize

Co-authored-by: Jake Lishman <[email protected]>

* add reno

Co-authored-by: Jake Lishman <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
mergify bot added a commit that referenced this pull request Jan 13, 2023
* performance improvement of _DiagonalEstimator

* optimize

Co-authored-by: Jake Lishman <[email protected]>

* add reno

Co-authored-by: Jake Lishman <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 0df0d29)

Co-authored-by: Takashi Imamichi <[email protected]>
Co-authored-by: Julien Gacon <[email protected]>
Co-authored-by: Jake Lishman <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
ElePT pushed a commit to ElePT/qiskit that referenced this pull request Jun 27, 2023
* performance improvement of _DiagonalEstimator

* optimize

Co-authored-by: Jake Lishman <[email protected]>

* add reno

Co-authored-by: Jake Lishman <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
ElePT pushed a commit to ElePT/qiskit-algorithms-test that referenced this pull request Jul 17, 2023
* performance improvement of _DiagonalEstimator

* optimize

Co-authored-by: Jake Lishman <[email protected]>

* add reno

Co-authored-by: Jake Lishman <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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 mod: algorithms Related to the Algorithms module performance 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.

5 participants