Add DEDUPLICATE_RECORDS option to RevertSam. #1029
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nh13
force-pushed
the
nh_revert_sam_deduplicate_records
branch
from
6869acf
to
175984e
Compare
tfenne
reviewed
Dec 13, 2017
| @@ -164,6 +167,10 @@ | |||
| "the program will exit with an Exception instead of exiting cleanly. Output BAM will still be valid.") | |||
| public double MAX_DISCARD_FRACTION = 0.01; | |||
|
|
|||
| @Argument(doc = "If SANITIZE=true discard duplicate records. Duplicate records will have the same values for all field" + | |||
| "including tags.") | |||
There was a problem hiding this comment.
I think you probably need to add a note here that "deduplicate" doesn't have anything do with the usual "is duplicate flag" definition of the word. Maybe we can find a better word to use instead?
| // Remove records that have the SAM SAMString (*** SLOW ***) | ||
| if (DEDUPLICATE_RECORDS) { | ||
| final Iterator<SAMRecord> iter = recs.iterator(); | ||
| final Set<String> samStrings = new HashSet<>(); |
There was a problem hiding this comment.
- I'm unclear why this is better than just requiring that there be only a single unpaired/R1/R2 in the collection. That's tested below, so even if they are not identical, it'll still fail if there are, e.g. two R1s in the collection.
- I'd be a little concerned that if the extended attributes are in a different order they might generate different strings while otherwise being identical. Is
SAMRecord.equals()not trustworthy for some reason?
nh13
force-pushed
the
nh_revert_sam_deduplicate_records
branch
from
2af221b
to
a7bfa5d
Compare
|
@tfenne thanks for the quick review. I implemented your suggestion to keep only the first read for R1/R2/unpaired respectively. I improved the error message a little too. |
nh13
force-pushed
the
nh_revert_sam_deduplicate_records
branch
2 times, most recently
from
9fb5fa9
to
93ac7b4
Compare
tfenne
approved these changes
Dec 14, 2017
| discarded += recs.size() - 2; | ||
| recs = Arrays.asList(firstRecord, secondRecord); | ||
| } else { | ||
| log.debug("Discarding " + recs.size() + " reads with name " + recs.get(0).getReadName() + " because we found " + firsts + " R1s " + seconds + " R2s and " + unpaired + " unpaired reads."); |
There was a problem hiding this comment.
I would just use commas here instead of + so that all the string concatenation is only done if the log level is debug.
In some cases, the same record may be found multiple times in a BAM file. This option allows the user to to keep only the first record encountered for R1, R2, and unpaired reads respectively.
nh13
force-pushed
the
nh_revert_sam_deduplicate_records
branch
from
93ac7b4
to
7f4832b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In some cases, the same record may be found multiple times in a BAM
file. This option allows the user to remove the duplicate record,
whereas the default behavior is to discard the records (and mates if
present).
Checklist
Content
Review