Fix Hoare optimizer failure for successive gates of three

[yjt98765]

Summary

This PR fixes the bug reported by #7271.
When three H gates apply on the same qubit successively, the H gate in the middle would be deleted twice by the Hoare optimizer. It causes an exception.

Details and comments

The function _multigate_opt examines the sequences of successive gates generated by _target_successive_seq. If a sequence combines to the identity, all the gates in the sequence are removed. It works for most cases except three successive H gates. See this example proposed by @ewinston:

from qiskit import QuantumCircuit
from qiskit.converters import circuit_to_dag
import qiskit.transpiler.passes.optimization.hoare_opt as hoare
qc = QuantumCircuit(1)
qc.h(0)
qc.h(0)
qc.h(0)
hoare.HoareOptimizer().run(circuit_to_dag(qc))

I will use H1, H2, and H3 to denote the DAGOpNode corresponding to the three H gates. The sequences generated by _target_successive_seq regarding qubit[0] is [[H1, H2], [H2, H3]]. After analyzing the first sequence ([H1, H2]), the optimizer deleted H1 and H2. After analyzing the second sequence ([H2, H3]), the optimizer tried to delete these two gates and found that the key related to H2 does not exist ().

To resolve this issue, I have moved the gate-deleting code from _multigate_opt to _target_successive_seq. When two gates are removed, the iteration index adds an extra 1, so that the deleted gate will not be considered in the next iteration. Since the function of _target_successive_seq has changed, I renamed it as _remove_successive_identity.

I have added a test case to demonstrate the change. The test case is adapted based on @ewinston 's example and existing test cases. Before the change, the test case would fail with a KeyError. It passes after the modifications.

Close #7271

[coveralls]

Pull Request Test Coverage Report for Build 1508471456

• 1 of 13 (7.69%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.004%) to 82.804%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/transpiler/passes/optimization/hoare_opt.py	1	13	7.69%

Totals
Change from base Build 1508123490:	0.004%
Covered Lines:	50018
Relevant Lines:	60405

---

💛 - Coveralls

[ewinston]

What if instead you move the condition,

if self._is_identity(seq) and self._seq_as_one(seq):

to _target_successive_seq
and then just check set intersection between new pair and last added pair? Something like,

new_seq = [node1, node2]
if append:
    if self._is_identity(new_seq) and self._seq_as_one(new_seq):
        if not (seqs and set(new_seq) & set(seqs[-1])):
             seqs.append(new_seq)

This way you only have to check the last added pair.

[yjt98765]

What if instead you move the condition,

if self._is_identity(seq) and self._seq_as_one(seq):

to _target_successive_seq and then just check set intersection between new pair and last added pair? Something like,

new_seq = [node1, node2]
if append:
    if self._is_identity(new_seq) and self._seq_as_one(new_seq):
        if not (seqs and set(new_seq) & set(seqs[-1])):
             seqs.append(new_seq)

This way you only have to check the last added pair.

Yes, it seems better. Maybe the whole block can be moved to _target_successive_seq:

                for node in seq:
                    dag.remove_op_node(node)
                    removed.add(node)
                    # if recursive call, gate will be removed further down
                    if max_idx is None:
                        for qbt in node.qargs:
                            self.gatecache[qbt].remove(node)
                    else:
                        if self.gatecache[qubit].index(node) > max_idx:
                            for qbt in node.qargs:
                                self.gatecache[qbt].remove(node)

It is not related to the other parts of _multigate_opt, but close to _target_successive_seq. After the moving, _target_successive_seq can be renamed as something like _remove_successive_identity.

[ewinston]

@yjt98765 That sounds good.

[yjt98765]

@ewinston I have updated the code. By moving that block to _target_successive_seq, it turns out that the code can be further improved. We only need to adjust the iteration index to skip the deleted gate. No set is needed. Even the seq list can be removed. Thank you for the hint!

[ewinston]

@yjt98765 Thanks for the fix!

[jakelishman]

Thanks for the fix, and sorry for the delay! I had a couple more comments down below - don't worry about the bit that says it's out-of-scope, that's just for history purposes. Please could you also add a release note describing the bugfix as well?

[jakelishman]

This documentation is a bit confusing now - _target_successive_seq doesn't call itself recursively.

[yjt98765]

I see your point here: the moved code/comment should make sense in the new context. I have updated the docstring. In addition, max_idx is a bit misleading here, so I renamed it to from_idx.

[jakelishman]

A similar comment here - _remove_successive_identity isn't a recursive function.

Also, I know this was pre-existing, but this whole logic block is equivalent to

if max_idx is None or self.gatecache[qubit].index(node) > max_idx:
    for qbt in node.qargs:
        self.gatecache[qbt].remove(node)

which avoids an extra scope, and removes some duplication.

(A comment far beyond the scope of the PR: this pre-existing code smells of poor scaling - list.index and list.remove are very tricky to use well, and this is highly likely to be at least quadratic in the circuit depth.)

[yjt98765]

Indeed, I have merged the code as you suggested here.

[yjt98765]

@jakelishman sure, I have added a release note.

[jakelishman]

I pushed a commit just to reword the release note a little - mostly I wanted to avoid referring to a private function in it, because users might not know what that meant.

I was about to merge this, but then I realised I'm a bit confused - the max_idx is now called from_idx, which makes sense to me for how it's used, but the whole function seems to use it as the minimum index (and it's still called max_idx in the parent functions). Is this intended? I'm not really sure what's going on here. If it all is meant to be the minimum index not the maximum, then it'd be good to change the names everywhere to match.

[yjt98765]

@jakelishman I am not the author of the original program, but I guess the key to understanding the parameter is the last line of the parent function _multigate_opt:

self.gatecache[qubit] = self.gatecache[qubit][max_idx + 1 :]

The variable max_idx is a boundary between the removed and the remained indexes. It is the maximum index to be removed. Its next index is the minimum index to be kept. So max_idx and min_idx both make some sense and both cause some confusion. Maybe we can call it max_tbr_idx or something more accurate?

[jakelishman]

Thanks - it seems ok to me to leave it as it is, then.