Replace TwoPassReadWalker with more general MultiplePassReadWalker. #6154
Conversation
cmnbroad
force-pushed
the
cn_multipass_readwalker
branch
from
70f1839 to
633d1aa
Compare
| void consume( GATKRead read, ReferenceContext reference, FeatureContext features ); | ||
| } | ||
|
|
||
| public abstract void traverseReads(); |
There was a problem hiding this comment.
Add a more descriptive comment to this method explaining how to implement this method here.
There was a problem hiding this comment.
Also perhaps add a comment mentioning what we discussed about resetting the random generator if necessary.
| public final void traverse() { | ||
| countedFilter = makeReadFilter(); | ||
| traverseReads(); | ||
| logger.info(countedFilter.getSummaryLine()); |
There was a problem hiding this comment.
Upon thinking about this, I actually believe it will be considerably less confusing to the user to print the countedFilter summary line for only one pass of the tool. (imagine gatk telling my bam has 2x the number of reads I know it to have? That could easily cause enough confusion to be be asked by a forum user some day) I would advocate reconstructing the filter every pass and just returning the most current one (which should be equivalent)
| private boolean firstPass = true; | ||
|
|
||
| @FunctionalInterface | ||
| public interface GATKReadConsumer { |
There was a problem hiding this comment.
This should have similar documentation to the apply() method of the normal read walker (perhaps just link to that methods documentation).
| read, | ||
| new ReferenceContext(reference, readInterval), // will be empty if reference or readInterval is null | ||
| new FeatureContext(features, readInterval)); // will be empty if features or readInterval is null | ||
| progressMeter.update(readInterval); |
There was a problem hiding this comment.
Considering that the progress meter is going to print the position with the traversal perhaps it would be bettter to print a log statement each time just stating, "re-traversing intervals for traversal n" or some such.
There was a problem hiding this comment.
These changes look good and have addressed my comments @cmnbroad. Feel free to merge.
Replace TwoPassReadWalker with the more general MultiplePassReadWalker to support SV team's request, and also addresses an issue found by @tedsharpe where TwoPassReadWalker never closed the ReadsDataSource from the previous iteration.