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

Hamming weight phasing with configurable number of ancilla #1450

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

Conversation

dobbse42
Copy link

@dobbse42 dobbse42 commented Oct 8, 2024

#645: Added HammingWeightPhasingWithConfigurableAncilla and a temp test file, still debugging. TODO: add unit tests to hamming_weight_phasing_test.py

Currently having issues joining the slices of the phased register back together with
x = bb.join(x_parts, dtype=QUInt(self.bitsize.bit_length()))
at the end of build_composite_bloq() (line 272).

Copy link

google-cla bot commented Oct 8, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@dobbse42 dobbse42 marked this pull request as ready for review October 14, 2024 06:44
@mpharrigan
Copy link
Collaborator

Thanks for opening a PR! Please ping me or @tanujkhattar when this is ready for review or if you have any specific questions

@mpharrigan mpharrigan changed the title #645 Hamming weight phasing with configurable number of ancilla Hamming weight phasing with configurable number of ancilla Oct 16, 2024
#print("shape after flatten: ", np.shape(x_parts))
for part in x:
print("next elem: ", part)
x = bb.join(x_parts, dtype=QUInt(self.bitsize.bit_length()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

try bb.join(np.array(x_parts), ...)

Elsewhere, we tried to be careful about accepting either lists or numpy arrays but seem to have missed this one.
#1470

@dobbse42 dobbse42 marked this pull request as draft October 16, 2024 07:38
@dobbse42 dobbse42 marked this pull request as ready for review October 17, 2024 08:34
@dobbse42
Copy link
Author

dobbse42 commented Oct 17, 2024

@mpharrigan This should be in a functional state at least. There are a few TODO comments which indicate things I couldn't immediately tell how to do but aren't essential, so suggestions for those would be appreciated. I also have a few questions:

  • Do we want to perform input validation (e.g. ancillasize < bitsize-1)? If so, what is the preferred method?
  • Should I write gatecount assertions to get gatecounts of subcomponents, or just hardcode them? e.g. should I just write "4*num_toffolis" or is there some way to get the T-count of a specified toffoli decomposition in tests?
  • Is there any particular logic to the choice of parameter values to test, or just common sense?
  • Are we testing against what we logically expect or what the paper says should happen? i.e. we can often get more precise gatecounts by just thinking about the situation (in which case we might assert that counts are exactly equal to what we expect), whereas papers are often just concerned with asymptotics or upper bounds (in which case we would assert that gatecounts are <= what the paper reports). Further, there are situations where computing the precise gatecount for every input is tedious and not very helpful for testing compared to just saying "less than" or "approx equal to".
  • When is a build_call_graph override necessary? For example, HWphasegradient doesn't have one but I got errors when I tried to test HWConfigurableancilla without one.

All feedback is welcome, especially on the tests as I do not have much experience with pytest. I expect to have to make quite a few modifications to this.

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.

3 participants