Skip to content

don't annotate StopLoss variants that are immediately followed by another stop codon#57

Merged
iskandr merged 3 commits intomasterfrom
silent-stop-loss
Apr 15, 2015
Merged

don't annotate StopLoss variants that are immediately followed by another stop codon#57
iskandr merged 3 commits intomasterfrom
silent-stop-loss

Conversation

@iskandr
Copy link
Copy Markdown
Contributor

@iskandr iskandr commented Apr 14, 2015

Fixes #56

Also:

  • Adds "warmup" to timing benchmarks
  • Make sure PrematureStop is actually generating a truncated protein (some variants that end in Stop codons inserted at the end may actually extend the protein)

Review on Reviewable

@tavinathanson
Copy link
Copy Markdown
Contributor

How about adding at least 1 test that would fail without the fix?


Reviewed files:

  • test/test_timings.py @ r1
  • varcode/effects.py @ r1
  • varcode/frameshift_coding_effect.py @ r1
  • varcode/in_frame_coding_effect.py @ r1
  • varcode/maf.py @ r1
  • varcode/variant.py @ r1

varcode/effects.py, line 493 [r1] (raw file):
Weird, Reviewable doesn't recognize this without adjusting the settings to not collapse whitespace changes.


varcode/frameshift_coding_effect.py, line 73 [r1] (raw file):
Can you clarify here that n_skip is the count of those same amino acids? (Or rename to something that automatically clarifies it?)


varcode/frameshift_coding_effect.py, line 96 [r1] (raw file):
It took me a few minutes to understand why this was a StopLoss, and to understand the n_skip stuff.

I think a bit more commenting could be helpful. This was helpful for me to write out for myself: n_skip = # of amino acids that are the same as the original protein. When all the amino acids are the same as the original, we either have the original protein or we've extended it. If we've extended it, it means we must have lost our stop codon.

(That last sentence, while obvious in hindsight, wasn't immediately obvious to me.)



Comments from the review on Reviewable.io

@iskandr
Copy link
Copy Markdown
Contributor Author

iskandr commented Apr 14, 2015

Added the variant from your issue to test_problematic_variants


varcode/effects.py, line 493 [r1] (raw file):
Huh, that's odd


varcode/frameshift_coding_effect.py, line 73 [r1] (raw file):
I got rid of this loop in favor of a helper I already wrote: string_helpers.trim_shared_prefix and renamed some of the variables to hopefully be clearer.


varcode/frameshift_coding_effect.py, line 96 [r1] (raw file):
That last sentence makes for a good comment :-)



Comments from the review on Reviewable.io

@tavinathanson
Copy link
Copy Markdown
Contributor

Merge away!


Reviewed files:

  • test/test_problematic_variants.py @ r2
  • varcode/frameshift_coding_effect.py @ r2

Comments from the review on Reviewable.io

iskandr added a commit that referenced this pull request Apr 15, 2015
don't annotate StopLoss variants that are immediately followed by another stop codon
@iskandr iskandr merged commit 377a502 into master Apr 15, 2015
@iskandr iskandr deleted the silent-stop-loss branch April 15, 2015 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Issue with n_skip?

2 participants