Implement decomposition of ECAddR, ECWindowAddR, and ECAdd bloqs #1406
Conversation
|
tests? |
I added a classical simulation test for ECAddR(). Any recommendations on what tests I should add for ECPhaseEstimateR()? Also, for the call graph should I include the XorK() bloqs in the counts? I notice you don't include the IntState counts and this is kind of a roundabout way of initializing an intstate in a controlled manor. I can add call graph tests once that is sorted (and once I fix the .controlled() bug on the call graph). |
There was a problem hiding this comment.
Let's also add a concrete bloq example for ECAdd, which I think will trigger the decomposition bug (?) You can also use this bloq_example to write a minimal unit test that you can indeed bloq.call_classically(...) although you won't be able to assert anything particularly insightful about the results
| @@ -33,6 +37,8 @@ class ECAdd(Bloq): | |||
| Args: | |||
| n: The bitsize of the two registers storing the elliptic curve point | |||
| mod: The modulus of the field in which we do the addition. | |||
| curve_a: The $a$ coefficient of an elliptic curve given in the standard form of | |||
There was a problem hiding this comment.
does this actually affect the decomposition?
This is a little confusing. In litinski they use (x, y) + (a, b) for the curve points and c1, c2 for the curve coefficients. Other references use (x1, y1) + (x2, y2) and a, b for the curve points. Maybe we should switch to using this second scheme.
There was a problem hiding this comment.
I no longer need curve_a for the classical simulation, I can get it from a, b, and lambda_r. I think a and b for the curve coefficients is much more clear than c1, c2. I can swap from (x,y) + (a,b) to (x1,y1) + (x2,y2), but I think (x, y) + (a, b) is easier to understand. I think since we use curve_a instead of just a it is clear that it is different. I'll defer judgement to you on the naming convention and I can change it if you think that's better.
There was a problem hiding this comment.
point taken on curve_a but we'll probably want to write latex expressions in some places and then
| @@ -67,6 +85,41 @@ def signature(self) -> 'Signature': | |||
| [Register('ctrl', QBit()), Register('x', QUInt(self.n)), Register('y', QUInt(self.n))] | |||
| ) | |||
|
|
|||
| def build_composite_bloq( | |||
There was a problem hiding this comment.
where does this decomposition come from? it does not match the references.
There was a problem hiding this comment.
I'm a little confused myself on whether ECAddR and ECWindowAddR are necessary distinctions. Is ECAddR not just supposed to be a single window ECWindowAddR? In that case we can make it one bloq with a var window_size that changes decomposition. Otherwise I couldn't find a special decomposition for the ECAddR circuit so I put what I imagined it would be (initialize a,b from R.x,R.y; ECPointAdd; uninitialize a,b;).
There was a problem hiding this comment.
I think we chatted in person, but the Roetteler / q# reference has a different circuit for this. What do you want to do here? I think we discussed having the "main call graph" go via ECWindowAddR. We'll keep the reference to the special circuit for adding a constant (without additional register allocation) but not use it
|
@mpharrigan I'll spend some time today finishing the PR (more tests and fix the call graph for ECAdd). I was wondering if you think ECAdd should be split into sub-bloqs since the decomposition is long and messy? |
fpapa250
changed the title
|
@fpapa250 can you break down this PR into smaller pieces? e.g. "Add Equals", "support serialization of ECPoint", ...etc |
|
Can you please link all the sub-PRs that you broke this into for the historical record |
|
One more PR: Now that they are all merged this PR can be closed as all of it's pieces are merged. |
|
wooooo |
Implements decomposition of ECAddR and ECPhaseEstimateR to match that of https://arxiv.org/abs/2306.08585.
Removes MeasureQFT() bloq in favor of an actual QFT() bloq and a shimmed in Measure() bloq.