Create a bloq for galois multiplication #1557
Conversation
tanujkhattar
left a comment
tanujkhattar
left a comment
There was a problem hiding this comment.
Left a round of comments, mainly around the API of the bloqs.
| galois_field: 'galois.FieldArrayMeta' | ||
| m_x: Poly = attrs.field(converter=lambda x: x if isinstance(x, Poly) else Poly.Degrees(x)) | ||
| element_repr: Literal['int', 'poly', 'power'] = attrs.field(default='int', eq=False) |
There was a problem hiding this comment.
Why this change? For users who just want a bloq that can multiply a constant to a GF register, the previous API of specifying a "const" and "galois_field" type was more intuitive. Needing to specify m_x and element_repr explicitly is more verbose. Why do we need this change?
qualtran/_infra/data_types.py
Outdated
| degree: SymbolicInt | ||
| irreducible_poly: Optional['galois.Poly'] = attrs.field() | ||
| element_repr: Literal["int", "poly", "power"] = attrs.field(default='int') | ||
| element_repr: Literal["int", "poly", "power"] = attrs.field(default='int', eq=False) |
There was a problem hiding this comment.
Please add a test to data_types_test.py with examples of which QGF types should be equal and which should be different.
There was a problem hiding this comment.
Also, can you remind me again why we needed to add element_repr to the QGF class in the first place? ]
It looks like its a property of the GF class that can be easily modified by calling gf_type.repr(int). If this is used mainly as a tool for debugging and visualizing the field elements, then its better to not have it as part of the QGF class and simply change the property on the underlying QGF.galois_field type wherever needed.
https://mhostetter.github.io/galois/v0.3.5/api/galois.FieldArray.element_repr/
| sub-quadratic Toffoli gate count](https://arxiv.org/abs/1910.02849v2) Section 3.1 | ||
| """ | ||
|
|
||
| m_x: Poly = attrs.field(converter=lambda x: x if isinstance(x, Poly) else Poly.Degrees(x)) |
There was a problem hiding this comment.
Please consider taking the galois field type GF everywhere instead of taking Poly type. It makes the API simpler. You could also consider taking the QGF type directly, which encoded all the necessary information about the underlying field and represents the type of the quantum register on which you wish to perform the operations.
The signature of all the bloqs would change to dtype: QGF and other parameters like k: SymbolicInt here and const: const above for multiplication by a constant and dtype: QGF and uncompute: bool for GF2Mul bloq below.
| class _GF2MulImpl(Bloq): | ||
| """Multiply two GF2 numbers (or binary polynomials) modulu m(x).""" | ||
|
|
||
| m_x: Poly = attrs.field(converter=lambda x: x if isinstance(x, Poly) else Poly.Degrees(x)) |
There was a problem hiding this comment.
Change to dtype: QGF instead
| Register('f', dtype=self.qgf), | ||
| Register('g', dtype=self.qgf), | ||
| Register('h', dtype=self.qgf), |
There was a problem hiding this comment.
Preferably change to x, y, result but it's also fine if you want to leave it as is since it's a private bloq.
There was a problem hiding this comment.
keeping for the private bloq but changed for the public bloq
| class _GF2MulImpl(Bloq): | ||
| """Multiply two GF2 numbers (or binary polynomials) modulu m(x).""" |
There was a problem hiding this comment.
Please add a more detailed docstring if possible. How is this bloq different from GF2Mul below ?
| def build_composite_bloq( | ||
| self, bb: 'BloqBuilder', f: 'Soquet', g: 'Soquet', h: Optional['Soquet'] = None | ||
| ) -> Dict[str, 'Soquet']: |
There was a problem hiding this comment.
I'd personally prefer taking a **soqs: 'SoquetT' and simply popping the necessary registers and avoiding the need to use a cast and a dedicated early return based on the if self.uncompute condition.
| g = bb.join(g_arr, self.qgf) | ||
| h = bb.join(h_arr, self.qgf) | ||
| return {'f': f, 'g': g, 'h': h} | ||
|
|
There was a problem hiding this comment.
Please implement a build_call_graph to support symbolic cost analysis.
There was a problem hiding this comment.
build_call_graph is already implemented for the public bloq ... this is a private bloq that will never appear to users ... it's used by the public bloq in a add_from statement
|
What's the status of this? |
|
@tanujkhattar ptal, I addressed all of your comments @mpharrigan I got distracted by other things .. this PR is ready for another reivew |
|
how long do you expect |
|
@mpharrigan I'm almost sure that it wasn't that slow when I wrote it ... anyway #1611 |
3 participants
This is the final PR in the series #1554, #1533, and #1516. These PRs implemented the GF($2^n$ ) multiplication construction from https://arxiv.org/abs/1910.02849v2 which has a toffoli complexity of $n^{\log_2{3}}$