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

Restrict controlled from non-thru registers #1305

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

mpharrigan
Copy link
Collaborator

For #1304

  • Only thru registers allowed in Controlled
  • Prevents triggering the bug in classical simulation where controlled allocations are not supported

@tanujkhattar
Copy link
Collaborator

Left a comment on the original issue - #1304 (comment)

@mpharrigan
Copy link
Collaborator Author

Let's do this and relax later

@mpharrigan mpharrigan requested a review from fdmalone September 9, 2024 23:10
Comment on lines -388 to -397
@attrs.frozen
class TestCtrlStatePrepAnd(Bloq):
"""Decomposes into a Controlled-AND gate + int effects & targets where ctrl is active.

Tensor contraction should give the output state vector corresponding to applying an
`And(and_ctrl)`; assuming all the control bits are active.
"""

ctrl_spec: CtrlSpec
and_ctrl: Tuple[int, int]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a private flag to the Controlled class, something of the form _THRU_REGISTERS_ONLY = True, and keep this test around? Since we plan to relax this constraint later, it'll be nice to not delete the test.

Comment on lines +315 to +320
@cached_property
def _thru_registers_only(self) -> bool:
for reg in self.subbloq.signature:
if reg.side != Side.THRU:
return False
return True
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See other comment -- it'll be great if we can guard this with a flag set to TRUE by default. When we relax the condition, we can get rid of the flag and also keep the tests around.

Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % one request. Please add the restriction controlled by a flag set to TRUE by default.

@mpharrigan mpharrigan added this to the v1.0 milestone Oct 10, 2024
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.

2 participants