Add hash functions supporting k > 32 to Counttable by ctb · Pull Request #1511 · dib-lab/khmer

ctb · 2016-11-12T15:41:22Z

This follows from #1504 and adds support for irreversible hash functions (supporting k > 32).

Specifically, in this PR we:

update Counttable to use murmurhash and thus supported k > 32;
introduce a MurmurHashKmerIterator for hashing strings;
use parameterized tests to test the CPython API for _Countgraph, _Counttable, _SmallCountgraph, _SmallCounttable _Nodegraph, and _Nodetable;
provide some minimal documentation for the Python API for tables;

Basically this is many small changes that are now reasonably well tested and understood, taking advantage of the refactoring done in #1504.

Is it mergeable?
make test Did it pass the tests?
make clean diff-cover If it introduces new functionality in
scripts/ is it tested?
make format diff_pylint_report cppcheck doc pydocstyle Is it well
formatted?
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?
http://en.wikipedia.org/wiki/Changelog#Format
Was a spellchecker run on the source code and documentation after
changes were made?
Do the changes respect streaming IO? (Are they
tested for streaming IO?)
Is the Copyright year up to date?

codecov-io · 2016-11-12T15:52:26Z

Codecov Report

❗ No coverage uploaded for pull request base (master@8c2b32c). Click here to learn what that means.
The diff coverage is 34.09%.

@@            Coverage Diff            @@
##             master    #1511   +/-   ##
=========================================
  Coverage          ?   69.79%           
=========================================
  Files             ?       66           
  Lines             ?     8966           
  Branches          ?     3059           
=========================================
  Hits              ?     6258           
  Misses            ?     1025           
  Partials          ?     1683

Impacted Files	Coverage Δ
khmer/_cpy_nodetable.hh	`50% <ø> (ø)`
khmer/init.py	`96.87% <100%> (ø)`
lib/hllcounter.cc	`60.6% <100%> (ø)`
khmer/_khmer.cc	`57.42% <22.33%> (ø)`
lib/hashtable.cc	`55.39% <32.43%> (ø)`
lib/hashtable.hh	`74.13% <36.36%> (ø)`
lib/kmer_hash.cc	`86.27% <50%> (ø)`
lib/storage.cc	`47.89% <50%> (ø)`
lib/storage.hh	`89.79% <75%> (ø)`
lib/kmer_hash.hh	`86.95% <80%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c2b32c...be56172. Read the comment docs.

ctb · 2016-11-12T17:35:27Z

w00t!

This code:

import khmer

a = khmer.Counttable(5, 1e6, 3)
b = khmer.Countgraph(5, 1e6, 3)

print(a.get_kmer_hashes("ATGGAGGACAAGTAT"))
print(b.get_kmer_hashes("ATGGAGGACAAGTAT"))

print(a.hash('ATGGA'))
print(b.hash('ATGGA'))

now yields:

[2898244513215582206, 7782325612895303798, 17716873001369615262, 5266100104194697999, 16527671766578686784, 1954197895523434265, 9372126835751090531, 2912821416709855779, 8698352977978348149, 13735319396097939869]
[124, 499, 666, 422, 242, 474, 374, 131, 151, 52, 73]
2898244513215582206
124

so we're finally able to swap out hash functions on Hashtable classes!

ctb · 2016-11-12T17:35:54Z

(and reversibility is no longer a requirement on Hashtable derived classes, only on Hashgraph derivatives.)

ctb · 2016-11-12T17:41:03Z

So this now works, too:

import khmer

a = khmer.Counttable(33, 1e6, 3)

print(a.get_kmer_counts("ATGGATATGGAGGACAAGTATATGGAGGACAAGTATATGGAGGACAAGTAT"))
print(a.consume("ATGGATATGGAGGACAAGTATATGGAGGACAAGTATATGGAGGACAAGTAT"))
print(a.get_kmer_counts("ATGGATATGGAGGACAAGTATATGGAGGACAAGTATATGGAGGACAAGTAT"))

betatim · 2016-11-14T18:21:54Z

What am I missing here:

s = "ATGGATATGGAGGACAAGTATATGGAGGACAAGTATATGGAGGACAAGTAT"
a =khmer.Counttable(33, 1e6, 3)
a.get_kmer_hashes(s[:33]) # -> []
a.get_kmer_hashes(s[:34]) # -> [48584371158645721]

ctb · 2016-11-14T19:34:22Z

looks like an off by one error to me (i.e. a bug). will investigate.

betatim · 2016-11-15T06:29:36Z

                                std::vector<BoundedCounterType> &counts) const
 {
-    KmerIterator kmers(s.c_str(), _ksize);
+    KmerHashIterator * kmers = new_kmer_iterator(s);


I would wrap it in std::unique_ptr< KmerHashIterator > kmers(new_kmer_iterator(s)) and then get rid of the delete.

You could probably also change new_kmer_iterator to return a unique_ptr directly.

betatim · 2016-11-15T12:30:07Z

+def test_counttable_no_unhash():
+    x = khmer.Counttable(4, 21, 3)
+
+    try:


->

with pytest.raises(ValueError): x.reverse_hash(1)

betatim · 2016-11-15T13:01:36Z

@@ -148,29 +148,9 @@ static bool convert_PyObject_to_HashIntoType(PyObject * value,
 {


the comment for this function needs updating (delete last two lines)

ctb · 2016-11-15T13:35:14Z

perfect, thanks!

ctb · 2016-11-15T14:15:50Z

done, much cleaner, thanks!

ctb · 2016-11-15T14:20:41Z

Question for all you foolish mortals: how should we handle testing of CPython class hierarchies?

Do we just test most of the methods on the base types and then test only the derived methods on derived types, and rely on code coverage to tell us when we're missing something?

Or do we massively duplicate tests?

Or what?

betatim · 2016-11-15T15:26:07Z

I'd probably go for option (a).

Another option I like is to devise a set of tests/checks for things that are graph-ish and apply them (via a for loop) to all things that want to be graph-ish (same for table-ish, X-ish). And then you add tests for each type that test what is special about that type.

betatim · 2016-11-15T17:06:17Z

I can't work out why travis is ignoring this. According to the logs it is ignoring this "as per configuration" which isn't super helpful for debugging. One thing that might wake travis up is if you change the target of this PR to master (which it should be anyway no?)

standage · 2016-11-15T17:19:56Z

@betatim Consider the following (from our .travis.yml file).

branches:
  only: 
    - master

ctb · 2016-11-15T17:43:31Z

yes! that is almost certainly the problem. willfix.

ctb · 2017-02-12T21:36:17Z

@codecov-io claims that coverage is unchanged: https://codecov.io/gh/dib-lab/khmer/pulls/1511

betatim · 2017-02-13T14:27:35Z

when I run the coverage tools locally coverage-gcovr.xml is empty. At some point we stopped generating gcda files when the tests run which is what gcovr needs (I think). Trying to find a "good" commit to git bisect.

standage

Looks good!

standage · 2017-02-13T22:55:31Z

+* get_raw_tables
+* do_subset_partition_with_abundance
+
+Nodegraph:


Not sure why this is under the "counting types" heading.

fixed 8f9e8b0

standage · 2017-02-13T22:56:32Z

+    } catch (khmer_exception &e) {
+        PyErr_SetString(PyExc_ValueError, e.what());
+        return NULL;
+    }


standage · 2017-02-13T23:04:01Z

@@ -0,0 +1,442 @@
+"""


Tests are all pretty simple and cover the basic features well.

luizirber · 2017-02-14T03:09:46Z

    return posns;
 }

+class MurmurKmerHashIterator : public KmerHashIterator


shouldn't this be in lib/kmer_hash.cc (or lib/kmer_hash.hh)?

standage · 2017-02-14T06:27:01Z

+    else:
+        raise Exception("unknown tabletype")
+
+    z = kh.get('ATGGC')


Oops, this should be loaded.get shouldn't it?

Fixed dc6b287 - thanks!

ctb · 2017-02-14T17:03:10Z

On Mon, Feb 13, 2017 at 03:04:36PM -0800, Daniel Standage wrote: +* get_raw_tables +* do_subset_partition_with_abundance + +Nodegraph: Not sure why this is under the "counting types" heading.

Fixed 8f9e8b0

ctb · 2017-02-14T17:03:55Z

On Mon, Feb 13, 2017 at 07:09:47PM -0800, Luiz Irber wrote: luizirber commented on this pull request. > @@ -509,6 +507,53 @@ const return posns; } +class MurmurKmerHashIterator : public KmerHashIterator shouldn't this be in `lib/kmer_hash.cc` (or `lib/kmer_hash.hh`)?

it should never be used publicly, and I think we should actually be hiding the TwoBit iterator. -0 on moving it for now. Presumably as part of Cythonization it will need to be exposed?

luizirber · 2017-02-14T17:19:59Z

On February 14, 2017 9:03:56 AM PST, "C. Titus Brown" ***@***.***> wrote: On Mon, Feb 13, 2017 at 07:09:47PM -0800, Luiz Irber wrote: > luizirber commented on this pull request. > > > > > @@ -509,6 +507,53 @@ const > return posns; > } > > +class MurmurKmerHashIterator : public KmerHashIterator > > shouldn't this be in `lib/kmer_hash.cc` (or `lib/kmer_hash.hh`)? it should never be used publicly, and I think we should actually be hiding the TwoBit iterator. -0 on moving it for now. Presumably as part of Cythonization it will need to be exposed? -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #1511 (comment)

Not about exposing it externally, I would use that for HLL

ctb · 2017-02-14T17:30:10Z

Not about exposing it externally, I would use that for HLL

oh! cool. I'd rather get this merged first tho :). is ok?

luizirber · 2017-02-14T17:43:27Z

On February 14, 2017 9:30:10 AM PST, "C. Titus Brown" ***@***.***> wrote: > Not about exposing it externally, I would use that for HLL oh! cool. I'd rather get this merged first tho :). is ok? -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #1511 (comment)

That's the idea =]

ctb · 2017-02-14T20:46:31Z

The Mac OS X build is holding this up. Can we merge, pretty please? :)

ctb added 3 commits November 12, 2016 06:26

remove apparently untested/unused hashset str->hash conversion code

652bdf3

add try/catch around reverse_hash

ec1e92a

added new_kmer_iterator method on Hashtable

b340a0a

ctb added 3 commits November 12, 2016 08:45

add test for no-reverse-hash on counttable

ab5d8f0

add new KmerHashIterator class

0385b13

initial proof of concept for k > 32 hashing using Counttable

7208d11

add a ksize check

43ddd23

betatim reviewed Nov 15, 2016

View reviewed changes

ctb added 2 commits November 15, 2016 06:03

Merge branch 'refactor/storage2' into refactor/hashing2

509e3df

switched to using unique_ptr in new_kmer_iterator

1eab6d2

switch to using pytest.raises

645fed8

Merge branch 'master' of github.com:dib-lab/khmer into refactor/hashing2

28cde19

ctb changed the base branch from refactor/storage2 to master November 15, 2016 17:49

ctb added 2 commits November 15, 2016 10:04

added test for get_kmer_hashes

3cc972a

pep8 fixes

2ec5bc4

updated ChangeLog

32fcf25

ctb mentioned this pull request Feb 12, 2017

Update and nicely format doc/dev/python-api.rst #1614

Open

betatim mentioned this pull request Feb 13, 2017

Fix generation of coverage for c++ code #1617

Merged

2 tasks done

standage approved these changes Feb 13, 2017

View reviewed changes

Merge branch 'master' of github.com:dib-lab/khmer into refactor/hashing2

c1af197

luizirber reviewed Feb 14, 2017

View reviewed changes

standage reviewed Feb 14, 2017

View reviewed changes

betatim mentioned this pull request Feb 14, 2017

HLL refactor: C++11 simplifications #1316

Merged

7 of 8 tasks

fix counting types in python-apis

8f9e8b0

ctb added 2 commits February 14, 2017 09:05

Merge branch 'master' of github.com:dib-lab/khmer into refactor/hashing2

87d17fb

fix test_tabletype::test_save_load

dc6b287

betatim mentioned this pull request Feb 14, 2017

Explore @lemire's rolling hash function as an alternative for Counttable #1616

Closed

Merge branch 'master' of github.com:dib-lab/khmer into refactor/hashing2

be56172

standage merged commit b5659c9 into master Feb 14, 2017

standage deleted the refactor/hashing2 branch February 14, 2017 20:56

ctb mentioned this pull request Feb 14, 2017

Split CPython and begin the Cython revolution #1595

Merged

8 tasks done

camillescott mentioned this pull request Feb 14, 2017

[MRG] remove redundant ACGT-checking code. #1590

Merged

8 tasks done

This was referenced Feb 16, 2017

[WIP] Add a -H/--hash-function option. #1623

Open

Support for k>32 #1426

Closed

Allow k > 32 in khmer #60

Closed

Plan for hash function refactoring #1441

Closed

Storing reversible hashes for k>32 #1442

Closed

		@@ -148,29 +148,9 @@ static bool convert_PyObject_to_HashIntoType(PyObject * value,
		{

Conversation

ctb commented Nov 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Nov 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ctb commented Nov 12, 2016

Uh oh!

ctb commented Nov 12, 2016

Uh oh!

ctb commented Nov 12, 2016

Uh oh!

betatim commented Nov 14, 2016

Uh oh!

ctb commented Nov 14, 2016 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

betatim Nov 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctb commented Nov 15, 2016 via email

Uh oh!

ctb commented Nov 15, 2016 via email

Uh oh!

ctb commented Nov 15, 2016

Uh oh!

betatim commented Nov 15, 2016

Uh oh!

betatim commented Nov 15, 2016

Uh oh!

standage commented Nov 15, 2016

Uh oh!

ctb commented Nov 15, 2016 via email

Uh oh!

ctb commented Feb 12, 2017

Uh oh!

betatim commented Feb 13, 2017

Uh oh!

standage left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctb commented Feb 14, 2017 via email

Uh oh!

ctb commented Feb 14, 2017 via email

Uh oh!

luizirber commented Feb 14, 2017 via email

Uh oh!

ctb commented Feb 14, 2017 via email

Uh oh!

luizirber commented Feb 14, 2017 via email

Uh oh!

ctb commented Feb 14, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

ctb commented Nov 12, 2016 •

edited

Loading

codecov-io commented Nov 12, 2016 •

edited

Loading

betatim Nov 15, 2016 •

edited

Loading