Skip to content
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 generic constraint functions - annotate_mutation_type(), trimer_from_heptamer(), collapse_strand(), add_most_severe_csq_to_tc_within_vep_root() #474

Merged
merged 15 commits into from
Sep 22, 2022

Conversation

averywpx
Copy link
Contributor

Add generic functions that were in gnomad_lof repo.

@klaricch
Copy link
Contributor

klaricch commented Sep 1, 2022

since there are only three functions here you can add the names of them in the title of this PR to be more descriptive (there will also probably be future PRs moving over more of the generic constraint functions so it's helpful to have a little more detail in the titles)

gnomad/utils/constraint.py Outdated Show resolved Hide resolved
gnomad/utils/constraint.py Outdated Show resolved Hide resolved
gnomad/utils/constraint.py Outdated Show resolved Hide resolved
from gnomad.utils.vep import add_most_severe_consequence_to_consequence


def annotate_variant_types(
Copy link
Contributor

Choose a reason for hiding this comment

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

there's a similarly named function with a different definition for variant_type here:

def add_variant_type(alt_alleles: hl.expr.ArrayExpression) -> hl.expr.StructExpression:

so i would suggest changing the function name, maybe "annotate_mutation_type" and change below instances of "variant_type" to "mutation_type"....these names are very similar but it might at least help clear up some confusion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does any place in pipeline use 'variant_type'? I can't find any.

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 it was mostly used for plotting purposes

gnomad/utils/constraint.py Outdated Show resolved Hide resolved
gnomad/utils/constraint.py Outdated Show resolved Hide resolved
gnomad/utils/constraint.py Outdated Show resolved Hide resolved
gnomad/utils/constraint.py Outdated Show resolved Hide resolved
gnomad/utils/constraint.py Outdated Show resolved Hide resolved
gnomad/utils/constraint.py Outdated Show resolved Hide resolved
@averywpx averywpx requested a review from klaricch September 6, 2022 15:47
@klaricch
Copy link
Contributor

klaricch commented Sep 7, 2022

since there are only three functions here you can add the names of them in the title of this PR to be more descriptive (there will also probably be future PRs moving over more of the generic constraint functions so it's helpful to have a little more detail in the titles)

this comment still needs to be resolved

gnomad/utils/constraint.py Outdated Show resolved Hide resolved
gnomad/utils/vep.py Outdated Show resolved Hide resolved
gnomad/utils/constraint.py Outdated Show resolved Hide resolved
gnomad/utils/constraint.py Outdated Show resolved Hide resolved
gnomad/utils/constraint.py Outdated Show resolved Hide resolved
gnomad/utils/constraint.py Outdated Show resolved Hide resolved
gnomad/utils/constraint.py Outdated Show resolved Hide resolved
- variant_type_model

:param t: Input Table or MatrixTable.
:param trimer: Whether to use trimers for context. Defaults to False (uses heptamers as context).
Copy link
Contributor

Choose a reason for hiding this comment

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

If trimer is set to true here, but the context is actually formatted as a heptamer within the ht/mt this function wouldn't behave as expected. I think it's actually safer to drop this parameter and within the function set the mid_index by first checking the length of context to see if it is formatted as a trimer or a heptamer.

from gnomad.utils.vep import add_most_severe_consequence_to_consequence


def annotate_variant_types(
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 it was mostly used for plotting purposes

@averywpx averywpx changed the title Add generic constraint functions Add generic constraint functions - annotate_mutation_type(), trimer_from_heptamer(), collapse_strand(), add_most_severe_csq_to_tc_within_vep_root() Sep 7, 2022
@averywpx averywpx requested a review from klaricch September 8, 2022 16:10
gnomad/utils/constraint.py Outdated Show resolved Hide resolved
Copy link
Contributor

@klaricch klaricch left a comment

Choose a reason for hiding this comment

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

just one last change on this one

@averywpx averywpx requested a review from klaricch September 13, 2022 13:59
gnomad/utils/constraint.py Outdated Show resolved Hide resolved
gnomad/utils/constraint.py Outdated Show resolved Hide resolved
@averywpx averywpx requested a review from klaricch September 15, 2022 14:40
gnomad/utils/constraint.py Outdated Show resolved Hide resolved
gnomad/utils/constraint.py Outdated Show resolved Hide resolved
gnomad/utils/constraint.py Outdated Show resolved Hide resolved
@averywpx averywpx requested a review from klaricch September 19, 2022 19:04
Copy link
Contributor

@klaricch klaricch left a comment

Choose a reason for hiding this comment

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

LGTM!

@averywpx averywpx merged commit 2edccf5 into main Sep 22, 2022
@averywpx averywpx deleted the generic_constraint_funcs branch November 1, 2022 18:08
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