-
Notifications
You must be signed in to change notification settings - Fork 31
Add filter_vep_transcript_csqs_expr
, a version of filter_vep_transcript_csqs
that takes and returns an ArrayExpression
#713
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
Add filter_vep_transcript_csqs_expr
, a version of filter_vep_transcript_csqs
that takes and returns an ArrayExpression
#713
Conversation
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 split makes sense to me here. I have a suggestion on an arg name though I know this PR was a rearrangement but since were updating the module I put it in. I think its more clear and should be an quick update elsewhere.
mane_select: bool = False, | ||
filter_empty_csq: bool = True, | ||
ensembl_only: bool = True, | ||
filter_empty_csq: bool = True, |
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.
Thoughts on updating to remove_empty_csq
?
filter_empty_csq: bool = True, | |
remove_empty_csq: bool = True, |
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 we'd only need to update gnomad_qc in one place and then there would be a bulk replace in your other VEP PR. I just think remove is more clear since filter is a bit overloaded and can be ambiguous as to whether you are keeping or removing the filtered material.
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 think it makes sense to do it in it's own PR where that is all that is done since it's not a name introduced in this PR, and it is technically a breaking change
Filter VEP transcript consequences based on specified criteria, and optionally filter to variants where transcript consequences is not empty after filtering. | ||
If `filter_empty_csq` parameter is set to True, the Table/MatrixTable is filtered | ||
to variants where 'transcript_consequences' within the VEP annotation is not empty | ||
after the specified filtering criteria is applied. | ||
.. note:: | ||
By default, the Table/MatrixTable is filtered to variants where | ||
'transcript_consequences' within the VEP annotation is not empty after filtering | ||
to Ensembl canonical transcripts with a most severe consequence of | ||
'synonymous_variant'. |
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.
Filter VEP transcript consequences based on specified criteria, and optionally filter to variants where transcript consequences is not empty after filtering. | |
If `filter_empty_csq` parameter is set to True, the Table/MatrixTable is filtered | |
to variants where 'transcript_consequences' within the VEP annotation is not empty | |
after the specified filtering criteria is applied. | |
.. note:: | |
By default, the Table/MatrixTable is filtered to variants where | |
'transcript_consequences' within the VEP annotation is not empty after filtering | |
to Ensembl canonical transcripts with a most severe consequence of | |
'synonymous_variant'. | |
Filter VEP transcript consequences based on specified criteria, and optionally remove variants where transcript consequences is empty after filtering. | |
If `remove_empty_csq` parameter is set to True, the Table/MatrixTable is filtered | |
to variants where 'transcript_consequences' within the VEP annotation is not empty | |
after the specified filtering criteria is applied. | |
.. note:: | |
By default, the Table/MatrixTable is filtered to variants where | |
'transcript_consequences' within the VEP annotation is not empty after filtering | |
to Ensembl canonical transcripts with a most severe consequence of | |
'synonymous_variant'. |
If the above suggestion is accepted.
:param filter_empty_csq: Whether to filter out rows where 'transcript_consequences' | ||
is empty, after filtering 'transcript_consequences' to the specified criteria. | ||
Default is True. |
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.
:param filter_empty_csq: Whether to filter out rows where 'transcript_consequences' | |
is empty, after filtering 'transcript_consequences' to the specified criteria. | |
Default is True. | |
:param remove_empty_csq: Whether to remove rows where 'transcript_consequences' | |
is empty, after filtering 'transcript_consequences' to the specified criteria. | |
Default is True. |
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.
Dependent on arg rename suggestion
} | ||
t = t.annotate_rows(**vep_data) if is_mt else t.annotate(**vep_data) | ||
|
||
if filter_empty_csq: |
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 filter_empty_csq: | |
if remove_empty_csq: |
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.
Same comment as above
No description provided.