Add IGate rules to standard equivalence library by mtreinish · Pull Request #7146 · Qiskit/qiskit

mtreinish · 2021-10-15T14:33:41Z

Summary

This commit adds a missing rules to the equivalence library for an
identity gate. Without this the basis translator has difficulty finding
a match if the basis does not include an id gate (see
Qiskit/qiskit-metapackage#1346)

Details and comments

Fix #9485.

This commit adds a missing rules to the equivalence library for an identity gate. Without this the basis translator has difficulty finding a match if the basis does not include an id gate (see Qiskit#1346)

jakelishman

These seem all fine. In normal transpiler runs, will these even activate? It feels like IGate should generally just get removed from the circuit (unless I suppose it has calibrations/noise models attached to it, but then the equivalences shouldn't run either).

(I think black doesn't like your single quotes)

mtreinish · 2021-10-15T14:57:24Z

These seem all fine. In normal transpiler runs, will these even activate? It feels like IGate should generally just get removed from the circuit (unless I suppose it has calibrations/noise models attached to it, but then the equivalences shouldn't run either).

It actually would come up if there is an id gate in the input circuit and not in the basis set at all optimization levels I think. Since we run the basis translator prior to any optimizations that will remove the id gate.

I specifically hit this in a pretty synthetic case though, in the benchmarks I'm adding in: #1346 which just uses random_circuit() and just runs the basis translator bare on it using a basis without an id gate because random_circuit() just randomly picks gates from the standard library and inserts them into the circuit and I was ending up with some id gates in the circuit which caused the basis translator to stall since it couldn't find an equivalence.

jakelishman · 2021-10-15T15:01:46Z

Yeah, I'd seen the synthetic benchmark. I suppose it makes sense that the basis-gate translator would be run first; if we ran the hypothetical "check for no-ops" too early, we'd probably have to run it again later to make sure we hadn't introduced any more by cancellations or parameter combinations or something.

kdk

Comments in Qiskit/qiskit-metapackage#1346 (comment) .

mtreinish · 2022-12-01T15:30:04Z

Comments in Qiskit/qiskit#1346 (comment) .

This shouldn't hold true anymore as new backends will no longer support the id instruction. It's also been raising a warning for > a year if a user submits a circuit to the ibmq provider with an id gate right?

coveralls · 2022-12-01T16:16:11Z

Pull Request Test Coverage Report for Build 3658676946

16 of 16 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.003%) to 84.56%

Totals
Change from base Build 3658575673:	0.003%
Covered Lines:	63238
Relevant Lines:	74785

💛 - Coveralls

mtreinish · 2022-12-09T15:28:58Z

Additionally I just pushed: Qiskit/qiskit-ibm-provider#468 to have IBM backend's preserve their custom definition of id == delay(sx.duration) through transpile via custom backend plugin defaults. This means that combined with this PR IBM backends get their custom behavior and for all other backends id == identity (especially when combined with #7403 )

jakelishman

Do we need to supply equivalences to other no-op gates, or do you think it would be suitable to just supply and equivalence to the empty 1q DAG?

If we do need the no-op gates, then this seems fine to me (pending Kevin's review as well).

kdk · 2023-01-12T16:38:29Z

Additionally I just pushed: Qiskit/qiskit-ibm-provider#468 to have IBM backend's preserve their custom definition of id == delay(sx.duration) through transpile via custom backend plugin defaults. This means that combined with this PR IBM backends get their custom behavior and for all other backends id == identity (especially when combined with #7403 )

I'm a bit confused as to why Qiskit/qiskit-ibm-provider#468 is needed in addition to the this PR and #7403. Wouldn't that override this PR and #7403 to reinstate the deprecated behavior (and maybe silence the deprecation warning)?

jakelishman · 2023-01-12T16:41:34Z

Yeah, that's the idea: the old backends that do maintain that definition translate id into the delay they associate with it themselves, so the rest of the transpiler stack can move away from it, without affecting the old backends.

This and #7403 are about not enforcing that custom behaviour on every other hardware backend, and letting various other parts of Qiskit now reason about the identity operation at a high-level in the expected way.

kdk · 2023-01-12T17:14:07Z

Yeah, that's the idea: the old backends that do maintain that definition translate id into the delay they associate with it themselves, so the rest of the transpiler stack can move away from it, without affecting the old backends.

I think I'm a bit lost on the motivation. It might be a good time to write down all the cases we're thinking of, and the various timelines and expected behaviors. Why would the backends that still maintain 'id' want or need a conversion into the equivalent 'delay'? Is the expectation that these backends will maintain an 'id' until they're retired?

mtreinish · 2023-03-31T16:09:04Z

This is superseded by #7403 as I don't think we need explicit equivalence lib entries for IGate if it has a no-op definition

Adding the id equivalence rules to the equivalence library is changing the output translation for one of the tests and this causes the test which is using an exact circuit comparison to fail. This commit updates the test to use the new output but then also does an operator comparison to ensure that the output circuit is equivalent too.

jakelishman

A minor question, but I'm in general happy to merge.

coveralls · 2023-09-11T17:27:35Z

Pull Request Test Coverage Report for Build 6424802268

16 of 16 (100.0%) changed or added relevant lines in 1 file are covered.
7 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.001%) to 87.027%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/expr.rs	1	93.76%
crates/qasm2/src/lex.rs	3	90.91%
qiskit/pulse/library/waveform.py	3	93.75%

Totals
Change from base Build 6414428479:	0.001%
Covered Lines:	74184
Relevant Lines:	85243

💛 - Coveralls

jakelishman · 2023-09-11T17:29:38Z

        expected = QuantumCircuit(2)
        expected.u(pi / 2, 0, pi, qr[0])
        expected.u(0, 0, -pi / 2, qr[0])
-        expected.u(-pi / 2, -pi / 2, pi / 2, qr[1])
+        expected.u(pi, 0, 0, qr[0])
+        expected.u(pi / 2, -pi / 2, pi / 2, qr[1])
        expected.ecr(0, 1)
-        expected.u(pi, 0, pi, qr[0])
        expected_dag = circuit_to_dag(expected)


My question from the prior review! It got lost before - I must have forgotten to save the comment.

Do we know why this changes? We haven't added any rules that go to IGate, so I'd not have expected the basis-translation path to change.

TBH, I'm not sure. I didn't really think to check. I just saw the output circuit was equivalent and updated the test. I'll do some debugging locally because the qpy tests are failing in a weird way now too so this is blocked on that anyway.

kdk

Following some updates since this was last discussed (id will stick around on IBM backends, and is being defined in Qiskit as a single-qubit no-op of backend-defined duration), I think this is good to go (pending #7146 (comment) ).

It would also be good to call out (in the IGate docstring and maybe as a release note) that IGate is no longer strictly opaque, and should be expected to be optimized away by the transpiler except at optimization_level=0.

Add IGate rules to session equivalence library

8ead825

This commit adds a missing rules to the equivalence library for an identity gate. Without this the basis translator has difficulty finding a match if the basis does not include an id gate (see Qiskit#1346)

mtreinish requested a review from a team as a code owner October 15, 2021 14:33

jakelishman previously approved these changes Oct 15, 2021

View reviewed changes

mtreinish mentioned this pull request Oct 15, 2021

Add benchmarks for missing passes Qiskit/qiskit-metapackage#1346

Merged

Run black

bd4f71a

mtreinish dismissed jakelishman’s stale review via bd4f71a October 15, 2021 14:51

jakelishman previously approved these changes Oct 15, 2021

View reviewed changes

kdk requested changes Oct 15, 2021

View reviewed changes

mtreinish mentioned this pull request Feb 4, 2022

Use unitary equivalence instead of circuit equality in noise tests Qiskit/qiskit-aer#1446

Merged

jakelishman mentioned this pull request Feb 23, 2022

Circuit with identity gate fails to transpile to universal gate set #7641

Closed

mtreinish added this to the 0.23.0 milestone Oct 4, 2022

Merge remote-tracking branch 'origin/main' into id-equivalence-lib

d79caab

mtreinish dismissed jakelishman’s stale review via d79caab December 1, 2022 15:29

mtreinish requested a review from kdk December 1, 2022 15:29

Merge branch 'main' into id-equivalence-lib

c1bc5f8

Merge branch 'main' into id-equivalence-lib

8c7b0c9

mtreinish assigned kdk Jan 9, 2023

jakelishman previously approved these changes Jan 11, 2023

View reviewed changes

kdk changed the title ~~Add IGate rules to session equivalence library~~ Add IGate rules to standard equivalence library Jan 12, 2023

kdk reviewed Jan 12, 2023

View reviewed changes

Comment thread qiskit/circuit/library/standard_gates/equivalence_library.py

mtreinish added the priority: medium label Jan 17, 2023

mtreinish removed this from the 0.23.0 milestone Jan 19, 2023

jakelishman mentioned this pull request Jan 30, 2023

transpile function with basis_gates=['u', 'cx'] fails with any circuit with an id gate #9485

Closed

mtreinish closed this Mar 31, 2023

kdk mentioned this pull request Jul 19, 2023

Give IGate a no-op definition #7403

Closed

mtreinish reopened this Jul 19, 2023

mtreinish added 3 commits July 19, 2023 16:28

Merge branch 'main' into id-equivalence-lib

c7fa3ec

Merge branch 'main' into id-equivalence-lib

102ab00

mtreinish dismissed jakelishman’s stale review via 029e284 September 11, 2023 16:53

jakelishman approved these changes Sep 11, 2023

View reviewed changes

jakelishman requested a review from kdk September 11, 2023 16:56

jakelishman added this to the 0.45.0 milestone Sep 11, 2023

jakelishman reviewed Sep 11, 2023

View reviewed changes

kdk approved these changes Sep 15, 2023

View reviewed changes

mtreinish added 2 commits October 5, 2023 17:47

Merge branch 'main' into id-equivalence-lib

e9628de

Merge remote-tracking branch 'origin/main' into id-equivalence-lib

955eb02

jakelishman enabled auto-merge October 16, 2023 19:31

jakelishman added this pull request to the merge queue Oct 16, 2023

Merged via the queue into Qiskit:main with commit e92b0df Oct 16, 2023

mtreinish deleted the id-equivalence-lib branch October 17, 2023 01:37

This was referenced Oct 20, 2023

Issue in Creating Controlled Custom Gate if Circuit Containing IGate #11033

Closed

Gate.control() method rasies the error 'Cannot make controlled gate with id instruction' #4010

Closed

ElePT added the Changelog: Fixed Add a "Fixed" entry in the GitHub Release changelog. label Nov 1, 2023

Conversation

mtreinish commented Oct 15, 2021 • edited by kdk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details and comments

Uh oh!

jakelishman left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtreinish commented Oct 15, 2021

Uh oh!

jakelishman commented Oct 15, 2021

Uh oh!

kdk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtreinish commented Dec 1, 2022

Uh oh!

coveralls commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 3658676946

💛 - Coveralls

Uh oh!

mtreinish commented Dec 9, 2022

Uh oh!

jakelishman left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kdk commented Jan 12, 2023

Uh oh!

jakelishman commented Jan 12, 2023

Uh oh!

kdk commented Jan 12, 2023

Uh oh!

mtreinish commented Mar 31, 2023

Uh oh!

jakelishman left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Sep 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 6424802268

💛 - Coveralls

Uh oh!

jakelishman Sep 11, 2023

Choose a reason for hiding this comment

Uh oh!

mtreinish Sep 11, 2023

Choose a reason for hiding this comment

Uh oh!

kdk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mtreinish commented Oct 15, 2021 •

edited by kdk

Loading

jakelishman left a comment •

edited

Loading

kdk left a comment •

edited

Loading

coveralls commented Dec 1, 2022 •

edited

Loading

jakelishman left a comment •

edited

Loading

coveralls commented Sep 11, 2023 •

edited

Loading