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 aqua from benchmark #1189

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

hhorii
Copy link
Collaborator

@hhorii hhorii commented Mar 20, 2021

Summary

This PR removes aqua from benchmark in test

Details and comments

Aqua is now being refactored and VQE and UCCSD will be in qiskit-nature.
However, their new versions are under development.
Until they are released, benchmarks do not include VQE applications.

@hhorii hhorii force-pushed the remove_aqua_from_benchmark branch from 5d87bd7 to fdb25e0 Compare March 20, 2021 09:58
@mtreinish
Copy link
Member

mtreinish commented Mar 20, 2021

I would say lets keep the benchmarks in place as is until qiskit-nature is released, aqua will continue to work for several months as is, and we can replace the benchmarks at some point after the release with a qiskit-nature based versions (and just bump the version of the benchmark then). That way we don't lose any coverage, especially right up until the release.

@hhorii hhorii changed the title remove aqua from benchmark [WIP] remove aqua from benchmark Mar 22, 2021
@hhorii
Copy link
Collaborator Author

hhorii commented Mar 22, 2021

@mtreinish OK. Please let me leave this as a placeholder for future change after qiskit-nature is released.

@hhorii hhorii changed the title [WIP] remove aqua from benchmark Remove aqua from benchmark Nov 12, 2021
@hhorii
Copy link
Collaborator Author

hhorii commented Nov 12, 2021

https://hhorii.github.io/qiskit-aer/html/#/

@mtreinish qiskit_nature was released and now it is good timing to work this again.

test/asv.linux.conf.json Outdated Show resolved Hide resolved
test/asv.linux.cuda.conf.json Outdated Show resolved Hide resolved
test/benchmark/simulator_benchmark.py Outdated Show resolved Hide resolved
test/benchmark/vqe_application.py Outdated Show resolved Hide resolved
test/benchmark/vqe_application.py Outdated Show resolved Hide resolved
test/benchmark/vqe_application.py Outdated Show resolved Hide resolved
test/benchmark/vqe_application.py Outdated Show resolved Hide resolved
test/benchmark/vqe_application.py Outdated Show resolved Hide resolved
test/benchmark/vqe_application.py Outdated Show resolved Hide resolved
test/benchmark/vqe_application.py Outdated Show resolved Hide resolved
test/asv.linux.conf.json Outdated Show resolved Hide resolved
test/asv.linux.conf.json Outdated Show resolved Hide resolved
test/benchmark/simulator_benchmark.py Outdated Show resolved Hide resolved
test/benchmark/simulator_benchmark.py Outdated Show resolved Hide resolved
@hhorii hhorii changed the title Remove aqua from benchmark [WIP] Remove aqua from benchmark Jan 4, 2022
@hhorii hhorii force-pushed the remove_aqua_from_benchmark branch 4 times, most recently from d6eb2ee to 9e66f3d Compare January 14, 2022 00:52
Previous benchmarks tracked only C++ implementation.
However, in recent releases, functions in Python were increased to
transpile, assemble, and support control-flow.
With this change, benchmarks additionally track python implementation.
@hhorii hhorii changed the title [WIP] Remove aqua from benchmark Remove aqua from benchmark Jan 18, 2022
test/benchmark/default_simulator.py Outdated Show resolved Hide resolved
test/benchmark/default_simulator.py Outdated Show resolved Hide resolved
@hhorii hhorii force-pushed the remove_aqua_from_benchmark branch from 2e20fb0 to 3c4f77d Compare March 15, 2022 11:52
@hhorii hhorii force-pushed the remove_aqua_from_benchmark branch from 3c4f77d to 43a1b20 Compare March 15, 2022 12:37
@hhorii hhorii requested a review from mtreinish March 30, 2022 01:21
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

This LGTM, just a few small inline questions/comments but otherwise I'm fine with this.


// The number of characters to retain in the commit hashes.
// "hash_length": 8,

// `asv` will cache results of the recent builds in each
// environment, making them faster to install next time. This is
// the number of builds to keep, per environment.
// "build_cache_size": 2,
"build_cache_size": 100000
Copy link
Member

Choose a reason for hiding this comment

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

Heh, that's a lot of builds

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch :-)

@@ -33,27 +33,19 @@
"install_command": [
"python -c \"import shutil; shutil.rmtree('{build_dir}/qiskit', True)\"",
"python -c \"import shutil; shutil.rmtree('{build_dir}/qiskit_aer.egg-info', True)\"",
"pip install git+https://github.com/Qiskit/qiskit-terra",
"pip install git+https://github.com/Qiskit/qiskit-aqua",
"pip install -U qiskit-terra",
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we want to use matrix here to list qiskit-terra at a specific version so the benchmark results are tied to the qiskit-terra version (so if we bump the terra version in the matrix it shows as different from earlier runs).

Comment on lines +34 to +40
try:
from qiskit.providers.aer import AerSimulator
self._simulator = AerSimulator()
except ImportError:
from qiskit.providers.aer import QasmSimulator
self._simulator = QasmSimulator()
return self._simulator
Copy link
Member

Choose a reason for hiding this comment

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

Are you still supporting comparisons from the releases prior to AerSimulator?

self._simulator = None
self.qubits = qubits
self.params = (qubits)
self.param_names = ["qubits"]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.param_names = ["qubits"]
self.param_names = ["Number of qubits"]

@mtreinish mtreinish dismissed their stale review May 10, 2022 13:26

Earlier comments addressed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants