Extract storage into separate class; split Hashtable into Hashtable and Hashgraph.

[ctb]

This PR separates the storage details of hashes from the actual hash function. It should support both more types of storage (4-bit and two byte CountMin sketches? maybe even exact storage?) and multiple hash functions.

It also separates out table and graph operations, making it easier to split class defn's between functionality that needs reversible hashing (graph operations) and functions that don't (table functions). Ultimately this can lead to new CPython objects (maybe Nodetable and Counttable?) that support k > 32 while the Nodegraph and Countgraph CPython objects would remain limited to k <= 32.

I tried to use templates instead of composition but couldn't get the various friend & inheritance declarations to work. Suggestions welcome.

No discernable performance impact was observed in some basic benchmarking.

Built on #1494. Might be a way to do #1490 (irreversible hashing for k > 32).

• 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?

[ctb]

@betatim @luizirber I am having linker symbol resolution issues. Any suggestions?

[ctb]

Got it to compile!

Comments would be appreciated. Is it worth following through on this and fixing up the load and save code?

[ctb]

All the tests pass!

@betatim @luizirber @standage ready for an initial review!

[codecov-io]

Current coverage is 95.80% (diff: 100%)

No coverage report found for master at 4e5a234.

Powered by Codecov. Last update 4e5a234...25179ed

[ctb]

Ready for review! @betatim @standage

[betatim]

Mostly good 😄

One pedantic comment: all the variables that used to be of type Hashtable but now are Hashgraphs are still called hashtable :-/ It'll start getting confusing when Hashtables are used more why the variable called hashtable actually contains a graph...

I only spotted one used of Hashtable directly, everyone else goes through Hashgraph. Makes me wonder if the other stop_bf related things should continue to be Hashtables as well. This is where I'll resume the reviewing.

[standage]

I made a few minor (also pedantic) comments about the class design. But I must say that being able to make changes of this scale confidently is a HUGE testament to the robustness of the khmer test suite!

[standage]

Why duplicate these class members? Isn't the idea of inheritance that the base class stores the common members and functions?

[standage]

Isn't this redundant with _tablesizes.size()? Not a big deal, but why risk the two variables getting out of sync?

[ctb]

@standage, I'll fix your points on the next PR, which is forthcoming.

@betatim, the inheritance hierarchy will expand and I will do a more thorough review then.

Thanks all :).

[ctb]

On Thu, Nov 03, 2016 at 11:22:01AM -0700, Tim Head wrote:

One pedantic comment: all the variables that used to be of type Hashtable but now are Hashgraphs are still called hashtable :-/ It'll start getting confusing when Hashtables are used more why the variable called hashtable actually contains a graph...

this should all have been fixed in #1504.