Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed exception in SplitNCigarReads for non-reference consuming reads #5298

Merged
merged 1 commit into from
Oct 11, 2018

Conversation

jamesemery
Copy link
Collaborator

Fixes #5293

@jamesemery jamesemery force-pushed the je_splitNCigarReadsAllInsertion branch from 382c2d0 to 44b29bf Compare October 10, 2018 18:32
Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. My only thought is that it might make more sense to handle these with an upfront filter because they don't really make any sense.

@codecov-io
Copy link

Codecov Report

Merging #5298 into master will increase coverage by <.001%.
The diff coverage is 90.909%.

@@               Coverage Diff               @@
##              master     #5298       +/-   ##
===============================================
+ Coverage     86.777%   86.777%   +<.001%     
- Complexity     29910     29912        +2     
===============================================
  Files           1835      1835               
  Lines         138574    138583        +9     
  Branches       15255     15256        +1     
===============================================
+ Hits          120250    120258        +8     
- Misses         12772     12774        +2     
+ Partials        5552      5551        -1
Impacted Files Coverage Δ Complexity Δ
...er/tools/walkers/rnaseq/OverhangFixingManager.java 94.304% <100%> (+0.073%) 70 <0> (ø) ⬇️
.../walkers/rnaseq/OverhangFixingManagerUnitTest.java 97.838% <85.714%> (-0.477%) 33 <1> (+1)
...utils/smithwaterman/SmithWatermanIntelAligner.java 50% <0%> (-30%) 1% <0%> (-2%)
...ithwaterman/SmithWatermanIntelAlignerUnitTest.java 60% <0%> (ø) 2% <0%> (ø) ⬇️
...roadinstitute/hellbender/utils/read/ReadUtils.java 79.812% <0%> (+0.704%) 201% <0%> (+2%) ⬆️

@jamesemery
Copy link
Collaborator Author

@lbergelson It may, but I want them to get passed through the splitter and the filter and be written on the other side, which requires they get emitted into the splitter. I would rather the tool no remove reads from someones bam as maybe there is some reason their aligner emitted these reads, hence we emit them and add a check that prevents the bogus genomeLocs from being created for them (so they don't get processed by the manager).

@jamesemery jamesemery merged commit 73a1b67 into master Oct 11, 2018
@jamesemery jamesemery deleted the je_splitNCigarReadsAllInsertion branch October 11, 2018 14:59
EdwardDixon pushed a commit to EdwardDixon/gatk that referenced this pull request Nov 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants