-
Notifications
You must be signed in to change notification settings - Fork 57
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
Support spare Prepare with log_block_size=0 #1578
base: main
Are you sure you want to change the base?
Conversation
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.
Looks good, thanks! Your analysis is correct that the qrom bloqs should support log_block_size=0
, and if they don't then that's a bug that should be fixed.
if self.log_block_size == 0: | ||
return () |
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.
We could also write this in a way that we use the extra registers defined in self.build_qrom_bloq.signature
and return them here directly, or we could assert what we expect the shapes of those registers to be and then raise a value error if that's not the case, to find such bugs earlier.
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 I think I see what you're saying. I'm going to merge as-is for expediency and open an issue
ZGate().controlled(CtrlSpec(cvs=0)): 1, | ||
ZGate().controlled(CtrlSpec(cvs=1)): 1, |
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.
To confirm, this was always a bug and the consistency between build_call_graph and build_composite_bloq was not tested and is now fixed? Or is this because we are now supporting log_block_size=0 ?
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.
test_decompose_bloq_counts
was written in a brittle way that broke when I changed the test example to use the default block size, which is zero for the bloq example. I changed it to use the familiar qlt_testing.assert_equivalent_bloq_counts
which revealed that: yes, the CZs were missing from the call graph and should be there
Fixes #1547
PrepareSparse
would fail to decompose withlog_block_size=0
, which is extra unfortunate since the automatic-block-size determination would choose this sometimes. It looks like SelectSwapQROM and QROAMClean supportlog_block_size=0
, but I'd really love if @tanujkhattar or @fdmalone could give a second opinion. The bug fix itself is entirely withinPrepareSparse
-- better accounting of the qroam registers.Some drive-by doc formatting fixes as well