Hamming weight phasing with configurable number of ancilla

[dobbse42]

#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).

[google-cla[bot]]

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.

[mpharrigan]

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

[dobbse42]

@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]

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 https://github.com/quantumlib/Qualtran/blob/7bbec3b8e41bbefcfd5820830ca5bc582ff75a29/qualtran/resource_counting/_bloq_counts_test.py#L82-L85 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!

[NoureldinYosri]

@dobbse42 thank you for contributing to Qualtran ... I left a couple of code style comments

[mpharrigan]

hi @dobbse42 , I see you've pushed new commits. What's the status of this? Have you addressed the open review comments or are these still some outstanding?

[dobbse42]

hi @dobbse42 , I see you've pushed new commits. What's the status of this? Have you addressed the open review comments or are these still some outstanding?

Hello, I believe I have addressed all the review comments at this point. However, some of the automated tests are still failing so I'm currently going through those (seems to be mostly linting things).

[NoureldinYosri]

@dobbse42 to fix the formating CI you can run ./check/format-incremental --apply, for the others you can check the log to see why they complain, for example the pylint is complaining because in qualtran/bloqs/rotations/hamming_weight_phasing_test.py the class GateCounts is imported but never used