DepthOfCoverage port #5913

jamesemery · 2019-05-02T16:04:47Z

Currently this implementation could probably use more substantial testing, especially of the core functionality. It is also missing the following features from gatk3:

Depth of coverage by fragments
Rtable output format

The following important differences exist from gatk3:

Fixed numerous bugs with output line ordering, now we necessarily associate the right sample with the right data in the output tables. We also sort the output samples lexicographically consistently so the order of the output columns is deterministic.
Now gene outputs no longer rely on having exactly matching interval overlaps. Instead they are treated as intervals themselves with coverage information being collected for every exon base covered by the gene. There is an argument to demand that a gene must be completely covered by provided intervals to be in the result table. (Perhaps excising introns from coverage could be toggled?)
Intervals are no longer merged when reported in to the interval tables.

The following (new) features have been pushed into future branches:

Diagnose targets evaluation field (need to decide what partitioning to attach it to, perhaps sample partitioning info?)
Replacement for .refseq format for gene lists.
Optional output partitioning based a provided list of read filters.

Depends on #5887
Resolves #19

codecov · 2019-05-03T21:35:01Z

Codecov Report

❗ No coverage uploaded for pull request base (master@00f1e43). Click here to learn what that means.
The diff coverage is 73.997%.

@@            Coverage Diff             @@
##             master     #5913   +/-   ##
==========================================
  Coverage          ?   78.979%           
  Complexity        ?     30649           
==========================================
  Files             ?      2003           
  Lines             ?    150459           
  Branches          ?     16657           
==========================================
  Hits              ?    118831           
  Misses            ?     25832           
  Partials          ?      5796

Impacted Files	Coverage Δ	Complexity Δ
...lkers/coverage/DepthOfCoverageIntegrationTest.java	`0.758% <0.758%> (ø)`	`1 <1> (?)`
...nstitute/hellbender/utils/IntervalMergingRule.java	`100% <100%> (ø)`	`1 <0> (?)`
...org/broadinstitute/hellbender/utils/BaseUtils.java	`88.462% <100%> (ø)`	`59 <3> (?)`
...itute/hellbender/engine/LocusWalkerByInterval.java	`100% <100%> (ø)`	`7 <7> (?)`
...llbender/engine/LocusWalkerByIntervalUnitTest.java	`100% <100%> (ø)`	`5 <5> (?)`
...ender/utils/codecs/refseq/RefSeqCodecUnitTest.java	`100% <100%> (ø)`	`2 <2> (?)`
...titute/hellbender/utils/IntervalUtilsUnitTest.java	`91.906% <100%> (ø)`	`146 <0> (?)`
...broadinstitute/hellbender/utils/IntervalUtils.java	`92.11% <100%> (ø)`	`193 <4> (?)`
...nder/tools/walkers/annotator/CoverageUnitTest.java	`95.833% <100%> (ø)`	`12 <3> (?)`
.../hellbender/utils/codecs/refseq/RefSeqFeature.java	`39.706% <39.706%> (ø)`	`16 <16> (?)`
... and 9 more

kachulis · 2019-05-20T17:33:21Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/coverage/DepthOfCoverage.java

+     */
+    @Advanced
+    @Argument(fullName = "include-deletions", shortName = "dels", doc = "Include information on deletions", optional = true)
+    private boolean includeDeletions = false;


Could the default be switched to include deletions as contributing to coverage as opposed to not?

jamesemery · 2019-11-26T22:01:27Z

note to self, this happens if no interval is provided... this should be fixed:

org.broadinstitute.hellbender.exceptions.GATKException: Cannot call getTraversalParameters() without specifying either intervals to include or exclude.
	at org.broadinstitute.hellbender.cmdline.argumentcollections.IntervalArgumentCollection.getSpecifiedIntervalsWithoutMerging(IntervalArgumentCollection.java:154)
	at org.broadinstitute.hellbender.tools.walkers.coverage.DepthOfCoverage.getIntervalObjectsToQueryOver(DepthOfCoverage.java:443)
	at org.broadinstitute.hellbender.engine.LocusWalkerByInterval.traverse(LocusWalkerByInterval.java:55)
	at org.broadinstitute.hellbender.engine.GATKTool.doWork(GATKTool.java:1039)
	at org.broadinstitute.hellbender.cmdline.CommandLineProgram.runTool(CommandLineProgram.java:139)
	at org.broadinstitute.hellbender.cmdline.CommandLineProgram.instanceMainPostParseArgs(CommandLineProgram.java:191)
	at org.broadinstitute.hellbender.cmdline.CommandLineProgram.instanceMain(CommandLineProgram.java:210)
	at org.broadinstitute.hellbender.Main.runCommandLineProgram(Main.java:162)
	at org.broadinstitute.hellbender.Main.mainEntry(Main.java:205)
	at org.broadinstitute.hellbender.Main.main(Main.java:291)```

jamesemery · 2019-12-17T22:00:22Z

@droazen I have rebased this branch and cleaned up a few things that I noticed in the process. I think it could use your review now.

gatk-bot · 2019-12-17T22:32:11Z

Travis reported job failures from build 28390
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
python	openjdk8	28390.5	logs
integration	oraclejdk8	28390.11	logs
integration	openjdk11	28390.12	logs
integration	openjdk8	28390.2	logs

jamesemery · 2019-12-20T17:18:19Z

@droazen I have just rebased on top of the interval merging code change. I would love to get a review of this to chew on.

gatk-bot · 2019-12-20T17:49:11Z

Travis reported job failures from build 28445
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
integration	oraclejdk8	28445.11	logs
integration	openjdk11	28445.12	logs
integration	openjdk8	28445.2	logs

kachulis · 2020-02-12T14:27:27Z

@jamesemery shared with you offline some data that seems to give (incorrectly) output of 0 coverage for all genes. Hopefully won't be too difficult to figure out what the issue was with that.

droazen

@jamesemery I've finished reviewing the non-test classes. I'll post a review of the test classes separately later this week.

...va/org/broadinstitute/hellbender/cmdline/argumentcollections/IntervalArgumentCollection.java

src/main/java/org/broadinstitute/hellbender/engine/LocusWalkerByInterval.java

droazen · 2020-02-10T18:55:47Z

src/main/java/org/broadinstitute/hellbender/engine/LocusWalkerByInterval.java

+public abstract class LocusWalkerByInterval extends LocusWalker {
+
+    private OverlapDetector<Locatable> intervalsToTrack = null;
+    private Set<Locatable> previousIntervals = new HashSet<>();


Any potential ordering issues with using HashSet instead of LinkedHashSet here?

So as it is currently written the consequence here is very low. Auditing this one the worst case here is that the order in which one writes out genes could be shuffled somewhat (the other cases were order independent). I'm going to switch this to a linked hash set to avoid the trouble though.

src/main/java/org/broadinstitute/hellbender/engine/LocusWalkerByInterval.java

droazen · 2020-03-04T21:29:42Z

src/main/java/org/broadinstitute/hellbender/utils/codecs/refseq/RefSeqFeature.java

+     * Returns a count of the total number of reference bases spanned by gene summary. Will total the length of the exons or
+     * if absent, the lengthOnReference for the gene itself.
+     *
+     * NOTE: This currently makes the assumption that genes do not ever have overlapping exons. I do not know if this is a fair


According to my sources, genes can have overlapping exons, though...

I say lets burn this bridge when we get to it. It is a fairly edge case functionality that GATK3 used and ideally we will replace this class entirely with something more general

droazen · 2020-03-04T21:36:34Z

src/main/java/org/broadinstitute/hellbender/utils/codecs/refseq/RefSeqFeature.java

+
+    /** Returns true if the specified interval 'that' overlaps with any of the exons actually spliced into this transcript */
+    @Override
+    public boolean contains(Locatable that) {


The definition of contains() is that this Locatable completely contains the region represented by the Locatable passed in. Your implementation here, however, is merely checking for overlap with any of the exons, which seems wrong.

This is an important functionality. This is what enforces that we are only counting exon coverage for genes. Introns are note "really" part of the transcript for coverage in this case.

src/main/java/org/broadinstitute/hellbender/utils/codecs/refseq/Transcript.java

droazen

@jamesemery And here are my comments on the test classes

src/test/java/org/broadinstitute/hellbender/engine/LocusWalkerByIntervalUnitTest.java

droazen · 2020-03-05T20:42:48Z

...ava/org/broadinstitute/hellbender/tools/walkers/coverage/DepthOfCoverageIntegrationTest.java

+            Assert.assertEquals(lines.get(0)[0], "SLC35E2");
+            Assert.assertEquals(lines.get(1)[0], "SLC35E2");
+            Assert.assertNotEquals(lines.get(0), lines.get(1));
+        }


Here too you're not calling compareOutputDirectories() to fully check the output files?

droazen · 2020-03-05T20:47:55Z

...ava/org/broadinstitute/hellbender/tools/walkers/coverage/DepthOfCoverageIntegrationTest.java

+            Iterator<String[]> expectedLines = StreamSupport.stream(expectedParser.spliterator(),false).sorted(Comparator.comparing(l -> l[0]+","+l[1])).iterator();
+            // Special case to handle scrambled output header for the base coverage in gatk3
+            if (getDoCExtensionFromFile(actualFile, actualFileBaseName).equals("")) {
+                // Just assert all the correct counts are present for each line because the order is wrong in the expected files


Doesn't the sort above compensate for ordering differences? I don't understand this comment.

Yes, gatk3 test files included bugs that I fixed in terms of column ordering.

src/test/java/org/broadinstitute/hellbender/utils/IntervalUtilsUnitTest.java

src/test/java/org/broadinstitute/hellbender/utils/codecs/refseq/RefSeqCodecUnitTest.java

droazen · 2020-03-05T20:49:49Z

src/test/java/org/broadinstitute/hellbender/utils/codecs/refseq/RefSeqCodecUnitTest.java

+        Assert.assertTrue(codec.canDecode("filename" + EXTRA_CHAR + "." + RefSeqCodec.FILE_EXT));
+        Assert.assertFalse(codec.canDecode("filename." + RefSeqCodec.FILE_EXT + EXTRA_CHAR));
+        Assert.assertFalse(codec.canDecode("filename" + RefSeqCodec.FILE_EXT));
+    }


Weren't there any RefSeq tests from GATK3 that we could port over?

Yeah here they all are:

public class RefSeqCodecUnitTest { @Test public void testCanDecode() { final String EXTRA_CHAR = "1"; RefSeqCodec codec = new RefSeqCodec(); Assert.assertTrue(codec.canDecode("filename." + RefSeqCodec.FILE_EXT)); Assert.assertTrue(codec.canDecode("filename" + EXTRA_CHAR + "." + RefSeqCodec.FILE_EXT)); Assert.assertFalse(codec.canDecode("filename." + RefSeqCodec.FILE_EXT + EXTRA_CHAR)); Assert.assertFalse(codec.canDecode("filename" + RefSeqCodec.FILE_EXT)); } }

droazen · 2020-03-19T16:42:13Z

@jamesemery How close is this to being merged? Once you've addressed all review comments to your satisfaction and have rebased to resolve the conflicts, let me know and I can give final approval after tests are green.

…a few things for re-review

gatk-bot · 2020-03-19T19:50:47Z

Travis reported job failures from build 29604
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
unit	openjdk11	29604.14	logs
integration	oraclejdk8	29604.12	logs
integration	openjdk11	29604.13	logs
integration	openjdk8	29604.2	logs

droazen · 2020-03-20T18:34:54Z

@jamesemery There appear to be some lingering test failures here after the rebase (mismatches between actual and expected VCF files).

gatk-bot · 2020-03-23T16:40:07Z

Travis reported job failures from build 29680
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
unit	openjdk11	29680.14	logs

…tion of deletion sensitive locus code

jamesemery · 2020-03-23T19:36:24Z

@droazen I fixed the tests (it was an errant find and replace error). I have spot checked the changes and it looks like nothing obviously off-target in there. I need another approval in order to finally merge this branch.

droazen

@jamesemery If you've addressed all comments, go ahead and merge once tests pass

oguzhankalyon · 2020-09-29T10:43:31Z

src/main/java/org/broadinstitute/hellbender/utils/codecs/refseq/RefSeqCodec.java

+ *
+ * <p>
+ * Instructions for generating a RefSeq file for use with the RefSeq codec can be found on the documentation guide here
+ * <a href="https://gatkforums.broadinstitute.org/gatk/discussion/1329/where-can-i-get-a-gene-list-in-refseq-format">https://gatkforums.broadinstitute.org/gatk/discussion/1329/where-can-i-get-a-gene-list-in-refseq-format</a>


url is broken. Should be : https://gatk.broadinstitute.org/hc/en-us/articles/360035532032-RefSeq-gene-list-format
Also user should be notified that file extension must be .refseq

@oguzhankalyon Could you please file a new ticket about these issues instead of commenting on this merged pull request? Thanks!

droazen self-requested a review May 3, 2019 20:53

droazen self-assigned this May 3, 2019

jamesemery mentioned this pull request May 9, 2019

Added a simple TSV/CSV/XSV writer with cloud write support #5930

Merged

kachulis reviewed May 20, 2019

View reviewed changes

jamesemery requested a review from jonn-smith July 29, 2019 16:46

jamesemery force-pushed the je_portDepthOfCoverge branch from a7ed4fc to 2228393 Compare December 17, 2019 21:59

jamesemery force-pushed the je_portDepthOfCoverge branch from 2228393 to 4dc1ff1 Compare December 20, 2019 17:10

droazen changed the title ~~DepthOfCoverage port checkpoint~~ DepthOfCoverage port Feb 5, 2020

droazen suggested changes Mar 4, 2020

View reviewed changes

droazen suggested changes Mar 5, 2020

View reviewed changes

droazen assigned jamesemery Mar 5, 2020

This was referenced Mar 9, 2020

Lingering Improvements to DepthOfCoverage tool beta version #6491

Open

Make DepthOfCoverage not require a reference #6496

Open

jamesemery added 8 commits March 19, 2020 14:05

resovling merge conflicts

6a027ff

stylistic changes

ed759f5

fixing some broken imports

2e61cc1

fixed broken imports

e5187eb

revisited the branch after it has suffered much mould and cleaned up …

d313ed0

…a few things for re-review

reconciling some last minute argument changes with the tests

1b5dd41

responding to review comments

0b52faf

updated the docs to reflect the broken link

74f8505

jamesemery force-pushed the je_portDepthOfCoverge branch from 1271f32 to 74f8505 Compare March 19, 2020 18:16

fixing some issues that came up from the rebase

e805b1e

fixing the broken results from find and replace on simple words

b46e4e3

jamesemery added 2 commits March 23, 2020 13:22

went spelunking for the one place that I missed when undoing the dele…

d976875

…tion of deletion sensitive locus code

one typo

5feae31

droazen approved these changes Mar 23, 2020

View reviewed changes

jamesemery merged commit b459b72 into master Mar 23, 2020

oguzhankalyon reviewed Sep 29, 2020

View reviewed changes

GATKSupportTeam mentioned this pull request Mar 1, 2022

Add DiagnoseTarget to DepthofCoverage GATK4 #7702

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DepthOfCoverage port #5913

DepthOfCoverage port #5913

jamesemery commented May 2, 2019

codecov bot commented May 3, 2019 •

edited

Loading

kachulis May 20, 2019

jamesemery commented Nov 26, 2019

jamesemery commented Dec 17, 2019

gatk-bot commented Dec 17, 2019 •

edited

Loading

jamesemery commented Dec 20, 2019

gatk-bot commented Dec 20, 2019 •

edited

Loading

kachulis commented Feb 12, 2020

droazen left a comment

droazen Feb 10, 2020

jamesemery Mar 5, 2020

droazen Mar 4, 2020

jamesemery Mar 11, 2020

droazen Mar 4, 2020

jamesemery Mar 11, 2020

droazen left a comment

droazen Mar 5, 2020

droazen Mar 5, 2020

jamesemery Mar 11, 2020

droazen Mar 5, 2020

jamesemery Mar 11, 2020

droazen commented Mar 19, 2020

gatk-bot commented Mar 19, 2020 •

edited

Loading

droazen commented Mar 20, 2020

gatk-bot commented Mar 23, 2020

jamesemery commented Mar 23, 2020

droazen left a comment

oguzhankalyon Sep 29, 2020

droazen Sep 29, 2020

DepthOfCoverage port #5913

DepthOfCoverage port #5913

Conversation

jamesemery commented May 2, 2019

codecov bot commented May 3, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

jamesemery commented Nov 26, 2019

jamesemery commented Dec 17, 2019

gatk-bot commented Dec 17, 2019 • edited Loading

jamesemery commented Dec 20, 2019

gatk-bot commented Dec 20, 2019 • edited Loading

kachulis commented Feb 12, 2020

droazen 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

droazen 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

droazen commented Mar 19, 2020

gatk-bot commented Mar 19, 2020 • edited Loading

droazen commented Mar 20, 2020

gatk-bot commented Mar 23, 2020

jamesemery commented Mar 23, 2020

droazen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 3, 2019 •

edited

Loading

gatk-bot commented Dec 17, 2019 •

edited

Loading

gatk-bot commented Dec 20, 2019 •

edited

Loading

gatk-bot commented Mar 19, 2020 •

edited

Loading