Adding error in LiftoverVcf if reference dictionary does not exist (Issue #1157) #1189

kachulis · 2018-06-22T15:47:11Z

Description

In LiftoverVCF, checking if dictionary associated with reference .fast exists. If not, logging error and returning 1 from doWork().
Fixes Issue not having a .dict file next to the .fasta reference causes java.lang.NullPointerException #1157
Adding tests for nonexistent dictionary, and non-readable dictionary

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

Adding error in LiftoverVcf if reference dictionary does not exist Adding error in LiftoverVcf if reference dictionary does not exist Adding error in LiftoverVcf if reference dictionary does not exist

coveralls · 2018-06-22T16:17:39Z

Coverage increased (+0.3%) to 80.338% when pulling 702e383 on ck_dict_fasta_requirement_LiftoverVcf_1157 into b02e42e on master.

yfarjoun · 2018-07-04T19:51:07Z

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

+                "CHAIN=" + CHAIN_FILE,
+                "REFERENCE_SEQUENCE=" + REFERENCE_FILE,
+        };
+        Path dictionary = REFERENCE_FILE.toPath().resolveSibling(REFERENCE_FILE.getName().replace("fasta", "dict"));


final and use another variable for the moved dictionary

yfarjoun · 2018-07-04T19:51:49Z

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

+        dictionary = REFERENCE_FILE.toPath().resolveSibling(REFERENCE_FILE.getName().replace("fasta", "dict.tmp"));
+        try {
+            Assert.assertEquals(runPicardCommandLine(args), 1);
+        } finally {//return dictionary name to previous


space between // and comment for readability

yfarjoun · 2018-07-04T19:53:52Z

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

+                "REFERENCE_SEQUENCE=" + REFERENCE_FILE,
+        };
+        Path dictionary = REFERENCE_FILE.toPath().resolveSibling(REFERENCE_FILE.getName().replace("fasta", "dict"));
+        dictionary.toFile().renameTo(new File(dictionary.toString().replace("dict", "dict.tmp")));


this might fail (e.g. if read only). check return code?

grab new File(dictionary.toString().replace("dict", "dict.tmp") in a Path variable and use it here and below in 1196. the code will be clearer that way.

yfarjoun · 2018-07-04T19:57:56Z

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

@@ -284,6 +284,11 @@ protected int doWork() {
        log.info("Loading up the target reference genome.");
        final ReferenceSequenceFileWalker walker = new ReferenceSequenceFileWalker(REFERENCE_SEQUENCE);
        final Map<String, ReferenceSequence> refSeqs = new HashMap<>();
+        //check if sequence dictionary exists


space between // and comment for readability

yfarjoun · 2018-07-04T19:59:23Z

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

+                "CHAIN=" + CHAIN_FILE,
+                "REFERENCE_SEQUENCE=" + REFERENCE_FILE,
+        };
+        Path dictionary = REFERENCE_FILE.toPath().resolveSibling(REFERENCE_FILE.getName().replace("fasta", "dict"));


consider the case that the file has fasta in the part before the extension...safer to use replaceAll with a .fasta$ and replace it with .dict to be sure that only the extension is swapped.

yfarjoun · 2018-07-04T20:23:23Z

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

+        dictionary.toFile().setReadable(false);
+        try {
+            runPicardCommandLine(args);
+        } finally {//return dictionary to readable


yfarjoun · 2018-07-04T20:23:47Z

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

+                "CHAIN=" + CHAIN_FILE,
+                "REFERENCE_SEQUENCE=" + REFERENCE_FILE,
+        };
+        final Path dictionary = REFERENCE_FILE.toPath().resolveSibling(REFERENCE_FILE.getName().replace("fasta", "dict"));


see replaceAll comment from above

yfarjoun · 2018-07-05T14:20:11Z

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

@@ -1211,11 +1208,11 @@ public void testUnreadableDictionary() throws IOException {
                "CHAIN=" + CHAIN_FILE,
                "REFERENCE_SEQUENCE=" + REFERENCE_FILE,
        };
-        final Path dictionary = REFERENCE_FILE.toPath().resolveSibling(REFERENCE_FILE.getName().replace("fasta", "dict"));
+        final Path dictionary = REFERENCE_FILE.toPath().resolveSibling(REFERENCE_FILE.getName().replaceAll("fasta$", "dict"));


I like the solution you did above (of copying the data you need) much better than modifying the test files and then changing them back in a finally clause...could you also do it here?

kachulis · 2018-07-05T14:22:57Z

Sure, will do

…

On Thu, Jul 5, 2018 at 10:20 AM Yossi Farjoun ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/test/java/picard/util/LiftoverVcfTest.java <#1189 (comment)> : > @@ -1211,11 +1208,11 @@ public void testUnreadableDictionary() throws IOException { "CHAIN=" + CHAIN_FILE, "REFERENCE_SEQUENCE=" + REFERENCE_FILE, }; - final Path dictionary = REFERENCE_FILE.toPath().resolveSibling(REFERENCE_FILE.getName().replace("fasta", "dict")); + final Path dictionary = REFERENCE_FILE.toPath().resolveSibling(REFERENCE_FILE.getName().replaceAll("fasta$", "dict")); I like the solution you did above (of copying the data you need) much better than modifying the test files and then changing them back in a finally clause...could you also do it here? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1189 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AmE7MLXgCA76QdzZAaYoqrTRIGmBSPVbks5uDiCjgaJpZM4U0Bvg> .

yfarjoun · 2018-07-05T15:20:23Z

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

@@ -1201,19 +1201,21 @@ public void testUnreadableDictionary() throws IOException {
        final Path rejectOutput = Files.createTempFile("tmpreject", ".vcf");
        rejectOutput.toFile().deleteOnExit();
        final Path input = TEST_DATA_PATH.toPath().resolve("testLiftoverBiallelicIndels.vcf");
+        final Path referenceCopy = Files.createTempFile("ref", ".fasta");
+        referenceCopy.toFile().deleteOnExit();
+        Files.copy(REFERENCE_FILE.toPath(), referenceCopy, StandardCopyOption.REPLACE_EXISTING);


overwriting a temporary file?

yeah, once Files.createTempFile is called the file exists and Files.copy will need to overwrite. I can't really find a good way to get a temp path for the reference copy without creating the file; I could create a helper temp file and then resolveSibling on the helper to get the reference copy, or use Paths.get on an absolute path into the java tmp directory, but neither of those options seems better than just allowing the copy to overwrite.

I think what you want to do is use Files.createTempDirectory() to create the temp directory and then Path.resolve() on it to create your new temp file path.

I agree, that's better, thanks.

yfarjoun · 2018-07-05T15:20:41Z

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

+        Files.copy(REFERENCE_FILE.toPath(), referenceCopy, StandardCopyOption.REPLACE_EXISTING);
+        final Path dictCopy = referenceCopy.resolveSibling(referenceCopy.toFile().getName().replaceAll("fasta$", "dict"));
+        final Path dictionary = REFERENCE_FILE.toPath().resolveSibling(REFERENCE_FILE.getName().replaceAll("fasta$", "dict"));
+        Files.copy(dictionary, dictCopy, StandardCopyOption.REPLACE_EXISTING);


better to fail if the copy is about to overwrite.

yfarjoun · 2018-07-05T15:22:06Z

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

+        Assert.assertEquals(runPicardCommandLine(args), 1);
+    }
+
+    @Test(expectedExceptions = SAMException.class)


is this the most specific exception that you can catch?

As far as class, yes, this actually gets thrown out of the AbstractFastaSequenceFile constructor, calling IOUtil.assertFileIsReadable, which throws a SAMException. but I can add an expectedExceptionMessageRegExp to the test.

pshapiro4broad · 2018-07-05T19:06:50Z

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

@@ -284,6 +284,11 @@ protected int doWork() {
        log.info("Loading up the target reference genome.");
        final ReferenceSequenceFileWalker walker = new ReferenceSequenceFileWalker(REFERENCE_SEQUENCE);
        final Map<String, ReferenceSequence> refSeqs = new HashMap<>();
+        // check if sequence dictionary exists
+        if (walker.getSequenceDictionary() == null) {
+            log.error("Reference " + REFERENCE_SEQUENCE.getAbsolutePath() + " must have associated Dictionary .dict file in same directory");


May as well write this using a complete sentence, e.g. Reference x must have an associated Dictionary .dict file in the same directory.

pshapiro4broad · 2018-07-05T19:09:51Z

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

+        referenceCopy.toFile().deleteOnExit();
+        Files.copy(REFERENCE_FILE.toPath(), referenceCopy, StandardCopyOption.REPLACE_EXISTING);
+        final String[] args = new String[]{
+                "INPUT=" + input.toString(),


all calls to toString() here and below are redundant and can be removed

yfarjoun · 2018-07-06T03:06:20Z

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

+
+    @Test()
+    public void testNoDictionary() throws IOException {
+        final Path liftOutput = Files.createTempFile("tmpouput", ".vcf");


with all these deleteOnExit, I wonder if it's cleaner to have a
try{
....
} finally {
TestUtil.recursiveDelete(...);
}

maybe not....nevermind.

yfarjoun

I'm happy with this...

yfarjoun · 2018-07-06T13:47:44Z

thanks @kachulis. you can delete your branch now.

sooheelee · 2018-09-21T13:49:37Z

Hi @kachulis, would it be possible to improve the error messaging for when the dictionary is missing? We have a researcher who pointed this out at https://gatkforums.broadinstitute.org/gatk/discussion/comment/52324#Comment_52324. Currently the error message is uninformative: java.lang.NullPointerException.

kachulis · 2018-09-21T15:14:44Z

Hi @sooheelee, the messaging is already fixed in Picard, but wasn't yet part of gatk until broadinstitute/gatk#5173 (upgrading the picard version from 2.18.7 to 2.18.13). It looks like the user was using v4.0.8.1 of gatk, this fix didn't make it into a gatk release until yesterday with v4.0.9.0.

handling lack of sequence dictionary exception

ff698ab

Adding error in LiftoverVcf if reference dictionary does not exist Adding error in LiftoverVcf if reference dictionary does not exist Adding error in LiftoverVcf if reference dictionary does not exist

yfarjoun suggested changes Jul 4, 2018

View reviewed changes

some clean up of LiftoverVcf PR

c13a20b

yfarjoun reviewed Jul 5, 2018

View reviewed changes

LiftoverVcfTest changes

a4d3c8f

yfarjoun reviewed Jul 5, 2018

View reviewed changes

more cleanup

3e4a680

pshapiro4broad reviewed Jul 5, 2018

View reviewed changes

some more cleanup

702e383

yfarjoun reviewed Jul 6, 2018

View reviewed changes

yfarjoun approved these changes Jul 6, 2018

View reviewed changes

yfarjoun merged commit e0bb690 into master Jul 6, 2018

kachulis deleted the ck_dict_fasta_requirement_LiftoverVcf_1157 branch July 6, 2018 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding error in LiftoverVcf if reference dictionary does not exist (Issue #1157) #1189

Adding error in LiftoverVcf if reference dictionary does not exist (Issue #1157) #1189

kachulis commented Jun 22, 2018

coveralls commented Jun 22, 2018 •

edited

Loading

yfarjoun Jul 4, 2018

yfarjoun Jul 4, 2018

yfarjoun Jul 4, 2018

yfarjoun Jul 4, 2018

yfarjoun Jul 4, 2018

yfarjoun Jul 4, 2018

yfarjoun Jul 4, 2018

yfarjoun Jul 4, 2018

yfarjoun Jul 5, 2018

kachulis commented Jul 5, 2018 via email

yfarjoun Jul 5, 2018

kachulis Jul 5, 2018

pshapiro4broad Jul 5, 2018

kachulis Jul 5, 2018

yfarjoun Jul 5, 2018

yfarjoun Jul 5, 2018

kachulis Jul 5, 2018

pshapiro4broad Jul 5, 2018 •

edited

Loading

pshapiro4broad Jul 5, 2018

yfarjoun Jul 6, 2018

yfarjoun Jul 6, 2018

yfarjoun left a comment

yfarjoun commented Jul 6, 2018

sooheelee commented Sep 21, 2018 •

edited

Loading

kachulis commented Sep 21, 2018

Adding error in LiftoverVcf if reference dictionary does not exist (Issue #1157) #1189

Adding error in LiftoverVcf if reference dictionary does not exist (Issue #1157) #1189

Conversation

kachulis commented Jun 22, 2018

Description

Checklist (never delete this)

Content

Review

coveralls commented Jun 22, 2018 • 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

kachulis commented Jul 5, 2018 via email

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

pshapiro4broad Jul 5, 2018 • 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

yfarjoun left a comment

Choose a reason for hiding this comment

yfarjoun commented Jul 6, 2018

sooheelee commented Sep 21, 2018 • edited Loading

kachulis commented Sep 21, 2018

coveralls commented Jun 22, 2018 •

edited

Loading

pshapiro4broad Jul 5, 2018 •

edited

Loading

sooheelee commented Sep 21, 2018 •

edited

Loading