Use allele info in VariantContext comparisons for stable sorts #1593

clintval · 2022-02-14T00:47:33Z

TLDR: This PR will make coordinate VCF sorting in HTSJDK/Picard similar to bcftools.

I tried to use Picard's SortVcf for a VCF comparison task, but I had difficulty because it's output was non-stable based on the full primary information of a variant context (contig, start, and alleles). I finally chose the bcftools implementation because it has a slightly more-stable sorting method. However, I want to ensure future users of Picard don't hit the same issue I did, hence this PR.

Before I write unit tests, is this something you are willing to accept?

codecov-commenter · 2022-02-14T01:12:09Z

Codecov Report

Merging #1593 (2272c16) into master (b5af659) will decrease coverage by 0.064%.
The diff coverage is 0.000%.

❗ Current head 2272c16 differs from pull request most recent head 5089e22. Consider uploading reports for the commit 5089e22 to get more accurate results

@@               Coverage Diff               @@
##              master     #1593       +/-   ##
===============================================
- Coverage     69.839%   69.775%   -0.064%     
+ Complexity      9647      9641        -6     
===============================================
  Files            702       702               
  Lines          37638     37535      -103     
  Branches        6113      6080       -33     
===============================================
- Hits           26286     26190       -96     
+ Misses          8903      8888       -15     
- Partials        2449      2457        +8

Impacted Files	Coverage Δ
...riant/variantcontext/VariantContextComparator.java	`0.000% <0.000%> (ø)`
...htsjdk/samtools/util/nio/DeleteOnExitPathHook.java	`80.952% <0.000%> (-9.524%)`	⬇️
...sjdk/samtools/util/htsget/HtsgetErrorResponse.java	`73.333% <0.000%> (-6.667%)`	⬇️
...tools/cram/encoding/core/GammaIntegerEncoding.java	`86.667% <0.000%> (-6.667%)`	⬇️
.../samtools/cram/compression/ExternalCompressor.java	`69.565% <0.000%> (-4.509%)`	⬇️
...mtools/cram/encoding/core/BetaIntegerEncoding.java	`91.304% <0.000%> (-4.348%)`	⬇️
...am/encoding/core/CanonicalHuffmanByteEncoding.java	`52.941% <0.000%> (-2.941%)`	⬇️
...ing/core/huffmanUtils/HuffmanParamsCalculator.java	`80.000% <0.000%> (-1.818%)`	⬇️
...va/htsjdk/samtools/util/htsget/HtsgetResponse.java	`89.394% <0.000%> (-1.783%)`	⬇️
...java/htsjdk/beta/codecs/variants/vcf/VCFCodec.java	`81.818% <0.000%> (-1.515%)`	⬇️
... and 39 more

clintval · 2022-04-29T16:00:36Z

Any interest from the maintainers in this PR?

Anything I can do to help move it closer to merge?

clintval · 2022-09-13T20:03:05Z

@cmnbroad I hit another instance where this would have helped me.

Do you want me to invest the time to implement some unit tests, or is there no interest in pursuing this PR?

cmnbroad · 2022-09-15T13:47:16Z

@clintval Stable sounds good to me, but @lbergelson who is out on vacation really needs to weigh in (I'm not actually a maintainer).

lbergelson · 2022-09-21T20:09:54Z

@clintval I'm sorry I haven't been responsive. This is a good idea and having some tests would be great.

clintval · 2022-10-21T22:47:41Z

@lbergelson I don't normally write in Java so I hope my last commits are OK!

lbergelson

@clintval looks good to me! Thank you. I have a few minor things but I'll take them over and push my own patch since I've left you sitting for so long.

lbergelson · 2022-02-15T21:41:48Z

src/main/java/htsjdk/variant/variantcontext/VariantContextComparator.java

+		contigCompare = contigCompare == 0 ? firstVariantContext.getStart() - secondVariantContext.getStart() : contigCompare;
+		if (contigCompare == 0) {
+			// Compare variants that have the same genomic span (chr:start-end) lexicographically by all alleles (ref and alts).
+			for (int i = 0; i < firstVariantContext.getAlleles().size(); i++) {


This is silly but's probably better to use getNAlleles() instead of getAlleles().size().

lbergelson · 2022-02-15T21:44:13Z

src/main/java/htsjdk/variant/variantcontext/VariantContextComparator.java

+		contigCompare = contigCompare == 0 ? firstVariantContext.getStart() - secondVariantContext.getStart() : contigCompare;
+		if (contigCompare == 0) {
+			// Compare variants that have the same genomic span (chr:start-end) lexicographically by all alleles (ref and alts).
+			for (int i = 0; i < firstVariantContext.getAlleles().size(); i++) {


I don't think there's any guarantee that alleles are sorted in any order. I think it would be ideal to treat them as logically the same if the have the same SET of alleles. It's going to be pretty rare that we get to this tie breaker so we could probably afford to sort them before we compare.

lbergelson · 2022-02-15T21:45:38Z

src/main/java/htsjdk/variant/variantcontext/VariantContextComparator.java

+			// Compare variants that have the same genomic span (chr:start-end) lexicographically by all alleles (ref and alts).
+			for (int i = 0; i < firstVariantContext.getAlleles().size(); i++) {
+				if (i > secondVariantContext.getAlleles().size()) { return 1; }
+				contigCompare = firstVariantContext.getAlleles().get(i).compareTo(secondVariantContext.getAlleles().get(i));


instead of reusing the variable contig compare it might make more sense to name it "alleleCompare"

* doing some refactoring to use the Comparator.comparing API * adding a few more tests

* Responding to my own pr comments on #1593 * doing some refactoring to use the Comparator.comparing API * adding a few more tests

clintval · 2022-10-31T01:44:08Z

@lbergelson nice work and thank you kindly for the review!

#1593)" This reverts commit 14a8cc0. This introduced a serious issue that causes existing valid VCFs to fail as unsorted.

Use allele info in VariantContext comparisons for stable sorts

a79b421

clintval force-pushed the cv_stable_vcf_sort branch from c6f2029 to a79b421 Compare February 14, 2022 00:47

clintval changed the title ~~Use END and allele info in VariantContext comparisons for stable sorts~~ Use allele info in VariantContext comparisons for stable sorts Feb 14, 2022

clintval added 2 commits October 21, 2022 15:42

Ensure a more stable sort for varying length alleles, add unit tests

27627ab

Ensure we test when two variants are equal

5089e22

lbergelson approved these changes Oct 28, 2022

View reviewed changes

lbergelson merged commit 14a8cc0 into samtools:master Oct 28, 2022

lbergelson added a commit that referenced this pull request Oct 28, 2022

Responding to my own pr comments on #1593

289601b

* doing some refactoring to use the Comparator.comparing API * adding a few more tests

lbergelson added a commit that referenced this pull request Oct 28, 2022

Refactoring VariantContextComparator

f1e088b

* Responding to my own pr comments on #1593 * doing some refactoring to use the Comparator.comparing API * adding a few more tests

lbergelson mentioned this pull request Oct 28, 2022

Refactoring VariantContextComparator #1629

Open

clintval deleted the cv_stable_vcf_sort branch October 31, 2022 01:44

clintval mentioned this pull request Nov 16, 2022

Bump htsjdk to 3.0.3 fulcrumgenomics/fgbio#884

Closed

lbergelson added a commit that referenced this pull request Nov 23, 2022

Revert "Use allele info in VariantContext comparisons for stable sorts (

e2943b7

#1593)" This reverts commit 14a8cc0. This introduced a serious issue that causes existing valid VCFs to fail as unsorted.

lbergelson mentioned this pull request Nov 23, 2022

VCF sorting change breaks existing VCFs #1637

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use allele info in VariantContext comparisons for stable sorts #1593

Use allele info in VariantContext comparisons for stable sorts #1593

clintval commented Feb 14, 2022 •

edited

Loading

codecov-commenter commented Feb 14, 2022 •

edited

Loading

clintval commented Apr 29, 2022

clintval commented Sep 13, 2022

cmnbroad commented Sep 15, 2022 •

edited

Loading

lbergelson commented Sep 21, 2022

clintval commented Oct 21, 2022

lbergelson left a comment

lbergelson Feb 15, 2022

lbergelson Feb 15, 2022

lbergelson Feb 15, 2022

clintval commented Oct 31, 2022

Use allele info in VariantContext comparisons for stable sorts #1593

Use allele info in VariantContext comparisons for stable sorts #1593

Conversation

clintval commented Feb 14, 2022 • edited Loading

codecov-commenter commented Feb 14, 2022 • edited Loading

Codecov Report

clintval commented Apr 29, 2022

clintval commented Sep 13, 2022

cmnbroad commented Sep 15, 2022 • edited Loading

lbergelson commented Sep 21, 2022

clintval commented Oct 21, 2022

lbergelson left a comment

Choose a reason for hiding this comment

lbergelson Feb 15, 2022

Choose a reason for hiding this comment

lbergelson Feb 15, 2022

Choose a reason for hiding this comment

lbergelson Feb 15, 2022

Choose a reason for hiding this comment

clintval commented Oct 31, 2022

clintval commented Feb 14, 2022 •

edited

Loading

codecov-commenter commented Feb 14, 2022 •

edited

Loading

cmnbroad commented Sep 15, 2022 •

edited

Loading