I would've liked to deprecate this as the Diagonal circuit exists, but DiagonalGate seems to be used in the Aer simulators.

imo it should be the circuit that gets removed not the gate, since circuits inherently define an eager synthesis, but that's not important.

Agreed, maybe this would best be part of an overhaul to consistently use Gate or Circuits (probably the first) 🙂

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)

I think we had the plans to deprecate Permutation circuit once PermutationGate is around. Do you think it's a good idea?

We could consider creating an evolutions submodule containing this and the PauliEvolutionGate.

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.

imo it should be the circuit that gets removed not the gate, since circuits inherently define an eager synthesis, but that's not important.

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.

Sequence isn't the correct type hint. ndarray is not a Sequence, and Diagonal._check_input requires the input to be list | ndarray anyway.

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.

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.

This might also be unnecessary -- after moving the gates there were loads of cyclic imports, and I might have added one too many ignores.... 🙈

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.

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.

Yeah, I pointed this out in #10725 (comment) as I wasn't sure whether we should take this precaution 🤔

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.

In that case it would show the deprecations warnings of both QC.snapshot and Snapshot or only the first?

The standard Python warning filters only show DeprecationWarnings 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.

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

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.

This might again be a side effect of Pylint's cyclic import complaints...

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.

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.