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

Fix an edge case bug in PalindromeArtifactReadTransformer #5038

Merged
merged 2 commits into from
Jul 22, 2018

Conversation

davidbenjamin
Copy link
Contributor

Closes #5036.

@meganshand This fixes the bug you found.

@codecov-io
Copy link

codecov-io commented Jul 19, 2018

Codecov Report

Merging #5038 into master will increase coverage by 0.088%.
The diff coverage is 66.667%.

@@              Coverage Diff               @@
##             master     #5038       +/-   ##
==============================================
+ Coverage     86.38%   86.469%   +0.088%     
- Complexity    28640     29108      +468     
==============================================
  Files          1782      1782               
  Lines        132603    134161     +1558     
  Branches      14761     15067      +306     
==============================================
+ Hits         114543    116007     +1464     
- Misses        12740     12805       +65     
- Partials       5320      5349       +29
Impacted Files Coverage Δ Complexity Δ
...itute/hellbender/tools/walkers/mutect/Mutect2.java 89.286% <0%> (-3.571%) 18 <1> (ø)
...formers/PalindromeArtifactClipReadTransformer.java 97.143% <80%> (-2.857%) 21 <0> (+4)
...iscovery/TestUtilsForAssemblyBasedSVDiscovery.java 88.889% <0%> (-6.633%) 12% <0%> (-1%)
...dataSources/vcf/VcfFuncotationFactoryUnitTest.java 97.656% <0%> (-0.43%) 40% <0%> (+11%)
.../AssemblyContigAlignmentsConfigPickerUnitTest.java 99.415% <0%> (-0.19%) 34% <0%> (+14%)
...utils/variant/GATKVariantContextUtilsUnitTest.java 87.194% <0%> (-0.088%) 316% <0%> (+156%)
...ct/CreateSomaticPanelOfNormalsIntegrationTest.java 100% <0%> (ø) 6% <0%> (+3%) ⬆️
.../validation/RemoveNearbyIndelsIntegrationTest.java 100% <0%> (ø) 5% <0%> (+2%) ⬆️
...walkers/validation/ConcordanceIntegrationTest.java 100% <0%> (ø) 10% <0%> (+4%) ⬆️
...dation/AnnotateVcfWithBamDepthIntegrationTest.java 100% <0%> (ø) 12% <0%> (+5%) ⬆️
... and 10 more

Copy link
Contributor

@meganshand meganshand left a comment

Choose a reason for hiding this comment

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

Thank you! In a perfect world it would be great to have a new test, but I did run this branch on my data and it works. 👍

@davidbenjamin
Copy link
Contributor Author

@meganshand Do you have a public mitochondrial bam that I could use for a test? If not, what if I took just the reads at the beginning of the contig, excluding any variants, and anonymized the header? That would be enough.

Actually, if you do have an entire mitochondrial bam that's public, I wouldn't mind adding a mitochondrial integration test to the repo. . .

@meganshand
Copy link
Contributor

@davidbenjamin I do have a public bam! This is just chrM for NA12878: /humgen/gsa-hpprojects/dev/mshand/SpecOps/Mitochondria/NA12878/NA12878_chrMOnly.bam

The coverage should be quite high, so if it's too big I can downsample it if you want.

And my mitochondria only reference would probably also be useful: /humgen/gsa-hpprojects/dev/mshand/SpecOps/Mitochondria/MitochondriaOnlyFastas/Homo_sapiens_assembly38.mt_only.fasta (the fai and dict are there too).

@davidbenjamin
Copy link
Contributor Author

@meganshand I will include testing for this bug as part of an overall mitochondria integration test using your NA12878 bam in a separate PR.

@davidbenjamin davidbenjamin merged commit 2a0bccc into master Jul 22, 2018
@davidbenjamin davidbenjamin deleted the db_m2_mito_bug branch July 22, 2018 03:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in PalindromeArtifactClipReadTransformer
3 participants