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 12 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.

@anurudhp
Copy link
Contributor

Hi @dobbse42! Here is some feedback on the PR, hope it is helpful!

Do we want to perform input validation (e.g. ancillasize < bitsize-1)? If so, what is the preferred method?

You can use the __attrs_post_init__ method - raise a ValueError on invalid inputs (https://www.attrs.org/en/stable/init.html#post-init)

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?

The current standard way is to use QECGatesCost, e.g. see

def test_qec_gates_cost():
algo = make_example_costing_bloqs()
gc = get_cost_value(algo, QECGatesCost())
assert gc == GateCounts(toffoli=100, t=2 * 2 * 10, clifford=2 * 10)

If you only care about the T-cost, you can use the .total_t_count method to get it (see class GateCounts for more such helper functions)

Is there any particular logic to the choice of parameter values to test, or just common sense?

We usually pick a few random small ones (especially for simulation tests), and a few slightly larger sizes (e.g. for gate cost tests). If you feel there are some edge cases that require careful testing, it's good to add more tests in!

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".

If the paper only has upper bounds, you can add "less than equal" tests against the reference formulas. Sometimes the formulas may only be asymptotically valid, or be missing constant factors, in which case you can leave out precise gate count tests. Another useful contribution is to plot the T-costs vs input size in the bloq notebook, just to visually verify.

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.

The override is needed when the bloq cannot be decomposed (e.g. because of symbolic bitsizes). It is also good to add the override if the call graph is much simpler (and efficient to compute) compared to the bloq decomposition. E.g. in this particular case, the decomposition requires a lot of splits and joins, but the call graph is much simpler. So I'd say keep the override for convenience.

In case the bloq has a valid decomposition but the call graph still gives an error, that is a bug, please report it!

Comment on lines 275 to 290
# TODO: Surely there is a better way of doing this
if remainder > 1:

return {
HammingWeightPhasing(self.ancillasize+1, self.exponent, self.eps): num_iters,
HammingWeightPhasing(remainder, self.exponent, self.eps): bool(remainder),
}
elif remainder:
return {
HammingWeightPhasing(self.ancillasize+1, self.exponent, self.eps): num_iters,
ZPowGate(exponent=self.exponent, eps=self.eps): 1
}
else:
return {
HammingWeightPhasing(self.ancillasize+1, self.exponent, self.eps): num_iters,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use a Counter:

counts = Counter[Bloq]()

counts[HammingWeightPhasing(self.ancillasize+1, self.exponent, self.eps)] += num_iters
if remainder > 1:
    counts[HWP(remainder, ...)] += 1
elif remainder:
    counts[ZPowGate] += 1

return counts

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.

4 participants