-
Notifications
You must be signed in to change notification settings - Fork 43
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
Fix ClinVar track crash #1532
Fix ClinVar track crash #1532
Conversation
Tagging @mattsolo1 for review because of a big picture question: Frameshift variants in the expanded clinvar plot are rendered with a line that differs in texture based on if its going through an exon or not, this is transcript dependent: In this (seemingly rare) case, there is a frameshift variant from ClinVar with an associated transcript that the browser doesn't display. As far as I can tell, the two main options are:
I realize this is probably pretty niche, but I wanted to check in on what you thought is the better option (or if there's some other better thing to do here), fwiw I think I'm in favor of option B, and using the canonical or mane select transcript to inform the texture of the line |
From discussion with Moriel, Since it seems as though this is exceedingly rare, really picking any of the options seems to be a reasonable fix. A third option brought up is to not even display ClinVar variants that are not on the transcripts we display. Ultimately the sentiment was that any of the choices are acceptable fixes, so given that I'm opting to go with the solution in this PR -- not display the line for the length of the frameshift. |
…mshift variant In rare cases, there can be a frameshift variant from ClinVar on a transcript that the browser doesn't display, here we manually set the exon array to [] to prevent a crash when trying to render the line with CDS or UTR lines
ef61635
to
90c654e
Compare
Heya @phildarnowsky-broad This should be a relatively smol one. After speaking with Moriel, it was deemed this solution is fine, and it stops a page crash on expanding the ClinVar plot in this very specific and seemingly quite rare scenario. |
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 like a good fix but I had a couple of suggestions about expanding on the fix.
feature_type: e.feature_type, | ||
})) | ||
|
||
// if a frameshift variant-consequence pair from ClinVar exists for a transcript |
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.
So this fix is fine far as it goes, but I'd suggest you also do two other things:
- add a type annotation a couple lines above here reflecting that
transcript
might be undefined, i.e. change line 213 to something like:const transcript: Transcript | undefined = ...
. That way we won't get bitten here by a similar case in the future. - Update
getGlobalFrameshiftCoordinates
also to reflect thattranscript
might beundefined
, the logic might be fine as-is but a type annotation on there would be good in any case.
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.
Aha very good call, I didn't think to add a type annotation on the variable itself.
I added a fixup commit to address point 1.
Point 2. is already addressed before this PR (getGlobalFrameshift... expects the possibility of undefined
and handles it if so), so that's why theres not fixup commit to address that.
Applied changes, with note in response to comment about why one change was not included ( Re-requesting review at your convenience! |
Resolves: #1525
Frameshift variants are displayed with a line that is either solid or dotted, depending on if the region being shifted through is an exon that is part of the transcript or not.
In rare cases, there can be a frameshift variant from ClinVar on a transcript that the browser doesn't display, here we manually set the exon array to an empty array to prevent a crash when trying to render the line.
Alternatively, we could edit the logic to always return one type of line or another for the entire length of the frameshift, but without knowledge of which exons are part of the transcript, we can very possibly make a mistake and be inconsistent with how we display things. However, by not displaying any line, we are already being inconsistent with how we display frameshift variants. We should discuss on which is the lesser of the two evils.
One thing to try and better understand is why we don't display some transcripts that ClinVar has entries for.