Yf stratify chimeric #1269
Merged
Yf stratify chimeric #1269
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
fleharty
requested changes
Jan 22, 2019
| @@ -74,7 +74,7 @@ | |||
| "<p>" + | |||
| "The resulting metric file will be named according to a provided prefix and a suffix which is generated " + | |||
| " automatically according to the error metric. " + | |||
| "The tool cal collect multiple metrics in a single pass and there should be hardly any " + | |||
| "The tool can collect multiple metrics in a single pass and there [ be hardly any " + | |||
There was a problem hiding this comment.
replace "[" with "should"
| return -1; | ||
| } | ||
| return cigar.getCigarElements().stream() | ||
| .filter(e->e.getOperator()== CigarOperator.S) |
There was a problem hiding this comment.
spaces around operators -> and ==
| POST_DINUC(() -> postDiNucleotideStratifier, "Stratifies bases by the read base at the previous cycle, and the current reference base."), | ||
| HOMOPOLYMER_LENGTH(() -> homoPolymerLengthStratifier, "Stratifies bases based on the length of homopolymer they are part of (only accounts for bases that were read prior to the current base)."), | ||
| HOMOPOLYMER(() -> homopolymerStratifier, "Stratifies bases based on the length of homopolymer, the base that the homopolymer is comprised of, and the reference base."), | ||
| GC_CONTENT(() -> gcContentStratifier, "The gc content of their read."), |
There was a problem hiding this comment.
their -> the
Also, gc should be "GC"
| READ_ORDINALITY(() -> readOrdinalityStratifier, "The read ordinality (i.e. first or second)."), | ||
| READ_BASE(() -> currentReadBaseStratifier, "the base in the original reading direction."), | ||
| READ_DIRECTION(() -> readDirectionStratifier, "The alignment direction of the read (encoded as + or -)."), | ||
| PAIR_ORIENTATION(() -> readOrientationStratifier, "The reads orientation and ordinality. (into F1R2 or F2R1) Assumes reads are \"innies\"."), |
There was a problem hiding this comment.
either "reads" should have a possessive or be singular. I would vote for "read".
Not sure I understand what "into" means.
| READ_BASE(() -> currentReadBaseStratifier, "the base in the original reading direction."), | ||
| READ_DIRECTION(() -> readDirectionStratifier, "The alignment direction of the read (encoded as + or -)."), | ||
| PAIR_ORIENTATION(() -> readOrientationStratifier, "The reads orientation and ordinality. (into F1R2 or F2R1) Assumes reads are \"innies\"."), | ||
| PAIR_PROPERNESS(() -> readPairednessStratifier, "The properness of the reads alignment. Looks for indications of chimerism."), |
There was a problem hiding this comment.
"reads" should be possessive or singular. I vote for "read".
| private static Integer stratifySoftClippedBases(final SAMRecord sam) { | ||
| final Cigar cigar = sam.getCigar(); | ||
| if (cigar == null) { | ||
| return -1; |
There was a problem hiding this comment.
Could you declare a constant for this?
| PAIR_PROPERNESS(() -> readPairednessStratifier, "The properness of the reads alignment. Looks for indications of chimerism."), | ||
| REFERENCE_BASE(() -> referenceBaseStratifier, "The reference base in the read's direction."), | ||
| PRE_DINUC(() -> preDiNucleotideStratifier, "The read base at the previous cycle, and the current reference base."), | ||
| POST_DINUC(() -> postDiNucleotideStratifier, "The read base at the previous cycle, and the current reference base."), |
There was a problem hiding this comment.
I think "previous" should be "subsequent"
| READ_GROUP(() -> readgroupStratifier, "The read-group id of the read."), | ||
| CYCLE(() -> baseCycleStratifier, "The machine cycle during which the base was read."), | ||
| BINNED_CYCLE(() -> binnedReadCycleStratifier, "The binned machine cycle. Similar to CYCLE, but binned into 5 evenly spaced ranges across the size of the read. This stratifier may produce confusing results when used on datasets with variable sized reads."), | ||
| SOFT_CLIPS(() -> softClipsLengthStratifier, "The number of softclipped bases their read has."), |
fleharty
reviewed
Jan 23, 2019
|
Thanks for the review @fleharty. I think I addressed all your points. |
fleharty
approved these changes
Jan 31, 2019
src/main/java/picard/sam/SamErrorMetric/ReadBaseStratification.java
Outdated
Show resolved
Hide resolved
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
Adds two stratifiers to CollectSamErrorMetrics that allow exploring the data with respect to Chimeric reads and soft-clipped bases.
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:/broadinstitute/picard/wiki/Guidelines-for-pull-requests