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

Bugfixes in cirq interop #963

Closed

Conversation

tanujkhattar
Copy link
Collaborator

A stab at fixing #818

This is still a draft. More investigations are needed to figure out exactly what's causing the bug. Based on my investigations so far, during the interop, there can be multiple issues:

  • In _cirq_style_decompose_from_decompose_bloq, we call cbloq.to_cirq_circuit_and_quregs and then explicitly call qm.qfree on the output qubits. Most likely, this is the place which is causing the bug. It's because for fancier qubit managers like GreedyQubitManager, us explicitly calling the qm.qfree messes up the state of the qubit manager and doesn't play well with the cirq <-> bloq <-> cirq <-> bloq etc. interop. For example:
image

Other potential issues (may or may not be the case):

  • decompose_from_cirq_style_method does not accept a context and always uses a SimpleQubitManager irrespective of what the user has supplied. This may or may not be fine?
  • Alloc and Free bloqs should define a as_cirq_op instead of getting wrapped in a BloqAsCirqGate.

@tanujkhattar tanujkhattar changed the title First attempt at bugfix Bugfixes in cirq interop May 17, 2024
@mpharrigan
Copy link
Collaborator

Did you see the test failure in #961 ? is there a chance this could be related? Changing SwapWithZero to go through decompose_from_cirq_style_method actually causes incorrect behavior of SelectSwapQROM under simulation. it looks qubit-allocate-y

@mpharrigan
Copy link
Collaborator

what's the status of this?

@tanujkhattar
Copy link
Collaborator Author

I didn't have cycles to work on this

@anurudhp
Copy link
Contributor

On deeper inspection, it looks like GreedyQubitManager is indeed propagated correctly, but _cbloq_to_cirq_circuit seems to destroy ordering:

for binsts in nx.topological_generations(binst_graph):

In the example with two alloc-cnot-free, the topological ordering first processes the two allocs, then cnot, free, cnot, free. So the greedy qubit manager is unable to reuse the qubits. As bloq.decompose_bloq does not preserve/have any moment structure, I doubt this is avoidable.

One way around it would be to use intermediate bloqs to decompose to, for example:

import cirq
from attrs import frozen
from numpy.typing import NDArray
from qualtran import Bloq, BloqBuilder, GateWithRegisters, Signature, SoquetT
from qualtran.bloqs.basic_gates import CNOT
from qualtran.cirq_interop.testing import GateHelper


@frozen
class MultiAlloc(Bloq):
    rounds: int = 2

    @property
    def signature(self) -> "Signature":
        return Signature.build(q=1)

    def build_composite_bloq(
        self, bb: "BloqBuilder", q: "SoquetT"
    ) -> dict[str, "SoquetT"]:
        for _ in range(self.rounds):
            a = bb.allocate(1)
            a, q = bb.add(CNOT(), ctrl=a, target=q)
            bb.free(a)

        return {"q": q}

@frozen
class MultiAllocLayered(Bloq):
    rounds: int = 2

    @property
    def signature(self) -> "Signature":
        return Signature.build(q=1)

    def build_composite_bloq(
            self, bb: "BloqBuilder", q: "SoquetT"
    ) -> dict[str, "SoquetT"]:
        for _ in range(self.rounds):
            q = bb.add(MultiAlloc(1), q=q)

        return {"q": q}


def visualize(bloq: Bloq | GateWithRegisters):
    print(bloq)
    gate = GateHelper(
        gate=bloq,
        context=cirq.DecompositionContext(
            cirq.GreedyQubitManager(prefix="_greedy", maximize_reuse=True)
        ),
    )
    print(gate.circuit)
    print()
    print(gate.decomposed_circuit)
    print()


if __name__ == "__main__":
    visualize(MultiAlloc())
    visualize(MultiAllocLayered())
MultiAlloc
q: ───q───

_greedy_0: ───alloc────@───free─────────────────────────
                       │
_greedy_1: ───alloc────┼────────────────────@───free────
                       │                    │
q: ───────────Y^-0.5───@───Y^0.5───Y^-0.5───@───Y^0.5───

MultiAllocLayered
q: ───q───

_greedy_0: ───alloc────@───free────alloc────@───free────
                       │                    │
q: ───────────Y^-0.5───@───Y^0.5───Y^-0.5───@───Y^0.5───

@tanujkhattar
Copy link
Collaborator Author

This is now merged in a separate PR. Closing the draft now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants