-
Notifications
You must be signed in to change notification settings - Fork 371
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.
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
New CheckDuplicateMarking CLP #1507
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor comments and questions. Also, what is the rationale to not make this documented?
private final Log log = Log.getInstance(CheckDuplicateMarking.class); | ||
private final ProgressLogger progress = new ProgressLogger(log, (int) 1e6, "Checked."); | ||
|
||
static private int NUM_WARNINGS = 100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be final
static private int NUM_WARNINGS = 100; | |
private static final int NUM_WARNINGS = 100; |
|
||
//sort by queryname if not already | ||
final Iterable<SAMRecord> samRecordIterable; | ||
if (reader.getFileHeader().getSortOrder() != SAMFileHeader.SortOrder.queryname) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be clearer to have this if
block in a method, ensureSortedBam()
|
||
checkDuplicateMarkingsInIterable(samRecordIterable); | ||
log.info("Found " + numBadRecords + " bad records."); | ||
return numBadRecords > 0 ? 1:0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitespace
return numBadRecords > 0 ? 1:0; | |
return numBadRecords > 0 ? 1 : 0; |
} | ||
|
||
checkDuplicateMarkingsInIterable(samRecordIterable); | ||
log.info("Found " + numBadRecords + " bad records."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use error
not info
if there are bad records, and/or only log if there's an error.
private String currentReadName = ""; | ||
private boolean currentReadDuplicateMarked = false; | ||
|
||
private final Log log = Log.getInstance(CheckDuplicateMarking.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be static
private final Log log = Log.getInstance(CheckDuplicateMarking.class); | |
private static final Log log = Log.getInstance(CheckDuplicateMarking.class); |
if (!rec.getReadName().equals(currentReadName)) { | ||
currentReadName = rec.getReadName(); | ||
currentReadDuplicateMarked = rec.getDuplicateReadFlag(); | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary return
since this will fall through to the return at line 136
|
||
|
||
private void checkDuplicateMarkingsInIterable(final Iterable<SAMRecord> iterable) throws IOException { | ||
try (PrintWriter writer = OUTPUT==null? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitespace
try (PrintWriter writer = OUTPUT==null? | |
try (PrintWriter writer = OUTPUT == null ? | |
new PrintWriter(NullOutputStream.NULL_OUTPUT_STREAM) : | |
import java.nio.file.Paths; | ||
|
||
public class CheckDuplicateMarkingTest { | ||
private static final String TEST_FILES_DIR="testdata/picard/sam/CheckDuplicateMarking"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reformat new code to follow conventions
private static final String TEST_FILES_DIR="testdata/picard/sam/CheckDuplicateMarking"; | |
private static final String TEST_FILES_DIR = "testdata/picard/sam/CheckDuplicateMarking"; |
|
||
import org.testng.Assert; | ||
import org.testng.annotations.DataProvider; | ||
import org.testng.annotations.Test; | ||
import picard.sam.DuplicationMetrics; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to move the test but not the class? Normally the test for a class is in the same package as the class being tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that mean you will move it back? It looks like this wasn't addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. not sure what happened there...sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a few nits left.
src/test/java/picard/sam/markduplicates/CheckDuplicateMarkingTest.java
Outdated
Show resolved
Hide resolved
src/test/java/picard/sam/markduplicates/CheckDuplicateMarkingTest.java
Outdated
Show resolved
Hide resolved
|
||
import org.testng.Assert; | ||
import org.testng.annotations.DataProvider; | ||
import org.testng.annotations.Test; | ||
import picard.sam.DuplicationMetrics; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that mean you will move it back? It looks like this wasn't addressed.
src/main/java/picard/sam/markduplicates/CheckDuplicateMarking.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
private void checkDuplicateMarkingsInIterable(final Iterable<SAMRecord> iterable) throws IOException { | ||
try (PrintWriter writer = OUTPUT == null ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could put ternary op inside constructor
try (PrintWriter writer = new PrintWriter(OUTPUT != null ? new FileWriter(OUTPUT) : NullOutputStream.NULL_OUTPUT_STREAM)) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no...it's a different constructor..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PrintWriter(Writer)
vs. PrintWriter(OutputStream)
… with the same queryname have the same duplicate marking. - added tests - moved another test into a markduplicates test directory
5488339
to
4c295d4
Compare
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