-
Notifications
You must be signed in to change notification settings - Fork 44
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
Improve interface for bloqs with specialized single-qubit-controlled versions #1451
Improve interface for bloqs with specialized single-qubit-controlled versions #1451
Conversation
@mpharrigan thoughts on the design? |
I took a very brief look and this looks really nice! I'll likely have more specific questions when I take a closer look. How does single-bit zero-control work? it looks like the protocol interface lets the bloq specify its control value but the logic seemingly only works for cv=1 (at least in contrast to SpecializedSingleQubitControl) |
Is |
Updated to account for all CtrlSpecs with 1 qubit
This was the idea, but |
One solution with the least changes would be to update the default def get_ctrl_system(self, ctrl_spec: 'CtrlSpec') -> Tuple['Bloq', 'AddControlledT']:
from qualtran import Controlled
from qualtran.bloqs.mcmt import ControlledViaAnd
if ctrl_spec.num_qubits != 1:
return ControlledViaAnd.make_ctrl_system(self, ctrl_spec=ctrl_spec)
return Controlled.make_ctrl_system(self, ctrl_spec=ctrl_spec) prototyped in #1456 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some initial detailed thoughts. Curious to hear your responses
qualtran/bloqs/mcmt/bloq_with_specialized_single_qubit_control.py
Outdated
Show resolved
Hide resolved
qualtran/bloqs/mcmt/bloq_with_specialized_single_qubit_control.py
Outdated
Show resolved
Hide resolved
qualtran/bloqs/mcmt/bloq_with_specialized_single_qubit_control.py
Outdated
Show resolved
Hide resolved
qualtran/bloqs/mcmt/bloq_with_specialized_single_qubit_control.py
Outdated
Show resolved
Hide resolved
qualtran/bloqs/mcmt/bloq_with_specialized_single_qubit_control.py
Outdated
Show resolved
Hide resolved
qualtran/bloqs/mcmt/bloq_with_specialized_single_qubit_control.py
Outdated
Show resolved
Hide resolved
@mpharrigan I simplifed the interface using your suggestions: I now simply pass the controlled bloq, the ctrl reg name, and the un-controlled bloq. (removed the protocol). I updated the two unittest examples as well. Let me know if it looks good, and I'll update the other examples. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool! What do you think? I think I prefer this style. Would be nice to see one of the examples upgraded to use the just-a-function version
qualtran/bloqs/mcmt/bloq_with_specialized_single_qubit_control.py
Outdated
Show resolved
Hide resolved
qualtran/bloqs/mcmt/bloq_with_specialized_single_qubit_control.py
Outdated
Show resolved
Hide resolved
qualtran/bloqs/mcmt/bloq_with_specialized_single_qubit_control.py
Outdated
Show resolved
Hide resolved
qualtran/bloqs/mcmt/bloq_with_specialized_single_qubit_control.py
Outdated
Show resolved
Hide resolved
qualtran/bloqs/mcmt/bloq_with_specialized_single_qubit_control.py
Outdated
Show resolved
Hide resolved
qualtran/bloqs/mcmt/bloq_with_specialized_single_qubit_control_test.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now really elegant, imo. You can see the flow of information through the function calls clear as day.
A couple small nits, then LGTM except
We've got to pare down the name somehow :p The module name is very long.
from qualtran.bloqs.mcmt.bloq_with_specialized_single_qubit_control import (
get_ctrl_system_for_bloq_with_specialized_single_qubit_control,
)
this is an absolute unit of an import line haha. Let me brainstorm some ideas. This function is a helper function for get_ctrl_system
for the case where the user has a way of constructing a single-bit controlled version of the bloq. The module lives in qualtran.mcmt
because it handles multi-controlled stuff and uses and ladders. What parts of the name can be removed without causing ambiguity? We can drop "bloq". I don't think "specialized" adds much. Single qubit seems important, but maybe we can abbreviate to 1bit
. get_ctrl_system
should probably be kept verbatim since it is meant to be used as a one-liner within a method of that name. "control" can be "ctrl".
qualtran.mcmt.specialized_ctrl.get_ctrl_system_1bit
would be my overall suggestion. Or maybe get_ctrl_system_1cv
or one_cv
or 1bit_cv
qualtran/bloqs/mcmt/bloq_with_specialized_single_qubit_control.py
Outdated
Show resolved
Hide resolved
ctrl_bloq_and_reg = get_ctrl_bloq_and_ctrl_reg_name(current_ctrl_bit) | ||
if ctrl_bloq_and_reg is None: | ||
raise ValueError( | ||
f"Expected a controlled bloq (self) matching the current control bit {current_ctrl_bit}, got None" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does (self) mean here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually generally confused about this error message. From the docstring it sounds like get_ctrl_bloq_and_ctrl_reg_name
is allowed to return None. But not if current_ctrl_bit is set to something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it can only return None
for cv=0
. For cv=1
a specialization must be provided. The type is a bit confusing now after merging the two cases into a single callable, but I don't think there's a cleaner way of doing it (using a single callable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One option could be to make the main function private, and only expose the helpers:
def _get_ctrl_sys(unctrl_bloq, ctrl_getter: Callable[[ControlBit], Optional[Bloq]]): ...
def get(unctrl_bloq, ctrl_getter: Callable[[ControlBit], Bloq]): ...
def get_from_list(unctrl_bloq, ctrl_bloq: Bloq, ctrl_0_bloq: Optional[Bloq]=None): ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry looks like I misunderstood your question.
get_ctrl_bloq_and_ctrl_reg_name(current_ctrl_bit)
should return the bloq from which this is called (i.e. self
of the caller), so it doesn't make sense if it returns None
.
- use a helper bloq that accepts CU to build CCU - do not pass `bloq_without_ctrl`
I simplified it further - avoided passing Now one needs to only pass the current ctrl bit, and a getter which takes |
return _get_ctrl_system_1bit_cv( | ||
bloq, | ||
ctrl_spec, | ||
current_ctrl_bit=current_ctrl_bit, | ||
get_ctrl_bloq_and_ctrl_reg_name=get_ctrl_bloq_and_ctrl_reg_name, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is literally just the same method? why not just make the private one public? is it because you wanted a different, private docstring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The public method does not have Optional
in the return type of get_ctrl_bloq
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see
ctrl_spec: 'CtrlSpec', | ||
*, | ||
current_ctrl_bit: Optional['ControlBit'], | ||
bloq_with_ctrl: 'Bloq', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this now just ctrl_bloq
? I think the "with" phrasing was to support passing in both positive and negative control
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this is the controlled variant of the bloq. I wanted to not confuse it with bloq.controlled()
(i.e. when current_ctrl_bit
is 1
, bloq_with_ctrl
is bloq
), so was hesitant to name it ctrl_bloq
. But I think ctrl_bloq
is also okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that might be worth keeping distinct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's merge this puppy!
get_ctrl_system_1bit_cv
that bloqs can use in theirget_ctrl_system
ControlledViaAnd
bloq.controlled().controlled()
) by using a singly-controlled bloq and merging the controls as aboveI have reduced the uses of the old interface (
SpecializedSingleQubitControlledExtension
), but some of the uses are not easy to update (as there is no easy way to get an uncontrolled bloq from the controlled bloq in these cases). These should eventually be updated carefully and the old interface removed.