baf fixes #7861

tedsharpe · 2022-05-19T15:14:21Z

Allows negative BAFs (so that we can process legacy files).
Skips same-site vcf records so that we can glide over the redundancies in the dbsnp sites vcf.

codecov · 2022-05-19T16:43:14Z

Codecov Report

Merging #7861 (2e27b12) into master (add34e1) will increase coverage by 0.062%.
The diff coverage is 79.736%.

❗ Current head 2e27b12 differs from pull request most recent head 6e94ccd. Consider uploading reports for the commit 6e94ccd to get more accurate results

@@               Coverage Diff               @@
##              master     #7861       +/-   ##
===============================================
+ Coverage     86.930%   86.992%   +0.062%     
- Complexity     36954     36960        +6     
===============================================
  Files           2221      2221               
  Lines         173829    173904       +75     
  Branches       18779     18784        +5     
===============================================
+ Hits          151110    151283      +173     
+ Misses         16087     15982      -105     
- Partials        6632      6639        +7

Impacted Files	Coverage Δ
...roadinstitute/hellbender/tools/sv/BafEvidence.java	`70.000% <ø> (+2.258%)`	⬆️
...institute/hellbender/tools/sv/PrintSVEvidence.java	`77.500% <ø> (ø)`
...ellbender/tools/DumpTabixIndexIntegrationTest.java	`100.000% <ø> (ø)`
...ute/hellbender/utils/codecs/SiteDepthBCICodec.java	`10.714% <10.714%> (ø)`
.../broadinstitute/hellbender/tools/sv/SiteDepth.java	`62.857% <33.333%> (ø)`
...titute/hellbender/utils/codecs/SiteDepthCodec.java	`74.194% <75.000%> (ø)`
...dinstitute/hellbender/tools/sv/SiteDepthtoBAF.java	`82.418% <80.952%> (ø)`
...hellbender/tools/walkers/sv/CollectSVEvidence.java	`79.878% <86.207%> (+29.058%)`	⬆️
...er/tools/walkers/sv/CollectSVEvidenceUnitTest.java	`97.521% <97.500%> (-0.010%)`	⬇️
...itute/hellbender/tools/sv/SiteDepthSortMerger.java	`100.000% <100.000%> (ø)`
... and 15 more

mwalker174

Thank you, looks good! It just occurred to me that we're missing some testing for BAF collection. Otherwise, minor comments.

mwalker174 · 2022-06-03T16:13:26Z

src/main/java/org/broadinstitute/hellbender/tools/sv/LocusDepthtoBAF.java

+            Arrays.sort(vals);
+            final int midIdx = nBafs / 2;
+            final double median =
+                    (nBafs & 1) != 0 ? vals[midIdx] : (vals[midIdx] + vals[midIdx - 1])/2.;


Just out of curiosity, what's the & 1 for?

(i & 1) is the way that 133t h4x0r d00dz such as myself test for odd integers. will change to (i % 2).

Ah I see it now

mwalker174 · 2022-06-03T16:26:45Z

src/main/java/org/broadinstitute/hellbender/tools/sv/BafEvidence.java

-        Utils.validateArg(value >= 0. || value == MISSING_VALUE,
-                "value must be non-negative or missing");


Can you add tests for values 0 and <0?

mwalker174 · 2022-06-03T16:32:14Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/sv/CollectSVEvidence.java

@@ -591,7 +591,7 @@ public AlleleCounter( final SAMSequenceDictionary dict,
            final FeatureDataSource<VariantContext> snpSource =
                    new FeatureDataSource<>(inputPath.toPath().toString());
            dict.assertSameDictionary(snpSource.getSequenceDictionary());
-            this.snpSourceItr = snpSource.iterator();
+            this.snpSourceItr = new BAFSiteIterator(snpSource.iterator());


It looks like we're missing tests for BAF, can you add unit/integration tests?

The BAFEvidence unit test also tests the codecs by making round trips. I could add an integration test for each codec. Would that suffice?

mwalker174 · 2022-06-03T16:41:11Z

Actually, a second thing just popped into my head. The name LocusDepth makes perfect sense but the abbreviation LD is overloaded with Linkage Disequilibrium. Not a huge problem in context, but it's likely the abbreviation will make its way into VCF annotations in the future, which could create confusion.

Is there another name we could use? I think SiteDepth or BaseDepth are also clear and unambiguous. @cwhelan any thoughts?

cwhelan · 2022-06-03T16:45:31Z

SiteDepth seems like it would work well in this context.

tedsharpe · 2022-06-03T20:33:41Z

Regarding finding a new name for LocusDepth: I'm fine with SiteDepth, but it seems to me that what really makes this class different from DepthEvidence, for example, is that it measures the read depth for each possible call. Should it be AlleleDepth?

tedsharpe · 2022-06-06T15:25:27Z

I believe the most recent commit addresses all review comments except for "missing tests for BAF".

gatk-bot · 2022-06-06T16:04:56Z

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

Test Type	JDK	Job ID	Logs
integration	11	2448670766.12	logs
integration	8	2448670766.0	logs

mwalker174 · 2022-06-06T16:05:44Z

Regarding finding a new name for LocusDepth: I'm fine with SiteDepth, but it seems to me that what really makes this class different from DepthEvidence, for example, is that it measures the read depth for each possible call. Should it be AlleleDepth?

I think that makes sense too but could be confusing mainly because AlleleDepth is already a haplotypecaller VCF field AD, which refers to called alleles rather than all possible alleles and also can refer to non-SNP alleles.

tedsharpe · 2022-06-06T23:48:24Z

I think this is ready to go. Differences of opinion welcomed.

tedsharpe · 2022-06-06T23:49:00Z

The coverage issues were, I believe, due to failing tests.

tedsharpe · 2022-06-15T14:56:31Z

I believe this is ready now. Added a couple of unit tests on the AlleleCounter. @mwalker174: Can I get another quick look from you?

mwalker174

Looks good! A couple of tiny comments and then good to merge

mwalker174 · 2022-06-17T16:23:02Z

src/main/java/org/broadinstitute/hellbender/tools/sv/SiteDepthtoBAF.java

-        summary = "Convert LocusDepth to BafEvidence",
-        oneLineSummary = "Convert LocusDepth to BafEvidence",
+        summary = "Convert SiteDepth to BafEvidence",
+        oneLineSummary = "Convert SiteDepth to BafEvidence",
        programGroup = StructuralVariantDiscoveryProgramGroup.class
 )
 @ExperimentalFeature


Suggested change

@ExperimentalFeature

@ExperimentalFeature

@DocumentedFeature

mwalker174 · 2022-06-17T21:57:41Z

src/main/java/org/broadinstitute/hellbender/utils/codecs/SiteDepthBCICodec.java

+/** Codec to handle SiteDepths in BlockCompressedInterval files */
+public class SiteDepthBCICodec extends AbstractBCICodec<SiteDepth> {
+    private boolean versionChecked = false;
+    private static final String LD_BCI_FILE_EXTENSION = ".sd.bci";


Suggested change

private static final String LD_BCI_FILE_EXTENSION = ".sd.bci";

private static final String SD_BCI_FILE_EXTENSION = ".sd.bci";

mwalker174 · 2022-06-17T21:58:38Z

src/test/java/org/broadinstitute/hellbender/tools/sv/SiteDepthtoBAFIntegrationTest.java

-public class LocusDepthtoBAFIntegrationTest extends CommandLineProgramTest {
-    public static final String ld2bafTestDir = toolsTestDir + "walkers/sv/LocusDepthtoBAF/";
+public class SiteDepthtoBAFIntegrationTest extends CommandLineProgramTest {
+    public static final String ld2bafTestDir = toolsTestDir + "walkers/sv/SiteDepthToBAF/";


Suggested change

public static final String ld2bafTestDir = toolsTestDir + "walkers/sv/SiteDepthToBAF/";

public static final String sd2bafTestDir = toolsTestDir + "walkers/sv/SiteDepthToBAF/";

ld files become sd files

tedsharpe requested a review from mwalker174 May 19, 2022 15:14

mwalker174 requested changes Jun 3, 2022

View reviewed changes

tedsharpe force-pushed the tws_bafFixes branch from 54b3e06 to 5ddefd3 Compare June 14, 2022 20:49

mwalker174 approved these changes Jun 17, 2022

View reviewed changes

baf fixes

6e94ccd

ld files become sd files

tedsharpe force-pushed the tws_bafFixes branch from 2e27b12 to 6e94ccd Compare June 21, 2022 14:20

tedsharpe merged commit 6170e83 into master Jun 21, 2022

tedsharpe deleted the tws_bafFixes branch June 21, 2022 18:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

baf fixes #7861

baf fixes #7861

tedsharpe commented May 19, 2022

codecov bot commented May 19, 2022 •

edited

Loading

mwalker174 left a comment

mwalker174 Jun 3, 2022

tedsharpe Jun 3, 2022

mwalker174 Jun 8, 2022

mwalker174 Jun 3, 2022

mwalker174 Jun 3, 2022

tedsharpe Jun 3, 2022

mwalker174 commented Jun 3, 2022

cwhelan commented Jun 3, 2022

tedsharpe commented Jun 3, 2022

tedsharpe commented Jun 6, 2022

gatk-bot commented Jun 6, 2022 •

edited

Loading

mwalker174 commented Jun 6, 2022

tedsharpe commented Jun 6, 2022

tedsharpe commented Jun 6, 2022

tedsharpe commented Jun 15, 2022

mwalker174 left a comment

mwalker174 Jun 17, 2022

mwalker174 Jun 17, 2022

mwalker174 Jun 17, 2022

		Utils.validateArg(value >= 0. \|\| value == MISSING_VALUE,
		"value must be non-negative or missing");

	private static final String LD_BCI_FILE_EXTENSION = ".sd.bci";
	private static final String SD_BCI_FILE_EXTENSION = ".sd.bci";

	public static final String ld2bafTestDir = toolsTestDir + "walkers/sv/SiteDepthToBAF/";
	public static final String sd2bafTestDir = toolsTestDir + "walkers/sv/SiteDepthToBAF/";

baf fixes #7861

baf fixes #7861

Conversation

tedsharpe commented May 19, 2022

codecov bot commented May 19, 2022 • edited Loading

Codecov Report

mwalker174 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

mwalker174 commented Jun 3, 2022

cwhelan commented Jun 3, 2022

tedsharpe commented Jun 3, 2022

tedsharpe commented Jun 6, 2022

gatk-bot commented Jun 6, 2022 • edited Loading

mwalker174 commented Jun 6, 2022

tedsharpe commented Jun 6, 2022

tedsharpe commented Jun 6, 2022

tedsharpe commented Jun 15, 2022

mwalker174 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

codecov bot commented May 19, 2022 •

edited

Loading

gatk-bot commented Jun 6, 2022 •

edited

Loading