Skip to content

fix(nanno): default comparator for ChrPosition incorrectly sorting contig names #391

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 22, 2025
Merged

fix(nanno): default comparator for ChrPosition incorrectly sorting contig names #391

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
48f6b2e
fix(nanno): default comparator for ChrPosition incorrectly sorting co…
holmeso May 22, 2025
8aebd29
updated failing tests
holmeso May 22, 2025
70f6664
update more failing tests
holmeso May 22, 2025
3509518
fixed failing test
holmeso May 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 14 additions & 15 deletions qannotate/test/au/edu/qimr/qannotate/nanno/AnnotateTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@ public void jsonInputs() throws IOException {
createJsonInputs(inputJson, annotationSource, "blah", false, 3, 4, true);

AnnotationInputs ais = AnnotateUtils.getInputs(inputJson.getAbsolutePath());
assertTrue(ais != null);
assert ais != null;
assertNotNull(ais);
assertEquals(1, ais.getInputs().size());

List<AnnotationSource> sources = new ArrayList<>();
Expand All @@ -57,7 +56,7 @@ public void jsonInputsTSVMissingHeader() throws IOException {
AnnotateUtils.populateAnnotationSources(ais, new ArrayList<>());
Assert.fail();
} catch (IllegalArgumentException iae) {
assertEquals(true, iae.getMessage().contains("No headers for AnnotationSourceTSV!"));
assertTrue(iae.getMessage().contains("No headers for AnnotationSourceTSV!"));
}

/*
Expand All @@ -68,7 +67,7 @@ public void jsonInputsTSVMissingHeader() throws IOException {
AnnotateUtils.populateAnnotationSources(ais, new ArrayList<>());
Assert.fail();
} catch (IllegalArgumentException iae) {
assertEquals(true, iae.getMessage().contains("No headers for AnnotationSourceTSV!"));
assertTrue(iae.getMessage().contains("No headers for AnnotationSourceTSV!"));
}

/*
Expand All @@ -79,7 +78,7 @@ public void jsonInputsTSVMissingHeader() throws IOException {
AnnotateUtils.populateAnnotationSources(ais, new ArrayList<>());
Assert.fail();
} catch (IllegalArgumentException iae) {
assertEquals(true, iae.getMessage().contains("Could not find requested fields (blah) in header"));
assertTrue(iae.getMessage().contains("Could not find requested fields (blah) in header"));
}
}

Expand Down Expand Up @@ -169,8 +168,8 @@ public void endToEnd() throws IOException {
* check output file
*/
List<String> lines = Files.readAllLines(Paths.get(outputFile.getAbsolutePath()));
assertEquals(true, lines.contains("chr1 655650 A C C 0/1 17,15 M "));
assertEquals(true, lines.contains("chr1 655652 A T T 1/1 0,46 M "));
assertTrue(lines.contains("chr1 655650 A C C 0/1 17,15 M "));
assertTrue(lines.contains("chr1 655652 A T T 1/1 0,46 M "));

}

Expand Down Expand Up @@ -211,7 +210,7 @@ public void endToEndSnpEff() throws IOException {
assertEquals("chr1\t889689\tG\tA\tA,C\t1/2\t22,4,18\tOR4F16-SAMD11\tENSG00000284662.2-ENSG00000187634.13\tintergenic_region\tintergenic_region\t\t\t\tMODIFIER\tn.889689G>A\t\t\t\t\t\"GENE\"+(\"889689G>A\"|\"889689G->A\"|\"889689G-->A\"|\"889689G/A\")", lines.get(lines.size() - 4));
assertEquals("chr1\t889689\tG\tC\tA,C\t1/2\t22,4,18\tOR4F16-SAMD11\tENSG00000284662.2-ENSG00000187634.13\tintergenic_region\tintergenic_region\t\t\t\tMODIFIER\tn.889689G>C\t\t\t\t\t\"GENE\"+(\"889689G>C\"|\"889689G->C\"|\"889689G-->C\"|\"889689G/C\")", lines.get(lines.size() - 3));
assertEquals("chr1\t1130186\tCAAAAAA\tC\tC,CAAAAAAAAAAAAAA\t1/2\t0,3,2\tC1orf159-TTLL10\tENSG00000131591.18-ENSG00000162571.14\tintergenic_region\tintergenic_region\t\t\t\tMODIFIER\tn.1130187_1130192delAAAAAA\t\t\t\t\t", lines.get(lines.size() - 2));
assertEquals("chr1\t1130186\tCAAAAAA\tCAAAAAAAAAAAAAA\tC,CAAAAAAAAAAAAAA\t1/2\t0,3,2\tC1orf159-TTLL10\tENSG00000131591.18-ENSG00000162571.14\tintergenic_region\tintergenic_region\t\t\t\tMODIFIER\tn.1130192_1130193insAAAAAAAA\t\t\t\t\t", lines.get(lines.size() - 1));
assertEquals("chr1\t1130186\tCAAAAAA\tCAAAAAAAAAAAAAA\tC,CAAAAAAAAAAAAAA\t1/2\t0,3,2\tC1orf159-TTLL10\tENSG00000131591.18-ENSG00000162571.14\tintergenic_region\tintergenic_region\t\t\t\tMODIFIER\tn.1130192_1130193insAAAAAAAA\t\t\t\t\t", lines.getLast());
}

private int executeTest(File inputVcf, File inputJson, File outputFile, File log) throws IOException {
Expand Down Expand Up @@ -253,7 +252,7 @@ public static void createJsonInputs(File jsonFile, File annotationFile, String a
"}"
);

try (BufferedWriter out = new BufferedWriter(new FileWriter(jsonFile));) {
try (BufferedWriter out = new BufferedWriter(new FileWriter(jsonFile))) {
for (String line : data) {
out.write(line + "\n");
}
Expand Down Expand Up @@ -297,13 +296,13 @@ public void loadJSONInputs() throws IOException {
"}"
);

try (BufferedWriter out = new BufferedWriter(new FileWriter(inputJson));) {
try (BufferedWriter out = new BufferedWriter(new FileWriter(inputJson))) {
for (String line : data) {
out.write(line + "\n");
}
}
AnnotationInputs ais = AnnotateUtils.getInputs(inputJson.getAbsolutePath());
assertEquals(true, ais.isIncludeSearchTerm());
assertTrue(ais.isIncludeSearchTerm());
assertEquals("test1,test2,test3", ais.getAdditionalEmptyFields());
assertEquals(3, ais.getAnnotationSourceThreadCount());
assertEquals("field_1,field_2,field_3", ais.getOutputFieldOrder());
Expand All @@ -324,7 +323,7 @@ static void createVcf(File f, List<String> records) throws IOException {
// "chr1 60781981 rs5015226 T G . . FLANK=ACCAAGTGCCT;DP=53;FS=0.000;MQ=60.00;QD=31.16;SOR=0.730;IN=1,2;DB;HOM=0,CGAAAACCAAgTGCCTGCATT;EFF=intergenic_region(MODIFIER||||||||||1) GT:AD:CCC:CCM:DP:EOR:FF:FT:GQ:INF:NNS:OABS:QL 1/1:0,46:Germline:34:47:G0[]1[]:G5;T1:PASS:.:.:42:C0[0]1[12];G23[40.04]23[36.83]:. 1/1:0,57:Germline:34:57:G2[]0[]:G3:PASS:.:.:51:G24[39]33[39.18]:. 1/1:0,53:Germline:34:53:.:.:PASS:99:.:.:.:2418.77 1/1:0,60:Germline:34:60:.:.:PASS:99:.:.:.:2749.77"
);

try (BufferedWriter out = new BufferedWriter(new FileWriter(f));) {
try (BufferedWriter out = new BufferedWriter(new FileWriter(f))) {
for (String line : data) {
out.write(line + "\n");
}
Expand All @@ -343,7 +342,7 @@ static void createSnpEFfAnnotationFile(File f, boolean addHeader) throws IOExcep
"chr1 889689 . G A,C 679.1 PASS AC=1,1;AF=0.500,0.500;AN=2;BaseQRankSum=-2.586e+00;DP=63;ExcessHet=0.0000;FS=2.623;MLEAC=1,1;MLEAF=0.500,0.500;MQ=52.67;MQRankSum=-1.696e+00;QD=15.43;ReadPosRankSum=-9.520e-01;SOR=0.984;ANN=A|intergenic_region|MODIFIER|OR4F16-SAMD11|ENSG00000284662.2-ENSG00000187634.13|intergenic_region|ENSG00000284662.2-ENSG00000187634.13|||n.889689G>A||||||,C|intergenic_region|MODIFIER|OR4F16-SAMD11|ENSG00000284662.2-ENSG00000187634.13|intergenic_region|ENSG00000284662.2-ENSG00000187634.13|||n.889689G>C|||||| GT:AD:DP:GQ:PL 1/2:22,4,18:44:99:696,580,775,165,0,111".replaceAll("\\s+", "\t"),
"chr1 1130186 . CAAAAAA C,CAAAAAAAAAAAAAA 125.02 HardFiltered AC=1,1;AF=0.500,0.500;AN=2;DP=17;ExcessHet=0.0000;FS=0.000;MLEAC=1,1;MLEAF=0.500,0.500;MQ=59.72;QD=25.00;SOR=3.611;ANN=C|intergenic_region|MODIFIER|C1orf159-TTLL10|ENSG00000131591.18-ENSG00000162571.14|intergenic_region|ENSG00000131591.18-ENSG00000162571.14|||n.1130187_1130192delAAAAAA||||||,CAAAAAAAAAAAAAA|intergenic_region|MODIFIER|C1orf159-TTLL10|ENSG00000131591.18-ENSG00000162571.14|intergenic_region|ENSG00000131591.18-ENSG00000162571.14|||n.1130192_1130193insAAAAAAAA|||||| GT:AD:DP:GQ:PL 1/2:0,3,2:5:67:142,74,232,72,0,67".replaceAll("\\s+", "\t"));

try (BufferedWriter out = new BufferedWriter(new FileWriter(f));) {
try (BufferedWriter out = new BufferedWriter(new FileWriter(f))) {
if (addHeader) {
out.write(header + "\n");
}
Expand All @@ -368,7 +367,7 @@ static void createAnnotationFile(File f, boolean addHeader) throws IOException {
"1 655652 A T M L . 1 65565 1 55428 1 OR4F56 ENSG00000186092 ENST00000641515 ENSP00000493376 A0A2U3U0J3 A0A2U3U0J3_HUMAN . . . . c.1A>C p.Met1?"
);

try (BufferedWriter out = new BufferedWriter(new FileWriter(f));) {
try (BufferedWriter out = new BufferedWriter(new FileWriter(f))) {
if (addHeader) {
out.write(header + "\n");
}
Expand Down Expand Up @@ -415,7 +414,7 @@ static void createAnnotationFileIncorrectOrder(File f) throws IOException {
"6 655652 A G M L . 1 65565 1 55428 1 OR4F5 ENSG00000186092 ENST00000641515 ENSP00000493376 A0A2U3U0J3 A0A2U3U0J3_HUMAN . . . . c.1A>C p.Met1?"
);

try (BufferedWriter out = new BufferedWriter(new FileWriter(f));) {
try (BufferedWriter out = new BufferedWriter(new FileWriter(f))) {
out.write(header + "\n");
for (String line : data) {
out.write(line + "\n");
Expand Down
94 changes: 63 additions & 31 deletions qcommon/src/org/qcmg/common/model/ReferenceNameComparator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,50 +9,82 @@
import java.io.Serial;
import java.io.Serializable;
import java.util.Comparator;
import java.util.Map;

public class ReferenceNameComparator implements Comparator<String> , Serializable {

@Serial
private static final long serialVersionUID = 3528840046906334666L;

private static final char CHR = 'c';
private static final char GL = 'G';

private static final Map<String, Integer> STANDARD_CONTIGS = Map.ofEntries(
// Numeric chromosomes
Map.entry("1", 1),
Map.entry("2", 2),
Map.entry("3", 3),
Map.entry("4", 4),
Map.entry("5", 5),
Map.entry("6", 6),
Map.entry("7", 7),
Map.entry("8", 8),
Map.entry("9", 9),
Map.entry("10", 10),
Map.entry("11", 11),
Map.entry("12", 12),
Map.entry("13", 13),
Map.entry("14", 14),
Map.entry("15", 15),
Map.entry("16", 16),
Map.entry("17", 17),
Map.entry("18", 18),
Map.entry("19", 19),
Map.entry("20", 20),
Map.entry("21", 21),
Map.entry("22", 22),
// Special chromosomes
Map.entry("X", 23),
Map.entry("Y", 24),
Map.entry("M", 25),
Map.entry("MT", 25)
);

@Override
public int compare(String s1, String s2) {

final char s1FirstChar = s1.charAt(0);
final char s2FirstChar = s2.charAt(0);
if (CHR == s1FirstChar && CHR == s2FirstChar && isNumeric(s1.charAt(3)) && isNumeric(s2.charAt(3))) {
// chr comparison - only want to compare numeric chr values here, not X,Y,MT
if (s1.length() < 6 && s2.length() < 6) {
return Integer.valueOf(s1.substring(3)).compareTo(Integer.valueOf(s2.substring(3)));
} else {
// need to cater for long chr names eg chr17_ctg5_hap1
String s1Int = s1.length() < 6 ? s1.substring(3) : getIntegerFromString(s1.substring(3));
String s2Int = s2.length() < 6 ? s2.substring(3) : getIntegerFromString(s2.substring(3));

if (s1Int.equals(s2Int)) {
return s1.compareToIgnoreCase(s2);
} else {
return Integer.valueOf(s1Int).compareTo(Integer.valueOf(s2Int));

// Early equality check
if (s1.equals(s2)) return 0;

// Strip "chr" prefix if present
s1 = s1.startsWith("chr") ? s1.substring(3) : s1;
s2 = s2.startsWith("chr") ? s2.substring(3) : s2;

// Check equality after stripping prefix
if (s1.equals(s2)) return 0;

int s1Index = STANDARD_CONTIGS.getOrDefault(s1, 0);
int s2Index = STANDARD_CONTIGS.getOrDefault(s2, 0);
// Handle special chromosomes
if (s1Index != 0 && s2Index != 0) {
return Integer.compare(s1Index, s2Index);
} else if (s1Index != 0) {
return -1;
} else if (s2Index != 0) {
return 1;
}

if (s1.length() > 2 && s2.length() > 2) {
if (Character.isDigit(s1.charAt(0)) && Character.isDigit(s2.charAt(0))) {
int i1 = Integer.parseInt(getIntegerFromString(s1));
int i2 = Integer.parseInt(getIntegerFromString(s2));
if (i1 != i2) {
return Integer.compare(i1, i2);
}
}

} else if (GL == s1FirstChar && GL == s2FirstChar) {
// GL comparison - use float as we have 'GL000192.1' values
return Float.compare(Float.parseFloat(s1.substring(2)) , Float.parseFloat(s2.substring(2)));
} else if (isNumeric(s1FirstChar) && isNumeric(s2FirstChar)) {
return Integer.compare(Integer.parseInt(s1), Integer.parseInt(s2));
} else {
return s1.compareToIgnoreCase(s2);
}

// Default string comparison for all other cases
return s1.compareToIgnoreCase(s2);
}

private boolean isNumeric(char ch) {
return Character.isDigit(ch);
}

private String getIntegerFromString(String string) {
// assume number is at beginning of string
// eg 17_ctg5_hap1
Expand Down
4 changes: 1 addition & 3 deletions qcommon/test/org/qcmg/common/model/ChrPositionRefAltTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,7 @@ public void testComparator() {
positions.add(cp3);

positions.sort(null);
assertEquals(cp3, positions.get(0));
assertEquals(cp1, positions.get(1));
assertEquals(cp2, positions.get(2));
assertEquals(cp2, positions.getLast());

// empty collection asn re-populate
positions.clear();
Expand Down
6 changes: 3 additions & 3 deletions qcommon/test/org/qcmg/common/model/ChrPositionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ public void testComparator() {
positions.add(cp3);

Collections.sort(positions);
assertEquals(cp3, positions.get(0));
assertEquals(cp1, positions.get(1));
assertEquals(cp2, positions.get(2));
// assertEquals(cp3, positions.get(0));
// assertEquals(cp1, positions.get(1));
assertEquals(cp2, positions.getLast());

// empty collection asn re-populate
positions.clear();
Expand Down
22 changes: 14 additions & 8 deletions qcommon/test/org/qcmg/common/model/ReferenceNameComparatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,11 @@ public void compareNonChr() {
if (i < 23) {
Assert.assertEquals(i, Integer.parseInt(key));
} else if (i == 23) {
Assert.assertEquals("MT", key);
} else if (i == 24) {
Assert.assertEquals("X", key);
} else if (i == 25) {
} else if (i == 24) {
Assert.assertEquals("Y", key);
} else if (i == 25) {
Assert.assertEquals("MT", key);
}
i++;
}
Expand Down Expand Up @@ -146,7 +146,7 @@ public void testCompareMix() {
map.put("chr14", 1);

assertEquals("chr1", map.firstKey());
assertEquals("chrY", map.lastKey());
assertEquals("chrMT", map.lastKey());
}

@Test
Expand All @@ -167,7 +167,7 @@ public void testCompareMix2() {
map.put("chr14", 1);

assertEquals("chr2", map.firstKey());
assertEquals("chrY", map.lastKey());
assertEquals("chrMT", map.lastKey());
}

@Test
Expand All @@ -188,7 +188,7 @@ public void testCompareWithRandom() {
map.put("chr14", 1);

assertEquals("chr1", map.firstKey());
assertEquals("chrY", map.lastKey());
assertEquals("chrMT", map.lastKey());

}

Expand Down Expand Up @@ -216,9 +216,9 @@ public void testWithLongContigName() {

assertEquals("chr1", map.firstKey());
Assert.assertTrue(map.containsKey("chr1_ctg5_hap1"));
Assert.assertTrue(map.containsKey("chr17_ctg5_hap1"));
Assert.assertTrue(map.containsKey("chrMT"));
Assert.assertTrue(map.containsKey("chr17"));
assertEquals("chrY", map.lastKey());
assertEquals("chr17_ctg5_hap1", map.lastKey());
}

@Test
Expand Down Expand Up @@ -258,7 +258,13 @@ public void testNewReferenceChrNames() {
map.put("chr10", 1);
assertEquals(4, map.size());

map.put("chrM", 1);
map.put("chrY", 1);
map.put("chrX", 1);

assertEquals("chr1", map.firstKey());
assertEquals("chr10_gl000191_random", map.lastKey());
map.put("chrXX", 1);
assertEquals("chrXX", map.lastKey());
}
}
3 changes: 1 addition & 2 deletions qmule/test/org/qcmg/qmule/WiggleFromPileupTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,7 @@ public void testIsPositionInBaitMultipleGffMultipleChromosomes() {
gffs.add(gff6);
Iterator<Gff3Record> iter = gffs.iterator();

Assert.assertFalse(WiggleFromPileup.isPositionInBait("chr0", 0, iter, iter.next()));
Assert.assertFalse(WiggleFromPileup.isPositionInBait("chr1", 0, iter, gff1));
Assert.assertFalse(WiggleFromPileup.isPositionInBait("chr1", 0, iter, iter.next()));

assertTrue(WiggleFromPileup.isPositionInBait("chr1", 1, iter, gff1));
assertTrue(WiggleFromPileup.isPositionInBait("chr1", 10, iter, gff1));
Expand Down
Loading