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

Tame 1Q simplification warning #6868

Merged
merged 1 commit into from
Aug 4, 2021
Merged

Tame 1Q simplification warning #6868

merged 1 commit into from
Aug 4, 2021

Conversation

ecpeterson
Copy link
Contributor

@ecpeterson ecpeterson commented Aug 4, 2021

closes #6848

Summary

Tames an overly active warning which is triggered during synthesis-based 1Q simplification.

Details and comments

In #6553, we added a warning in Optimize1qGatesDecomposition.__call__ which was meant to check that gate sequences emitted by an operator-based 1Q synthesis routine were always more efficient (i.e., shorter) than the sequence used to form the input. The idea was that this would warn us if a synthesis routine were ever to emit a sequence which is not maximally efficient.

We made a mistake in the definition of 'maximally efficient'. In general, a synthesis routine promises to emit gates whose types form a subset of some larger set of native gates, and it cannot make guarantees (nor be expected to make guarantees) about optimality of its circuits within the larger set, but only within the smaller set. For instance, PSX decomposition emits p(pi/2) ; sx ; p(pi/2) as a circuit embodying h. Its output ["p", "sx"] conforms to the set of native gates ["p", "sx", "h"] and the emitted circuit is depth-optimal within ["p", "sx"], but it is not depth-optimal within the larger set — a lone h is shorter.

I don't believe it's possible to reason about optimality over unions of synthesis targets. So, instead, this PR modifies the predicate which guards the warning about increased depth, specializing it to each synthesis routine. In order to do this specialization, we have to remember which routines target which sets of output, and this is the primary source of diff size. (In retrospect, it might have been preferable to use the .basis slot, rather than retaining the information locally...)

(This isn't exactly a bug — a gnat, at worst — so I've elected not to mention it in the change log.)

@ecpeterson ecpeterson requested a review from a team as a code owner August 4, 2021 20:55
@mtreinish mtreinish added Changelog: None Do not include in changelog stable backport potential The bug might be minimal and/or import enough to be port to stable labels Aug 4, 2021
@mergify mergify bot merged commit bdedeae into Qiskit:main Aug 4, 2021
mergify bot pushed a commit that referenced this pull request Aug 4, 2021
@levbishop
Copy link
Member

Would a better approach be to partially revert to the behavior prior to #6553 and pass through the input in favour of the resynthesized sequence in the case that the input sequence is both: 1) shorter; and 2) fully-contained in the native gate set?

@ecpeterson
Copy link
Contributor Author

I think that this describes the behavior currently. The predicate guarding replacement is

                    uncalibrated_and_not_basis_p
                    or (uncalibrated_p and len(run) > len(new_circ))
                    or isinstance(run[0].op, U3Gate)

The uncalibrated_and_not_basis_p flag will be lowered if the whole input sequence is in the larger native gate set, and the flag check will also pass if the original sequence is shorter than the synthesized one.

mergify bot added a commit that referenced this pull request Aug 13, 2021
(cherry picked from commit bdedeae)

Co-authored-by: Eric Peterson <Eric.Peterson@ibm.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@kdk kdk added this to the 0.19 milestone Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warning statement upon compiling cz gate into ['h', 'cx', 'rz', 'sx', 'x'] gate set
5 participants