Initial implementation of read-only buffer access to raw tables

[camillescott]

This addresses #667: exposes the tables as a read-only buffer object. I've chosen read-only as there will be undefined behavior from writing directly to the tables outside of the count functions; of course, the idea is that you can still use the python-exposed hashtable object as you see fit, and the buffer view will update to reflect that.

For now, I've just exported a list of N buffers. Eventually I'd like this to properly define the buffer interface on the hashtable type object, exposing a single buffer or memoryview object with proper striding / offsets for the N dimension array, but... :effort:.

Some example usage:

In [1]: import khmer

In [2]: import numpy as np

In [3]: ht = khmer.new_counting_hash (20, 1e8, 4)

In [4]: tables = ht.get_raw_tables ()

In [5]: tables[0]
Out[5]: <read-only buffer ptr 0x7ff6a7015010, size 100000007 at 0x7ff6acff2930>

In [6]: arr = np.frombuffer (tables[0], dtype=np.uint8)

In [7]: arr
Out[7]: array([0, 0, 0, ..., 0, 0, 0], dtype=uint8)

In [8]: arr.sum()
Out[8]: 0

In [9]: ht.consume_fasta('tests/test-data/test-reads.fa')
Out[9]: (25000, 1425000L)

In [10]: arr.sum()
Out[10]: 1424408

[kdm9]

This is great! Thanks very much. Will have a go with this, and let you know how I get on! Thanks for the speedy implementation 😄

[mr-c]

+1

[camillescott]

No problem! Now it's actually tested as well :)

[mr-c]

@kdmurray91 Was this useful? Should it get merged in?

[ctb]

ping @kdmurray91

[kdm9]

Apologies all, I've been on holiday and this must have been lost in a sea of notifications. This was very useful, and still is. Thanks very much @camillescott et al.

[kdm9]

Is there anything that I need to do to get this merged?

[mr-c]

@camillescott claims that she'll be joining the sprint Monday. So hopefully she brings this up to speed for @kdmurray91

[mr-c]

ping @camillescott :-)

[kdm9]

I swear I added a long comment to this PR yesterday, it appears to have disappeared. Is it just me?

[mr-c]

@kdmurray91 Last comment I see from you is February 22nd :-/

[kdm9]

Ah crap :)

From memory, it went something like:

FYI, I've rebased on master here

I'd like to get this merged, and am hacking on the code that uses this actively, so consider me willing to do whatever it takes to get this, a) stable, and more user friendly if that's desired, and b) tested/documented/all the other things on the PR checklist.

To that end, I proposed making what @camillescott has done an internal feature, then using some wrapper in python land to expose this as either a single numpy array or just do what she has in her demo in the PR description. This could be done by wrapping (i.e. subclassing as is done with HLLCounter in __init__.py) the CPython CountingHash class in python land, with a python implementation of a method that does import numpy. This means that unless you call that method, you don't need numpy to use the package (I think). Thoughts?

Also, let me know your thoughts on a read/write version of this too, as algorithm I'm working on relies on adding/subtracting hashes (ignoring bigcount which we're not using). It would be useful to have, and am happy to work on ways of doing this safely under circumstances where it is safe (e.g., using the busywait locks and __sync_add_and_fetch to add the values). And checking that we're not using bigcount. And this would break things like _n_unique_kmers etc, unless we also added those.

Anyway, that turned out to be more mental diarrhea than a comment, but that's my 0.000002 BTC, as the cool kids say.

Cheers,
K

[mr-c]

+1 for merging this w/o a script frontend as that does not commit us to anything.
+1 for iterating on top of that later :-)

I'm fine with experimental Python & C++ APIs. @camillescott or @kdmurray91 can you either update this PR with a filled out checklist or make a new PR?

[kdm9]

Yup! Unless @camillescott rebases in the next hour, I'll make a new PR from my rebased branch. Or could I push to this branch?

On a side note, is there any merit to having the likes of a khmer.__future__ module to make experimental APIs more segregated?

[kdm9]

• Is it mergable?
• Did it pass the tests?
• If it introduces new functionality in scripts/ is it tested?
  Check for code coverage.
• Is it well formatted? Look at make pep8, make diff_pylint_report,
  make cppcheck, and make doc output. Use make format and manual
  fixing as needed.
• Did it change the command-line interface? Only additions are allowed
  without a major version increment. Changing file formats also requires a
  major version number increment.
• Is it documented in the ChangeLog?
• Was a spellchecker run on the source code and documentation after
  changes were made?

[mr-c]

The APIs are getting a slow overhaul and are subject to change at any time so there is no current need to segregate; thanks though!

[kdm9]

OK, so to avoid nuking anything I've made a backup of this branch here:
https://github.com/kdmurray91/khmer/tree/upstream/feature/pymemoryview

and will have a go at pushing directly to this branch

[kdm9]

What's the policy on cleaning formatting errors etc in places not touched by the changes in a PR?

E.g., there's some formatting issues in the HLL counter, I assume I leave those for another PR?

[kdm9]

All ticked off!

[mr-c]

You're on the hook for lines of code you touch, and no other :-) I'm sure @luizirber will want to hear about any HLL issues.

[mr-c]

LGTM

[mr-c]

Share & enjoy!

[camillescott]

Thanks for putting in the work to make this mergeable @kdmurray91! And sorry for ducking out on it myself; got distracted with other things.

[kdm9]

All good, thanks for your initial work 😄