Summary stats

[tomwhite]

This is a draft implementation of #29 (based on @eric-czech's code there) for discussion. I'm quite happy for someone else to take if over if interested (e.g. @daletovar, @jerowe).

API docs need to be added still. Also sample summary stats.

A couple of points for discussion:

1. Method naming. We currently have count_alleles (verb), but some of the new methods don't easily fit into that pattern: e.g. call_rate, variant_stats. Perhaps insisting the function name for each method is a verb is going too far, and we should just allow the name that we think sounds best (this seems to be the approach taken by Hail and Glow which both have a mixture of both styles).
2. Conditionally computing variables, and merging. The allele_frequency function calls count_alleles, but perhaps it should check first if it has already been computed (I think @eric-czech does something similar in LD prune, for example). In that case, should it return the variant_allele_count variable in the returned dataset? I'm wondering whether we should actually merge the new variables with the original dataset as a general rule. The original dataset would be unchanged, and this would mean the user no longer had to call merge themselves, which might work better in pipelines, for example.

[jeromekelleher]

@daletovar, @jerowe I think this would be a great place to start getting some real code merged. Would either of you like to work with @tomwhite on getting this fleshed out?

[eric-czech]

Method naming. We currently have count_alleles (verb), but some of the new methods don't easily fit into that pattern

I've actually been struggling with that a good bit @tomwhite. I keep trying to think of a good verb for things like regenie, linear_regression, or ld_prune but the fact that they're generally going to correspond to lazily defined arrays seems to put them between two stools for verbs like get and run. I had been (and still am) generally picking nouns for method names but only because I can't think of a good alternative. The nouns come easily though and given that there are unlikely to be many things in the API other than functions, perhaps verbs aren't as useful as a distinguishing modifier?

[ericdatakelly]

@daletovar, @jerowe I think this would be a great place to start getting some real code merged. Would either of you like to work with @tomwhite on getting this fleshed out?

@jeromekelleher: @daletovar is out of town until Thursday but I think he'd be interested in this.

[eric-czech]

To more of @tomwhite's original questions:

In that case, should it return the variant_allele_count variable in the returned dataset?

From an efficiency perspective, I think it would be ok to define upstream variables if not provided and then not merge/return them. I had thought Dask would be smart enough to know when the same computation is defined twice and in some experiments I haven't been able to find a counterexample, meaning that if we were to throw away the variant_allele_count created internally within an allele_frequency right before a user calls count_alleles, the Dask graph would be the same.

I'm wondering whether we should actually merge the new variables with the original dataset as a general rule. The original dataset would be unchanged, and this would mean the user no longer had to call merge themselves, which might work better in pipelines, for example.

I opened https://github.com/pystatgen/sgkit/issues/103 with some thoughts on that so we can keep that discussion going.

[daletovar]

@tomwhite, I'm happy to help with this. Would you prefer we keep this PR open and I fork your repo to collaborate?

[tomwhite]

@daletovar thanks for picking this up. I don't have a strong opinion on whether you fork this, or start from scratch.

[eric-czech]

FYI this should be https://github.com/pystatgen/sgkit/issues/29#issuecomment-656691069 instead.

[tomwhite]

This is now fixed.

[daletovar]

@tomwhite, I see. Well, do we know what all summary stats we want to have?

[tomwhite]

After discussion with @daletovar I've picked this one up again.

I've rebased on top of #217 (which is still a draft) so that the allele counting functions return datasets, and to use the code to merge datasets. I've also added docs, and a test for the case where variant_allele_count already exists in the dataset.

[eric-czech]

Sorry it took so long to approve this, but LGTM.

[jeromekelleher]

@Mergifyio rebase

[mergify]

Command rebase: success

Branch has been successfully rebased

[tomwhite]

@Mergifyio update

[mergify]

Command update: success

Branch has been successfully updated

[jeromekelleher]

@Mergifyio update

[mergify]

Command update: success

Branch has been successfully updated

[jeromekelleher]

The updates weren't working as we hadn't merged #244. Hopefully this will sort it out now.