add optional pos parameter to start search from - Githubissues

diamondman / bitarray

efficient arrays of booleans for Python

Other

5 stars 2 forks source link

add optional pos parameter to start search from #3

Closed andre-merzky closed 8 years ago

andre-merzky commented 8 years ago

This change is backward compatible. It adds an additional int (or rather ssize_t typed) pos parameter to ba.search(), and the search is started from that position on. That significantly improves runtime for consecutive searches in large bitarrays. In my specific use case of searching 1k long, distinct patterns 1M times in a 16MBit array, the time reduced from 80 seconds to 0.5 seconds.

This PR was moved over from https://ilanschnell/bitarray/pull/35 .

diamondman commented 8 years ago

Looks like my moving of the tests broke all your tests. All I did was move the file and remove the lines for test.append since py.test just does that detection for us.

andre-merzky commented 8 years ago

Yeah, seems like I did not sync the tests with a method name change... Yay CI ! :)

diamondman commented 8 years ago

The docs in README.rst should be updated for search to include your new pos argument

andre-merzky commented 8 years ago

The docs in README.rst should be updated for search to include your new pos argument

Yep, will do.

diamondman commented 8 years ago

Ok, I like the API. only thing left is the docs in the RST. I am not trying to be too nitpicky :). The previous repo was just kept so clean!

andre-merzky commented 8 years ago

No worries, picky is fine wrt. code :)

diamondman commented 8 years ago

Perfect. I see no other issues. Merging.

eltoder commented 8 years ago

Hi guys! Thank you for taking over the project so that we can merge some much needed fixes! Two comments:

Moving test_bitarray.py broke bitarray.test(), which is described in README, and is a nice way to test one's installation. It used to be copied to the install directory. I guess setup.py needs updating.
It would be nice to squish commits before merging into master to not pollute the history.

diamondman commented 8 years ago

@eltoder I noticed the information was still in the README, and was going to replace it with the py.test steps. But frustratingly enough, the 'modern' way of testing has trouble running unless you install into the same directory with pip install -e . Very frustrating. There were a few reasons I changed it in the first place:

py.test provides very good output and has many plugins. I believe it even handles coverage reports of C modules.
Including the test suite in the full package of the software is very uncommon and I consider it useless. I believe tests should be available to people with a full source repository, and not an installed egg/wheel.
Keep test runner code where it belongs: in the test runner codebase. Not in bitarray.

Currently I am unsatisfied with the need to install the package with -e for the tests to work, and am frustrated with the number of PRs break because of the test file being moved.

For now maybe I should move the test file back so we can more easily merge the PRs, and ten discuss what to do after that.

diamondman commented 8 years ago

Also @eltoder I created a github issue for these types of problems. https://github.com/diamondman/bitarray/issues/1

eltoder commented 8 years ago

@diamondman I'm not a fan of including the tests in the installation either, but it is not uncommon in Python. Numpy and Scipy do this, and I've noticed other packages following the trend. Whatever you decide, it would be nice to have updated instructions in README.

diamondman commented 8 years ago

I will work on cleaning this up. At least to make it easier to merge tests in. Then I can see if there is a more sane way to run C tests without the -e. install. And I agree I need to update README no matter what I do.