Update cirq.decompose protocol to perform a DFS instead of a BFS on the decomposed OP-TREE

[tanujkhattar]

As mentioned in the title, the PR updates the internal implementation of cirq.decompose() protocol to perform DFS on the implicit compute graph instead of a BFS. This is useful in order to add support for qubit allocations and deallocations using a qubit manager inside the decompose protocol. This PR does the following:

1. Delegates the cirq.decompose logic to a recursive DFS based implementation as part of _decompose_dfs
2. Significantly simplifies the logic for preserve_structure=True by deleting the special purpose _decompose_preserving_structure method.

Note that all previously existing tests still pass, which implies that the decompose behavior is unaffected and it's only an internal implementation detail. I will add more tests in a follow-up PR with cleanups that show the benefits of this recursive implementation from the perspective of qubit allocation / deallocation.

cc @NoureldinYosri

part of #6040

[NoureldinYosri]

wouldn't replacing queue.pop(0) with queue.pop() and queue[:0] = x with queue.extend(reversed(x)) do the trick ?

dfs order is just stack order so replacing the lines (and renaming queue to stack) should do the job.

[tanujkhattar]

Yes, this would be sufficient for replacing the BFS order to a DFS order. But what we really is not just replacing the output ordering but also make sure that the items in the recursive OP-TREE are iterated upon in true DFS based ordering. So, in a follow up PR (since it requires a bunch of changes to the interfaces), I want the following test to pass:

def test_decompose_recursive_dfs():
    mock_qm = mock.Mock()

    class RecursiveDecompose(cirq.Gate):
        def __init__(self, recurse: bool = True):
            self.recurse = recurse

        def _num_qubits_(self) -> int:
            return 1

        def _decompose_(self, qubits):
            mock_qm.qalloc(self.recurse)
            yield RecursiveDecompose(recurse=False).on(*qubits) if self.recurse else cirq.Z(*qubits)
            mock_qm.qfree(self.recurse)

    gate = RecursiveDecompose()
    _ = cirq.decompose(gate.on(cirq.NamedQubit("q")))
    expected_calls = [
        mock.call.qalloc(True),
        mock.call.qalloc(False),
        mock.call.qfree(False),
        mock.call.qfree(True),
    ]
    mock_qm.assert_has_calls(expected_calls, any_order=False)

If you simply use a stack instead of a queue but continue the with the iterative approach; in the first step of iteration you'll end up yielding all the elements of RecursiveDecompose(True)._decompose_() instead of getting only the first value from the generator and then recursively decomposing it before moving on to the next statement.

If we get a list of all elements from RecursiveDecompose(True)._decompose_() in a single shot and insert them in a stack; the output ordering of decomposed operations would follow the DFS ordering but we would be processing all children of a node at once; instead of processing them one by one.

We could potentially get around this constraint by adding more if/else blocks and checking whether If the operation on top of the stack is an OP-TREE; only yield the first value from the generator and then push the modified OP-TREE back in the stack along with the first child. Then pop the first child and continue to decompose. But I think the recursive implementation here is a lot more intuitive; so unless there are significant performance concerns with using the recursive approach; I'd say let's make the change to a recursive approach now and we can always optimize back to the iterative approach later.

What do you think? I can also prototype the iterative approach directly if you have a strong preference.

[NoureldinYosri]

flattening the OP_TREE and pushing it to the stack in reverse order is what we need

stack.extend(reversed(cirq.flatten_to_ops(cirq.decompose_once(val))))

---

I'm okay either way. it's just that it seems that modifying the iterative approach is a lot less work

[tanujkhattar]

As I highlighted in my previous comment, flattening the OP-TREE will not work in this case. My argument will become more clear in my next follow up PR. I'll tag you once it's out and we can come back to the iterative vs recursive discussion at that point.

[95-martin-orion]

Couple of pretty minor nits. The fact that the tests are passing gives me a godd amount of confidence in this PR - would it be worth adding a test that this now operates in DFS order instead of BFS?

[tanujkhattar]

Thanks for the review @95-martin-orion!

would it be worth adding a test that this now operates in DFS order instead of BFS?

Watch out for my next PR with a detailed test with a couple more changes to the interfaces. Since decompose is fairly complex; I wanted to keep the interface changes separate from the internal implementation changes (like this one). This PR only gets us to a half-way point. See my comment above for more details on the upcoming test - #6116 (comment)