search

thegenemyers / DALIGNER

Find all significant local alignments between reads
Other
139 stars 61 forks source link

Fix apparent MT bug. #30

Closed pb-cdunn closed 8 years ago

pb-cdunn commented 8 years ago

Herr Myers,

This little bug-fix works well for us with PacBio.

Not really a multi-threading bug, it's just a simple buffer over-run causing one thread to write into the next. The root cause was a logic error in dividing hits into threads when nhits < NTHREADS.

The assertion is not needed anymore but would catch the bug.

Fixes PacificBiosciences/DALIGNER#9

~Christopher Dunn (We met at Genome in a Bottle/SMRT Informatics Dev. Conference in Gaithersburg.)

thegenemyers commented 8 years ago

Herr Dunn,

Thanks for the bug fix, I incorporated into my version along with a

couple of other fixes and just commited it to my repository at Github.

 I noticed that you are pretty far behind the current version, which

uses space a bit more efficiently and most importantly does not produce "contained" overlaps (i.e. two overlaps between the same pair of reads that could be merged into one, or in some cases, where one completely contains the other). Probably worth uploading and merging with your release.

  Daligner needs enough memory to build an index of each block and

with typical block size (200-400Mbp) these require 2-4GB each. A guess as to the memory problem would be that memory is so small that daligner cannot operate. It is supposed to warn you and exit, but I noticed that the check for that wasn't quite correct, effectively allowing a negative number for the memory needed for the merge list, and since this is a uint64, basically one big hairy number. Please let me know if I fixed the problem with the version I just uploaded. If not, the problem should be somewhere between line 1942 and 2001, where MEM_LIMIT is the number bytes the program thinks it is to use.

 Best regards,
       Gene

On 11/19/15, 6:23 PM, Christopher Dunn wrote:

Herr Myers,

This little bug-fix works well for us with PacBio.

Not really a multi-threading bug, it's just a simple buffer over-run causing one thread to write into the next. The root cause was a logic error in dividing hits into threads when |nhits < NTHREADS|.

The assertion is not needed anymore but would catch the bug.

Fixes PacificBiosciences/DALIGNER#9 https://github.com/PacificBiosciences/DALIGNER/issues/9

~Christopher Dunn (We met at /Genome in a Bottle/SMRT Informatics Dev. Conference/ in Gaithersburg.)

    You can view, comment on, or merge this pull request online at:

https://github.com/thegenemyers/DALIGNER/pull/30

    Commit Summary

— Reply to this email directly or view it on GitHub https://github.com/thegenemyers/DALIGNER/pull/30.

pb-cdunn commented 8 years ago

Danke schön!

We will rebase to your latest code by next week. I know of one other crash which might be difficult to solve -- since we do not have a small test-case to reproduce it -- but maybe your memory fix will solve that too.

I'll close this PullRequest.

thegenemyers commented 8 years ago

Chris,

Wow even got the umlaut on shoen -- Sprichst du ein bischen Deutsch?

Albeit a bit slow and inattentive to the Github site at times due to

my many responsibilities, this is my "pet" project and I am committed and would very much like the modules to be as free of bugs as possible. So if the error persists perhaps we can exchange larger data sets with say Drop Box ?

Cheers,
    Gene

P.S. Another thing we are working on is making daligner an efficient read mapper. Currently daligner can map, but for example when we map 25,000 reads to hg39, we get 7million hits, i.e. all the repeat-induced hits are there. The current daligner is a highly sensitive local alignment finder which is not quite the correct requirement for mapping where you want to see say the "best" 10 hits. For example, BLASR reports only 30,000 hits on the same data set. Nevertheless we are still 3X faster even though we report a "sea" of hits. So I figure if we get daligner to only look at say the top 10 hits it will really be fast.

On 12/2/15, 11:59 PM, Christopher Dunn wrote:

Danke schön!

We will rebase to your latest code by next week. I know of one other crash which might be difficult to solve -- since we do not have a small test-case to reproduce it -- but maybe your memory fix will solve that too.

I'll close this PullRequest.

— Reply to this email directly or view it on GitHub https://github.com/thegenemyers/DALIGNER/pull/30#issuecomment-161460877.

pb-cdunn commented 8 years ago

Wow even got the umlaut on shoen -- Sprichst du ein bischen Deutsch?

Ya, aber nur ein bißchen.

I'll try find that old test-case...

pb-cdunn commented 8 years ago

Wow even got the umlaut on shoen -- Sprichst du ein bischen Deutsch?

Ya, aber nur ein bißchen.

I'll try find that old test-case...