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

PlusEqualProduct version of GFMultiplication for GF($2^m$) #1457

Merged
merged 4 commits into from
Oct 16, 2024

Conversation

tanujkhattar
Copy link
Collaborator

Updates the existing GFMultiplication bloq to support an in-place PlusEqualProduct version of the bloq where the multiplied integer gets added to the result register. The decomposition is very similar except we need an inverse of the linear reversible circuit at the beginning, so I decided to add this as a special case controlled by a flag instead of adding a new Bloq.

Part of a larger effort to support galois field arithmetic in qualtran. xref #1419

Copy link
Contributor

@NoureldinYosri NoureldinYosri left a comment

Choose a reason for hiding this comment

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

LGTM%important comment

@@ -143,14 +156,15 @@ def qgf(self) -> QGF:
def reduction_matrix_q(self) -> np.ndarray:
m = int(self.bitsize)
f = self.qgf.gf_type.irreducible_poly
M = np.zeros((m - 1, m))
M = np.zeros((m, m))
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this and the line M[m - 1][m - 1] = 1 only happen when the new flag is true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, the original matrix represents a transformation that maps m - 1 inputs to m outputs. The bloq for synthesizing the circuit assumes it acts on m inputs and outputs (notice that the signature has only one THRU register of size m) and the m'th input is mapped to the output as an identity. This change just makes the implicit assumption explicit, and it now has the benefit that the matrix is square and invertible so it works for both the cases.

Tl;Dr - It's working as intended.

@mpharrigan
Copy link
Collaborator

looks fine to me, modulo nour's comment. If that is indeed an issue, can we write a unit test that would have caught it?

@tanujkhattar tanujkhattar enabled auto-merge (squash) October 16, 2024 20:25
@tanujkhattar tanujkhattar merged commit 6e4b7ad into quantumlib:main Oct 16, 2024
8 checks passed
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