Skip to content

Conversation

KoalaQin
Copy link
Contributor

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.

Copy link
Contributor

@jkgoodrich jkgoodrich left a 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

Comment on lines 425 to 426
if additional_strata_grouping_expr is None:
additional_strata_grouping_expr = {}
Copy link
Contributor

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)

Comment on lines 530 to 534
if strata not in cut_data.keys():
raise KeyError(
"%s is not an existing annotation and thus cannot be combined with"
" additional strata"
)
Copy link
Contributor

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

Copy link
Contributor

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.

@mike-w-wilson mike-w-wilson merged commit d580a0d into main Apr 5, 2023
@mike-w-wilson mike-w-wilson deleted the qhmw/add_freq_strata_grouping branch April 5, 2023 19: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.

3 participants