Add cmd line to VCF generated by GATKSparkTool #4981

SHuang-Broad · 2018-07-03T19:26:56Z

There's such feature for GATKTool, but not yet for GATKSparkTool.

Much of the code is copied from the GATKTool version
Engine team, please comment if a refactor is needed and how. Thanks!
(Tagging @droazen @lbergelson and @cmnbroad )

cmnbroad · 2018-07-03T20:31:52Z

src/main/java/org/broadinstitute/hellbender/engine/spark/GATKSparkTool.java

@@ -404,6 +410,34 @@ public boolean useVariantAnnotations() {
        return Collections.emptyList();
    }

+    // TODO: 7/3/18 the two functions below are copy-pasted from GATKTool, and probably some refactoring can be done


@SHuang-Broad Thanks for doing this. You're right that we definitely don't want to duplicate all of this code. I would factor out a makeDefaultToolVCFHeaderLines method that takes the toolkit short name, toolkit version, the tool class name, and the command line, and returns the set of header lines, and delegate to that from both GATKTool and GATKSparkTool when addOutputVCFCommandLine is true. For now it can probably live in GATKVariantContextUtils with createVCFWriter. (@droazen @lbergelson we might want to create a ToolUtils class with static stuff like this - I think there is a bunch of code we could move into that).

Thanks @cmnbroad .

I looked at GATKVariantContextUtils.createVCFWriter(...), and where GATKTool.getDefaultToolVCFHeaderLines(...) is used, it seems that the individual tools are

constructing their headers by calling explicitly into GATKTool.getDefaultToolVCFHeaderLines(...) , then

get the writer by calling into GATKVariantContextUtils.createVCFWriter(...), then

let the writer writeHeader()

in order. So I'm not quite understanding the comment:

For now it can probably live in GATKVariantContextUtils with createVCFWriter.

Are you suggesting that I factor out the method makeDefaultToolVCFHeaderLines(...), place it in GATKVariantContextUtils, but not let the code be absorbed into createVCFWriter?

In addition, I'm suspecting more could be done, probably extracting out an abstract class like AbstractGATKBaseTool?

codecov-io · 2018-07-10T02:00:42Z

Codecov Report

Merging #4981 into master will increase coverage by 26.014%.
The diff coverage is 94.737%.

@@               Coverage Diff                @@
##              master     #4981        +/-   ##
================================================
+ Coverage     60.157%   86.171%   +26.014%     
- Complexity     12768     28906     +16138     
================================================
  Files           1095      1783       +688     
  Lines          64608    134295     +69687     
  Branches       10395     15240      +4845     
================================================
+ Hits           38866    115723     +76857     
+ Misses         21504     13163      -8341     
- Partials        4238      5409      +1171

Impacted Files	Coverage Δ	Complexity Δ
...lbender/utils/variant/GATKVariantContextUtils.java	`85.045% <100%> (+33.365%)`	`226 <1> (+114)`	⬆️
...hellbender/engine/spark/GATKSparkToolUnitTest.java	`100% <100%> (ø)`	`2 <2> (?)`
...stitute/hellbender/cmdline/CommandLineProgram.java	`85.906% <100%> (+8.879%)`	`44 <1> (+5)`	⬆️
...org/broadinstitute/hellbender/engine/GATKTool.java	`89.595% <100%> (+3.232%)`	`144 <0> (+51)`	⬆️
...stitute/hellbender/engine/spark/GATKSparkTool.java	`84.298% <66.667%> (-0.92%)`	`63 <1> (+1)`
...der/tools/walkers/variantutils/SelectVariants.java	`78.719% <0%> (-0.563%)`	`149% <0%> (+27%)`
...nstitute/hellbender/engine/MultiVariantWalker.java	`96.154% <0%> (-0.142%)`	`24% <0%> (+12%)`
...er/engine/ReadWalkerGCSSupportIntegrationTest.java	`4.878% <0%> (ø)`	`2% <0%> (?)`
...specific/AlleleSpecificAnnotationDataUnitTest.java	`86.667% <0%> (ø)`	`4% <0%> (?)`
...nder/tools/spark/sv/utils/SVVCFWriterUnitTest.java	`97.917% <0%> (ø)`	`3% <0%> (?)`
... and 1278 more

…TKSparkTool (allow Spark-tool-generated VCF to have cmd line history)

cmnbroad

A couple of minor changes requested, otherwise looks good.

cmnbroad · 2018-07-16T14:19:28Z

src/main/java/org/broadinstitute/hellbender/utils/variant/GATKVariantContextUtils.java

+     * @return A set of VCF header lines containing the tool name, version, date and command line.
+     */
+    public static Set<VCFHeaderLine> getDefaultVCFHeaderLines(final String toolkitShortName, final String toolName,
+                                                              final String versionString, final String dataTime,


dataTime -> dateTime

Also, javadoc should include the param list with short descriptions

cmnbroad · 2018-07-16T17:02:44Z

src/main/java/org/broadinstitute/hellbender/engine/spark/GATKSparkTool.java

+     * TODO: This should be refactored and moved up into CommandLineProgram, with this value
+     * TODO: stored in the jar manifest, like {@link CommandLineProgram#getToolkitName}
+     */
+    protected String getToolkitShortName() { return "GATK"; }


Actually, we might as well move this method up into CommandLineProgram now, adjacent to getToolkitName, along with a DEFAULT_TOOLKIT_SHORT_NAME static constant, and return that. Then we can remove the two identical implementations. Also, lets keep the second part of the TODO comment with the new code.

refactored, not sure if they are what you mean?

SHuang-Broad · 2018-07-17T13:31:44Z

@cmnbroad , I've done the suggested refactoring and documentation changes.
Can you take a look again please? Thanks!

cmnbroad

@SHuang-Broad Two very minor changes requested, then we can merge.

cmnbroad · 2018-07-20T14:02:35Z

src/main/java/org/broadinstitute/hellbender/cmdline/CommandLineProgram.java

@@ -60,6 +60,8 @@
    // abstract, this is fine (as long as no logging has to happen statically in this class).
    protected final Logger logger = LogManager.getLogger(this.getClass());

+    public static final String DEFAULT_TOOLKIT_SHORT_NAME = "GATK";


This can be private unless there is a compelling reason otherwise.

cmnbroad · 2018-07-20T14:03:14Z

src/main/java/org/broadinstitute/hellbender/cmdline/CommandLineProgram.java

+     * @return An abbreviated name of the toolkit for this tool. Subclasses may override to provide
+     *         a custom toolkit name.
+     *
+     * TODO: stored in the jar manifest, like {@link CommandLineProgram#getToolkitName}


Can you move this TODO out to a separate comment in the body so it doesn't show up in the javadoc.

done, but not sure if that's what you'd like?

SHuang-Broad requested review from lbergelson, droazen and cmnbroad July 3, 2018 19:27

cmnbroad reviewed Jul 3, 2018

View reviewed changes

SHuang-Broad force-pushed the sh-add-cmd-line-to-spark-tool-vcf branch 3 times, most recently from 39b2487 to 1f66a73 Compare July 10, 2018 00:59

droazen assigned cmnbroad Jul 10, 2018

droazen removed their request for review July 10, 2018 15:58

SHuang-Broad force-pushed the sh-add-cmd-line-to-spark-tool-vcf branch from 1f66a73 to 97e32cb Compare July 11, 2018 00:03

SHuang-Broad added 3 commits July 12, 2018 10:27

copy functionality getDefaultToolVCFHeaderLines() from GATKTool to GA…

9057d0b

…TKSparkTool (allow Spark-tool-generated VCF to have cmd line history)

add test

e0a0720

?count as refactor?

2ad08e2

SHuang-Broad force-pushed the sh-add-cmd-line-to-spark-tool-vcf branch from 97e32cb to 2ad08e2 Compare July 12, 2018 14:27

cmnbroad requested changes Jul 16, 2018

View reviewed changes

cmnbroad assigned SHuang-Broad Jul 16, 2018

making suggested changes

2c55c15

cmnbroad reviewed Jul 20, 2018

View reviewed changes

more suggested changes

c4a64f3

cmnbroad merged commit 8ad39d7 into master Jul 23, 2018

cmnbroad deleted the sh-add-cmd-line-to-spark-tool-vcf branch July 23, 2018 12:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cmd line to VCF generated by GATKSparkTool #4981

Add cmd line to VCF generated by GATKSparkTool #4981

SHuang-Broad commented Jul 3, 2018

cmnbroad Jul 3, 2018

SHuang-Broad Jul 9, 2018

SHuang-Broad Jul 9, 2018

codecov-io commented Jul 10, 2018 •

edited

Loading

cmnbroad left a comment

cmnbroad Jul 16, 2018

cmnbroad Jul 16, 2018

SHuang-Broad Jul 17, 2018

cmnbroad Jul 16, 2018

SHuang-Broad Jul 17, 2018

SHuang-Broad commented Jul 17, 2018

cmnbroad left a comment

cmnbroad Jul 20, 2018

SHuang-Broad Jul 20, 2018

cmnbroad Jul 20, 2018

SHuang-Broad Jul 20, 2018

Add cmd line to VCF generated by GATKSparkTool #4981

Add cmd line to VCF generated by GATKSparkTool #4981

Conversation

SHuang-Broad commented Jul 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 10, 2018 • edited Loading

Codecov Report

cmnbroad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SHuang-Broad commented Jul 17, 2018

cmnbroad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 10, 2018 •

edited

Loading