-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add GF2Multiplication
bloq for multiplication over GF($2^m$)
#1436
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.
approval modulo some nits
qubits in each of the two input registers $a$ and $b$ that should be multiplied. | ||
|
||
Registers: | ||
- $x$: Input THRU register of size $m$ that stores elements from $GF(2^m)$. |
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.
format this section like Args so it can be parsed by the autgen thing. The register names shouldn't be latex anywho since they are indeed the python strings 'x', 'y', and 'result' that we are documenting.
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.
done
ctrl, result[i] = bb.add(Toffoli(), ctrl=ctrl, target=result[i]) | ||
x[j], y[i - j] = ctrl[0], ctrl[1] | ||
|
||
# Done :) |
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.
:)
@@ -69,7 +69,7 @@ def adjoint(self) -> 'Bloq': | |||
return Free(self.dtype, self.dirty) | |||
|
|||
def on_classical_vals(self) -> Dict[str, int]: | |||
return {'reg': 0} | |||
return {'reg': self.dtype.from_bits([0] * self.dtype.num_qubits)} |
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.
This is fine for now, but let's add this to the forthcoming issue to track "friction related to adding new data types that aren't integers". Specifically: having allocate always allocate a bunch of zero bits that get interpreted in whatever encoding the dtype specifies is a fine choice but it's a choice nonetheless that we should probably discuss in the broader context of datatypes and it should be documented somewhere.
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.
Added to #1437
def test_gf2_multiplication_classical_sim(m): | ||
bloq = GF2Multiplication(m) | ||
GFM = GF(2**m) | ||
for x in GFM.elements: |
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.
I think you should be able to use @NoureldinYosri 's helper function for writing tests like these
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.
What's the helper function? assert_consistent_classical_action
?
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.
refactored using assert_consistent_classical_action
for y in GFM.elements: | ||
xout, yout, resout = bloq.call_classically(x=x, y=y) | ||
assert x == xout and y == yout | ||
expected_res = x * y |
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.
Usually, we can put the "reference behavior" as the bloq's on_classical_vals
method and test call_classically
's outputs on bloq
vs bloq.decompose_bloq()
.
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.
added on_classical_vals to the bloq
bloq_autotester(_gf2_multiplication_symbolic) | ||
|
||
|
||
@pytest.mark.parametrize('m', [2, 3, 4, 5]) |
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.
This is a borderline case, but consider factoring into a slow, exhaustive test marked with slow
and a quick check that finishes in < 2 seconds.
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.
done
Addressed all nits, merging now. |
* Add GF2Multiplication for multiplication over GF(2^m) * Fix formatting * Address nits
Part of a larger effort to support galois field arithmetic in qualtran. xref #1419