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

Fix AerBackend issues caused by upgrading BackendV2 #1995

Merged
merged 27 commits into from
Nov 24, 2023

Conversation

doichanj
Copy link
Collaborator

@doichanj doichanj commented Nov 10, 2023

Summary

This PR fixes BackendV2 implementations in Aer compatible for Qiskit 0.45

Details and comments

This PR fixes issues :

Also this PR fixes test failure in Qiskit test.python.circuit.test_controlled_gate it requires fix in Qiskit Qiskit/qiskit#11172

For #1987 if input backend does not have description, it fails in Qiskit, so this fix add description.

For #1988, Aer did not build coupling map from option, so this fix check additional options and build coupling map

For #1982, added custom transpiler pass to rebuild basis gates including only gates appears in the input circuit, that prevent unnecessary change of gate. This is enabled when optimization_level < 2

@doichanj doichanj added Changelog: Bugfix Include in the Fixed section of the changelog stable-backport-potential The issue or PR might be minimal and/or import enough to backport to stable labels Nov 10, 2023
@doichanj doichanj changed the title [WIP] Fix Aer Backend for Qiskit 0.45 Fix AerBackend issues caused by upgrading BackendV2 Nov 16, 2023
@doichanj doichanj requested a review from mtreinish November 16, 2023 09:21
@doichanj doichanj mentioned this pull request Nov 16, 2023
@@ -518,6 +518,7 @@ class AerSimulator(AerBackend):
"continue_loop",
"reset",
"switch_case",
"barrier",
Copy link
Member

Choose a reason for hiding this comment

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

Barrier typically isn't needed in a target because qiskit always assumes barrier is valid for a backend since it's primarily a compiler directive. Was there something specific that was failing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed barrier and add barrier inside plugin.
Adding barrier is needed because barrier can not be found in any name mapping and plugin returns with unsupported operation

Comment on lines 277 to 309
"u1": U1Gate,
"u2": U2Gate,
"u3": U3Gate,
"u": UGate,
"p": PhaseGate,
"r": RGate,
"rx": RXGate,
"rxx": RXXGate,
"ry": RYGate,
"ryy": RYYGate,
"rz": RZGate,
"rzz": RZZGate,
"rzx": RZXGate,
"id": IGate,
"h": HGate,
"x": XGate,
"y": YGate,
"z": ZGate,
"s": SGate,
"sdg": SdgGate,
"sx": SXGate,
"sxdg": SXdgGate,
"t": TGate,
"tdg": TdgGate,
"swap": SwapGate,
"cx": CXGate,
"cy": CYGate,
"cz": CZGate,
"ccx": CCXGate,
"csx": CSXGate,
"cp": CPhaseGate,
"cu": CUGate,
"cu1": CU1Gate,
Copy link
Member

Choose a reason for hiding this comment

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

For the standard gates they're covered by default from https://github.com/Qiskit/qiskit/blob/main/qiskit/circuit/library/standard_gates/__init__.py#L49 the custom name mapping passed to convert_to_target() is additive, the code takes get_standard_gate_name_mapping()'s output and runs .update(NAME_MAPPING) on it. Was there a reason you had to add these here too? Also in general if you pass in a gate class instead of an instance like this those instructions will be treated as variadic instructions which doesn't seem correct for standard library gates.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed standard gates from NAME_MAPPING

qiskit_aer/backends/plugin/aer_backend_plugin.py Outdated Show resolved Hide resolved
circuit_to_dag(block), ops
)
if node.name in self.config.target:
if node.name not in ops:
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this have a linear search overhead. Could you make ops a set instead I think everything else will work fine if it's a set.

qiskit_aer/backends/plugin/aer_backend_plugin.py Outdated Show resolved Hide resolved
from qiskit_aer.backends.name_mapping import NAME_MAPPING


class AerBackendRebuildGateSetsFromCircuit(TransformationPass):
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 this should actually be an AnalysisPass you're not actually transforming the circuit, but are updating the target instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because there are only translation and scheduling plugins, I used translation plugin to analyze before going to optimization pass

for name in ops:
if name not in self.config.target:
if name != "measure":
self.config.target.add_instruction(NAME_MAPPING[name], name=name)
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 not sure this will do the correct thing in the case of a noise model. If you have a noise model won't this remove all the connectivity constraints caused by that? (this also connects with my comment below, you'd need to update the coupling map for any optimization or scheduling plugins).

@doichanj doichanj requested a review from hhorii November 24, 2023 04:18
Copy link
Collaborator

@hhorii hhorii left a comment

Choose a reason for hiding this comment

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

Though I added a minor comment, I believe that it is not so important.

if self._target is not None:
return self._target

return convert_to_target(self.configuration(), self.properties(), None, NAME_MAPPING)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is just a minor comment. Previously, converted target was cached. With this change, it will not be cached. I think frequent access of target property will produce performance regression. Introducing caching may help performance (though setting method may need clear cache of target).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because if target is cached and used for transpilation to multiple circuits, basis gates for the first circuit is reused that can be a problem if the second circuit has gate that the first one does not have

@doichanj doichanj merged commit ce27d70 into Qiskit:main Nov 24, 2023
31 checks passed
doichanj added a commit to doichanj/qiskit-aer that referenced this pull request Nov 24, 2023
* add description if no description is provided, build coupling map if it is provided

* move import line

* fix target for simulator backend

* format

* remove unused import

* use translation plugin to rebuild gate sets for simulator

* rename plugin

* rebuild of gate sets is eanbled only for opt level 0 and 1

* fix custom pass manager

* fix pass_manager function

* added ccx in NAME_MAPPING

* added missed gates in NAME_MAPPING

* added release note

* add check if opnodes is None

* add check config

* decrease return

* check opt level

* fix searching ops in control flow blocks

* Update qiskit_aer/backends/plugin/aer_backend_plugin.py

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

* Update qiskit_aer/backends/plugin/aer_backend_plugin.py

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

* refer review comments

* remove unused import

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
doichanj added a commit that referenced this pull request Nov 24, 2023
* add skip Python 3.12 for GPU build (#1965)

* Fix basis gates of Aer backends (#1976)

* move reset and switch_case ops to custom istr

* Fix reset in AerStatevector

* add test case

* format

* fix installing built Aer in some test cases

* Applying global phase multiplication to initialize operation (#1980)

* Applying global phase to initialize operation

* fix format

* remove recursive, add omp

* Release 0.13.1

* Revert too many deprecations in Estimator (#1990)

* Revert too much deprecation

* fix typo

* fix tests

* Change priority of method selection of noise simulation (#1989)

* Avoid selecting stabilizer method when noise model contains rotational gates

* remove checking noise opsets, change priority selecting density_matrix

* format

* modify test cases use auto method result may change by this PR

* modify one more test case

* Remove use of opflow in Estimator (#1996)

* Update misspelling apply_gate method doc (#1998)

Co-authored-by: Jun Doi <doichan@jp.ibm.com>

* Add optimization_level=0 to transpiler for compiling dynamic circuits (#2000)

``id`` gate was removed by transpiler called from aer_compiler without optimization_level for dynamic circuits.
This commits adds ``optimization_level=0`` to avoid removing id gates

* fix ry gate for stabilizer (#2001)

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

* Directly use psutil to get total system memory (#2002)

Currently Aer is using Qiskit's local_hardware_info() function which to
determine the total amount of system memory which is used to compute the
largest statevector the system can build. However, this function wasn't
really intended to be used outside of Qiskit and also Qiskit is looking
to remove the memory reporting (see: Qiskit/qiskit#11254). This commit
just pivots to using psutil directly which is what qiskit is doing
internally.

* test build fix (#2004)

* Reverse ordering to read out error in sampling measure (#2003)

* reverse ordering of read out error in sampling measure

* fix batch check

---------

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

* Fix extended stabilizer thread safety in apply_ops_parallel (#1993)

* Extended stabilizer simulator no longer shares RngEngine amongst states when ops are applied in parallel

* Added release note

* Fixed ugly cast

---------

Co-authored-by: Jun Doi <doichan@jp.ibm.com>

* add note (#1992)

Co-authored-by: Jun Doi <doichan@jp.ibm.com>

* Fix AerBackend issues caused by upgrading BackendV2 (#1995)

* add description if no description is provided, build coupling map if it is provided

* move import line

* fix target for simulator backend

* format

* remove unused import

* use translation plugin to rebuild gate sets for simulator

* rename plugin

* rebuild of gate sets is eanbled only for opt level 0 and 1

* fix custom pass manager

* fix pass_manager function

* added ccx in NAME_MAPPING

* added missed gates in NAME_MAPPING

* added release note

* add check if opnodes is None

* add check config

* decrease return

* check opt level

* fix searching ops in control flow blocks

* Update qiskit_aer/backends/plugin/aer_backend_plugin.py

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

* Update qiskit_aer/backends/plugin/aer_backend_plugin.py

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

* refer review comments

* remove unused import

---------

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

* add prelude

---------

Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
Co-authored-by: jon <70080228+notcruz@users.noreply.github.com>
Co-authored-by: Hiroshi Horii <hhorii@users.noreply.github.com>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: eliotheinrich <38039898+eliotheinrich@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 stable-backport-potential The issue or PR might be minimal and/or import enough to backport to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants