-
Notifications
You must be signed in to change notification settings - Fork 31
Add additional groupings to optional stratified allele frequencies #523
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
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.
Just a couple small questions, and this will work for now, but I agree with you that we should probably refactor the frequency code after this release to make this more flexible.
A couple small things that wont change functionality
gnomad/utils/annotations.py
Outdated
if additional_strata_grouping_expr is None: | ||
additional_strata_grouping_expr = {} |
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.
could move up to if statement above so make above:
if additional_strata_grouping_expr is None:
additional_strata_grouping_expr = {}
else:
_freq_meta_expr = _freq_meta_expr.annotate(**additional_strata_grouping_expr)
gnomad/utils/annotations.py
Outdated
if strata not in cut_data.keys(): | ||
raise KeyError( | ||
"%s is not an existing annotation and thus cannot be combined with" | ||
" additional strata" | ||
) |
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.
Will this ever happen? strata should be in cut_data right because it's added to _freq_meta_expr
which is annotated as mt._freq_meta
and that is used to build the cut_dict for the cut_data aggregation
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're right it should not! This is an artifact of a previous approach, before we added additional_strata_groupings_expr
to _freq_meta_expr- will remove this check.
Add additional groupings to optional stratified allele frequencies to allow stratified frequencies by multiple annotations: e.g. platform and population, or GATK version and population.