Several changes to metrics collections (AlignmentSummaryMetrics and WgsMetrics) + some fluff #1555

yfarjoun · 2020-07-31T21:04:26Z

Added Clips (Soft and Hard) to AlignmentSummaryMetrics
Added read-length histogram and pdf output to AlignmentSummaryMetrics
Added Tests
Added a TEMP_OUTPUT_DIR to CommandLineProgramTest.java which is deleted once the derived class is done testing
Modified Two test classes to use TEMP_OUTPUT_DIR
Modified enum in CollectMultipleMetrics so that it will display what the options are and what each program does in the help.

gbggrant

LGTM

pshapiro4broad

Not done yet but wanted to submit my comments so far

pshapiro4broad · 2020-08-03T14:49:00Z

src/test/java/picard/cmdline/CommandLineProgramTest.java

+            TEMP_OUTPUT_DIR.delete();
+            TEMP_OUTPUT_DIR.mkdir();
+        } catch (IOException e) {
+            throw new PicardException("Couldn't create temp directory") ;


Is there any reason to not use RuntimeException() here?

Well...PicardException is a RuntimeException...but this was it's more clear that it comes from Picard...I don't mind either way....what's the argument for RuntimeException?

pshapiro4broad · 2020-08-03T14:51:08Z

src/test/java/picard/cmdline/CommandLineProgramTest.java

@@ -15,6 +20,24 @@
    public static final File CHR_M_REFERENCE = new File(REFERENCE_TEST_DIR,"chrM.reference.fasta");
    public static final File CHR_M_DICT = new File(REFERENCE_TEST_DIR,"chrM.reference.dict");

+
+    // A per-test-class directory that will be deleted after the tests are complete.


This is OK, an improvement would be to make this lazy, as not every test needs to create temp output. E.g.,

protected File getTempDir() { if (TEMP_OUTPUT_DIR == null) { ... }

pshapiro4broad · 2020-08-03T15:06:23Z

src/test/java/picard/cmdline/CommandLineProgramTest.java

+    final protected File TEMP_OUTPUT_DIR;
+    {
+        try {
+            TEMP_OUTPUT_DIR = File.createTempFile(FileUtils.getTempDirectory().getAbsolutePath(),this.getClass().getSimpleName());


Why not

TEMP_OUTPUT_DIR = Files.createTempDirectory(getClass().getName()).toFile();

pshapiro4broad · 2020-08-03T15:14:45Z

src/test/java/picard/analysis/CollectAlignmentSummaryMetricsTest.java

@@ -51,8 +55,7 @@ public String getCommandLineProgramName() {
    public void test() throws IOException {
        final File input = new File(TEST_DATA_DIR, "summary_alignment_stats_test.sam");
        final File reference = new File(TEST_DATA_DIR, "summary_alignment_stats_test.fasta");
-        final File outfile = File.createTempFile("alignmentMetrics", ".txt");
-        outfile.deleteOnExit();
+        final File outfile = File.createTempFile("test", ".txt", TEMP_OUTPUT_DIR);


It looks like this is a common case. You could add createTempFile to CommandLineProgramTest to avoid some typing.

not much less typing...but OK.

pshapiro4broad · 2020-08-03T15:19:26Z

src/test/java/picard/analysis/CollectAlignmentSummaryMetricsTest.java

+    @DataProvider()
+    Object[][] TrueFalse() {
+        return new Object[][]{
+                new Object[]{true},


unnecessary array new: {{true}, {false}} should work here

pshapiro4broad · 2020-08-03T15:26:08Z

src/test/java/picard/analysis/CollectWgsMetricsTest.java

+    @Test(dataProvider = "wgsDataProvider")
+    public void testSmallIntervals(final File input, final String reference_name,
+                                   final String useFastAlgorithm) throws IOException {
+        final File outfile = File.createTempFile("testSmallIntervals", ".wgs_metrics",TEMP_OUTPUT_DIR);


whitespace

Suggested change

final File outfile = File.createTempFile("testSmallIntervals", ".wgs_metrics",TEMP_OUTPUT_DIR);

final File outfile = File.createTempFile("testSmallIntervals", ".wgs_metrics", TEMP_OUTPUT_DIR);

pshapiro4broad · 2020-08-03T15:27:37Z

src/test/java/picard/analysis/CollectWgsMetricsTest.java

+
+        setBuilder.setReadLength(100);
+
+        setBuilder.addPair("all_in",0,200,200,false,false,"100M","100M",true,false,30);


whitespace

Suggested change

setBuilder.addPair("all_in",0,200,200,false,false,"100M","100M",true,false,30);

setBuilder.addPair("all_in", 0, 200, 200, false, false, "100M", "100M", true, false, 30);

pshapiro4broad · 2020-08-03T15:29:07Z

src/test/java/picard/analysis/CollectWgsMetricsTest.java

+        final File outfile = File.createTempFile("testIntervalOneRead", ".wgs_metrics", TEMP_OUTPUT_DIR);
+        final File intervals = new File(TEST_DIR, "smallIntervals.interval_list");
+        final int sampleSize = 1000;
+        final String[] args = new String[]{


unnecessary new

Suggested change

final String[] args = new String[]{

final String[] args = {

pshapiro4broad · 2020-08-03T15:33:19Z

src/test/java/picard/analysis/CollectOxoGMetricsTest.java

@@ -12,7 +12,7 @@

 public class CollectOxoGMetricsTest {
    private static final File TEST_DATA_DIR = new File("testdata/picard/sam/");
-    private static final File SAM_FILE = new File(TEST_DATA_DIR, "summary_alignment_stats_test.sam");
+    private static final File SAM_FILE = new File(CollectAlignmentSummaryMetricsTest.TEST_DATA_DIR, "summary_alignment_stats_test.sam");


This seems like it could lead to problems, if someone changed the file while editing the other test but didn't realize the file was used here as well. If you made a separate constant for each file shared like this (e.g., build the path to the file in CollectAlignmentSummaryMetricsTest as a constant), that would at least make it easier to find other tests that depended on that file.

This is no different than what it was (regarding someone changing the file) but at least now there's a variable that is traced to where it's being used...I fail to see how building the path from scratch helps...I moved the files into a subdirectory and suddenly other tests started failing....with this construction it will not happen...

pshapiro4broad · 2020-08-03T15:35:39Z

src/main/java/picard/analysis/WgsMetrics.java


        // Get Theoretical Het SNP Sensitivity
-        if (unfilteredBaseQHistogram != null && unfilteredDepthHistogram != null) {
+        if (unfilteredBaseQHistogram != null) {


Why remove this null check? If this is null, it will cause TheoreticalSensitivity.normalizeHistogram to throw a PicardException.

already checked in the first line of test

pshapiro4broad

Many minor comments, looks OK as far as I can tell otherwise.

pshapiro4broad · 2020-08-03T18:13:57Z

src/main/java/picard/analysis/AlignmentSummaryMetricsCollector.java

+     * Class that counts reads that match various conditions
+     */
+    public class IndividualAlignmentSummaryMetricsCollector implements PerUnitMetricCollector<AlignmentSummaryMetrics, Integer, SAMRecordAndReference> {
+        private long numPositiveStrand = 0;


= 0 initializers unnecessary

pshapiro4broad · 2020-08-03T18:14:08Z

src/main/java/picard/analysis/AlignmentSummaryMetricsCollector.java

+        private final Histogram<Integer> readLengthHistogram = new Histogram<>("count", "readLength");
+        private final Histogram<Integer> alignedReadLengthHistogram = new Histogram<>("count", "alignedReadLength");
+
+        private AlignmentSummaryMetrics metrics;


can be final, I think

pshapiro4broad · 2020-08-03T18:17:41Z

src/main/java/picard/analysis/AlignmentSummaryMetricsCollector.java

+                    metrics.PCT_READS_ALIGNED_IN_PAIRS = MathUtil.divide((double) metrics.READS_ALIGNED_IN_PAIRS, (double) metrics.PF_READS_ALIGNED);
+                    metrics.PCT_PF_READS_IMPROPER_PAIRS = MathUtil.divide((double) metrics.PF_READS_IMPROPER_PAIRS, (double) metrics.PF_READS_ALIGNED);
+                    metrics.STRAND_BALANCE = MathUtil.divide(numPositiveStrand, (double) metrics.PF_READS_ALIGNED);
+                    metrics.PCT_CHIMERAS = MathUtil.divide(this.chimeras, (double) this.chimerasDenominator);


I wouldn't normally comment on this. but the code is inconsistent in its use. Why do these use this. but no other fields do? I think it would be better to omit them completely unless necessary for disambiguation purposes.

pshapiro4broad · 2020-08-03T18:23:52Z

src/main/java/picard/analysis/CollectMultipleMetrics.java

                // Generally programs should not be accessing these directly but it might make things smoother
                // to just set them anyway. These are set here to make sure that in case of a the derived class
                // overrides
                program.INPUT = input;

                return program;
            }
-        }
+        };


unnecessary ;

Suggested change

};

}

pshapiro4broad · 2020-08-03T18:29:06Z

src/main/java/picard/analysis/CollectMultipleMetrics.java

+            final String METRICS_EXTENSION = ".insert_size_metrics";
+            final String PDF_EXTENSION = ".insert_size_histogram.pdf";
+
+            final List<String> OUTPUT_EXTENSIONS = CollectionUtil.makeList(


why makeList instead of Arrays.asList?

Suggested change

final List<String> OUTPUT_EXTENSIONS = CollectionUtil.makeList(

final List<String> OUTPUT_EXTENSIONS = Arrays.asList(

pshapiro4broad · 2020-08-03T18:32:20Z

src/main/java/picard/analysis/CollectMultipleMetrics.java

+                    PDF_EXTENSION);
+
+            @Override
+            public String getHelpDoc() {


It looks like this is the same for most (all?) subclasses. If you make USAGE_SUMMARY and OUTPUT_EXTENSIONS either constructor arguments or abstract methods then the duplicate code can be removed.

pshapiro4broad · 2020-08-03T18:32:47Z

src/main/java/picard/analysis/CollectMultipleMetrics.java

+
+            @Override
+            public String getHelpDoc() {
+                return picard.analysis.CollectInsertSizeMetrics.USAGE_SUMMARY +


I think this should be CollectAlignmentSummaryMetrics.USAGE_SUMMARY

oops. good catch.

- Added Clips (Soft and Hard) to AlignmentSummaryMetrics - Added read-length histogram and pdf output to AlignmentSummaryMetrics - Added Tests - Added a TEMP_OUTPUT_DIR to CommandLineProgramTest.java which is delete once the derived class is done testing - Modified Two test classes to use TEMP_OUTPUT_DIR - Modified enum in CollectMultipleMetrics so that it will display what the options are and what each program does in the help.

- fixed fialing test (protect against empty histogram)

- fixing other tests failures

…istribution we use.

yfarjoun · 2020-08-04T20:32:14Z

@pshapiro4broad thanks for the comments. I responded to (almost) all of them. please take another glance.

yfarjoun · 2020-08-04T20:32:51Z

oh, you approved already...nm then. I'll wait till tests pass and merge.

gbggrant approved these changes Jul 31, 2020

View reviewed changes

pshapiro4broad reviewed Aug 3, 2020

View reviewed changes

pshapiro4broad approved these changes Aug 3, 2020

View reviewed changes

Yossi Farjoun added 12 commits August 4, 2020 16:30

WIP

b924381

WIP

690fdc6

WIP

6cc4f7e

- fixed failing tests (moved files)

c515e9b

- fixed fialing test (protect against empty histogram)

- testing failures (ggplot2?)

215e12c

- fixing other tests failures

- test that one can load the reshape library in R in the test env

64f7435

- add test with library in R

8891b58

- now plotting like a champ...no ggplot, no reshape2....

05d03a3

- responding to review comments

d3c121a

remove r_packages test since we do not have these packages in the r d…

8e5b875

…istribution we use.

- updated the javaDoc

d1d1754

yfarjoun force-pushed the yf_add_fold_penalty_to_wgs_metrics branch from d0f7b67 to d1d1754 Compare August 4, 2020 20:31

yfarjoun merged commit bd603aa into master Aug 5, 2020

yfarjoun deleted the yf_add_fold_penalty_to_wgs_metrics branch August 5, 2020 02:13

yfarjoun restored the yf_add_fold_penalty_to_wgs_metrics branch August 11, 2020 16:23

yfarjoun mentioned this pull request Aug 12, 2020

Stop removing periods from the end of output names #1433

Merged

5 tasks

yfarjoun deleted the yf_add_fold_penalty_to_wgs_metrics branch December 22, 2020 21:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Several changes to metrics collections (AlignmentSummaryMetrics and WgsMetrics) + some fluff #1555

Several changes to metrics collections (AlignmentSummaryMetrics and WgsMetrics) + some fluff #1555

yfarjoun commented Jul 31, 2020

gbggrant left a comment

pshapiro4broad left a comment

pshapiro4broad Aug 3, 2020

yfarjoun Aug 4, 2020

pshapiro4broad Aug 3, 2020

pshapiro4broad Aug 3, 2020

yfarjoun Aug 4, 2020

pshapiro4broad Aug 3, 2020

yfarjoun Aug 4, 2020

pshapiro4broad Aug 3, 2020

pshapiro4broad Aug 3, 2020

pshapiro4broad Aug 3, 2020

pshapiro4broad Aug 3, 2020

pshapiro4broad Aug 3, 2020

yfarjoun Aug 4, 2020

pshapiro4broad Aug 3, 2020

yfarjoun Aug 4, 2020

yfarjoun Aug 4, 2020

pshapiro4broad left a comment

pshapiro4broad Aug 3, 2020

pshapiro4broad Aug 3, 2020

pshapiro4broad Aug 3, 2020

pshapiro4broad Aug 3, 2020

yfarjoun Aug 4, 2020

pshapiro4broad Aug 3, 2020

pshapiro4broad Aug 3, 2020

pshapiro4broad Aug 3, 2020

yfarjoun Aug 4, 2020

yfarjoun commented Aug 4, 2020

yfarjoun commented Aug 4, 2020

	final File outfile = File.createTempFile("testSmallIntervals", ".wgs_metrics",TEMP_OUTPUT_DIR);
	final File outfile = File.createTempFile("testSmallIntervals", ".wgs_metrics", TEMP_OUTPUT_DIR);


		setBuilder.setReadLength(100);

		setBuilder.addPair("all_in",0,200,200,false,false,"100M","100M",true,false,30);

	setBuilder.addPair("all_in",0,200,200,false,false,"100M","100M",true,false,30);
	setBuilder.addPair("all_in", 0, 200, 200, false, false, "100M", "100M", true, false, 30);

	final List<String> OUTPUT_EXTENSIONS = CollectionUtil.makeList(
	final List<String> OUTPUT_EXTENSIONS = Arrays.asList(

Several changes to metrics collections (AlignmentSummaryMetrics and WgsMetrics) + some fluff #1555

Several changes to metrics collections (AlignmentSummaryMetrics and WgsMetrics) + some fluff #1555

Conversation

yfarjoun commented Jul 31, 2020

gbggrant left a comment

Choose a reason for hiding this comment

pshapiro4broad left a 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

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

pshapiro4broad left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yfarjoun commented Aug 4, 2020

yfarjoun commented Aug 4, 2020