-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Deprecate qiskit.extensions
#10725
Deprecate qiskit.extensions
#10725
Conversation
One or more of the the following people are requested to review this:
|
@@ -181,6 +181,7 @@ | |||
:template: autosummary/class_no_inherited_members.rst | |||
|
|||
Diagonal | |||
DiagonalGate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would've liked to deprecate this as the Diagonal
circuit exists, but DiagonalGate
seems to be used in the Aer simulators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo it should be the circuit that gets removed not the gate, since circuits inherently define an eager synthesis, but that's not important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, maybe this would best be part of an overhaul to consistently use Gate or Circuits (probably the first) 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this is quite confusing. Maybe it's worth to add a comment somewhere about the difference between them, and which one will be deprecated?
(although we also have Permutation
and PermutationGate
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we had the plans to deprecate Permutation
circuit once PermutationGate
is around. Do you think it's a good idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could consider creating an evolutions
submodule containing this and the PauliEvolutionGate
.
data = np.array(data, dtype=complex) | ||
# Check input is unitary | ||
if not is_hermitian_matrix(data): | ||
raise ValueError("Input matrix is not Hermitian.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some errors were changed from ExtensionsError
to ValueError
, which is strictly speaking not backwards compatible. It seemed small enough to be still fine, but we could also create an intermediate class, covering both, for the deprecation period.
@@ -181,6 +181,7 @@ | |||
:template: autosummary/class_no_inherited_members.rst | |||
|
|||
Diagonal | |||
DiagonalGate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo it should be the circuit that gets removed not the gate, since circuits inherently define an eager synthesis, but that's not important.
Pull Request Test Coverage Report for Build 6248054115
💛 - Coveralls |
- capture warning of SQU - update reno to explicitly mention pending deprecation, add DiagonalGate and ExtensionError
I think this is ready to be merged: all the review comments have been addressed. @Cryoris, thanks for the massive work on this, I am personally very excited to see the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Sasha, Shelly and Elena for reviewing this and Julien for doing it! In general, this was a complex deprecation, and it looks like it's gone fairly well.
I think I was meant to look over this before it merged, just as a final check, so I've done so now, and I just left a couple of notes (nothing major that really needs changing). Top level stuff:
- There's a lot of unrelated code changes in here (type hints, renaming standard imports, refactoring the moved gates) that probably would have been better in separate PRs, especially because some of them will have (minor) behavioural changes.
- The title of this PR / commit as it merged was kind of wrong - most of
qiskit.extensions
isn't deprecated this release, it's pending because this PR is more about moving everything into new places.
# The qiskit.extensions.x imports needs to be placed here due to the | ||
# mechanism for adding gates dynamically. | ||
import qiskit.extensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this in conjunction with all the rest of the work on removing uses of qiskit.extensions
actually was a bit of a breaking change, because qiskit.extensions
is no longer available just by having done import qiskit
. In other words:
import qiskit
qiskit.extensions.CXGate
would have worked without error before, and now it'll say
AttributeError: module 'qiskit' has no attribute 'extensions'
and require you to do import qiskit.extensions
.
That's minor and we can probably get away with it, but it was a dedicated submodule that was previously automatically imported, so just pointing out that these removals can cause issues.
def __init__(self, diag: list[complex] | np.ndarray) -> None: | ||
"""Create a new Diagonal circuit. | ||
|
||
def __init__(self, diag: Sequence[complex]) -> None: | ||
r""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sequence
isn't the correct type hint. ndarray
is not a Sequence
, and Diagonal._check_input
requires the input to be list | ndarray
anyway.
if not isinstance(diag, (list, np.ndarray)): | ||
raise CircuitError("Diagonal entries must be in a list or numpy array.") | ||
num_qubits = np.log2(len(diag)) | ||
if num_qubits < 1 or not num_qubits.is_integer(): | ||
raise CircuitError("The number of diagonal entries is not a positive power of 2.") | ||
if not np.allclose(np.abs(diag), 1, atol=_EPS): | ||
raise CircuitError("A diagonal element does not have absolute value one.") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might have been a bit better to make any code refactors like these in a follow-up PR, rather than squashed into a +1.5k,-1.3k changeset that notionally shouldn't have any effect on the objects.
|
||
# pylint: disable=cyclic-import | ||
from ..standard_gates import XGate, YGate, ZGate, HGate, TGate, TdgGate, SGate, SdgGate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This extra suppression is indicative to me that we've put a gate somewhere in the wrong place - I suspect it's UnitaryGate
, which it might have been cleaner to give special treatment, since it's used by the base Gate
class. standard_gates
shouldn't need to cyclically import generalized_gates
.
We can re-arrange things in other PRs, though, no worries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might also be unnecessary -- after moving the gates there were loads of cyclic imports, and I might have added one too many ignores.... 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's probably a true one - UnitaryGate
being inside generalized_gates
is one of the things we maybe want to reconsider before the actual release; it's kind of a base gate, and while I can see the argument for calling it "generalized", it's fundamental to a lot of how synthesis and the Gate
class works, whereas everything else in generalized_gates
is rather higher level and should be 100% safe to import after standard_gates
.
if not is_unitary_matrix(data): | ||
raise ExtensionError("Input matrix is not unitary.") | ||
raise ValueError("Input matrix is not unitary.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like an error people could conceivably have been catching as part of a validation workflow. During the deprecation period, we might have wanted to use a custom error type that derived both ExtensionError
and ValueError
, and only switched the documentation over to saying it raised ValueError
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I pointed this out in #10725 (comment) as I wasn't sure whether we should take this precaution 🤔
# catch deprecation warning from instantiating the Snapshot instruction, | ||
# as a warning is already triggered from this method | ||
with warnings.catch_warnings(): | ||
warnings.simplefilter("ignore", category=DeprecationWarning) | ||
snap = Snapshot( | ||
label, snapshot_type=snapshot_type, num_qubits=len(qubits), params=params | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't really do this in general; it just slows things down for no real gain. The stacklevel
of the Snapshot
warning should be set correctly such that the QuantumCircuit.snapshot
method is "blamed", which will mean that the inner warning is not shown to users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case it would show the deprecations warnings of both QC.snapshot
and Snapshot
or only the first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The standard Python warning filters only show DeprecationWarning
s if the stacklevel
causes the "blamed" code to be in the module __main__
(the name for the script file being executed / code typed into the interpreter / Jupyter cells / etc). If the stack level of Snapshot
's warning is set correctly, then the warning will blame the QuantumCircuit.snapshot
method, whose module is qiskit.circuit.quantumcircuit
, and so it won't be shown to users.
# isometry is an alias for iso | ||
QuantumCircuit.isometry = QuantumCircuit.iso |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not important at all, just saying: it might have been a shade cleaner to do this within the definition of QuantumCircuit
-
class QuantumCircuit:
def iso(self, ...): ...
# isometry is an alias for iso
isometry = iso
from qiskit.extensions import UnitaryGate | ||
from qiskit.circuit.library.generalized_gates.unitary import UnitaryGate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not too important, but imo this is is too specific an import - the documented location is qiskit.circuit.library
, and hyperspecific import locations like this are part of the reason it's difficult to reorganise code in the future. If it doesn't import without the hyperspecificity, it's a strong sign that the objects have been put in the wrong place, and there are true cyclic-import problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might again be a side effect of Pylint's cyclic import complaints...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm saying that in this case, this would be pylint flagging something that's actually a real problem with our logical organisation - generalized_gates
should depend on standard_gates
, not vice versa. UnitaryGate
is a core gate; it's used in the definition of Gate
itself, so it probably would have been better for it to be given special treatment in the hierarchy, such as being in the top level of qiskit.circuit.library
or the like.
To streamline the structure of Qiskit's gates and operations, the :mod:`qiskit.extensions` | ||
module is pending deprecation and will be deprecated in a future release. | ||
The following objects have been moved to :mod:`qiskit.circuit.library` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of qiskit.circuit.library
is accessible from qiskit.extensions
as well via a star import - it might have been good to mention that all not-immediately-deprecated gates from qiskit.extensions
should be imported from qiskit.circuit.library
.
* big moves, import still works * most tests pass (some I cannot seem to run locally) * fix tests -- how to remove DiagonalGate? * typehints and docs * more type hints * Deprecate SQU * deprecate Snapshot * Fix missing future annotations import * minimize deprecation effort * Change to pending deprecation, no exact-location import supported * fix MCG<->MGC typo and snapshot deprecation * fix pylint, try fixing docs * remove gates from extensions toctree * Add reno, fully deprecate SQU and Snapshot * Apply Sasha's review comments - fix usage of .squ and .snapshot w/o import - fix docstring usage of extensions - fix tests * capture snapshot deprecation warning * review comments - capture warning of SQU - update reno to explicitly mention pending deprecation, add DiagonalGate and ExtensionError * missed `diagonal` method --------- Co-authored-by: Alexander Ivrii <[email protected]>
* big moves, import still works * most tests pass (some I cannot seem to run locally) * fix tests -- how to remove DiagonalGate? * typehints and docs * more type hints * Deprecate SQU * deprecate Snapshot * Fix missing future annotations import * minimize deprecation effort * Change to pending deprecation, no exact-location import supported * fix MCG<->MGC typo and snapshot deprecation * fix pylint, try fixing docs * remove gates from extensions toctree * Add reno, fully deprecate SQU and Snapshot * Apply Sasha's review comments - fix usage of .squ and .snapshot w/o import - fix docstring usage of extensions - fix tests * capture snapshot deprecation warning * review comments - capture warning of SQU - update reno to explicitly mention pending deprecation, add DiagonalGate and ExtensionError * missed `diagonal` method --------- Co-authored-by: Alexander Ivrii <[email protected]>
* big moves, import still works * most tests pass (some I cannot seem to run locally) * fix tests -- how to remove DiagonalGate? * typehints and docs * more type hints * Deprecate SQU * deprecate Snapshot * Fix missing future annotations import * minimize deprecation effort * Change to pending deprecation, no exact-location import supported * fix MCG<->MGC typo and snapshot deprecation * fix pylint, try fixing docs * remove gates from extensions toctree * Add reno, fully deprecate SQU and Snapshot * Apply Sasha's review comments - fix usage of .squ and .snapshot w/o import - fix docstring usage of extensions - fix tests * capture snapshot deprecation warning * review comments - capture warning of SQU - update reno to explicitly mention pending deprecation, add DiagonalGate and ExtensionError * missed `diagonal` method --------- Co-authored-by: Alexander Ivrii <[email protected]>
* big moves, import still works * most tests pass (some I cannot seem to run locally) * fix tests -- how to remove DiagonalGate? * typehints and docs * more type hints * Deprecate SQU * deprecate Snapshot * Fix missing future annotations import * minimize deprecation effort * Change to pending deprecation, no exact-location import supported * fix MCG<->MGC typo and snapshot deprecation * fix pylint, try fixing docs * remove gates from extensions toctree * Add reno, fully deprecate SQU and Snapshot * Apply Sasha's review comments - fix usage of .squ and .snapshot w/o import - fix docstring usage of extensions - fix tests * capture snapshot deprecation warning * review comments - capture warning of SQU - update reno to explicitly mention pending deprecation, add DiagonalGate and ExtensionError * missed `diagonal` method --------- Co-authored-by: Alexander Ivrii <[email protected]>
* big moves, import still works * most tests pass (some I cannot seem to run locally) * fix tests -- how to remove DiagonalGate? * typehints and docs * more type hints * Deprecate SQU * deprecate Snapshot * Fix missing future annotations import * minimize deprecation effort * Change to pending deprecation, no exact-location import supported * fix MCG<->MGC typo and snapshot deprecation * fix pylint, try fixing docs * remove gates from extensions toctree * Add reno, fully deprecate SQU and Snapshot * Apply Sasha's review comments - fix usage of .squ and .snapshot w/o import - fix docstring usage of extensions - fix tests * capture snapshot deprecation warning * review comments - capture warning of SQU - update reno to explicitly mention pending deprecation, add DiagonalGate and ExtensionError * missed `diagonal` method --------- Co-authored-by: Alexander Ivrii <[email protected]>
) ### Summary After `qiskit.extensions` was deprecated in Qiskit/qiskit#10725, this PR updates the path of `HamiltonianGate` so tests against Qiskit main pass again. This also adds `atol` and `rtol` tolerance parameters to `PulseBackend` for speeding up slow tests.
…skit-community#1280) ### Summary After `qiskit.extensions` was deprecated in Qiskit/qiskit#10725, this PR updates the path of `HamiltonianGate` so tests against Qiskit main pass again. This also adds `atol` and `rtol` tolerance parameters to `PulseBackend` for speeding up slow tests.
) ### Summary After `qiskit.extensions` was deprecated in Qiskit/qiskit#10725, this PR updates the path of `HamiltonianGate` so tests against Qiskit main pass again. This also adds `atol` and `rtol` tolerance parameters to `PulseBackend` for speeding up slow tests. (cherry picked from commit 73d0a03) # Conflicts: # qiskit_experiments/test/__init__.py # test/library/calibration/test_half_angle.py # test/library/calibration/test_rough_amplitude.py
Summary
Deprecates the
qiskit.extensions
submodule. Contents are either moved to the circuit library or deprecated. In future it would be nice to build high-level synthesis passes for algorithms that synthesis the same type of operation. E.g. isometry could be a synthesis for unitaries (or for state initialization).Details and comments
Here's a detailed list of moves:
Moved to generalized gates
uc<any>.py
: These are multiplexed gates, as such they are generalizations of gates we have in the standard gate libraryunitary.py
: General unitary gate (hence generalized gate)isometry.py
: Implements isometries, and as subset of this also unitaries, so it should live in the same place asUnitaryGate
.mcg_up_diag.py
: Implements a multicontrolled gate up to a diagonal, hence a generalization of controlled gates.diagonal.py
: Moved theDiagonalGate
into the same file as theDiagonal
circuitMoved to data preparation
StatePreparation
, which is a data preparation operation. Could also live next toUnitaryGate
, but then we'd also have to move the state preparation.Moved to circuit library root:
hamiltonian_gate.py
: Implements an exact operator evolution and should be next toPauliEvolutionGate
.Deprecated:
squ.py
: Implements a 1q-unitary matrix and thus a duplicate functionality of UnitaryGate. Also not used anywhere in the source code.snapshot.py
: Superseded by Aer'ssave_*
instructions.Monkey-patched circuit methods:
diagonal
,hamiltonian
,uc
,ucrx
,ucry
,ucrz
,squ
,snapshot
unitary
,initialize