From 40babcf1f128fc8d337aa191e873f6e1798bd166 Mon Sep 17 00:00:00 2001 From: Adam Kiezun Date: Mon, 27 Apr 2015 14:38:42 -0400 Subject: [PATCH] review comments --- .../org/broadinstitute/hellbender/Main.java | 4 +-- .../illumina/ExtractIlluminaBarcodes.java | 32 +++++++++---------- .../illumina/ExtractIlluminaBarcodesTest.java | 20 +++++------- 3 files changed, 25 insertions(+), 31 deletions(-) diff --git a/src/main/java/org/broadinstitute/hellbender/Main.java b/src/main/java/org/broadinstitute/hellbender/Main.java index 840268c76e4..42e85ce06ed 100644 --- a/src/main/java/org/broadinstitute/hellbender/Main.java +++ b/src/main/java/org/broadinstitute/hellbender/Main.java @@ -45,12 +45,12 @@ public class Main { /** * exit value when an unrecoverable {@link UserException} occurs */ - private static final int USER_EXCEPTION_EXIT_VALUE = 2; + public static final int USER_EXCEPTION_EXIT_VALUE = 2; /** * exit value when any unrecoverable exception other than {@link UserException} occurs */ - private static final int ANY_OTHER_EXCEPTION_EXIT_VALUE = 1; + public static final int ANY_OTHER_EXCEPTION_EXIT_VALUE = 1; /** * The packages we wish to include in our command line * diff --git a/src/main/java/org/broadinstitute/hellbender/tools/picard/illumina/ExtractIlluminaBarcodes.java b/src/main/java/org/broadinstitute/hellbender/tools/picard/illumina/ExtractIlluminaBarcodes.java index db16c4d8e38..6674a7140c3 100644 --- a/src/main/java/org/broadinstitute/hellbender/tools/picard/illumina/ExtractIlluminaBarcodes.java +++ b/src/main/java/org/broadinstitute/hellbender/tools/picard/illumina/ExtractIlluminaBarcodes.java @@ -1,5 +1,6 @@ package org.broadinstitute.hellbender.tools.picard.illumina; +import htsjdk.samtools.SAMException; import htsjdk.samtools.metrics.MetricBase; import htsjdk.samtools.metrics.MetricsFile; import htsjdk.samtools.util.Log; @@ -7,12 +8,14 @@ import org.broadinstitute.hellbender.cmdline.CommandLineProgramProperties; import org.broadinstitute.hellbender.cmdline.PicardCommandLineProgram; import org.broadinstitute.hellbender.cmdline.programgroups.IlluminaProgramGroup; +import org.broadinstitute.hellbender.exceptions.UserException; import org.broadinstitute.hellbender.tools.picard.illumina.parser.*; import org.broadinstitute.hellbender.tools.picard.illumina.parser.readers.BclQualityEvaluationStrategy; import org.broadinstitute.hellbender.utils.text.parsers.TabbedTextFileWithHeaderParser; import java.io.BufferedWriter; import java.io.File; +import java.io.IOException; import java.text.NumberFormat; import java.util.*; @@ -161,18 +164,22 @@ protected Object doWork() { ); extractors.add(extractor); } - extractors.forEach(ExtractIlluminaBarcodes.PerTileBarcodeExtractor::run); - + for (PerTileBarcodeExtractor perTileBarcodeExtractor: extractors){ + try { + perTileBarcodeExtractor.run(); + } catch (IOException | SAMException e) { + //Note: HTSJDK wraps IOExceptions in SAMExceptions which causes us to have to catch both kinds and report as UserExceptions. + LOG.error("Abandoning metrics calculation because one or more PerTileBarcodeExtractors failed."); + throw new UserException("Abandoning metrics calculation because one or more PerTileBarcodeExtractors failed.", e); + } + } LOG.info("Processed " + extractors.size() + " tiles."); + for (final PerTileBarcodeExtractor extractor : extractors) { for (final String key : barcodeToMetrics.keySet()) { barcodeToMetrics.get(key).merge(extractor.getMetrics().get(key)); } noMatchMetric.merge(extractor.getNoMatchMetric()); - if (extractor.getException() != null) { - LOG.error("Abandoning metrics calculation because one or more PerTileBarcodeExtractors failed."); - return 4; - } } // Finish metrics tallying. @@ -467,7 +474,6 @@ private static class PerTileBarcodeExtractor { private final File barcodeFile; private final Map metrics; private final BarcodeMetric noMatch; - private Exception exception = null; private final boolean usingQualityScores; private final IlluminaDataProvider provider; private final ReadStructure outputReadStructure; @@ -528,14 +534,10 @@ public BarcodeMetric getNoMatchMetric() { return this.noMatch; } - public Exception getException() { - return this.exception; - } - /** * run method which extracts barcodes and accumulates metrics for an entire tile */ - public void run() { + public void run() throws IOException { try { LOG.info("Extracting barcodes for tile " + tile); @@ -561,14 +563,10 @@ public void run() { for (final byte[] bc : barcodeSubsequences) { writer.write(bytesToString(bc)); } - writer.write("\t" + yOrN + "\t" + match.barcode + "\t" + valueOf(match.mismatches) + - "\t" + valueOf(match.mismatchesToSecondBest)); + writer.write("\t" + yOrN + "\t" + match.barcode + "\t" + valueOf(match.mismatches) + "\t" + valueOf(match.mismatchesToSecondBest)); writer.newLine(); } } - } catch (final Exception e) { - LOG.error(e, "Error processing tile ", this.tile); - this.exception = e; } finally { provider.close(); } diff --git a/src/test/java/org/broadinstitute/hellbender/tools/picard/illumina/ExtractIlluminaBarcodesTest.java b/src/test/java/org/broadinstitute/hellbender/tools/picard/illumina/ExtractIlluminaBarcodesTest.java index db654a7061e..35aa085feb0 100644 --- a/src/test/java/org/broadinstitute/hellbender/tools/picard/illumina/ExtractIlluminaBarcodesTest.java +++ b/src/test/java/org/broadinstitute/hellbender/tools/picard/illumina/ExtractIlluminaBarcodesTest.java @@ -1,16 +1,15 @@ package org.broadinstitute.hellbender.tools.picard.illumina; import htsjdk.samtools.metrics.MetricsFile; -import htsjdk.samtools.util.IOUtil; import org.broadinstitute.hellbender.CommandLineProgramTest; +import org.broadinstitute.hellbender.Main; +import org.broadinstitute.hellbender.exceptions.UserException; import org.broadinstitute.hellbender.tools.picard.illumina.parser.ClusterData; import org.broadinstitute.hellbender.tools.picard.illumina.parser.IlluminaDataProvider; import org.broadinstitute.hellbender.tools.picard.illumina.parser.IlluminaDataProviderFactory; -import org.broadinstitute.hellbender.tools.picard.illumina.parser.IlluminaDataType; import org.broadinstitute.hellbender.tools.picard.illumina.parser.ReadStructure; import org.broadinstitute.hellbender.tools.picard.illumina.parser.readers.BclQualityEvaluationStrategy; import org.broadinstitute.hellbender.utils.text.parsers.BasicInputParser; -import org.testng.Assert; import org.testng.annotations.AfterTest; import org.testng.annotations.BeforeTest; import org.testng.annotations.DataProvider; @@ -18,16 +17,13 @@ import java.io.File; import java.io.FileReader; -import java.util.*; +import java.util.ArrayList; +import java.util.List; -import static htsjdk.samtools.util.IOUtil.copyDirectoryTree; -import static htsjdk.samtools.util.IOUtil.deleteDirectoryTree; -import static htsjdk.samtools.util.IOUtil.getFilesMatchingRegexp; +import static htsjdk.samtools.util.IOUtil.*; import static java.util.Arrays.asList; import static java.util.Arrays.sort; -import static org.broadinstitute.hellbender.tools.picard.illumina.parser.IlluminaDataType.Barcodes; -import static org.broadinstitute.hellbender.tools.picard.illumina.parser.IlluminaDataType.BaseCalls; -import static org.broadinstitute.hellbender.tools.picard.illumina.parser.IlluminaDataType.QualityScores; +import static org.broadinstitute.hellbender.tools.picard.illumina.parser.IlluminaDataType.*; import static org.broadinstitute.hellbender.tools.picard.illumina.parser.readers.BclQualityEvaluationStrategy.ILLUMINA_ALLEGED_MINIMUM_QUALITY; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertTrue; @@ -109,7 +105,7 @@ public void testPairedEndWithBarcodeOnSecondEnd() throws Exception { assertEquals(metricsFile.getMetrics().get(12).PERFECT_MATCHES, 1); } - @Test + @Test(expectedExceptions = UserException.class) public void testNonWritableOutputFile() throws Exception { final File existingFile = new File(basecallsDir, "s_1_1101_barcode.txt.gz"); try { @@ -131,7 +127,7 @@ public void testNonWritableOutputFile() throws Exception { args.add("--BARCODE"); args.add(barcode); } - assertEquals(runCommandLine(args), 4); + assertEquals(runCommandLine(args), Main.ANY_OTHER_EXCEPTION_EXIT_VALUE); } finally { existingFile.setWritable(true); }