More improvements to the Controlled infrastructure #879

tanujkhattar · 2024-04-16T20:14:42Z

Fixes #865
Fixes #686

The improvements are -

GateWithRegisters.controlled() now returns Controlled bloq
cirq_gate_to_bloq now maps a cirq.Controlled gates to Controlled bloq.
Controlled bloq is now a GateWithRegisters. This is required because we want to support cirq style simulations for circuits containing Controlled(Rx()) bloqs and BloqAsCirqGate(Controlled()) is not sufficient because we need special logic to derive the unitary matrix for controlled bloqs (eg: when subbloq is a GateWithRegisters but doesn't have only THRU registers).

After this PR, we should never need to invoke the cirq.Controlled gates in either cirq-style or bloq-style API in Qualtran and we should always use Controlled bloq in all cases. All existing uses of cirq.Controlled gates would be updated to Controlled bloq when invoking the bloq-style API using the cirq_gate_to_bloq mapping.

This is the final PR in my series of PRs to improve the controlled infrastructure. cc @mpharrigan

anurudhp · 2024-04-16T21:26:55Z

qualtran/_infra/gate_with_registers.py

    def controlled(
        self,
-        num_controls: Optional[int] = None,
+        num_controls: Union[Optional[int], 'CtrlSpec'] = None,
        control_values=None,
        control_qid_shape: Optional[Tuple[int, ...]] = None,
-    ) -> 'cirq.Gate':
-        from qualtran.cirq_interop import BloqAsCirqGate
-
-        controlled_gate = cirq.ControlledGate(
-            self,
-            num_controls=num_controls,
-            control_values=control_values,
-            control_qid_shape=control_qid_shape,
-        )
-        ctrl_spec = CtrlSpec.from_cirq_cv(controlled_gate.control_values)
-        return BloqAsCirqGate(Controlled(self, ctrl_spec))
+    ) -> 'GateWithRegisters':


Would it be useful to provide overloaded type hints?

# pylint: disable=arguments-renamed @overload def controlled( self, num_controls: Optional[int] = None, control_values=None, control_qid_shape: Optional[Tuple[int, ...]] = None, ) -> 'GateWithRegisters': ... # pylint: disable=signature-differs @overload def controlled(self, ctrl_spec: 'CtrlSpec') -> 'GateWithRegisters': ... # pylint: disable=arguments-renamed def controlled(self, num_controls=None, control_values=None, control_qid_shape=None):

Right now it's difficult to understand the usage without looking at the function body.

yeah, I think this needs a big docstring that describes the two modes of operation

Added a detailed docstring and the overloads. Also added tests to verify the different cases raise errors.

mpharrigan

looks good. approval but please fix the issues raised

mpharrigan · 2024-04-16T22:35:48Z

qualtran/_infra/adjoint.py

@@ -150,6 +150,13 @@ def decompose_from_registers(
            return cirq.inverse(self.subbloq.decompose_from_registers(context=context, **quregs))
        return super().decompose_from_registers(context=context, **quregs)

+    def _circuit_diagram_info_(self, args: 'cirq.CircuitDiagramInfoArgs'):


return type annotation

mpharrigan · 2024-04-16T22:36:28Z

qualtran/_infra/controlled.py

+                cirq.ControlledGate(self.subbloq, control_values=self.ctrl_spec.to_cirq_cv())
+            )
+        if all(reg.side == Side.THRU for reg in self.subbloq.signature):
+            # subbloq has only THRU registers, so the unitary is well defined.


pycharm tells me this is supposed to be "well-defined"

mpharrigan · 2024-04-16T22:38:09Z

qualtran/_infra/gate_with_registers.py

    def controlled(
        self,
-        num_controls: Optional[int] = None,
+        num_controls: Union[Optional[int], 'CtrlSpec'] = None,
        control_values=None,
        control_qid_shape: Optional[Tuple[int, ...]] = None,
-    ) -> 'cirq.Gate':
-        from qualtran.cirq_interop import BloqAsCirqGate
-
-        controlled_gate = cirq.ControlledGate(
-            self,
-            num_controls=num_controls,
-            control_values=control_values,
-            control_qid_shape=control_qid_shape,
-        )
-        ctrl_spec = CtrlSpec.from_cirq_cv(controlled_gate.control_values)
-        return BloqAsCirqGate(Controlled(self, ctrl_spec))
+    ) -> 'GateWithRegisters':


yeah, I think this needs a big docstring that describes the two modes of operation

mpharrigan · 2024-04-16T22:40:18Z

qualtran/bloqs/phase_estimation/text_book_qpe.py


        yield self.ctrl_state_prep.on(*phase_qubits)
        for i, qbit in enumerate(phase_qubits[::-1]):
-            yield cirq.pow(unitary_op.controlled_by(qbit), 2**i)
+            yield Power(self.unitary.controlled(), 2**i).on(qbit, *target_qubits)
+            # yield cirq.pow(unitary_op.controlled_by(qbit), 2**i)


remove old code

Reverted the file so now there are no changes and the original implementation uses bloqs instead of cirq gates. See added tests in gate_with_registers_test.py for clarity.

anurudhp · 2024-04-17T08:47:52Z

qualtran/bloqs/qsp/generalized_qsp.py

                num_inverse_applications -= 1
            else:
                # apply C[0]-U
-                yield self.U.on_registers(**quregs).controlled_by(signal_qubit, control_values=[0])
+                yield self.U.controlled(control_values=[0]).on(signal_qubit, *flat_qubits_for_u)


Should we default to using qualtran style controls CtrlSpec(cvs=0) instead of cirq style control_values=[0]?

Reverted this change because self.U.on_registers(**quregs).controlled_by(signal_qubit) now uses the Controlled bloq instead of cirq gate. The op.controlled_by still expects the cirq-style API since we don't have a custom GWROperation where we can override the controlled_by to expect the bloq-style API as well. But I think it's fine right now to do things this way

…ntrolled_improve

tanujkhattar · 2024-04-18T22:01:23Z

@mpharrigan I'm ready to merge this but feel free to take another look since I've made some minor upgrades. Using gwr.controlled().on_registers() and gwr.on_registers().controlled_by are now equivalent and both result GateOperation(Controlled(gwr)) where the wrapped gate is a the Controlled bloq.

Tl;Dr - the discrepancy in usage from a users perspective that I described in Qualtran sync today is now gone and users don't need to worry about the intricacies when using the Cirq or Bloq style APIs.

qualtran/_infra/gate_with_registers.py

mpharrigan · 2024-04-18T23:17:51Z

where is the logic for controlled_by?

tanujkhattar · 2024-04-18T23:20:29Z

where is the logic for controlled_by?

No override is needed because of the way Cirq and Qualtran works. GateOperation.controlled_by delegates to Gate.controlled() by default, which now uses GateWithRegisters.controlled() which returns a Controlled(GateWithRegisters) which is a cirq gate and gets wrapped in the GateOperation(controlled_bloq).

This was true today morning as well, I just realized it when making these changes and thus added tests to gate_with_registers_test.py to make this concrete.

tanujkhattar · 2024-04-19T00:20:47Z

Merging this now

More improvemetns to the Controlled infrastructure

c72bcef

tanujkhattar requested a review from mpharrigan April 16, 2024 20:14

tanujkhattar assigned mpharrigan Apr 16, 2024

Fix pylint

065cabf

anurudhp reviewed Apr 16, 2024

View reviewed changes

mpharrigan approved these changes Apr 16, 2024

View reviewed changes

anurudhp reviewed Apr 17, 2024

View reviewed changes

tanujkhattar mentioned this pull request Apr 17, 2024

Add walk operator example for sparse chem ham. #882

Merged

tanujkhattar added 2 commits April 18, 2024 12:47

Merge branch 'main' of https://github.com/quantumlib/Qualtran into co…

fd1e74f

…ntrolled_improve

Add docstrings, simplify usage of cirq-style API and address comments

d92095b

mpharrigan reviewed Apr 18, 2024

View reviewed changes

qualtran/_infra/gate_with_registers.py Outdated Show resolved Hide resolved

Add docstring

0ba0f69

tanujkhattar merged commit 7bf19b3 into quantumlib:main Apr 19, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More improvements to the Controlled infrastructure #879

More improvements to the Controlled infrastructure #879

tanujkhattar commented Apr 16, 2024

anurudhp Apr 16, 2024

mpharrigan Apr 16, 2024

tanujkhattar Apr 18, 2024

mpharrigan left a comment

mpharrigan Apr 16, 2024

tanujkhattar Apr 18, 2024

mpharrigan Apr 16, 2024

tanujkhattar Apr 18, 2024

mpharrigan Apr 16, 2024

mpharrigan Apr 16, 2024

tanujkhattar Apr 18, 2024 •

edited

Loading

anurudhp Apr 17, 2024

tanujkhattar Apr 18, 2024

tanujkhattar commented Apr 18, 2024

mpharrigan commented Apr 18, 2024

tanujkhattar commented Apr 18, 2024

tanujkhattar commented Apr 19, 2024

More improvements to the Controlled infrastructure #879

More improvements to the Controlled infrastructure #879

Conversation

tanujkhattar commented Apr 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpharrigan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanujkhattar Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanujkhattar commented Apr 18, 2024

mpharrigan commented Apr 18, 2024

tanujkhattar commented Apr 18, 2024

tanujkhattar commented Apr 19, 2024

tanujkhattar Apr 18, 2024 •

edited

Loading