Skip to content

Conversation

jkgoodrich
Copy link
Contributor

No description provided.

Copy link
Contributor

@mike-w-wilson mike-w-wilson left a 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,
Copy link
Contributor

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 ?

Suggested change
filter_empty_csq: bool = True,
remove_empty_csq: bool = True,

Copy link
Contributor

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.

Copy link
Contributor Author

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

Comment on lines +810 to +821
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'.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested 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'.
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.

Comment on lines +831 to +833
: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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
: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.

Copy link
Contributor

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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if filter_empty_csq:
if remove_empty_csq:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above

@jkgoodrich jkgoodrich merged commit 2387430 into main Jul 9, 2024
@jkgoodrich jkgoodrich deleted the jg/make_expr_version_of_filter_vep_transcript_csqs branch July 9, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants