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.

Sign up for GitHub

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

Jump to bottom

Resolve regression from introduction of AER::Config #1747

Merged
merged 3 commits into from
Apr 20, 2023
Merged

Resolve regression from introduction of AER::Config #1747

merged 3 commits into from
Apr 20, 2023

Conversation

hhorii
Copy link
Collaborator

@hhorii hhorii commented Mar 14, 2023 •
edited
Loading

Summary

Resolve #1746.

Details and comments

AER::Config is copied frequently in AER::Transpile::CircuitOptimization. However, the copied instance is not necessary. This PR removes this redundant copy and resolve performance regression from 0.11.2.

image

@hhorii hhorii force-pushed the improve_aer_config branch from 41b015f to 5deb2a1 Compare March 14, 2023 03:07
jakelishman
jakelishman reviewed Mar 17, 2023
View reviewed changes
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.

Overall this seems sensible enough to me. There's quite a bunch of unrelated formatting changes that make it hard to quickly scan the changeset - if those could be undone and made part of the general clangfmt PR it might be a bit cleaner.

I think there's one place that might have been translated incorrectly, but the rest looks fine.

qiskit_aer/backends/aerbackend.py Outdated Show resolved Hide resolved
qiskit_aer/backends/aer_compiler.py Outdated Show resolved Hide resolved
src/controllers/aer_controller.hpp Outdated Show resolved Hide resolved
src/simulators/density_matrix/densitymatrix_state.hpp Outdated Show resolved Hide resolved
src/simulators/statevector/statevector_state.hpp Outdated Show resolved Hide resolved
src/simulators/statevector/statevector_state.hpp Outdated Show resolved Hide resolved
@hhorii hhorii force-pushed the improve_aer_config branch 3 times, most recently from 11a462b to d9d66f2 Compare March 20, 2023 08:09
@hhorii hhorii added this to the Aer 0.12.1 milestone Apr 6, 2023
@hhorii hhorii added the stable-backport-potential The issue or PR might be minimal and/or import enough to backport to stable label Apr 6, 2023
jakelishman
jakelishman reviewed Apr 13, 2023
View reviewed changes
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'm still confused by the use of JSON in this PR, but in principle the separation of common vs non-common operations seems ok to me.

src/framework/config.hpp Outdated Show resolved Hide resolved
src/simulators/statevector/statevector_state.hpp Outdated Show resolved Hide resolved
@hhorii hhorii force-pushed the improve_aer_config branch from 6ccf398 to 41e23fa Compare April 19, 2023 03:25
@hhorii hhorii changed the title Improve flexibility and performance of AER::Config Resolve regression from introduction of AER::Config Apr 19, 2023
@hhorii
Copy link
Collaborator Author

hhorii commented Apr 19, 2023

@jakelishman sorry. I re-investigated that where is the biggest overheads. I found that AER::Transpile::CircuitOptimization.set_config() copies AER::Config redundantly and then performance regression happens since 0.11.2.
In 0.11.2, json_t is used instead of AER::Config. This json has few fields in most cases and then the copy overheads are few.
Now, AER::Config has many fields and then copy overheads are increased. I tried to reduce this overheads by reducing not-frequently-used fields of AER::Config. But I found that just stopping this redundant copy makes performance better.

Now, this PR becomes very simple :-).

jakelishman
jakelishman approved these changes Apr 19, 2023
View reviewed changes
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.

Ha, that's a pretty funny fix in the end!

@hhorii hhorii merged commit c3070fb into Qiskit:main Apr 20, 2023
hhorii added a commit to hhorii/qiskit-aer that referenced this pull request Apr 20, 2023
@hhorii
Resolve regression from introduction of AER::Config (Qiskit#1747)
13f2a6c 
This commit reduces redundant copy in `AER::Transpile::CircuitOptimization`.
Since 0.12.0, configuration is `AER::Config` instead of `json_t`. This new class
has many fields and then its copy overheads become high. Reduction of
redundant copy improves performance especially for low-qubit simulation.
hhorii added a commit that referenced this pull request Jun 13, 2023
@hhorii @thesamesam
@mergify @mtreinish @1ucian0 @derwind @jakelishman @ihincks @hitomitak @SquidDev @ikkoham @doichanj @Eric-Arellano @deji725 @itoko @Innanov @dakk @TerraVenil
Release 0.12.1 (#1776)
462bade 
* Bump version strings to prepare for release

* Remove redundant wheel dep from pyproject.toml (#1741)

Remove the redundant `wheel` dependency, as it is added by the backend automatically.

Listing it explicitly in the documentation was a historical mistake and has been fixed since,
see: [pypa/setuptools@f7d30a9](pypa/setuptools@f7d30a9).

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* add code-formatting with black for python and with clang-format for c++ (#1630)

* add black and clang-format

* apply black and clang-format

* manually format codes that were not covered in black and clang-format

* add targets of clang-format and clean up tox.ini

* apply black with new configuration

* Update CONTRIBUTING.md

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Update .github/workflows/tests.yml

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* reformat qiskit_aer, test, tools, and setup.py with length 100

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Add git blame ignore file (#1745)

This commit adds a new file with the SHA1 of commits to ignore when
running git blame. This is important because of the recent adoption of
black and clang-format as our code formatting tool in #1630 we caused a
large amount of code churn to change the code formatting. However, using
the ignore file is a local opt-in feature for git and not something we
can enable globally by default. To facilitate this a section is added to
the bottom of the contributing guide to document how this file can be
used.

* deploy documentation in qiskit.org/ecosystem (#1748)

* deploy documentation in qiskit.org/ecosystem

* comment

* Remove a release note in an invalid location (#1750)

Essentially equivalent content is already incorporated in #1739.

* Fix how to reference to config.cuStateVec_enable (#1749)

* Fix how to reference to config.cuStateVec_enable

* Add a release note

* Update fix-cuStateVec_enable-0936f2269466e3be.yaml

---------

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* use `omp_set_max_active_levels` instead of `omp_set_nested` (#1752)

omp_set_nested was deprecated since OpenMP 3.0 and this commit
replaces it with omp_set_max_active_levels.

* use omp_set_max_active_levels instead of omp_set_nested
* use omp_set_nested in WIN
* Update releasenotes/notes/use_omp_set_max_active_levels-7e6c1d301c4434a6.yaml
* use _OPENMP to select omp_set_max_active_levels or omp_set_nested
---------

Co-authored-by: Jake Lishman <jake@binhbar.com>

* Ensure QuantumCircuit.metadata is always a dict (#1761)

* Ensure QuantumCircuit.metadata is always a dict

This is for compatibility with Qiskit/qiskit#9849

* fix order

* fix typo :(

* revert object->object() change

* make backportable

* Add Tutorials to Documentation Page (#1768)

* Add tutorials

* Add pandoc installation

---------

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>

* Defer gathering backends until they are needed (#1760)

* Defer gathering backends until they are needed

* Disable the not-an-iterable warning

Pylint infers _get_backends to always return None, even if we add type
annotations. Suppress the warning.

* Add @staticmethod to AerProvider._get_backends

---------

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>

* Fix wrong set of parameters in circuits with barriers (#1775)

* Fix wrong set of parameters in circuits with barriers

`AerCircuit` is created from a circuit by iterating its instrucitons
while skpping barrier. This leads inconsistency with positions of
parameter bindings. This commit adds barrier instruction to the class
and keeps positions of parameter bindings.

* fix lint error in test

* remove unused variable in test

* Add API docs for QuantumCircuit methods (#1753)

* Add new doc for circuit method apis

* Remove QuantumCircuit description

---------

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>

* Resolve regression from introduction of AER::Config (#1747)

This commit reduces redundant copy in `AER::Transpile::CircuitOptimization`.
Since 0.12.0, configuration is `AER::Config` instead of `json_t`. This new class
has many fields and then its copy overheads become high. Reduction of
redundant copy improves performance especially for low-qubit simulation.

* Remove REQUIRED to find PythonLibs in FindPybind11 (#1786)

* pin the version of scikit-build to 0.17.2

* Pin scikit-build version by 0.17.2

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* remove REQUIRED to find PythonLibs in FindPybind11

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Remove Sampler.close, assert DeprecationWarning of opflow, and update dependency (#1804)

Since qiskit-terra 0.24, `Sampler` does not have `close()` but Aer's `Sampler` still have the method. This commit takes the method as well as upgrade of Python versions in tutorial tests from 3.7 to 3.8.

* rm close
* assert DeprecationWarning
* update dependency

* Fix setting MPI processes and ranks (#1808)

0.12.0 accidentally removed MPI support if application does not use qobj. This commit reverts the change.

* Fix library path to cuQuantum and cuTENSOR (#1801)

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>

* Activate jQuery in docs (#1802)

Sphinx 6 no longer activates jQuery by default but `qiskit_sphinx_theme` still uses it.
This commits enables jQuery by adding the theme to extensions in conf.py.

---------

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>

* fix cuQuantum static linking (#1812)

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>

* Update standard_errors.py (#1799)

There was a typo in the rendering of the mathematical text on Depolarizing Error Page (https://qiskit.org/ecosystem/aer/stubs/qiskit_aer.noise.depolarizing_error.html)

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* Add implicit cast of argument types (#1770)

* Add implicit cast of argument types

Since 0.12.0, AerConfig is used to configure simulation, which
is directly bound to a AER::Config object through pybind.
This change requires application to specify strictly correct types
of configuration options. This commit allows implicit casting
to arguments if application specifies them with wrong types.
This commit resolves #1754.

* does not warning in cast of numpy classes if they are compatible with expected type
* fix lint issue
* simplify class checking

* Fix handling of None in noise model construction from BackendV2 (#1818)

Fixes `NoiseModel.from_backend` not to fail when calling it with a V2 backend having
faulty qubits such that T1 and T2 are `None` in the `target.qubit_properties`. 

* fix handling of None in noise model construction from BackendV2
* add reno
* simplify a bit
* update docs

* Add Getting Started page (#1780)

This commit adds documentation "Getting Started" page.

* Add Get Started page
* Remove unused class

---------
Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>

* avoid kernel crash from blas call errors (#1791)

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* fix fail of qobj run without run_options (#1792)

* fix fail of qobj run without run_options

* fix lint error

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* Support parameterized global phase (#1814)

* Support parameterized global phase

Though `ParameterExpression` can be set to `QuantumCircuit.global_phase`,
Aer does not bind parameter values to it in simulation phase. This commit fixes
this problem by resolving values of `global_phase` with specified `parameter_binds`
in `AerSimulator.run`.

* define AER::CONFIG::GLOBAL_PHASE_POS and its pybind
* fix lint issues
---------
Co-authored-by: Jun Doi <doichan@jp.ibm.com>

* check parameter_binds arguments for parameterized circuits (#1817)

Aer raises an error if `parameter_binds` is not specified though circuits have parameters.
This resolves the following issue:
#1773

* Set the number of qubits with the coupling map (#1825)

* Set n_qubits from coupling map

* Add a test and release notes

* Fix lint

---------

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>

* Fixing some typos (#1827)

* Update aer_densitymatrix.py

* Update aer_statevector.py

* Update README.md

* Update CONTRIBUTING.md

---------

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>

* Use transpile and run instead of execute in docstring (#1830)

* use transpile and run instead of execute in docstring

* use backend.run instead of execute in README

* use fake_provider for noise_model example

* Revert edits on README.md

* Update examples in docstring for qiskit_aer.noise

* Fix depolarizing noise model example

* Remove incorrect markup in noise/__init__.py

---------

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>

* Fix typos in aer_simulator, qasm_simulator docs (#1833)

* Fix typos.

* Add missing aer_simulator options.

* Update reference to SaveProbabilities according comment to PR.

* Upgrade to qiskit_sphinx_theme 1.12 (#1822)

* Warn if approximation=False and shots=None in Estimator (#1823)

* fix 1820

* rm unnecessary comment

* update docs

---------

Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>

* Fix the grouping index bug in Estimator (#1839)

* Fix the bug

* lint

* Validate parameters of each gate in a circuit (#1834)

This commit adds checks of arguments (A number of qubits, clbits,
and parameters) for each gate to prevent from unexpected memory access
when a user defines wrong custom gate with a number of a standard gate.

* validate parameters of each gate in a circuit
* fix lint error
* fix lint error

* Correct a type of variance from complex to real (#1767)

Previously Aer Estimator wrongly returned a complex value as variance of estimation values.
This commit fixes this behavior to return a real value.

* update version in docs/conf.py

* Do not use circuit metadata to internally manage simulation results (#1772)

* Stop using circuit metadata to internaly manage simulation results

This fixes `AerSimulator` to use circuit metadata to maintain mapping
from input and output of an executor call. This fixes an issue
#1723.

* add index of AER::Circuit and ExperimentResult

* add a link to an input circuit in each experiment result

* fix MPI build break (#1842)

* Batch QuantumCircuit with multiple parameter_binds (#1840)

`Estimator` in Aer did not use parameter bindings appropriately. Backend is called multiple times 
to simulate a circuit with multiple parameter-sets. This commit is to call backend once for multiple
parameter-sets for a circuit.

* WIP
* refactor
* lint
* add reno

* add release note with prelude

* *Support initialize with int (#1841)

Previously `QuantumCircuit.initialize` was not correctly preocessed
if initial state is specified with single `int` value. This commit
fixes this issue by decomposing initialize instructions.

---------

Co-authored-by: Sam James <sam@cmpct.info>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
Co-authored-by: derwind <uncertainty_principle_ss@yahoo.co.jp>
Co-authored-by: Jake Lishman <jake@binhbar.com>
Co-authored-by: Ian Hincks <ian.hincks@gmail.com>
Co-authored-by: Hitomi Takahashi <hitomi@jp.ibm.com>
Co-authored-by: Jonathan Coates <git@squiddev.cc>
Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
Co-authored-by: Jun Doi <doichan@jp.ibm.com>
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Co-authored-by: Deji Oyerinde <deji725@gmail.com>
Co-authored-by: Toshinari Itoko <15028342+itoko@users.noreply.github.com>
Co-authored-by: INNAN Nouhaila <64653897+Innanov@users.noreply.github.com>
Co-authored-by: Davide Gessa <gessadavide@gmail.com>
Co-authored-by: Oleksii Borodenko <kosunix@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakelishman jakelishman jakelishman approved these changes

Assignees
No one assigned
Labels
stable-backport-potential The issue or PR might be minimal and/or import enough to backport to stable
Projects
None yet
Milestone
Aer 0.12.1
Development

Successfully merging this pull request may close these issues.

Performance regression from 0.11.2 to 0.12.0
2 participants
@hhorii @jakelishman