Skip to content
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

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

Merged
merged 5 commits into from
Jul 6, 2018
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ff698ab
handling lack of sequence dictionary exception
kachulis Jun 21, 2018
c13a20b
some clean up of LiftoverVcf PR
kachulis Jul 5, 2018
a4d3c8f
LiftoverVcfTest changes
kachulis Jul 5, 2018
3e4a680
more cleanup
kachulis Jul 5, 2018
702e383
some more cleanup
kachulis Jul 5, 2018
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
5 changes: 5 additions & 0 deletions src/main/java/picard/vcf/LiftoverVcf.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Copy link
Member

@pshapiro4broad pshapiro4broad Jul 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return 1;
}
for (final SAMSequenceRecord rec : walker.getSequenceDictionary().getSequences()) {
refSeqs.put(rec.getSequenceName(), walker.get(rec.getSequenceIndex()));
}
Expand Down
51 changes: 50 additions & 1 deletion src/test/java/picard/util/LiftoverVcfTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@
import htsjdk.samtools.util.Interval;
import htsjdk.variant.variantcontext.*;
import htsjdk.variant.vcf.VCFFileReader;
import htsjdk.samtools.SAMException;
import org.testng.Assert;
import org.testng.annotations.AfterClass;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;
import picard.PicardException;
import picard.cmdline.CommandLineProgramTest;
import picard.vcf.LiftoverVcf;
import picard.vcf.VcfTestUtils;
Expand All @@ -21,7 +23,9 @@
import java.io.File;
import java.io.IOException;
import java.util.*;

import java.nio.file.Path;
import java.nio.file.Files;
import java.nio.file.StandardCopyOption;
/**
* Test class for LiftoverVcf.
* <p>
Expand Down Expand Up @@ -1169,4 +1173,49 @@ public void testLiftOverNoCallAndSymbolic(final LiftOver liftOver, final Variant
Assert.assertEquals(resultAlleles, result.getAttributeAsStringList(LiftoverVcf.ORIGINAL_ALLELES, null));
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe not....nevermind.

liftOutput.toFile().deleteOnExit();
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);
final String[] args = new String[]{
"INPUT=" + input.toString(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"OUTPUT=" + liftOutput.toString(),
"REJECT=" + rejectOutput.toString(),
"CHAIN=" + CHAIN_FILE,
"REFERENCE_SEQUENCE=" + referenceCopy.toString(),
};
Assert.assertEquals(runPicardCommandLine(args), 1);
}

@Test(expectedExceptions = SAMException.class, expectedExceptionsMessageRegExp = "File exists but is not readable:.*")
public void testUnreadableDictionary() throws IOException {
final Path liftOutput = Files.createTempFile("tmpouput", ".vcf");
liftOutput.toFile().deleteOnExit();
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overwriting a temporary file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, that's better, thanks.

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);
dictCopy.toFile().deleteOnExit();
dictCopy.toFile().setReadable(false);
final String[] args = new String[]{
"INPUT=" + input.toString(),
"OUTPUT=" + liftOutput.toString(),
"REJECT=" + rejectOutput.toString(),
"CHAIN=" + CHAIN_FILE,
"REFERENCE_SEQUENCE=" + referenceCopy.toString(),
};
runPicardCommandLine(args);
}
}