-
Notifications
You must be signed in to change notification settings - Fork 29
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
Modify filter_vep_transcript_csqs_expr
so it can also accept hl.expr.StructExpression
#748
Conversation
…r.StructExpression
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.
If this function is intended this way, there might be a difference with browser numbers.
if csqs is not None: | ||
if "most_severe_consequence" not in csq_expr.dtype.element_type.fields: | ||
fields = csq_expr if is_struct else csq_expr.dtype.element_type.fields | ||
if "most_severe_consequence" not in fields: | ||
logger.info("Adding most_severe_consequence annotation...") |
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.
This will actually get a different number of variants as the Browser, because they used ht.vep.most_severe_consequence
directly. This function is getting less than the browser, is it because our CSQ_ORDER is updated?
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.
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.
That's why I used filter_vep_transcript_csqs_expr(ht.vep)
in the toolbox
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.
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.
oh, sorry, I haven't seen the toolbox PR yet. One moment.
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.
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.
I figured out the other, I was being stupid. Will fix, but it's in the other PR
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.
LGTM!
Only a small question left.
@@ -860,19 +861,21 @@ def filter_vep_transcript_csqs( | |||
|
|||
|
|||
def filter_vep_transcript_csqs_expr( | |||
csq_expr: hl.expr.ArrayExpression, | |||
csq_expr: Union[hl.expr.StructExpression, hl.expr.ArrayExpression], |
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.
You add struct, considering if it's exploded?
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.
well, it can be used in the way we do for the toolbox because the annotation we are filtering on is top-level, or if it's exploded. Really any situation where the annotation you want to filter on isn't in a list
No description provided.