Add num_children C level #2316

GertjanBisschop · 2022-06-02T20:32:01Z

First step towards adding num_children array for Trees. #2274

codecov · 2022-06-02T20:39:44Z

Codecov Report

Merging #2316 (688b875) into main (6086531) will increase coverage by 5.61%.
The diff coverage is n/a.

❗ Current head 688b875 differs from pull request most recent head 33ccde2. Consider uploading reports for the commit 33ccde2 to get more accurate results

@@            Coverage Diff             @@
##             main    #2316      +/-   ##
==========================================
+ Coverage   93.28%   98.89%   +5.61%     
==========================================
  Files          28       17      -11     
  Lines       26451     5994   -20457     
  Branches     1202     1188      -14     
==========================================
- Hits        24675     5928   -18747     
+ Misses       1744       36    -1708     
+ Partials       32       30       -2

Flag	Coverage Δ
c-tests	`?`
lwt-tests	`?`
python-c-tests	`?`
python-tests	`98.89% <ø> (+0.03%)`	⬆️

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

Impacted Files	Coverage Δ
python/tskit/trees.py	`98.14% <0.00%> (-0.03%)`	⬇️
c/tskit/trees.c
python/_tskitmodule.c
c/tskit/core.h
c/tskit/tables.c
c/tskit/stats.c
python/lwt_interface/example_c_module.c
c/tskit/genotypes.c
python/lwt_interface/tskit_lwt_interface.h
c/tskit/haplotype_matching.c
... and 3 more

Continue to review full report at Codecov.

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

GertjanBisschop · 2022-06-02T20:42:50Z

C level:

I have currently mainly added assert statements to existing tests.
As mentioned by @jeromekelleher, num_children can be used to query the number of roots. I have not yet touched/simplified any of this code.

Python level:
There is currently no other code testing the QuintuplyLinkedTree class, so I was a bit unsure as to how to proceed with that before tackling the actual lowlevel/highlevel tests. I have added only a simple test.

jeromekelleher

LGTM!

The num_roots stuff should be easy now, it's just tree.num_children[tree.virtual_root]

jeromekelleher · 2022-06-06T09:36:52Z

c/tskit/trees.c

    parent[c] = TSK_NULL;
    left_sib[c] = TSK_NULL;
    right_sib[c] = TSK_NULL;
+    self->num_children[p]--;


We should follow the same pattern above and declare a local restrict pointer for num_children. This is to help the compiler optimise the code, avoiding some pointer chasing. (Probably doesn't make much difference in practice, but no point in not doing it here.)

Similarly below.

jeromekelleher · 2022-06-06T09:39:51Z

c/tests/test_trees.c

+    ret = tsk_tree_first(&t);
+    CU_ASSERT_EQUAL(t.num_children[2], 1);
+    CU_ASSERT_EQUAL(t.num_children[4], 2);
+    CU_ASSERT_EQUAL(t.num_children[5], tsk_tree_get_num_roots(&t));


This will be a circular test once it's implemented - better to put the actual value in here. Likewise below.

Yep. Will change that. Thanks for the comments. Will address them and the Lint error.

benjeffery

LGTM % JK comments

jeromekelleher · 2022-06-07T11:06:20Z

Looks like this much is basically ready to merge. How should we stage the rest of the update @benjeffery? I guess we could hold this off until after 0.5.0 to try and get it shipped and then do the Python updates then? Or should we just push on with the changes now?

benjeffery · 2022-06-07T11:31:23Z

The python side of this should be relatively straightforward right? I'm keen to get 0.5.0 out ASAP, but this is a really nice addition to the release. I think we include this and the edge-diffs then draw a hard line under the release.

jeromekelleher · 2022-06-07T12:31:01Z

Okey dokey - would you mind creating the follow up issues we'd need after this for Python so?

GertjanBisschop · 2022-06-07T12:41:59Z

Cool. I should be able to finish the follow up by the end of the week.
Shall I squash both commits?

jeromekelleher · 2022-06-07T12:51:42Z

Yep, squash and rebase would be good.

benjeffery · 2022-06-07T14:12:36Z

Okey dokey - would you mind creating the follow up issues we'd need after this for Python so?

Done

jeromekelleher

LGTM!

benjeffery · 2022-06-08T11:07:54Z

I've added a changelog. Will merge.

jeromekelleher · 2022-06-08T12:37:33Z

Looks like you missed updating tsk_tree_free @GertjanBisschop - this is essential! (Valgrind spotted it though, good ole valgrind)

GertjanBisschop · 2022-06-08T12:56:18Z

I did indeed. Will fix it. Thank you valgrind.

benjeffery · 2022-06-08T21:12:38Z

@jeromekelleher Did you mean to merge this? Github says you removed the merge label.

jeromekelleher · 2022-06-09T08:26:22Z

I was trying to get mergify to merge - I thought "turning it off and on again" might trigger things.

benjeffery · 2022-06-09T10:05:06Z

@Mergifyio refresh

mergify · 2022-06-09T10:05:38Z

refresh

✅ Pull request refreshed

benjeffery · 2022-06-09T10:10:00Z

Circle CI is missing! Will try re triggering.

benjeffery · 2022-06-09T10:14:17Z

@Mergifyio rebase

mergify · 2022-06-09T10:14:52Z

rebase

✅ Branch has been successfully rebased

jeromekelleher reviewed Jun 6, 2022

View reviewed changes

benjeffery reviewed Jun 7, 2022

View reviewed changes

GertjanBisschop force-pushed the add_num_children_c branch from 8e48174 to 688b875 Compare June 7, 2022 09:36

GertjanBisschop force-pushed the add_num_children_c branch from 688b875 to a504674 Compare June 7, 2022 13:07

jeromekelleher approved these changes Jun 7, 2022

View reviewed changes

benjeffery force-pushed the add_num_children_c branch from a504674 to e8db04a Compare June 8, 2022 11:07

benjeffery added AUTOMERGE-REQUESTED and removed AUTOMERGE-REQUESTED labels Jun 8, 2022

benjeffery force-pushed the add_num_children_c branch from e8db04a to c9591eb Compare June 8, 2022 11:09

benjeffery added the AUTOMERGE-REQUESTED label Jun 8, 2022

GertjanBisschop force-pushed the add_num_children_c branch from c9591eb to 1a5a6a2 Compare June 8, 2022 13:22

jeromekelleher added AUTOMERGE-REQUESTED and removed AUTOMERGE-REQUESTED labels Jun 8, 2022

First step towards adding num_children array

33ccde2

benjeffery force-pushed the add_num_children_c branch from 1a5a6a2 to 33ccde2 Compare June 9, 2022 10:14

mergify bot merged commit c0c32a5 into tskit-dev:main Jun 9, 2022

mergify bot removed the AUTOMERGE-REQUESTED label Jun 9, 2022

jeromekelleher mentioned this pull request Jun 10, 2022

Change type of tsk_tree_t.num_children to tsk_id_t #2333

Closed

jeromekelleher mentioned this pull request Jun 22, 2022

Add num_children to Tree arrays #2274

Closed

Add num_children C level #2316

Add num_children C level #2316

Uh oh!

Conversation

GertjanBisschop commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

GertjanBisschop commented Jun 2, 2022

Uh oh!

jeromekelleher left a comment

Choose a reason for hiding this comment

Uh oh!

jeromekelleher Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

jeromekelleher Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

GertjanBisschop Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

benjeffery left a comment

Choose a reason for hiding this comment

Uh oh!

jeromekelleher commented Jun 7, 2022

Uh oh!

benjeffery commented Jun 7, 2022

Uh oh!

jeromekelleher commented Jun 7, 2022

Uh oh!

GertjanBisschop commented Jun 7, 2022

Uh oh!

jeromekelleher commented Jun 7, 2022

Uh oh!

benjeffery commented Jun 7, 2022

Uh oh!

jeromekelleher left a comment

Choose a reason for hiding this comment

Uh oh!

benjeffery commented Jun 8, 2022

Uh oh!

jeromekelleher commented Jun 8, 2022

Uh oh!

GertjanBisschop commented Jun 8, 2022

Uh oh!

benjeffery commented Jun 8, 2022

Uh oh!

jeromekelleher commented Jun 9, 2022

Uh oh!

benjeffery commented Jun 9, 2022

Uh oh!

mergify bot commented Jun 9, 2022

✅ Pull request refreshed

Uh oh!

benjeffery commented Jun 9, 2022

Uh oh!

benjeffery commented Jun 9, 2022

Uh oh!

mergify bot commented Jun 9, 2022

✅ Branch has been successfully rebased

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

GertjanBisschop commented Jun 2, 2022 •

edited

Loading

codecov bot commented Jun 2, 2022 •

edited

Loading