Added an argument mode manager and a demonstration of how it might be used in HaplotypeCaller --dragen-mode #7745
Conversation
…d demonstrated on the HaplotypeCaller dragen mode
| * | ||
| * The method does not overwrite arguments already set on the command line (as indicated by the parser) | ||
| * Additionally, the method emits a warning message indicating which argument were set by the mode and | ||
| * to which values. |
There was a problem hiding this comment.
Add a comment documenting when in the arg parsing process this method needs to be invoked (eg., after args have already been parsed in customCommandLineValidation() )
src/main/java/org/broadinstitute/hellbender/cmdline/ModeArgumentUtils.java
Outdated
Show resolved
Hide resolved
| * @param modeName - the name of the mode being set. This is used in textual message, such as the warning | ||
| * issued to notify the user of the changed argument | ||
| */ | ||
| public static void setArgValues(final CommandLineParser parser, final String[] argValues, final String modeName) { |
There was a problem hiding this comment.
How do we avoid setting these arg values if we're not in the specified mode?
There was a problem hiding this comment.
This method should only be called if the tool has already determined that it must be set. I added a note about it.
src/main/java/org/broadinstitute/hellbender/cmdline/ModeArgumentUtils.java
Outdated
Show resolved
Hide resolved
| ModeArgumentUtils.setArgValues( | ||
| getCommandLineParser(), | ||
| hcArgs.getDragenNameValuePairs(), | ||
| HaplotypeCallerArgumentCollection.DRAGEN_GATK_MODE_LONG_NAME); |
There was a problem hiding this comment.
Would it be a good idea for setArgValues() itself to check whether the provided mode is activated, and proceed with setting the args only if it's active? Or is an external check on the mode like you have here preferable? Either way, we don't need to address in this PR.
There was a problem hiding this comment.
This is a good point. I think ultimately the right place for this utility to live is in barclay and consequently this should maybe be automatic? This is a stopgap that helps handle the proliferation of messy argument mode switches in our tools for now.
Codecov Report
@@ Coverage Diff @@
## master #7745 +/- ##
================================================
+ Coverage 18.644% 86.949% +68.305%
- Complexity 4635 36911 +32276
================================================
Files 1261 2215 +954
Lines 73745 173559 +99814
Branches 11768 18741 +6973
================================================
+ Hits 13749 150908 +137159
+ Misses 57944 16042 -41902
- Partials 2052 6609 +4557
|
| GenotypeCalculationArgumentCollection.CALL_CONFIDENCE_LONG_NAME, "3.0", | ||
| GenotypeCalculationArgumentCollection.USE_POSTERIORS_TO_CALCULATE_QUAL_LONG_NAME, "true", | ||
| }; | ||
| } |
There was a problem hiding this comment.
What about hcArgs.informativeReadOverlapMargin = 1; from the old settings?
Also, were ReadFilterArgumentDefinitions.MINIMUM_MAPPING_QUALITY_NAME, "1" and ALLELE_EXTENSION_LONG_NAME, "1" set using the old method? I don't see them
There was a problem hiding this comment.
hcArgs.informativeReadOverlapMargin = 1; is handled. That is the ALLELE_EXTENSION_LONG_NAME, "1" in that list. Its argument name and its argument collection variable are fairly confusingly discordant.
MAPPING_QUALITY_THRESHOLD_FOR_GENOTYPING_LONG_NAME, "1", is also handled (which is distinct from the mapping quality on the read filter but that is handled by HaplotypeCaller.customCommandLineValidaiton()
No description provided.