Introduce consume_seqfile_banding method by standage · Pull Request #1571 · dib-lab/khmer

standage · 2017-01-03T21:48:59Z

WIP that, when finished, closes #1570. Implements a variant of consume_fasta (and all of the functions it calls) that consumes a subset of the observed k-mers. Given num_batches = 2^n and batch \in range(num_batches), the function only stores k-mers whose n least significant bits encode batch.

Remaining tasks:

add checks to verify num_bins is a power of 2 and tag < num_bins
Implement CPython glue code
Add tests
Integrate with Add hash functions supporting k > 32 to Counttable #1511 and/or related issues; this new functionality requires a proper hash function, not just 2-bit encoding

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 backwards-compatible
additions are allowed without a major version increment. Changing file
formats also requires a major version number increment.
For substantial changes or changes to the command-line interface, is it
documented in CHANGELOG.md? See keepachangelog
for more details.
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?)

standage · 2017-01-03T21:56:40Z

This PR more or less duplicates the contents of several functions. If it turns out to be generally useful and something we want to keep long term, we'll need to consider how to refactor the code to minimize duplication and maintenance burden. Note, we've already acknowledged the need for some cleanup in #1378.

codecov-io · 2017-01-03T23:13:49Z

Codecov Report

Merging #1571 into master will decrease coverage by 0.02%.
The diff coverage is 57.4%.

@@            Coverage Diff             @@
##           master    #1571      +/-   ##
==========================================
- Coverage   64.16%   64.13%   -0.03%     
==========================================
  Files          79       79              
  Lines       12189    12244      +55     
  Branches     3072     3096      +24     
==========================================
+ Hits         7821     7853      +32     
- Misses       2670     2674       +4     
- Partials     1698     1717      +19

Impacted Files	Coverage Δ
lib/hashtable.hh	`68.42% <ø> (ø)`	⬆️
lib/hashtable.cc	`57.07% <56.75%> (-0.07%)`	⬇️
khmer/_khmer.cc	`57.49% <58.82%> (+0.01%)`	⬆️
lib/hashgraph.cc	`47.56% <0%> (+0.12%)`	⬆️

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 d40a81a...0dbe256. Read the comment docs.

standage · 2017-01-03T23:23:41Z

Ready for initial review @ctb @luizirber @camillescott @betatim. This may be easier to merge before #1511. If so I'll clean it up for final review.

betatim · 2017-01-04T07:48:53Z

    delete parser;
 }

+void Hashtable::consume_fasta_bitsplit(


bitsplit doesn't mean anything to me :-/ Coudl we call it consume_fasta_sampling or some such?

I agree the name is wonky. consume_fasta_sampling is better but honestly I've thought of half a dozen names and I'm not in love with any of them.

my suggestion for the name is "banding".

Yeah, I quite like banding. And I think num_batches and batch are good replacements for num_bins and tag.

betatim · 2017-01-04T07:49:22Z

+{
+    bool powerof2 = !(num_bins == 0) && !(num_bins & (num_bins - 1));
+    if (!powerof2 || tag >= num_bins) {
+        return;


why not raise a ValueError?

betatim · 2017-01-04T07:56:25Z

~~As mentioned here #1570 (comment) this needs a good hashfunction. Should make it either impossible to call with 2bit encoding or check at run time that the user knows what they are doing.~~

tables use murmurhash

standage · 2017-01-04T19:23:11Z

tables use murmurhash

And to be clear, the concern with the hash function isn't correctness per se, but getting a roughly even split. Unless the sequence is truly random (simulated and biologically uninteresting), some k-mer suffixes are going to be overrepresented and skew the split. At least for my purposes, this wouldn't be a problem for correctness as much as for performance. I want to be able to split the work into evenly sized bins.

standage · 2017-01-04T19:47:02Z

I did some basic profiling. For very small memory sizes, there is little difference in runtime between consume_fasta and consume_fasta_bitsplit on ≈1/8 of the k-mers. But as memory increases, the runtime of consume_fasta grows linearly-ish with memory while the runtime of consume_fasta_bitsplit increases only marginally. What's great is that consume_fasta_bitsplit can operate at a reasonable FPR using less memory, presumably taking less of a performance hit from NUMA.

standage · 2017-01-04T19:59:41Z

But regardless, increasing memory only has a marginal effect on runtime. Perhaps it becomes more I/O bound?

betatim · 2017-01-05T13:05:34Z

When you say increasing memory you mean making the hashtable bigger?

So for small hashtable sizes we spend most of our time doing I/O, hence throwing away kmers after reading them in doesn't speed things up. For large hashtables we spend most of our effort on entering stuff into the hashtable, so reading in kmers and then chucking them makes things faster?! Interesting.

ctb · 2017-01-05T13:55:08Z

excellent. @betatim I've lost track (vacation brain) of your results on cache misses; this is probably not due to cache missing, right?

ctb · 2017-01-05T15:10:16Z

@standage, you should be able to separate out the IO effects from the memory size effects by an affine fit, yes?

ctb · 2017-01-05T20:56:34Z

What's old is new: #70

ctb · 2017-01-05T20:57:06Z

Commit 786a3be

standage · 2017-02-10T00:02:49Z

+    (21, 5e7, 2, 16),
+])
+def test_banding_in_memory(ksize, memory, epsilon, numbands):
+    infile = utils.get_test_data('test-reads.fa')


I'm not sure this function meets @ctb's simplicity criteria for automated tests. I'm not sure there's a simpler, clearer way to iterate over the data.

standage · 2017-02-10T00:03:17Z

+def test_banding_to_disk(ksize, memory, numbands):
+    infile = utils.get_test_data('test-reads.fa')
+    path1 = utils.get_temp_filename('normal.ct')
+    path2 = utils.get_temp_filename('banding.ct')


This function is simple enough, and it provides a stronger guarantee. Perhaps it's all we need?

ctb · 2017-02-10T00:58:02Z

*shrug* I don't like procedural complexity. At least this is declarative :)

standage · 2017-02-10T04:21:09Z

Procedural complexity == branching?

standage · 2017-02-14T23:06:12Z

My recent tests have put my concerns regarding the hash functions and banding to rest. Now that #1511 is merged, this is now ready for review and merge. @ctb @betatim @camillescott @luizirber

betatim · 2017-02-16T14:06:05Z

+            continue;
+        }
+
+        bool is_valid = true;


Can I persuade you to use read.set_clean_seq(); plus read.cleaned_seq != read.sequence instead of doing your own processing/normalising?

Maybe what we need is a method that does the toupper() for us to avoid having "loop-over-seq-and-uppercase-then-check" here.

Two concerns / observations.

Comparing cleaned seq vs original seq will disqualify reads with lowercase ACGT, which I think is a lousy default with unexpected consequences.

The current consume_fasta function in Hashtable doesn't use this utility yet either.

As @ctb mentioned, there's some work to be done with removing redundant read cleaning code (see #1590), and that's not all going to be resolved in this PR. Ultimately this will be handled by the parser instead of the consume function, right?

Ah yes, humm, so we definitely need a method that can do the comparison that is centralised. And with cython'isation things will move again. Punt?

betatim · 2017-02-16T14:12:43Z

 code...
 """

+from __future__ import absolute_import


Was this missing before? Puzzled how travis didn't find it.

Yeah, weird.

Now we need it to make the tests pass or do they work without as well?

betatim · 2017-02-16T14:16:31Z

Do you want to add one more test that computes the hash of two kmers, one that should be added and one that shouldn't, and explicitly checks that they ended up in the CT? A bit like https://github.com/dib-lab/khmer/pull/1571/files#diff-7352ff13b9e5cdbeacb24c9e7bcb49b1R1136 except here I am guessing that this is what you wanted because I don't know the hash values off the top of my head.

ctb · 2017-02-16T16:59:22Z

see also #1590

standage · 2017-02-16T18:13:59Z

@betatim Yeah, that's exactly what I was going for with that test. Are you saying to test the hash value instead of the k-mer itself? Please clarify.

betatim · 2017-02-17T05:55:15Z

I was going for a test like this:

ct = Counttable(...)
h = ct.hash(some_kmer)
assert h % 8 == 3
ct.consume_seqfile_banding(file_that_contains_some_kmer, 8, 3)
assert ct.get(some_kmer) == 1

we could even add the assert for the hash value to the current test. The motivation is to leave an explicit clue as to why you expect a certain kmer to be present and which ones not.

betatim

LGTM.

ctb · 2017-02-25T15:19:06Z

On Thu, Feb 09, 2017 at 08:21:10PM -0800, Daniel Standage wrote: Procedural complexity == branching?

yes, specifically 'if' statements suck. if we have to debug our test code itself, it adds complexity.

standage · 2017-06-02T21:17:30Z

Closing in favor of #1720.

Introduce consume_fasta_bitsplit method

bd966f9

standage added 2 commits January 3, 2017 14:03

Some sanity checks for parameter values

fadda3a

Implement CPython interface, add tests

9ba9613

standage mentioned this pull request Jan 4, 2017

Nibble storage class #1551

Merged

12 of 13 tasks

betatim reviewed Jan 4, 2017

View reviewed changes

Raise ValueError instead of returning (0,0)

c877bae

standage added 11 commits January 5, 2017 13:10

Merge branch 'master' into research/banding_and_k32+

7b48b3f

Merge branch 'refactor/hashing2' into research/banding_and_k32+

2a1497c

bitsplit --> banding

fdfda14

Overload consume_fasta_banding

3602cfb

Merge branch 'master' into feature/consume_bitsplit

9436c3e

Merge branch 'feature/consume_bitsplit' into research/banding_and_k32+

82ec89a

Fix kmer iterator

e8a49dd

Merge branch 'refactor/hashing2' into research/banding_and_k32+

b26a766

Fix class definition that somehow got duplicated

92f93af

Merge branch 'refactor/hashing2' into research/banding_and_k32+

5298619

Merge branch 'refactor/hashing2' into research/banding_and_k32+

0b4cb9d

ctb mentioned this pull request Feb 9, 2017

Test banding code with some large-ish data #1610

Open

Added tests for banding accuracy. Closes #1610.

95f0d4d

standage commented Feb 10, 2017

View reviewed changes

Function docstrings accidentally omitted

b1087f2

standage added 3 commits February 14, 2017 13:54

Merge branch 'master' into feature/consume_bitsplit

18cd0c8

Merge branch 'master' into research/banding_and_k32+

5993e7d

Merge branch 'feature/consume_bitsplit' into research/banding_and_k32+

73a8c0f

Change log

3b32019

betatim reviewed Feb 16, 2017

View reviewed changes

Merge branch "master" and resolve conflicts

1915917

standage changed the title ~~Introduce consume_fasta_banding method~~ Introduce consume_seqfile_banding method Feb 16, 2017

betatim approved these changes Feb 17, 2017

View reviewed changes

standage and others added 4 commits May 4, 2017 12:35

Merge branch "master"

dd0d6dd

Accommodate new error message

ff5dd4b

Merge branch 'master' into feature/consume_bitsplit

aa37d25

Merge branch 'master' into feature/consume_bitsplit

0dbe256

standage mentioned this pull request Jun 2, 2017

[MRG] Introduce consume_seqfile_banding method, part duex #1720

Merged

8 tasks done

standage closed this Jun 2, 2017

standage deleted the feature/consume_bitsplit branch June 3, 2017 04:05

Conversation

standage commented Jan 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

standage commented Jan 3, 2017

Uh oh!

codecov-io commented Jan 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

standage commented Jan 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

betatim commented Jan 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

standage commented Jan 4, 2017

Uh oh!

standage commented Jan 4, 2017

Uh oh!

standage commented Jan 4, 2017

Uh oh!

betatim commented Jan 5, 2017

Uh oh!

ctb commented Jan 5, 2017

Uh oh!

ctb commented Jan 5, 2017

Uh oh!

ctb commented Jan 5, 2017

Uh oh!

ctb commented Jan 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctb commented Feb 10, 2017 via email

Uh oh!

standage commented Feb 10, 2017

Uh oh!

standage commented Feb 14, 2017

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!

betatim commented Feb 16, 2017

Uh oh!

ctb commented Feb 16, 2017 via email

Uh oh!

standage commented Feb 16, 2017

Uh oh!

betatim commented Feb 17, 2017

Uh oh!

betatim left a comment

Choose a reason for hiding this comment

Uh oh!

ctb commented Feb 25, 2017 via email

standage commented Jan 3, 2017 •

edited

Loading

codecov-io commented Jan 3, 2017 •

edited

Loading

standage commented Jan 3, 2017 •

edited

Loading

betatim commented Jan 4, 2017 •

edited

Loading