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

Tests and documentation for genetic_relatedness_weighted #2785

Merged
merged 6 commits into from Jul 13, 2023

Conversation

brieuclehmann
Copy link
Contributor

I've created a new PR to address the remaining tasks for #1246, namely adding tests and documentation. The tests are a bit messy but I think they cover everything. I'm not sure how to write the raw summary function under the general_stat framework, so would appreciate a hand / some pointers with this @petrelharp !

python/tskit/trees.py Outdated Show resolved Hide resolved
@petrelharp
Copy link
Contributor

petrelharp commented Jul 9, 2023

Well, let's see: do you mean writing the python general stat function, for tests here? We just need to translate the C code to python, really:

genetic_relatedness_weighted_summary_func(tsk_size_t state_dim, const double *state,
    tsk_size_t result_dim, double *result, void *params)
{
    indexed_weight_stat_params_t args = *(indexed_weight_stat_params_t *) params;
    const double *x = state;
    tsk_id_t i, j;
    tsk_size_t k;
    double meanx, ni, nj;

    meanx = state[state_dim - 1] / args.total_weights[state_dim - 1];
    for (k = 0; k < result_dim; k++) {
        i = args.index_tuples[2 * k];
        j = args.index_tuples[2 * k + 1];
        ni = args.total_weights[i];
        nj = args.total_weights[j];
        result[k] = (x[i] - ni * meanx) * (x[j] - nj * meanx) / 2;
    }
    return 0;
}

... or, better, go back and look at the definition of this and see what the python code ought to be? Let's see, I actually think it should be exactly the same as what you've got there, from genetic relatedness, which is:

        def f(x):
            mx = np.sum(x) / n  # this will be in the last entry of `state` in the C code
            return np.array(
                [(x[i] - n[i] * mx) * (x[j] - n[j] * mx) / 2 for i, j in indexes]

Edit: not quite, I think just mx might need adjusting? Need to check the theory.

docs/stats.md Outdated Show resolved Hide resolved
docs/stats.md Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@petrelharp
Copy link
Contributor

Comments above; otherwise, things look good! Thanks!

@jeromekelleher
Copy link
Member

I'll have a look after you've addressed @petrelharp's comments @brieuclehmann, and can sort out any remaining details (if they're giving you trouble)

@jeromekelleher jeromekelleher added this to the Python 0.5.5 milestone Jul 10, 2023
@brieuclehmann
Copy link
Contributor Author

Thanks @petrelharp, I forgot we had derived the weighted function in the overleaf too! That's all working now. I've updated the docs too.

image

@jeromekelleher I'm not sure how to resolve the remaining failed checks - I'm sure it's to do with the fact that the branch is quite old so would appreciate your help with any git surgery!

petrelharp and others added 4 commits July 13, 2023 09:20
Initialise total_weights to 0 and avoid an extra loop through arrays.

Fix compile error
First pass at genetic_relatedness_weighted tests

Full pass at tests for genetic_relatedness_weighted

Update python/tskit/trees.py

Co-authored-by: Peter Ralph <petrel.harp@gmail.com>

Add summary func to genetic_relatedness_weighted tests

Fix summary function definition in docs
@mergify
Copy link
Contributor

mergify bot commented Jul 13, 2023

⚠️ The sha of the head commit of this PR conflicts with #1246. Mergify cannot evaluate rules on this PR. ⚠️

@jeromekelleher
Copy link
Member

I just did a bit of git surgery @brieuclehmann - I'll take it from here, thanks!

@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #2785 (8f5e416) into main (555913c) will increase coverage by 0.04%.
The diff coverage is 94.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2785      +/-   ##
==========================================
+ Coverage   89.89%   89.93%   +0.04%     
==========================================
  Files          30       30              
  Lines       29043    29171     +128     
  Branches     5672     5697      +25     
==========================================
+ Hits        26107    26236     +129     
+ Misses       1667     1666       -1     
  Partials     1269     1269              
Flag Coverage Δ
c-tests 86.42% <93.75%> (+0.02%) ⬆️
lwt-tests 80.13% <ø> (ø)
python-c-tests 67.12% <71.95%> (+0.07%) ⬆️
python-tests 99.02% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
c/tskit/core.h 100.00% <ø> (ø)
c/tskit/trees.c 90.80% <93.33%> (+0.02%) ⬆️
python/_tskitmodule.c 88.56% <93.44%> (+0.15%) ⬆️
c/tskit/core.c 95.78% <100.00%> (+0.01%) ⬆️
python/tskit/trees.py 98.66% <100.00%> (+0.01%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 555913c...8f5e416. Read the comment docs.

@jeromekelleher jeromekelleher added the AUTOMERGE-REQUESTED Ask Mergify to merge this PR label Jul 13, 2023
@mergify mergify bot merged commit 60d75c6 into tskit-dev:main Jul 13, 2023
23 checks passed
@mergify mergify bot removed the AUTOMERGE-REQUESTED Ask Mergify to merge this PR label Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants