give BWA-spark better default read filters #4286
Conversation
…e default set of read filters.
There was a problem hiding this comment.
Review complete, back to @lbergelson for changes. Merge after addressing comments.
| @@ -45,6 +48,12 @@ public boolean requiresReads() { | |||
| return true; | |||
| } | |||
|
|
|||
| @Override | |||
| public List<ReadFilter> getDefaultReadFilters() { | |||
| // 1) unmapped or neither secondary nor supplementary and 2) has some sequence | |||
There was a problem hiding this comment.
Comment seems a little off here -- shouldn't this say "primary" rather than "unmapped"?
| @@ -121,6 +122,9 @@ public void close() { | |||
| this.bwaMemIndex = BwaMemIndexCache.getInstance(indexFileName); | |||
| this.readsHeader = readsHeader; | |||
| this.alignsPairs = alignsPairs; | |||
| if ( alignsPairs && readsHeader.getSortOrder() != SAMFileHeader.SortOrder.queryname ) { | |||
| throw new UserException("Input must be queryname sorted unless you use single-ended alignment mode."); | |||
There was a problem hiding this comment.
Can you add a test that exercises this case?
There was a problem hiding this comment.
What if it is "unsorted" but with group order "queryname"?
There was a problem hiding this comment.
@magicDGS That's a very good point. That should allowed as well. I'll update it.
| if (alignsPairs && (nReads & 1) != 0 ) { | ||
| throw new GATKException("We're supposed to be aligning paired reads, but there are an odd number of them."); | ||
| if ( alignsPairs ) { | ||
| if ( (nReads & 1) != 0 ) { |
There was a problem hiding this comment.
Why are you using bitwise-AND instead of the mod operator to check whether the number is odd? Mod is more straightforward I think.
There was a problem hiding this comment.
Because Ted Sharpe. Changed.
| final String readName1 = inputReads.get(idx).getName(); | ||
| final String readName2 = inputReads.get(idx+1).getName(); | ||
| if ( !Objects.equals(readName1,readName2) ) { | ||
| throw new GATKException("Read pair has varying template name: "+readName1+" .vs "+readName2); |
There was a problem hiding this comment.
Should this one be a UserException, since it's a problem with the input?
| throw new GATKException("We're supposed to be aligning paired reads, but there are an odd number of them."); | ||
| if ( alignsPairs ) { | ||
| if ( (nReads & 1) != 0 ) { | ||
| throw new GATKException("We're supposed to be aligning paired reads, but there are an odd number of them."); |
There was a problem hiding this comment.
Should this be a UserException?
| @@ -88,6 +88,7 @@ else if ( alwaysGenerateASandXS ) { | |||
| samRecord.setReadBases(SAMRecord.NULL_SEQUENCE); | |||
| samRecord.setBaseQualities(SAMRecord.NULL_QUALS); | |||
| } | |||
| //TODO: there ought to be a way to indicate a set of tag names that ought to be copied -- we're just doing RG | |||
| if ( readGroup != null ) samRecord.setAttribute(SAMTag.RG.name(), readGroup); | |||
There was a problem hiding this comment.
Add curly braces around if statement body.
There was a problem hiding this comment.
done, although I didn't hit the rest of the file
| @@ -60,5 +61,4 @@ public void testSingleEnd() throws Exception { | |||
|
|
|||
| SamAssertionUtils.assertSamsEqual(new File(output, "part-r-00000.bam"), expectedSam); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Aren't there any additional tests for BwaSpark that pass with this fix and failed without it? If not, can you add one?
|
@lbergelson Please address the code review comments in a separate PR, since they mainly have to do with comments and testing -- I'm merging this as-is for now. |
…e default set of read filters. (#4286)
Wellformed is too strict for BWA since it asserts things about the alignment. It also doesn't correctly remove secondary and supplementary alignments like we should we performing a new alignment.