Refactor is_valid fn #388
Merged
Refactor is_valid fn #388
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
macklin-10x
requested changes
Mar 4, 2024
vdj_ann/src/annotate.rs
Outdated
| @@ -3018,6 +3018,7 @@ pub struct ContigAnnotation { | |||
| pub invalidated_umis: Option<Vec<String>>, // invalidated UMIs | |||
| pub is_cell: bool, // was the barcode declared a cell? | |||
| pub productive: Option<bool>, // productive? (null means not full length) | |||
| pub productive_criteria: Option<ContigStatus>, // New field added in CR 8.1 | |||
There was a problem hiding this comment.
Use /// for documenting fields, so the docs show up in intellisense. Also, it's best for comments to be "timeless" - prefer describing what something does rather than, for example, when it was added.
Suggested change
| pub productive_criteria: Option<ContigStatus>, // New field added in CR 8.1 | |
| /// <insert description of what this field is> | |
| pub productive_criteria: Option<ContigStatus>, |
| @@ -3050,6 +3051,7 @@ impl ContigAnnotation { | |||
| invalidated_umis: Option<Vec<String>>, // invalidated UMIs | |||
| is_cellx: bool, // was the barcode declared a cell? | |||
| productivex: bool, // productive? | |||
| productive_criteria: ContigStatus, // criteria used to asess productive status | |||
There was a problem hiding this comment.
Same kind of thing here.
Suggested change
| productive_criteria: ContigStatus, // criteria used to asess productive status | |
| /// Criteria used to asess productive status. | |
| productive_criteria: ContigStatus, |
vdj_ann/src/transcript.rs
Outdated
Comment on lines
83
to
104
| pub struct VdjChainSpecificVJGenes { | ||
| v_type: String, | ||
| j_type: String, | ||
| } | ||
|
|
||
| impl From<VdjChain> for VdjChainSpecificVJGenes { | ||
| fn from(chain: VdjChain) -> Self { | ||
| VdjChainSpecificVJGenes { | ||
| v_type: format!("{chain}V"), | ||
| j_type: format!("{chain}J"), | ||
| } | ||
| let mut too_large = false; | ||
| const MIN_DELTA: i32 = -25; | ||
| const MIN_DELTA_IGH: i32 = -55; | ||
| const MAX_DELTA: i32 = 35; | ||
| if first_vstart >= 0 && last_jstop >= 0 { | ||
| let delta = (last_jstop_len + first_vstart_len + 3 * cdr3[0].1.len() as i32 - 20) | ||
| - (last_jstop - first_vstart); | ||
| if logme { | ||
| fwriteln!(log, "VJ delta = {}", delta); | ||
| } | ||
| let mut min_delta = MIN_DELTA; | ||
| if igh { | ||
| min_delta = MIN_DELTA_IGH; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think we actually need a type here - we only use it once, and we don't ever pass it around as a structure. I think you can delete this and replace it with these format calls in-place.
| pub fn is_productive_contig( | ||
| b: &DnaString, | ||
| refdata: &RefData, | ||
| ann: &[(i32, i32, i32, i32, i32)], |
There was a problem hiding this comment.
Not necessary in this PR, but we should at some point in the future replace these annotation tuples with structs.
shamoni
force-pushed
the
sm/refactor_is_valid
branch
from
cad8f5b
to
a2db8ff
Compare
macklin-10x
approved these changes
Mar 7, 2024
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.
No description provided.