Protection against using the wrong sample alias which produces zero L… #1242
Conversation
…OD. Now the tool will fail (i.e. return 1)
| @@ -365,11 +366,22 @@ protected int doWork() { | |||
|
|
|||
| summaryFile.addMetric(metrics); | |||
| log.info("Read Group: " + metrics.READ_GROUP + " / " + observedSampleAlias + " vs. " + metrics.SAMPLE + ": LOD = " + metrics.LOD_EXPECTED_SAMPLE); | |||
| if (metrics.LOD_EXPECTED_SAMPLE!=0) { | |||
| anyNonzeroLod |= true; | |||
There was a problem hiding this comment.
The bitwise |= here isn't necessary. The RHS of the statement is always true, so a simple = would suffice. Also, if we don't care about writing the metrics files in the case of this error, we can just output the error log from below and return 1
There was a problem hiding this comment.
"...and return 1" in what case? I am erroring out when I fail to find a non-zero LOD for all the FPs in the loop....so I need to wait until I'm out of the loop
| @@ -366,16 +368,15 @@ protected int doWork() { | |||
|
|
|||
| summaryFile.addMetric(metrics); | |||
| log.info("Read Group: " + metrics.READ_GROUP + " / " + observedSampleAlias + " vs. " + metrics.SAMPLE + ": LOD = " + metrics.LOD_EXPECTED_SAMPLE); | |||
| if (metrics.LOD_EXPECTED_SAMPLE!=0) { | |||
| anyNonzeroLod |= true; | |||
| if (metrics.LOD_EXPECTED_SAMPLE != 0) { | |||
There was a problem hiding this comment.
you could write
anyNonzeroLod |= metrics.LOD_EXPECTED_SAMPLE != 0;
(also, I think the proper spelling is anyNonZeroLod as non-zero is a hyphenated word)
Conversely avoiding negatives would make this a bit nicer, IMO, e.g.
allZeroLod &= metrics.LOD_EXPECTED_SAMPLE == 0;
...
if (allZeroLod) {
I find reading code with double negatives like !anyNonZeroLod to be unnecessarily confusing. The same is true of the text No Non-zero results found.
| @@ -224,7 +224,9 @@ protected int doWork() { | |||
| outputDetailMetricsFile = DETAIL_OUTPUT; | |||
| outputSummaryMetricsFile = SUMMARY_OUTPUT; | |||
| } else { | |||
| if (!OUTPUT.endsWith(".")) OUTPUT = OUTPUT + "."; | |||
| if (!OUTPUT.endsWith(".")) { | |||
| OUTPUT = OUTPUT + "."; | |||
There was a problem hiding this comment.
| OUTPUT = OUTPUT + "."; | |
| OUTPUT += "."; |
| @@ -268,38 +268,38 @@ protected int doWork() { | |||
| // Key: previous PG ID on a SAM Record (or null). Value: New PG ID to replace it. | |||
| final Map<String, String> chainedPgIds = getChainedPgIds(outputHeader); | |||
|
|
|||
| final SAMFileWriter out = new SAMFileWriterFactory().makeSAMOrBAMWriter(outputHeader, | |||
| try(SAMFileWriter out = new SAMFileWriterFactory().makeSAMOrBAMWriter(outputHeader, | |||
There was a problem hiding this comment.
| try(SAMFileWriter out = new SAMFileWriterFactory().makeSAMOrBAMWriter(outputHeader, | |
| try (SAMFileWriter out = new SAMFileWriterFactory().makeSAMOrBAMWriter(outputHeader, |
…OD. Now the tool will fail (i.e. return 1)
Description
Give your PR a concise yet descriptive title
Please explain the changes you made here.
Explain the motivation for making this change. What existing problem does the pull request solve?
Mention any issues fixed, addressed or otherwise related to this pull request, including issue numbers or hard links for issues in other repos.
You can delete these instructions once you have written your PR description.
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
Review
For more detailed guidelines, see https://github.com/broadinstitute/picard/wiki/Guidelines-for-pull-requests