Add support for building index from multiple bloom matrices with variable columns #2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since bitarray 0.9.0, the `numpy.where(bitarray)` call does not return
the correct non zero positions for bits. Instead, it returns the index
of non zero bytes. For example, for `bitarray("0000000001000000")`, the
function call returns non zero positions `[1]`, instead of `[9]`, since
bitarray release 0.9.0.
This change fixes this issue by iterating over the bitarray and returning
the list of indices at which the element is `True`.
The bitarray_bug branch contains fix required to support the feature developed in this branch.
`numpy.where(numpy.unpackbits(bitarray))[0].tolist()` seems to perform better than `[index for index, bit in enumerate(bitarray) if bit]` and `list(itertools.compress(itertools.count(), bitarray))` for a `bitarray` that has large number (>1000s) of elements.
-If we declare a bit array of size m, the bits are initialiased with random values (that is why that sometimes tests work, sometimes they do not); -we actually have to run self.bitarray.setall(0) after this command to ensure that the bitarray is built properly; -this is also written in the docs of bitarray: https://github.com/ilanschnell/bitarray/blob/master/bitarray/__init__.py#L24-L25 -this fixes this issue
mbhall88
reviewed
Apr 8, 2020
mbhall88
approved these changes
Apr 8, 2020
leoisl
reviewed
Apr 9, 2020
There was a problem hiding this comment.
Amazing work on this @Zhicheng-Liu . The code is really nice, I learned a bunch of stuff. The tests cover almost all your changes, so I feel confident about merging this to master. I have just some minor comments, I won't even request changes for this PR (you can either implement my comments, or put them as issues to be solved later).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request attempts to add support for building a single BIGSI index from multiple bloom matrices stored in different binary files, with different number of columns.
The PR adds two new subcommands:
merge_bloomsmerges multiple smaller bloom matrices into one larger bloom matrixlarge_buildbuilds a single BIGSI index from multiple bloom matricesThe benefits of this PR are: