Fix custom controlled versions of SelectOracles and QubitizationWalkOperators
#904
Conversation
anurudhp
force-pushed
the
2024-04-fix-custom-controlled
branch
2 times, most recently
from
ecb0d82
to
308b60e
Compare
qualtran/bloqs/hubbard_model.py
Outdated
| def get_ctrl_system( | ||
| self, ctrl_spec: Optional['CtrlSpec'] = None | ||
| ) -> Tuple['Bloq', 'AddControlledT']: | ||
| from qualtran._infra.gate_with_registers import get_ctrl_system_for_single_qubit_controlled |
There was a problem hiding this comment.
Why is the import happening inside the function?
There was a problem hiding this comment.
ah, this should be at top level.
I haven't yet decided if this is the proper way to implement this. Somehow the control_val interface is a bit ad-hoc and can't be type-checked properly. I'm open to suggestions for this interface.
anurudhp
force-pushed
the
2024-04-fix-custom-controlled
branch
from
308b60e
to
d0731d0
Compare
| with an API that has `control_val` and `control_registers`. | ||
| Add the `get_ctrl_system` implementation as shown below: | ||
|
|
||
| .. code:: python |
There was a problem hiding this comment.
we actually render all this as markdown, so this directive won't work properly. I think the indented section should be rendered as code; you might need backticks if you want to say "python"
| """Wrapper to correctly wire a custom implementation of a single-qubit controlled version of a gate. | ||
|
|
||
| For any gate that supports a single-qubit controlled version | ||
| with an API that has `control_val` and `control_registers`. |
There was a problem hiding this comment.
can you be more specific than "with an api that has..."
I think you require that the bloq has attributes named control_val and control_registers, right? Does this need to be a GateWithRegisters like the annotation?
There was a problem hiding this comment.
updated the design to use a mixin class
|
|
||
| if bloq.control_val is None and ctrl_spec.shapes in [((),), ((1,),)]: | ||
| cbloq = attrs.evolve(bloq, control_val=int(ctrl_spec.cvs[0].item())) | ||
| (ctrl_reg,) = cbloq.control_registers |
There was a problem hiding this comment.
so this assumes that control_registers is actually a property that depends on the control_val attribute, right? please document and/or code this more defensively
There was a problem hiding this comment.
added a check that raises TypeError
| assert hasattr(bloq, 'control_val') | ||
| assert hasattr(bloq, 'control_registers') |
There was a problem hiding this comment.
I don't know if this is a better or worse idea, but: did you consider making this a mixin?
There was a problem hiding this comment.
this does make the code cleaner, and enables type-checking. I made a class SpecializedSingleQubitControlledGate (similar to the design of UnaryIterationGate), please have a look
anurudhp
force-pushed
the
2024-04-fix-custom-controlled
branch
from
d0731d0
to
f14990d
Compare
anurudhp
force-pushed
the
2024-04-fix-custom-controlled
branch
from
0148f42
to
3f0855f
Compare
|
@dstrain115 can you advise on how to fix the type issues |
|
I think you can add "#type: ignore" to all of them. The first is because it doesn't know whether it is an attrs class, so it can't attrs.evolve it. The others are because qualtran and cirq disagree on the definition of "controlled" and the classes inherit from both. The current order should prefer Qualtran's definition to cirq's which seems fine to me. |
I added class SpecializedSingleQubitControlledGate(GateWithRegisters):
...
class QubitizationWalkOperator(SpecializedSingleQubitControlledGate, GateWithRegisters): # <- type check fails here
... |
| Example usage: | ||
|
|
||
| @attrs.frozen | ||
| class MyGate(SpecializedSingleQubitControlledGate, GateWithRegisters): |
There was a problem hiding this comment.
SpecializedSingleQubitControlledGate already derives from GateWithRegisters so we don't need both the classes in the list. If you want to make it a mixin, you shouldn't derive from GateWithRegisters here; or keep it as is and do not include GateWithRegisters as a parent class in bloqs that derive from SpecializedSingleQubitControlledGate
There was a problem hiding this comment.
fixed - used the second suggestion
|
I'm not totally sure. It's something with the multiple inheritance and the function definitions not matching. |
On top of #903
fixes #886