-
Notifications
You must be signed in to change notification settings - Fork 35
Bitpacked GF2 representation #594
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
base: main
Are you sure you want to change the base?
Conversation
# should this be using arr's data (as would be the case without packbits) or a new array? | ||
reshaped = arr[:, np.newaxis] | ||
reshaped = np.packbits(reshaped) | ||
reshaped[:, 0] = GF([0, 0, 0, 0]) |
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 still an issue because it is a separate, new array rather than an alias to a block of memory in arr
, which is what is expected. Perhaps we have to disallow this or issue a warning.
if func in field._FUNCTIONS_REQUIRING_VIEW: | ||
output = field._view(output) if not np.isscalar(output) else field(output, dtype=self.dtype) | ||
if func in field._FUNCTIONS_REQUIRING_VIEW: | ||
output = field._view(output) if not np.isscalar(output) else field(output, dtype=self.dtype) |
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 is the need for these various changes to __array_function__()
? Is it to ensure that _packbits()
is called for GF2BP
and not for GF2
?
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.
FunctionMixin
previously had convolve
, fft
, and ifft
overridden for all classes so that is why no try/except
was needed. I needed to add packbits
, unpackbits
, and concatenate
as well, but only GF2BP
overrides those. So, for all other classes it needs to fall back to the numpy
version.
src/galois/_fields/_gf2.py
Outdated
cls._negative = negative_ufunc(cls, override=np.positive) | ||
cls._subtract = subtract_ufunc_bitpacked(cls, override=np.bitwise_xor) | ||
cls._multiply = multiply_ufunc_bitpacked(cls, override=np.bitwise_and) | ||
cls._reciprocal = not_implemented # reciprocal(cls) |
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 happens if np.reciprocal()
is called on a GF2BP
instance?
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.
Good catch. These were left as potential TODOs, but I didn't mark them as such. I can work up bitpacked versions of them if you like.
Thanks for submitting this! This is a big change, so I'll need some time to review, checkout the code, play with it, and look for issues. I appreciate the contribution. |
…; Add tests for coverage
…ts for Python 3.7.
This draft PR continues from #583.
The approach here is to have an extended version of
GF2
that is used whennp.packbits
is used on aGF2
instance. The following operations have been overridden to operate within a bitpacked representation, unless it requires unpacking to complete the operation:add
subtract
multiply
divide
matmul
(handles matrix-vector and matrix-matrix multiplication via a repack of operandb
)concatenate
inverse
Performance-wise, we look to gain between 4-8x speed improvement for operations as well as an 8x reduction in memory.
One can pack bits along any axis, but depending on the operation it may require unpacking of operands.
Due to all the ways one can index arrays in Python the majority of the logic is in
GF2BP._normalize_indexing_to_tuple
andGF2BP.get_index_parameters
.