Fixes LiftoverVCF for indels (again) #1469

yfarjoun · 2020-02-15T20:22:04Z

included getStop() from rev'ed htsJDK.
fixes (again) LiftoverVCF + Badly formed variant context at location chr1:899903; getEnd() #1258 which was still broken due to the call to "make()" in the presence of a mismatching "END" attribute and "getEnd()" result.
modified a test that is fixed by this commit (checked that it failing prior to the change)

Thanks to the several users who reported this is still a problem, and sorry for ignoring you for a year. @nh13, @kirannarta, and @duartemolha!

Description

Give your PR a concise yet descriptive title
Please explain the changes you made here.
Explain the motivation for making this change. What existing problem does the pull request solve?
Mention any issues fixed, addressed or otherwise related to this pull request, including issue numbers or hard links for issues in other repos.
You can delete these instructions once you have written your PR description.

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

- fixes (again) #1258 which was still broken due to the call to "make()" in the presence of a mismatching "END" attribute and "getEnd()" result.

pshapiro4broad

Java code looks ok; can't say I fully understand the fix or the test though.

nh13 · 2020-02-18T16:48:35Z

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

+    public void testLiftOverIndels(final LiftOver liftOver, final ReferenceSequence reference, final VariantContext source, final VariantContext result) {
+
+        final Interval target = liftOver.liftOver(new Interval(source.getContig(), source.getStart(), source.getEnd()), 1);
+


nh13 · 2020-02-18T16:56:27Z

@yfarjoun I am going to see if I can make a simple test case for #1280 and see if this fixes it.

nh13 · 2020-02-18T17:19:47Z

@yfarjoun this fix does not solve my issue: #1280. I have a single-variant test case in that PR.

yfarjoun · 2020-02-18T20:36:17Z

@nh13 while this PR was not intended to fix your other PR, I found the cause and I think I fixed it...care to test? (and review the PR while you're at it...) 😄

yfarjoun · 2020-02-18T20:38:15Z

@pshapiro4broad the problem was that the old code called "build()" on a builder that was in an improper state, the end of the variant was not at the same place where the END attribute was (for a non symbolic variant), and this didn't validate.

The changes to the tests simply adds the END attribute so that this problem is triggered, indeed the modified tests failed before I made the change.

- Symbolic alleles are now lifted over as well, by simply carrying them along.

yfarjoun · 2020-02-19T22:54:49Z

@nh13 you inspired me to make this work on symbolic alleles as well...and then I also figured out that I can simplify the code around the liftover that involves reverse-complementing....anyhow, no good deed will go unpunished....want to review this?

nh13 · 2020-02-19T23:15:37Z

@yfarjoun I can review if it helps. I won't be able to look at it until this weekend given my current workload. Is that allright?

yfarjoun · 2020-02-20T07:53:26Z

Fine with me. Thanks!

…

On Thu, Feb 20, 2020 at 1:15 AM Nils Homer ***@***.***> wrote: @yfarjoun <https://github.com/yfarjoun> I can review if it helps. I won't be able to look at it until this weekend given my current workload. Is that allright? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1469?email_source=notifications&email_token=AAU6JUSMAUFOAKLSJ7KVOGTRDW4RVA5CNFSM4KV37CV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMKCSNY#issuecomment-588523831>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAU6JUREVBZEPQC4X56EFLDRDW4RVANCNFSM4KV37CVQ> .

chengdai · 2020-02-28T19:07:22Z

Any chance there has been an update to this? I'm encountering a similar error with the following genotype (test.b37.vcf.gz). For reference, I am trying to lift over genotypes from build 37 to build 38. I'm using the b37ToHg38 chain and GRCh38_full_analysis_set_plus_decoy_hla.fa found on the 1000 Genomes FTP

1       144609982       rs201830440     AATATATATATATAT A,<NON_REF>     788.73  .       DB;DP=28;ExcessHet=3.0103;MLEAC=1,1;MLEAF=0.5,0.5;RAW_MQ=68809;AC=1,1;AN=2      GT:AD:DP:GQ:PL:SB 1/2:0,19,0,0,0,0,0:19:2:824,41,194,783,0,780:0,0,9,10

The error message is:

[Fri Feb 28 19:05:05 UTC 2020] Executing as root@9823faf0b713 on Linux 4.14.154-128.181.amzn2.x86_64 amd64; OpenJDK 64-Bit Server VM 1.8.0_152-release-1056-b12; Deflater: Intel; Inflater: Intel; Provider GCS is not available; Picard version: 2.22.0-SNAPSHOT
INFO    2020-02-28 19:05:05     LiftoverVcf     Loading up the target reference genome.
INFO    2020-02-28 19:05:15     LiftoverVcf     Lifting variants over and sorting (not yet writing the output file.)
[Fri Feb 28 19:05:15 UTC 2020] picard.vcf.LiftoverVcf done. Elapsed time: 0.17 minutes.
Runtime.totalMemory()=6722945024
To get help, see http://broadinstitute.github.io/picard/index.html#GettingHelp
Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: -1
        at picard.util.LiftoverUtils.lambda$leftAlignVariant$4(LiftoverUtils.java:379)
        at java.util.stream.Collectors.lambda$groupingBy$45(Collectors.java:907)
        at java.util.stream.ReduceOps$3ReducingSink.accept(ReduceOps.java:169)
        at java.util.HashMap$ValueSpliterator.forEachRemaining(HashMap.java:1620)
        at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:481)
        at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:471)
        at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:708)
        at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
        at java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:499)
        at picard.util.LiftoverUtils.leftAlignVariant(LiftoverUtils.java:379)
        at picard.util.LiftoverUtils.reverseComplementVariantContext(LiftoverUtils.java:178)
        at picard.util.LiftoverUtils.liftVariant(LiftoverUtils.java:76)
        at picard.vcf.LiftoverVcf.doWork(LiftoverVcf.java:426)
        at picard.cmdline.CommandLineProgram.instanc
[test.b37.vcf.gz](https://github.com/broadinstitute/picard/files/4268648/test.b37.vcf.gz)
eMain(CommandLineProgram.java:305)
        at picard.cmdline.PicardCommandLine.instanceMain(PicardCommandLine.java:103)
        at picard.cmdline.PicardCommandLine.main(PicardCommandLine.java:113)

Thanks!

nh13 · 2020-02-28T20:09:51Z

Sorry, I'm extremely busy at work and haven't had time outside of work to look at this. Please do not wait on my review to hold this up, as I cannot commit to doing it in the next two weeks (work + vacation)

…ibute

pshapiro4broad

Looks OK to me, only some very minor formatting changes.

pshapiro4broad · 2020-02-20T18:16:16Z

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

+            return true;
+        }
+
+       return vc.getAlleles().stream().filter(a -> !a.isSymbolic()).filter(a -> !a.equals(Allele.SPAN_DEL)).


Not sure if the sun/oracle java code guidelines cover streaming operations yet, but I think it's more readable if each stream segment starts a new line:

return vc.getAlleles().stream() .filter(a -> !a.isSymbolic()) .filter(a -> !a.equals(Allele.SPAN_DEL)) .anyMatch(a -> a.length() != 1);

pshapiro4broad · 2020-04-03T14:40:27Z

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

@@ -365,7 +359,9 @@ protected static void leftAlignVariant(final VariantContextBuilder builder, fina

        // Put each allele into the alleleBasesMap unless it is a spanning deletion.
        // Spanning deletions are dealt with as a special case later in fixedAlleleMap.
-        alleles.stream().filter(a->!a.equals(Allele.SPAN_DEL)).forEach(a -> alleleBasesMap.put(a, a.getBases()));
+        alleles.stream()
+                .filter(a->!a.equals(Allele.SPAN_DEL)&&!a.isSymbolic())


whitespace

Suggested change

.filter(a->!a.equals(Allele.SPAN_DEL)&&!a.isSymbolic())

.filter(a -> !a.equals(Allele.SPAN_DEL) && !a.isSymbolic())

also, above you write filter(a -> !a.isSymbolic()).filter(a -> !a.equals(Allele.SPAN_DEL). The version with one call to filter() is probably clearer, either way I would use the same form in both places to be consistent.

pshapiro4broad · 2020-04-03T14:41:54Z

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

-
-        for (final Allele oldAllele : originalAlleles) {
-            alleles.add(LiftoverUtils.reverseComplement(oldAllele, target, refSeq, isIndel, addToStart));
+    private static boolean isIndelForLiftover(final VariantContext vc){


whitespace

Suggested change

private static boolean isIndelForLiftover(final VariantContext vc){

private static boolean isIndelForLiftover(final VariantContext vc) {

…ibute

- included getStop from rev'ed htsJDK.

1a2e5ee

- fixes (again) #1258 which was still broken due to the call to "make()" in the presence of a mismatching "END" attribute and "getEnd()" result.

yfarjoun requested a review from fleharty February 15, 2020 20:22

- typos

b9c6802

pshapiro4broad previously approved these changes Feb 18, 2020

View reviewed changes

nh13 reviewed Feb 18, 2020

View reviewed changes

- making Symbolic Alleles great again!

2366b46

yfarjoun dismissed pshapiro4broad’s stale review via 2366b46 February 18, 2020 20:27

nh13 mentioned this pull request Feb 18, 2020

Liftover gVCFs with indels #1280

Closed

Yossi Farjoun added 4 commits February 19, 2020 14:46

- WIP. problems with tests....

f93689d

- WIP. problems with tests....

828705b

- Greatly simplified liftover logic for indels.

34d6765

- Symbolic alleles are now lifted over as well, by simply carrying them along.

- cleanup

6f81b94

yfarjoun requested a review from nh13 March 25, 2020 19:30

Merge branch 'master' into yf_liftover_still_has_issues_with_END_attr…

cd95c01

…ibute

pshapiro4broad approved these changes Apr 3, 2020

View reviewed changes

Yossi Farjoun added 2 commits April 3, 2020 14:46

-whitespaces

f59b04e

Merge branch 'master' into yf_liftover_still_has_issues_with_END_attr…

6a1bdbe

…ibute

yfarjoun merged commit f20d6ec into master Apr 6, 2020

yfarjoun deleted the yf_liftover_still_has_issues_with_END_attribute branch April 6, 2020 18:14

Tintest mentioned this pull request Apr 7, 2020

LiftoverVCF doesn't seem to work with VCF from GATK4 Germline CNV caller #1495

Closed

1 task

karenfeng mentioned this pull request Jul 27, 2020

Variant Liftover exception projectglow/glow#261

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes LiftoverVCF for indels (again) #1469

Fixes LiftoverVCF for indels (again) #1469

yfarjoun commented Feb 15, 2020 •

edited

pshapiro4broad left a comment

nh13 Feb 18, 2020

nh13 commented Feb 18, 2020

nh13 commented Feb 18, 2020

yfarjoun commented Feb 18, 2020

yfarjoun commented Feb 18, 2020

yfarjoun commented Feb 19, 2020

nh13 commented Feb 19, 2020

yfarjoun commented Feb 20, 2020 via email

chengdai commented Feb 28, 2020 •

edited

nh13 commented Feb 28, 2020

pshapiro4broad left a comment

pshapiro4broad Feb 20, 2020

pshapiro4broad Apr 3, 2020

pshapiro4broad Apr 3, 2020

		public void testLiftOverIndels(final LiftOver liftOver, final ReferenceSequence reference, final VariantContext source, final VariantContext result) {

		final Interval target = liftOver.liftOver(new Interval(source.getContig(), source.getStart(), source.getEnd()), 1);

	.filter(a->!a.equals(Allele.SPAN_DEL)&&!a.isSymbolic())
	.filter(a -> !a.equals(Allele.SPAN_DEL) && !a.isSymbolic())

	private static boolean isIndelForLiftover(final VariantContext vc){
	private static boolean isIndelForLiftover(final VariantContext vc) {

Fixes LiftoverVCF for indels (again) #1469

Fixes LiftoverVCF for indels (again) #1469

Conversation

yfarjoun commented Feb 15, 2020 • edited

Description

Checklist (never delete this)

Content

Review

pshapiro4broad left a comment

Choose a reason for hiding this comment

nh13 Feb 18, 2020

Choose a reason for hiding this comment

nh13 commented Feb 18, 2020

nh13 commented Feb 18, 2020

yfarjoun commented Feb 18, 2020

yfarjoun commented Feb 18, 2020

yfarjoun commented Feb 19, 2020

nh13 commented Feb 19, 2020

yfarjoun commented Feb 20, 2020 via email

chengdai commented Feb 28, 2020 • edited

nh13 commented Feb 28, 2020

pshapiro4broad left a comment

Choose a reason for hiding this comment

pshapiro4broad Feb 20, 2020

Choose a reason for hiding this comment

pshapiro4broad Apr 3, 2020

Choose a reason for hiding this comment

pshapiro4broad Apr 3, 2020

Choose a reason for hiding this comment

yfarjoun commented Feb 15, 2020 •

edited

chengdai commented Feb 28, 2020 •

edited