Structural Variant Context #3476

vruano · 2017-08-22T18:06:07Z

Wrapper around VC object to access SVContext specific annotations.

vruano · 2017-08-22T18:07:43Z

This is part of a large piece of work, I'm trying to submit different part in separated self-standing pull-requests. @SHuang-Broad please review.

codecov-io · 2017-08-22T18:46:57Z

Codecov Report

Merging #3476 into master will decrease coverage by 0.011%.
The diff coverage is 74.286%.

@@               Coverage Diff               @@
##              master     #3476       +/-   ##
===============================================
- Coverage     80.098%   80.087%   -0.011%     
- Complexity     17741     17760       +19     
===============================================
  Files           1187      1188        +1     
  Lines          64305     64410      +105     
  Branches        9992     10004       +12     
===============================================
+ Hits           51507     51584       +77     
- Misses          8825      8840       +15     
- Partials        3973      3986       +13

Impacted Files	Coverage Δ	Complexity Δ
...tools/spark/sv/utils/StructuralVariantContext.java	`74.286% <74.286%> (ø)`	`19 <19> (?)`
...oadinstitute/hellbender/utils/gcs/BucketUtils.java	`76.316% <0%> (-1.316%)`	`38% <0%> (ø)`
...er/tools/spark/sv/discovery/AlignmentInterval.java	`90.678% <0%> (-0.847%)`	`24% <0%> (ø)`
...e/hellbender/engine/spark/SparkContextFactory.java	`73.973% <0%> (+2.74%)`	`11% <0%> (ø)`	⬇️

SHuang-Broad

@vruano Thanks for introducing this class.

However, I have several change requests:

override getType in its base class because there's no way this is a SNP
override the various is*** methods appropriately
test vcf is outdated (and is actually invalid, but my bad) in the sense that some annotations are not in the header and ALT allele header lines are missing. Simply use the latest master version of DiscoverVariantsFromContigAlignmentsSAMSpark to generate a valid vcf. In addition, there are records in the VCF that should be excluded for now
the method composeHaplotypeBasedOnReference() is a little heavy-weighted for this class, as well as the method getBreakPointIntervals(). I suggest (no strong feelings though) them being put in a SVContextUtils class or something similar

SHuang-Broad · 2017-08-22T18:12:46Z

src/main/java/org/broadinstitute/hellbender/tools/spark/sv/StructuralVariantContext.java

@@ -0,0 +1,280 @@
+package org.broadinstitute.hellbender.tools.spark.sv;


Would you object putting this in the utils subpackage? To me it fits there better.
And if you don't want to, then I insist this class be package-private because this class is experimental.

Ok utils it is.

SHuang-Broad · 2017-08-22T18:14:37Z

src/main/java/org/broadinstitute/hellbender/tools/spark/sv/StructuralVariantContext.java

+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.function.Supplier;


Unused import's found. Please remove or "optimize".

Ain't there anymore.

SHuang-Broad · 2017-08-22T18:14:48Z

...test/java/org/broadinstitute/hellbender/tools/spark/sv/StructuralVariantContextUnitTest.java

+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;


Unused import's found. Please remove or "optimize".

SHuang-Broad · 2017-08-22T18:15:37Z

src/main/java/org/broadinstitute/hellbender/tools/spark/sv/StructuralVariantContext.java

+import java.util.function.Supplier;
+
+/**
+ * Variant context with additional method to mind the structural variant specific information from structural


do you mean "mind" or "mine"?

The latter, thanks.

SHuang-Broad · 2017-08-22T18:16:01Z

src/main/java/org/broadinstitute/hellbender/tools/spark/sv/StructuralVariantContext.java

+    private int length = -1;
+
+    /**
+     * Returns a instance given a plain {@link VariantContext} instance.


"an" instatance

SHuang-Broad · 2017-08-22T19:28:32Z

...test/java/org/broadinstitute/hellbender/tools/spark/sv/StructuralVariantContextUnitTest.java

+        }
+    }
+
+    public static ReferenceMultiSource referenceMultiSource() {


private instead of public

SHuang-Broad · 2017-08-22T19:36:59Z

src/main/java/org/broadinstitute/hellbender/tools/spark/sv/StructuralVariantContext.java

+     * @param reference the reference to use as source.
+     * @return never {@code null}.
+     */
+    public Haplotype composeHaplotypeBasedOnReference(final int index, final int paddingSize, final ReferenceMultiSource reference)  {


should have extra parameter pipelineoptions instead of a implicit null below when accessing reference bases

To be honest I don't have a grasp on the PipelineOptions stuff yet, we follow your advice.

My bad. I just talked with the Engine team yesterday and they suggest, for tool developers, when PipelineOptions are needed, always use null and they'll have a smart way of dealing with the case when null is not enough. So please keep it the old way. Sorry!

SHuang-Broad · 2017-08-22T19:38:46Z

src/main/java/org/broadinstitute/hellbender/tools/spark/sv/StructuralVariantContext.java

+                Math.max(1, getStart() - paddingSize + 1),
+                Math.min(contigLength, getEnd() + paddingSize));
+
+        final ReferenceBases bases;


code above this line could be refactored into a "argument checking and get reference interval" utility method (perferably either private or package private to be tested)

What is the concern? that the method code is too long? @SHuang-Broad

not too long, but seems a utility method would serve the purpose. not a must though.

SHuang-Broad · 2017-08-22T19:41:01Z

src/main/java/org/broadinstitute/hellbender/tools/spark/sv/StructuralVariantContext.java

+        } catch (final IOException ex) {
+            throw new GATKException("could not read reference file");
+        }
+        if (index == 0) {


This has the implicit assumption that ref allele is at index 0. I remember from the days of fixing HaplotypeCaller this kind of implicit assumptions can give surprises. Please document clearly in the Javadoc for the parameters of this function that index==0 <=> reference allele

In think that given that this is a VC it is guaranteed that ref is at index 0. I imagine those problems where due to assume that the index 0 in order Allele containing objects also meant reference for example Allele array that was populated in a arbitrary manner... that said there is no much harm in calling getAllele(i) to verify, is just a added instructions.

SHuang-Broad · 2017-08-22T19:41:32Z

src/main/java/org/broadinstitute/hellbender/tools/spark/sv/StructuralVariantContext.java

+            result.setCigar(new Cigar(Collections.singletonList(new CigarElement(referenceInterval.size(), CigarOperator.M))));
+            result.setGenomeLocation(referenceInterval);
+            return result;
+        } else { //index == 1


This demands documentation for the biallelic assumption.

For now StructuralVariantContext is intrinsically balletic. What I can do is to make it "explode" if that is not the case.

create() is going to take care of making sure that is the case before returning a value.

vruano · 2017-08-24T05:46:27Z

@SHuang-Broad take a look and tell me if anything else needs to be changed.

SHuang-Broad · 2017-08-24T14:32:14Z

src/main/java/org/broadinstitute/hellbender/tools/spark/sv/utils/StructuralVariantContext.java

+    /**
+     * Returns the absolute structural variant length as recorded in the SVLEN annotation.
+     * <p>
+     *     When SVLEN is absent this method returns -1.


I suggest changing the "-1"'s mentioned in this doc to

public static final int MISSING_LENGTH = -1; // note currently it is private

SHuang-Broad

One minor comment. Merge at will. 👍

vruano · 2017-08-25T16:58:59Z

@SHuang-Broad I made a few "last" changes... MISSING_XX constants are only used internally to identify when the values have not been calculated. For length I have NO_LENGTH to indicate that in fact there is no length for that variant. Please take a look; hope to have this merged in today.

SHuang-Broad

Ready. Go. 👍

SHuang-Broad · 2017-08-25T18:37:11Z

src/main/java/org/broadinstitute/hellbender/tools/spark/sv/utils/StructuralVariantContext.java

@@ -127,7 +140,8 @@ public int getEnd() {
     * @param pipelineOptions pipeline options.
     * @return never {@code null}.
     */
-    public Haplotype composeHaplotypeBasedOnReference(final int index, final int paddingSize, final ReferenceMultiSource reference, final PipelineOptions pipelineOptions)  {
+    public Haplotype composeHaplotypeBasedOnReference(final int index, final int paddingSize, final ReferenceMultiSource reference,
+                                                      final PipelineOptions pipelineOptions)  {


My last comment about PipelineOptions is probably overwelmed by other changes, but here's the summary:
My suggestion about adding PipelineOptions are wrong, and please remove them. We need only null for them practically.

SHuang-Broad · 2017-08-25T18:40:09Z

src/main/java/org/broadinstitute/hellbender/tools/spark/sv/utils/StructuralVariantContext.java

-                end = getStart() + getStructuralVariantLength();
-            } else {
-                end = super.getEnd();
+        if (end == MISSING_END) {


Not suggesting that you change them now (though it will be good if you do so), I still believe that BND records should have no END concept associated, so having a hasEnd() method is good, and preferably users should call hasEnd() first, then getEnd().
But as said, you don't need to add this now.

vruano · 2017-08-25T20:20:07Z

No problem adding hasEnd()... but can you break down what SV types have it which don't ....

BUT I think is important to consider here what "end" means in reality.. I think that "end" here should be the last position continuously overlapped by the variant from its start position. So for insertions, translocations and bnds, typically it would be set equal to start.

Think about "start" itself.... it does not make reference to the first overlapped based but the ones before it. If a BND would not have an "end" why should it have an "start"?

I think start-end is just defined to what is practical for the sake of working with VCFs.

What do you think? @SHuang-Broad

vruano · 2017-08-25T20:21:56Z

@SHuang-Broad what is PipelineOptions needed for ... does one need it to access the reference if it stored in something that is not a ordinary file? (e.g. GS bucket?)

SHuang-Broad · 2017-08-25T20:44:19Z

For PipelineOptions, my understanding is that it is used for Google genomics API, and we seem to use it very infrequently in GATK (and never in SV), so it is safe to use null whenever engine level or other utility functions API needs it
For the END and START annotation, there is NOSTART in VCF spec, but POS, so I don't know where the start comes from. And yes, I agree that BND records don't have a start either, it is merely a novel adjacency between two genomic locations, none of which is a start or end.

Wrapper around VC object to access SVContext specific annotations.

vruano requested a review from SHuang-Broad August 22, 2017 18:06

SHuang-Broad self-assigned this Aug 22, 2017

SHuang-Broad requested changes Aug 22, 2017

View reviewed changes

SHuang-Broad assigned vruano and unassigned SHuang-Broad Aug 22, 2017

vruano force-pushed the vrr_svc branch 2 times, most recently from 86911ee to 8d77ce9 Compare August 23, 2017 20:59

SHuang-Broad reviewed Aug 24, 2017

View reviewed changes

SHuang-Broad approved these changes Aug 24, 2017

View reviewed changes

vruano force-pushed the vrr_svc branch from 8d77ce9 to 7af69d4 Compare August 25, 2017 16:56

SHuang-Broad approved these changes Aug 25, 2017

View reviewed changes

vruano added the question label Aug 25, 2017

vruano added 4 commits August 27, 2017 00:27

Structural Variant Context

615480f

Wrapper around VC object to access SVContext specific annotations.

Addressing comments

8245b01

Last changes, hopefully

c44ec77

Reverting the inclusion of Pipelineoptions in composeHaplotype methods

51bdf0a

vruano force-pushed the vrr_svc branch from 63007b6 to 51bdf0a Compare August 27, 2017 04:28

vruano merged commit bfa9af4 into master Aug 27, 2017

vruano deleted the vrr_svc branch August 27, 2017 05:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Structural Variant Context #3476

Structural Variant Context #3476

vruano commented Aug 22, 2017

vruano commented Aug 22, 2017

codecov-io commented Aug 22, 2017 •

edited

Loading

SHuang-Broad left a comment

SHuang-Broad Aug 22, 2017

vruano Aug 22, 2017

SHuang-Broad Aug 22, 2017

vruano Aug 22, 2017

SHuang-Broad Aug 22, 2017

SHuang-Broad Aug 22, 2017

vruano Aug 22, 2017

SHuang-Broad Aug 22, 2017

vruano Aug 22, 2017

SHuang-Broad Aug 22, 2017

SHuang-Broad Aug 22, 2017

vruano Aug 22, 2017

SHuang-Broad Aug 23, 2017

SHuang-Broad Aug 22, 2017

vruano Aug 23, 2017

SHuang-Broad Aug 23, 2017

SHuang-Broad Aug 22, 2017

vruano Aug 23, 2017

SHuang-Broad Aug 22, 2017

vruano Aug 23, 2017

vruano Aug 23, 2017

vruano commented Aug 24, 2017

SHuang-Broad Aug 24, 2017

SHuang-Broad left a comment

vruano commented Aug 25, 2017

SHuang-Broad left a comment

SHuang-Broad Aug 25, 2017

SHuang-Broad Aug 25, 2017

vruano commented Aug 25, 2017

vruano commented Aug 25, 2017 •

edited

Loading

SHuang-Broad commented Aug 25, 2017

		@@ -0,0 +1,280 @@
		package org.broadinstitute.hellbender.tools.spark.sv;

Structural Variant Context #3476

Structural Variant Context #3476

Conversation

vruano commented Aug 22, 2017

vruano commented Aug 22, 2017

codecov-io commented Aug 22, 2017 • edited Loading

Codecov Report

SHuang-Broad 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

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

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

vruano commented Aug 24, 2017

Choose a reason for hiding this comment

SHuang-Broad left a comment

Choose a reason for hiding this comment

vruano commented Aug 25, 2017

SHuang-Broad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vruano commented Aug 25, 2017

vruano commented Aug 25, 2017 • edited Loading

SHuang-Broad commented Aug 25, 2017

codecov-io commented Aug 22, 2017 •

edited

Loading

vruano commented Aug 25, 2017 •

edited

Loading