Enable LiftOverVcf to emit a variant when the reference base has changed #971

yfarjoun · 2017-11-08T13:00:50Z

Description

Enables LiftOverVcf to process variants where the reference changed, swapping the reference and the alternate alleles.

This has been implemented for bi-allelic SNPs only.

Checklist (never delete this)

Never delete this, it is our record that procedure was followed. If you find that for whatever reason one of the checklist points doesn't apply to your PR, you can leave it unchecked but please add an explanation below.

Content

Added or modified tests to cover changes and any new functionality
Edited the README / documentation (if applicable)
All tests passing on Travis

Review

Final thumbs-up from reviewer
Rebase, squash and reword as applicable

For more detailed guidelines, see https://github.com/broadinstitute/picard/wiki/Guidelines-for-pull-requests

… reference allele is taken from the reference file and the allele in the vcf is ignored....

…e it should be considered a failure...

- added option to reverse AF-like INFO annotations - added option to drop annotations that become invalid upon swapping (like MAX_AF)

coveralls · 2017-11-08T13:24:38Z

Coverage decreased (-0.001%) to 77.183% when pulling 2e16d48 on yf_fix_mismatching_allele into 28a441a on master.

pshapiro4broad · 2017-11-08T13:36:27Z

src/main/java/picard/util/LiftoverUtils.java

+
+    public static final String SWAPPED_ALLELES = "SwappedAlleles";
+
+    public static Log log = Log.getInstance(LiftoverUtils.class);


can be final

pshapiro4broad · 2017-11-08T13:37:17Z

src/main/java/picard/util/LiftoverUtils.java

@@ -192,6 +207,81 @@ protected static GenotypesContext fixGenotypes(final GenotypesContext originals,
    }

    /**
+     * method to swap the reference and alt alleles of a bi-allelic, SNP
+     *
+     * @param vc


Provide doc text for @param and @return or leave them out of the javadoc

pshapiro4broad · 2017-11-08T13:40:48Z

src/main/java/picard/util/LiftoverUtils.java

+        final Map<Allele, Allele> alleleMap = new HashMap<>();
+        for (int idx = 0; idx < vc.getAlleles().size(); idx++) {
+            // The index of the allele changed from 1 to 0 and vice versa, but the bases remain the same.
+            alleleMap.put(vc.getAlleles().get(idx), swappedBuilder.getAlleles().get(1 - idx));


IMO a loop here is less clear than writing out the two put() calls you'd otherwise have.

pshapiro4broad · 2017-11-08T13:43:55Z

src/main/java/picard/util/LiftoverUtils.java

+                swappedBuilder.rmAttribute(key);
+            }
+
+            if (annotationsToReverse.contains(key)) {


else if here? Seems like you can't both drop and reverse an annotation.

pshapiro4broad · 2017-11-08T13:45:02Z

src/main/java/picard/util/LiftoverUtils.java

+
+                if (afLikeAttribute == -1) {
+                    log.warn("Trying to reverse attribute " + key +
+                            "but found value that parsable as a double (" + vc.getAttribute(key, "") +


found value that isn't parsable ?

pshapiro4broad · 2017-11-08T13:48:52Z

src/main/java/picard/util/LiftoverUtils.java

+
+                if (afLikeAttribute < 0 || afLikeAttribute > 1) {
+                    log.warn("Trying to reverse attribute " + key +
+                            "but found value that isn't between 0 and 1 (" + afLikeAttribute + ") in variant " + vc + "results might be wrong.");


Both messages seem to be missing a : between the error message and the incorrect values. E.g. I'd expect to see "Trying to reverse attribute X but found value that isn't between 0 and 1: (X) in variant V. Results might be wrong."

pshapiro4broad · 2017-11-08T13:49:22Z

src/main/java/picard/util/LiftoverUtils.java

+                if (afLikeAttribute == -1) {
+                    log.warn("Trying to reverse attribute " + key +
+                            "but found value that parsable as a double (" + vc.getAttribute(key, "") +
+                            ") in variant " + vc + "results might be wrong.");


Missing a space before results, also looks like a typo, extra might be wrong. here and below.

pshapiro4broad · 2017-11-08T13:51:13Z

src/main/java/picard/util/LiftoverUtils.java

+        final String refString = StringUtil.bytesToString(referenceSequence.getBases(), start-1, end - start + 1);
+        //make sure that referenceAllele matches reference
+        final Allele refAllele = alleles.stream().filter(Allele::isReference).findAny().orElse(null);
+        if (refAllele == null) throw new RuntimeException("How did that happen? No reference allele?");


A shorter way to write this is to use orElseThrow() instead of orElse(). e.g. alleles.stream().filter(Allele::isReference).findAny().orElseThrow(new RuntimeException("How did that happen? No reference allele?"))

pshapiro4broad · 2017-11-08T13:56:00Z

src/test/java/picard/util/LiftoverVcfTest.java

+    }
+
+
+        @Test(dataProvider = "snpWithChangedRef")


fix indent, this line and next

pshapiro4broad · 2017-11-08T13:56:48Z

src/test/java/picard/util/LiftoverVcfTest.java

-        builder.source("test3");
-        for (start = 2; start <= 11; start++) {
+        for (start = 2; start <= 10; start++) {
+            builder.source("test3-"+ start);


should have a space between the string and the +

coveralls · 2017-11-08T15:43:03Z

Coverage decreased (-0.005%) to 77.179% when pulling 9fd7512 on yf_fix_mismatching_allele into 28a441a on master.

eitanbanks

Made a bunch of comments, but most are cosmetic.

eitanbanks · 2017-11-09T13:40:55Z

src/main/java/picard/util/LiftoverUtils.java

+    /**
+     * Attribute used to store the fact that the alt and ref alleles of the variant have been swapped, while all the INFO annotations have not.
+     */
+


I'd remove this line and maybe update the comment above so that it's applicable to both of the static variables below it.

(I meant remove the empty line)

eitanbanks · 2017-11-09T13:46:13Z

src/main/java/picard/util/LiftoverUtils.java

+
+        swappedBuilder.attribute(SWAPPED_ALLELES, true);
+
+        // You need to get the baseString in order to forget who's reference and who's variant


"who's" -> "which is"

Why "forget"?

I tried explaining why I put the getBaseString in the first place...but clearly my comment is unclear...I'll reword.

eitanbanks · 2017-11-09T13:49:47Z

src/main/java/picard/util/LiftoverUtils.java

+
+        final Map<Allele, Allele> alleleMap = new HashMap<>();
+
+        // The index of the allele changed from 1 to 0 and vice versa, but the bases remain the same.


This comment is confusing. Would just say "A mapping from old allele to new allele"

eitanbanks · 2017-11-09T13:53:43Z

src/main/java/picard/util/LiftoverUtils.java

+            }
+            // Flip AD
+            final GenotypeBuilder builder = new GenotypeBuilder(genotype).alleles(swappedAlleles);
+            if (genotype.hasAD() && genotype.getAD().length == 2) {


If the length is not 2 then I would invalidate the AD (set to null).

eitanbanks · 2017-11-09T13:55:01Z

src/main/java/picard/util/LiftoverUtils.java

+            }
+
+            //Flip PL
+            if (genotype.hasPL() && genotype.getPL().length == 3) {


Same here: If the length is not 3 then I would invalidate the PL.

eitanbanks · 2017-11-09T14:02:45Z

src/main/java/picard/util/LiftoverUtils.java

+        final String refString = StringUtil.bytesToString(referenceSequence.getBases(), start-1, end - start + 1);
+        //make sure that referenceAllele matches reference
+        final Allele refAllele = alleles.stream().filter(Allele::isReference).findAny().orElseThrow(()->new RuntimeException("How did that happen? No reference allele?"));
+


Do we really need this check here? It seems more appropriate to put this check where the ref allele was created.

hmmm. But that really doesn't fit with the flow of the original algorithm as the actual check against the reference happens at the end, before trying to add the VC...if you feel strongly about this, let's talk more in person.

I don't feel strongly... it's just weird. As a reviewer of the code it just looks out of place.
Can we move it up to the method that calls this?

eitanbanks · 2017-11-09T14:03:36Z

src/main/java/picard/vcf/LiftoverVcf.java

@@ -161,13 +164,14 @@
     */
    public static final String ATTEMPTED_LOCUS = "AttemptedLocus";

+


extra space added

eitanbanks · 2017-11-09T14:04:30Z

src/main/java/picard/vcf/LiftoverVcf.java

+
+        outHeader.addMetaDataLine(new VCFInfoHeaderLine(LiftoverUtils.SWAPPED_ALLELES, 0, VCFHeaderLineType.Flag,
+                "The REF and the ALT alleles have been swapped due to changes in the reference. " +
+                "NOTA BENE: No INFO field annotations reflect this swap, and in the genotypes, " +


Well, TAGS_TO_REVERSE do reflect this swap...

right....fixing.

eitanbanks · 2017-11-09T14:06:55Z

src/main/java/picard/vcf/LiftoverVcf.java

@@ -362,6 +373,13 @@ private void tryToAddVariant(final VariantContext vc, final ReferenceSequence re
                final String refString = StringUtil.bytesToString(ref, vc.getStart() - 1, vc.getEnd() - vc.getStart() + 1);

                if (!refString.equalsIgnoreCase(allele.getBaseString())) {
+                    // consider that the ref and the alt may have been swapped in a simple biallelic SNP
+                    if (vc.getAlleles().size() == 2 &&
+                            vc.getAlleles().stream().map(Allele::length).allMatch(l -> l == 1) &&


This is weird. Why not use isBiallelic() and isSNP()?

because I was being too technical and forgot about these utility functions....

eitanbanks · 2017-11-09T14:16:18Z

src/main/java/picard/vcf/LiftoverVcf.java

+                    if (vc.getAlleles().size() == 2 &&
+                            vc.getAlleles().stream().map(Allele::length).allMatch(l -> l == 1) &&
+                            refString.equalsIgnoreCase(vc.getAlleles().stream().filter(Allele::isNonReference).findFirst().get().getBaseString())) {
+                        sorter.add(LiftoverUtils.swapRefAlt(vc,TAGS_TO_REVERSE, TAGS_TO_DROP));


I feel like there should be one more piece of logic here, but will give you the final say in overriding this.
If the original AF was 1.0 then this isn't really an allele swap; rather it's a reference fix and we should just remove the record (since no samples are polymorphic in the new reference).

So, you would like the VC to be dropped if it turns out to be monomorphic after the allele swapping? My sense is that a monomorphic variant is easily removed post-hoc but that it could be useful for some to see them before being removed...

Okay, so maybe we should keep it in but filter it? Filter could be e.g. ReferenceFix.

coveralls · 2017-11-12T22:10:27Z

Coverage decreased (-0.009%) to 77.175% when pulling 616e23c on yf_fix_mismatching_allele into 28a441a on master.

yfarjoun · 2017-11-18T18:48:30Z

Merging this as it was approved by @eitanbanks who only asked for minimal changes (which were done)

yfarjoun added 3 commits November 3, 2017 21:00

added code to fix mismatching reference, but there's a bug, since the…

33e2fce

… reference allele is taken from the reference file and the allele in the vcf is ignored....

tests are no longer blowing up...one failure remains, but I'm not sur…

f307b4b

…e it should be considered a failure...

- final tests fixed

2e16d48

- added option to reverse AF-like INFO annotations - added option to drop annotations that become invalid upon swapping (like MAX_AF)

pshapiro4broad reviewed Nov 8, 2017

View reviewed changes

- responding to review comments

9fd7512

yfarjoun requested a review from eitanbanks November 9, 2017 11:17

eitanbanks approved these changes Nov 9, 2017

View reviewed changes

yfarjoun added 2 commits November 11, 2017 15:30

-responding to review comments (1)

d13ea3b

- responding to reviewer's comments

616e23c

yfarjoun changed the title ~~Yf fix mismatching allele~~ Enable LiftOverVcf to emit a variant when the reference base has changed Nov 18, 2017

yfarjoun merged commit c6a75ed into master Nov 18, 2017

yfarjoun deleted the yf_fix_mismatching_allele branch November 18, 2017 18:50

yfarjoun mentioned this pull request Dec 28, 2017

liftovervcf does not change genotypes when allele swap happens between two reference builds #1040

Closed

yfarjoun restored the yf_fix_mismatching_allele branch August 11, 2020 16:23

yfarjoun deleted the yf_fix_mismatching_allele branch December 2, 2020 20:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable LiftOverVcf to emit a variant when the reference base has changed #971

Enable LiftOverVcf to emit a variant when the reference base has changed #971

yfarjoun commented Nov 8, 2017

coveralls commented Nov 8, 2017 •

edited

Loading

pshapiro4broad Nov 8, 2017

pshapiro4broad Nov 8, 2017

pshapiro4broad Nov 8, 2017

pshapiro4broad Nov 8, 2017

pshapiro4broad Nov 8, 2017

pshapiro4broad Nov 8, 2017

pshapiro4broad Nov 8, 2017

pshapiro4broad Nov 8, 2017

pshapiro4broad Nov 8, 2017

pshapiro4broad Nov 8, 2017

coveralls commented Nov 8, 2017 •

edited

Loading

eitanbanks left a comment

eitanbanks Nov 9, 2017

eitanbanks Nov 9, 2017

eitanbanks Nov 9, 2017

eitanbanks Nov 9, 2017

yfarjoun Nov 12, 2017

eitanbanks Nov 9, 2017

eitanbanks Nov 9, 2017

eitanbanks Nov 9, 2017

eitanbanks Nov 9, 2017

yfarjoun Nov 12, 2017

eitanbanks Nov 13, 2017

eitanbanks Nov 9, 2017

eitanbanks Nov 9, 2017

yfarjoun Nov 12, 2017

eitanbanks Nov 9, 2017

yfarjoun Nov 12, 2017

eitanbanks Nov 9, 2017

yfarjoun Nov 12, 2017

eitanbanks Nov 13, 2017

coveralls commented Nov 12, 2017 •

edited

Loading

yfarjoun commented Nov 18, 2017


		public static final String SWAPPED_ALLELES = "SwappedAlleles";

		public static Log log = Log.getInstance(LiftoverUtils.class);


		swappedBuilder.attribute(SWAPPED_ALLELES, true);

		// You need to get the baseString in order to forget who's reference and who's variant


		final Map<Allele, Allele> alleleMap = new HashMap<>();

		// The index of the allele changed from 1 to 0 and vice versa, but the bases remain the same.

		@@ -161,13 +164,14 @@
		*/
		public static final String ATTEMPTED_LOCUS = "AttemptedLocus";

Enable LiftOverVcf to emit a variant when the reference base has changed #971

Enable LiftOverVcf to emit a variant when the reference base has changed #971

Conversation

yfarjoun commented Nov 8, 2017

Description

This has been implemented for bi-allelic SNPs only.

Checklist (never delete this)

Content

Review

coveralls commented Nov 8, 2017 • 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

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

coveralls commented Nov 8, 2017 • edited Loading

eitanbanks 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

coveralls commented Nov 12, 2017 • edited Loading

yfarjoun commented Nov 18, 2017

coveralls commented Nov 8, 2017 •

edited

Loading

coveralls commented Nov 8, 2017 •

edited

Loading

coveralls commented Nov 12, 2017 •

edited

Loading