Ensure that cirq.decompose traverses the yielded OP-TREE in dfs ordering #6117
Conversation
| class RecursiveDecompose(cirq.Gate): | ||
| def __init__(self, recurse: bool = True, mock_qm: Optional[mock.Mock] = None): | ||
| self.recurse = recurse | ||
| self.mock_qm = mock.Mock() if mock_qm is None else mock_qm |
There was a problem hiding this comment.
I see few places in Cirq where stubbing is used, lets not normalize that. Stubs can go horribly wrong => tests that are smoke tests, change detectors or worse tests that pass ragardless of the change.
lets do the test in a better way.
There was a problem hiding this comment.
In this specific case, I think its okay to use stubs since we want to test the ordering in which cirq.decompose will traverse the yielded OP-TREE and this test asserts that specifically. I don't care about the behavior of a Qubit Manager in this test -- it will become relevant when we add a qubit manager and that would be the time to add an integration test.
I could modify the test that asserts the traversal order via some other mechanism (eg: keep a global counter and append items to a list whenever the qalloc / qfree statements are executed) but the mock is essentially doing the same thing so I don't see a reason to replace it with some other equivalent mechanism.
While I generally appreciate the concern for not overusing mocks to write tests, I think the use here is well justified. I'll be curious to hear any alternative suggestions you have in mind for rewriting the test though.
This is a follow-up of #6116
The goal is to make sure that if a gate or operation has a
_decompose_defined such that operations are yielded one by one (i.e. the_decompose_()is a generator); thencirq.decompose(op)protocol should recursively decompose the yielded operations one by one.This guarantee ensures that, if a user allocates / deallocates new qubits as part of the
_decompose_protocol; then all operations yielded between an allocation / deallocation request are recursively decomposed in order after the initial allocation request and before the deallocation request.See the added test using mocks for more details.
Note that the default recommendation to users would be to use a
SimpleQubitManagerso they do not need to worry about this ordering. The guarantees here are brittle and depend largely on assuming that the user is doing the right thing; but this best effort implementation gets us closer to "doing the right thing" for the user with minimal changes to existing code.cc @95-martin-orion @NoureldinYosri
Part of #6040