quantumlib / Qualtran

Qᴜᴀʟᴛʀᴀɴ is a Python library for expressing and analyzing Fault Tolerant Quantum algorithms.
https://qualtran.readthedocs.io/en/latest/
Apache License 2.0
147 stars 39 forks source link

Add docstrings to basic gates like `CZPowGate`, `Rx`, `Ry` #1148

Open matthagan15 opened 1 month ago

matthagan15 commented 1 month ago

I'm trying to use CZPowGates as examples for some documentation and I run into an IndexError when trying to do a simple Qualtran style implementation of just a single CZPowGate:

from qualtran import BloqBuilder, QBit
from qualtran.bloqs.basic_gates.rotation import CZPowGate

bb = BloqBuilder()
ctrl = bb.add_register_from_dtype("ctrl", QBit())
sys = bb.add_register_from_dtype("sys", QBit())
ctrl, sys = bb.add(CZPowGate(), ctrl=ctrl, q=sys)
bb.finalize(ctrl, sys)

yields the error (ignoring a few intermediate frames of the stack trace):

[8] ctrl = bb.add_register_from_dtype("ctrl", QBit())
      [9] sys = bb.add_register_from_dtype("sys", QBit())
---> [10]ctrl, sys = bb.add(CZPowGate(), ctrl=ctrl, q=sys)
     [11] bb.finalize(ctrl, sys)

     ...

[976](qualtran/_infra/composite_bloq.py:976) """Add a new bloq instance to the compute graph and always return a tuple of soquets.
    [977](qualtran/_infra/composite_bloq.py:977) 
    [978](qualtran/_infra/composite_bloq.py:978) This method will always return a tuple of soquets. See `BloqBuilder.add_d(..)` for a
   (...)
...
--> [729](qualtran/_infra/composite_bloq.py:729)     idxed_soq = in_soq[li]
    [730](qualtran/_infra/composite_bloq.py:730)     assert isinstance(idxed_soq, Soquet), idxed_soq
    [731](qualtran/_infra/composite_bloq.py:731)     func(idxed_soq, reg, li)

IndexError: too many indices for array: array is 0-dimensional, but 1 were indexed

which is difficult to parse because as a user I have not done anything involving arrays. I can provide the intermediate frames of the trace if necessary. When attempting to reverse engineer the previous ways in which CZPowGate is used in the library they all appear to use a cirq DecompositionContext to use the on method of CZPowGate as a cirq gate. One final note: there is no documentation string for CZPowGate for the user to determine what registers the bloq is supposed to act on.

tanujkhattar commented 1 month ago

CZPowGate does not have a ctrl or target register. If you look at the signature of the bloq, this is what you get

In [3]: from qualtran.bloqs.basic_gates import CZPowGate

In [4]: CZPowGate().signature
Out[4]: Signature((Register(name='q', dtype=QBit(), _shape=(2,), side=<Side.THRU: 3>),))

This tells us that the bloq has a single register called "q" which expects an array of 2 soquets of type QBit(). You need to change the bb.add() call and pass it the correct argument as follows:

In [7]: from qualtran import BloqBuilder, QBit
   ...: from qualtran.bloqs.basic_gates.rotation import CZPowGate
   ...:
   ...: bb = BloqBuilder()
   ...: ctrl = bb.add_register_from_dtype("ctrl", QBit())
   ...: sys = bb.add_register_from_dtype("sys", QBit())
   ...: ctrl, sys = bb.add(CZPowGate(), q=[ctrl, sys])
   ...: bb.finalize(ctrl=ctrl, sys=sys)

which is difficult to parse because as a user I have not done anything involving arrays

I agree the error message should probably be more helpful.

One final note: there is no documentation string for CZPowGate for the user to determine what registers the bloq is supposed to act on

We should improve the documentation (PRs appreciated!) -- but beyond documentation an easy way to check the signature of any bloq is to simply print bloq.signature which is a required property that every bloq must have. As I have shown above, doing so correctly prints the signature of the CZ bloq.

tanujkhattar commented 1 month ago

I will change the title of the issue to reflect documentation improvements to the CZ Bloq

matthagan15 commented 1 month ago

Thank you, that helped. However, I do think that CZPowGate should follow CNot and Toffoli in having a ctrl register for the control bit and q or target to denote the target qubit.

tanujkhattar commented 1 month ago

The reason it doesn't have that is because for CZ, control and target is interchangeable and the gate is symmetric about the two qubits.

cc @mpharrigan has been looking at basic gates so I'll defer to him on deciding a convention to name the registers.

matthagan15 commented 1 month ago

Got it, although I disagree with the convention and think a consistent naming scheme would be easier for end users I see the technical merits. thanks for the rationale.