Reference Compatibility tool #7959

orlicohen · 2022-07-21T18:17:57Z

Tool to determine compatibility of a provided BAM/CRAM/VCF against provided references.

…sequence code

codecov · 2022-07-26T21:50:51Z

Codecov Report

Merging #7959 (42fac0f) into master (64f9953) will decrease coverage by 0.362%.
The diff coverage is 93.478%.

@@               Coverage Diff               @@
##              master     #7959       +/-   ##
===============================================
- Coverage     87.062%   86.700%   -0.362%     
- Complexity     37007     38450     +1443     
===============================================
  Files           2218      2310       +92     
  Lines         173758    180344     +6586     
  Branches       18769     19841     +1072     
===============================================
+ Hits          151277    156358     +5081     
- Misses         15896     17042     +1146     
- Partials        6585      6944      +359

Impacted Files	Coverage Δ
...ls/reference/CompareReferencesIntegrationTest.java	`77.465% <ø> (ø)`
...tute/hellbender/tools/reference/ReferencePair.java	`89.744% <50.000%> (+3.257%)`	⬆️
...bender/tools/reference/ReferenceSequenceTable.java	`87.097% <90.909%> (+1.786%)`	⬆️
...r/tools/reference/CheckReferenceCompatibility.java	`93.137% <93.137%> (ø)`
...ce/CheckReferenceCompatibilityIntegrationTest.java	`93.548% <93.548%> (ø)`
.../hellbender/tools/reference/CompareReferences.java	`66.667% <100.000%> (ø)`
...ools/reference/ReferenceSequenceTableUnitTest.java	`98.230% <100.000%> (+0.358%)`	⬆️
...ynumber/models/AlleleFractionGlobalParameters.java	`76.471% <0.000%> (-23.529%)`	⬇️
...nstitute/hellbender/engine/filters/ReadFilter.java	`83.784% <0.000%> (-16.216%)`	⬇️
...er/tools/walkers/annotator/StrandBiasBySample.java	`72.000% <0.000%> (-6.261%)`	⬇️
... and 181 more

jamesemery

Oerall I think this is in good shape. A bunch of minor comments and some medium sized comments about how the tool is structured.

jamesemery · 2022-07-27T13:24:44Z

src/main/java/org/broadinstitute/hellbender/tools/reference/CheckReferenceCompatibility.java

+        oneLineSummary = "",
+        programGroup = ReferenceProgramGroup.class
+)
+


@DocumentedFeature, @ExperimentalFeature

jamesemery · 2022-07-27T13:24:52Z

src/main/java/org/broadinstitute/hellbender/tools/reference/CheckReferenceCompatibility.java

+
+@CommandLineProgramProperties(
+        summary = "",
+        oneLineSummary = "",


src/main/java/org/broadinstitute/hellbender/tools/reference/CheckReferenceCompatibility.java

jamesemery · 2022-07-27T13:42:28Z

src/main/java/org/broadinstitute/hellbender/tools/reference/CheckReferenceCompatibility.java

+
+    private void initializeSequenceDictionaryForInput(){
+        // BAMs/CRAMs
+        if(hasReads() ^ vcfPath != null){


There needs to be a check here that fails if both are specified for right now. Currently this falls off the bottom with empty inputs i think.

I would rework the if statements here to first check if the inputs are valid, then maybe handle reads, then maybe handle vcfs checking explicitly each time. Then put a check at the bottom to throw an exception of nothing got set for some reason (indcating that there was no input).

jamesemery · 2022-07-27T13:44:43Z

src/main/java/org/broadinstitute/hellbender/tools/reference/CheckReferenceCompatibility.java

+        if(hasReads() ^ vcfPath != null){
+            if (hasReads()) {
+                if (readArguments.getReadPathSpecifiers().size() > 1) {
+                    throw new UserException.BadInput("Tool analyzes one BAM at a time.");


BAM -> "Reads Input"

src/main/java/org/broadinstitute/hellbender/tools/reference/CheckReferenceCompatibility.java

...rg/broadinstitute/hellbender/tools/reference/CheckReferenceCompatibilityIntegrationTest.java

jamesemery · 2022-07-27T14:26:15Z

...rg/broadinstitute/hellbender/tools/reference/CheckReferenceCompatibilityIntegrationTest.java

+        runCommandLine(args);
+    }
+
+    // TODO: compatibility based on MD5 faulty since MD5s not in sequence dictionary (see ticket #730 "VCFHeader drops sequence dictionary attributes")


Since #730 is in another repository (htsjdk) make this the full link since this is going to be confusing.

jamesemery · 2022-07-27T14:27:00Z

...rg/broadinstitute/hellbender/tools/reference/CheckReferenceCompatibilityIntegrationTest.java

+
+    // for quick stdout testing
+    @Test(enabled = false)
+    public void testStdOutput() throws IOException{


I would probably get rid of this before merging this branch.

gatk-bot · 2022-07-27T18:15:58Z

Github actions tests reported job failures from actions build 2748430417
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
integration	11	2748430417.12	logs
integration	8	2748430417.0	logs

jamesemery · 2022-07-27T19:31:24Z

src/main/java/org/broadinstitute/hellbender/tools/reference/CheckReferenceCompatibility.java

@@ -45,9 +48,9 @@
 * <pre>
 * #Current Reference: reads_data_source_test1_withmd5s_missingchr1.bam
 * Reference	Compatibility


you forgot to update the table line here.

jamesemery · 2022-07-27T19:33:50Z

src/main/java/org/broadinstitute/hellbender/tools/reference/CheckReferenceCompatibility.java

            }
            // VCFs
            else {
-                try(final FeatureDataSource<VariantContext> vcfReader = new FeatureDataSource<>(vcfPath.toString())){
+                try (final FeatureDataSource<VariantContext> vcfReader = new FeatureDataSource<>(vcfPath.toString())) {


Make this explicitly check for VCF in this else statement. vcfPath.toString() can still throw a null pointer exception if you get here in the code without specifying either VCF OR a bam.

jamesemery · 2022-07-27T19:36:03Z

...rg/broadinstitute/hellbender/tools/reference/CheckReferenceCompatibilityIntegrationTest.java

-        //final File output = createTempFile("testReferenceCompatibilityMultipleReferencesWithMD5s", ".table");
-        final File expectedOutput = new File(getToolTestDataDir(), "expected.testReferenceCompatibilityMultipleReferencesBAMWithMD5s.table");
+    @Test(expectedExceptions = UserException.BadInput.class)
+    public void testReferenceCompatibilityBAMandVCF() throws IOException {


add one for neither bam or vcf as well.

jamesemery · 2022-07-27T19:38:40Z

src/main/java/org/broadinstitute/hellbender/tools/reference/CheckReferenceCompatibility.java

+        private final GATKPath ref;
+        private final Compatibility status;
+
+        private enum Compatibility{


rename this to CompatibilityStatus,

…encePairsAgainstDictionary()

orlicohen added 5 commits July 21, 2022 14:10

added new reference compatibility checker tool

88703ce

compatibility for MD5 present, work in progress

4f9eaaa

initial integration tests for md5/without md5s reads

ea45730

compatibility checker for reads files

316f3bf

compatibility for VCFs, more integration tests, added subset missing …

7e3e1fc

…sequence code

orlicohen requested a review from jamesemery July 26, 2022 20:24

orlicohen assigned jamesemery Jul 26, 2022

jamesemery requested changes Jul 27, 2022

View reviewed changes

orlicohen force-pushed the oc_referencecompatibilitychecker branch from 063d164 to 5648af9 Compare July 27, 2022 18:25

jamesemery reviewed Jul 27, 2022

View reviewed changes

orlicohen force-pushed the oc_referencecompatibilitychecker branch from 5648af9 to 44dfc50 Compare July 27, 2022 20:50

responding to review comments, also added unit test for generateRefer…

42fac0f

…encePairsAgainstDictionary()

orlicohen force-pushed the oc_referencecompatibilitychecker branch from 44dfc50 to 42fac0f Compare July 27, 2022 21:55

jamesemery approved these changes Jul 28, 2022

View reviewed changes

orlicohen merged commit c22972a into master Jul 28, 2022

orlicohen deleted the oc_referencecompatibilitychecker branch July 28, 2022 19:42

droazen changed the title **DO NOT MERGE** Reference Compatibility tool Reference Compatibility tool Jul 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reference Compatibility tool #7959

Reference Compatibility tool #7959

orlicohen commented Jul 21, 2022 •

edited

Loading

codecov bot commented Jul 26, 2022 •

edited

Loading

jamesemery left a comment

jamesemery Jul 27, 2022

jamesemery Jul 27, 2022

jamesemery Jul 27, 2022

jamesemery Jul 27, 2022

jamesemery Jul 27, 2022

jamesemery Jul 27, 2022

jamesemery Jul 27, 2022

gatk-bot commented Jul 27, 2022 •

edited

Loading

jamesemery Jul 27, 2022

jamesemery Jul 27, 2022

jamesemery Jul 27, 2022

jamesemery Jul 27, 2022

Reference Compatibility tool #7959

Reference Compatibility tool #7959

Conversation

orlicohen commented Jul 21, 2022 • edited Loading

codecov bot commented Jul 26, 2022 • edited Loading

Codecov Report

jamesemery 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gatk-bot commented Jul 27, 2022 • edited Loading

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

orlicohen commented Jul 21, 2022 •

edited

Loading

codecov bot commented Jul 26, 2022 •

edited

Loading

gatk-bot commented Jul 27, 2022 •

edited

Loading