Give IGate a no-op definition

[jakelishman]

Summary

This allows it to work with all passes that decompose operations using
Instruction.definition, even if there is no particular special
handling for it.

Details and comments

Fix #7399.
Fix #4010.
Fix #9485.

[coveralls]

Pull Request Test Coverage Report for Build 4577211839

• 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
• 2 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.003%) to 85.414%

---

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/passes/basis/unroller.py	2	85.51%

Totals
Change from base Build 4576589370:	0.003%
Covered Lines:	67429
Relevant Lines:	78944

---

💛 - Coveralls

[jakelishman]

I am marginally concerned that those newly uncovered lines indicate a problem somewhere in our testing logic; they're meant to throw an error, but we didn't have any test failures before, and we don't now. Seems like somewhere might be overzealously catching exceptions.

[kdk]

Similar to #7146 , I think the approach here is inconsistent with how Qiskit currently defines IGate.
https://github.com/Qiskit/qiskit-terra/blob/309bfb173e946f441243f292a7655203419efde9/qiskit/circuit/library/standard_gates/i.py#L23

This behavior is deprecated but not yet removed from the IBMQ backends at which point we can IGate to it's canonical definition. Until then, perhaps we can raise a more meaningful error message when someone tries to unroll, decompose, or control an IGate.

[kdk]

This will be great to be able to finally reconcile. A few documentation comments but otherwise LGTM.

[mtreinish]

This LGTM, and I think this will mostly supersede #7146 so I think we should just do this for 0.23.0 and we can revisit whether #7146 is needed at all or not after the release

[mtreinish]

Suggested change

	some older hardware may choose to interpret it as a short-hand for the shortest-possible delay
	some backends may choose to interpret it as a short-hand for the shortest-possible delay

[kdk]

After a discussion with @taalexander and @dtmcclure , I'd like to wait a bit on this. It looks like there's a desire to keep the 'id' instruction on IBM hardware as a way to have an idle instruction with a well-defined duration. If that's the case, there might be a better definition for IGate than an empty circuit (like U2Gate(0, 0)).

[mtreinish]

I don't believe this will effect IBM backend's custom usage for single qubit delay. As id is in the basis set for those backends Qiskit won't ever translate away from id if it's present in the circuit (it can be optimized away, but that is also true today without this PR).

[jakelishman]

But the transpiler will also remove U2Gate(0, 0) in its optimisation passes, which I think is actually a bit worse than this setup. This way allows the IBM backends to insert their custom translation steps to convert IGate to whatever they'd like, and that dodges this definition (and since id is in their basis sets, we wouldn't even use this conversion).

[jakelishman]

I really don't like the proprietary IBM hardware preventing the backend-agnostic Qiskit from using the natural mathematical name id to refer to the identity operation, since it co-opts it to mean something different, especially when there's plenty of ways through Qiskit to let the hardware do whatever it wants, without affecting us.

Not having this be a true identity operation keeps causing problems at higher-levels, where algorithms and applications (via SparsePauliOp, for example), can't output a natural identity operation, but have to do it implicitly only. We can work-around them (and repeatedly have to), but it would be much easier if those guys didn't have to.

[kdk]

Agree that the result post-optimization of most common uses cases with the proposals on the table will look very similar in practice but I would like, if possible, to find a pathway forward that is consistent with how 'id' has historically been defined in Qiskit and has been understood by users.

This might look like defining IGate to be an identity/nop single-qubit instruction as a UGate(0,0,0) (apologies if this seems overly pedantic compared to the existing empty definition, but I want to be sure the unoptimized behavior can be represented by a do-nothing instruction on hardware). Then we can note that it may have a backend-dependent (but potentially non-zero) duration, that it can be expected to be optimized away by the transpiler at all optimization_levels > 0. Hopefully, that will resolve the conflict of 'id' being opaque for uses cases that want to rely on optimization and preserves the definition of 'id' as an instruction which can have an implementation on hardware.

[kdk]

Proposing some text that might cover both cases. Feedback very welcome.

[mtreinish]

I think now is the time to do this, I can't think of a use case that will be broken by this. For IBM backends with their custom behavior for id that will still work because id is in the basis gates for those backend and they can treat it special (there is also the custom translation plugin the ibm provider has to to translate id to single qubit gate duration delays). For other backends if they support id that will continue to work as is, but for backends that don't support id this fixes transpilation for those backends.

[kdk]

Coming back to this, starting from the idea that IGate is a single-qubit no-op gate, the core of my hesitation with this PR is that while removing an IGate from a circuit is valid optimization, it is not necessarily a valid translation (roughly, the difference between "IGate is nothing" as opposed to "IGate does nothing logically"). In the same way RXGate(4*pi) and other gates that implement a logical identity operation don't have an empty definition, it doesn't feel correct for IGate.

Where this could turn into a bug for users is that we allow translations at optimization_level=0 but not optimizations, and so users who are intentionally using IGate at optimization_level=0 could have them dropped entirely (as opposed to seeing a compile time error) if their backend doesn't support 'id'.

As an answer for #9485 , I think #7146 is more consistent with IGate being defined as a backend-dependent single-qubit no-op gate (and now that the plan is to maintain 'id' on IBM hardware, my prior concerns there no longer apply).

[mtreinish]

I'm fine with this or #7146, the specific approach doesn't matter to me. What I think is critical to fix is that transpile() with an id gate can be successfully run targeting a backend that doesn't have a native id instruction. That being an issue users have to deal with has been sitting around for too long.

[kdk]

Closing in favor of #7146 .