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

kebab case some arguments in LocusWalker and Spark walkers #5770

Merged
merged 2 commits into from
Mar 7, 2019

Conversation

davidbenjamin
Copy link
Contributor

Closes #5479. Closes #5478.

@lbergelson here you go.

@samuelklee
Copy link
Contributor

Thanks @davidbenjamin! Couple more in AssemblyRegionReadShardArgumentCollection?

@codecov-io
Copy link

codecov-io commented Mar 6, 2019

Codecov Report

Merging #5770 into master will increase coverage by 0.003%.
The diff coverage is 71.429%.

@@               Coverage Diff               @@
##              master     #5770       +/-   ##
===============================================
+ Coverage     86.982%   86.984%   +0.003%     
- Complexity     31861     31863        +2     
===============================================
  Files           1943      1943               
  Lines         146770    146768        -2     
  Branches       16223     16223               
===============================================
+ Hits          127663    127665        +2     
+ Misses         13194     13189        -5     
- Partials        5913      5914        +1
Impacted Files Coverage Δ Complexity Δ
...ark/AssemblyRegionReadShardArgumentCollection.java 100% <100%> (ø) 1 <0> (ø) ⬇️
.../broadinstitute/hellbender/engine/LocusWalker.java 79.245% <50%> (+1.467%) 15 <0> (ø) ⬇️
...tute/hellbender/engine/spark/LocusWalkerSpark.java 87.179% <66.667%> (+2.179%) 13 <0> (ø) ⬇️
...nder/utils/runtime/StreamingProcessController.java 67.299% <0%> (-0.474%) 33% <0%> (ø)
...itute/hellbender/tools/walkers/mutect/Mutect2.java 84.091% <0%> (ø) 22% <0%> (ø) ⬇️
...ithwaterman/SmithWatermanIntelAlignerUnitTest.java 60% <0%> (ø) 2% <0%> (ø) ⬇️
...utils/smithwaterman/SmithWatermanIntelAligner.java 80% <0%> (+30%) 3% <0%> (+2%) ⬆️

@davidbenjamin
Copy link
Contributor Author

@samuelklee They're in this PR (that's the #5478 part).

@samuelklee
Copy link
Contributor

Oops, duh, so they are---didn't realize it from the title alone and misread the diff between this and another one of my branches!

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.

@davidbenjamin One comment. Good to merge when that is addressed! Thanks for doing this!

@@ -39,7 +39,7 @@
*/
public abstract class LocusWalker extends GATKTool {

@Argument(fullName = "maxDepthPerSample", shortName = "maxDepthPerSample", doc = "Maximum number of reads to retain per sample per locus. Reads above this threshold will be downsampled. Set to 0 to disable.", optional = true)
@Argument(fullName = "max-depth-per-sample", shortName = "max-depth-per-sample", doc = "Maximum number of reads to retain per sample per locus. Reads above this threshold will be downsampled. Set to 0 to disable.", optional = true)
Copy link
Member

Choose a reason for hiding this comment

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

There are two error messages that refer to the old maxDepthPerSample one in LocusWalker.getDownsamplingInfo() and the other in LocusWalkerSpark. Could you extract a constant from this and have all three locations use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@samuelklee
Copy link
Contributor

Also @lbergelson please disregard that second review request! Man am I looking foolish on this branch.

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, thank you!

@davidbenjamin davidbenjamin merged commit 4f464aa into master Mar 7, 2019
@davidbenjamin davidbenjamin deleted the db_5479 branch March 7, 2019 01:04
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.

4 participants