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

add num_children Python #2332

Merged
merged 2 commits into from Jun 16, 2022

Conversation

GertjanBisschop
Copy link
Member

@GertjanBisschop GertjanBisschop commented Jun 9, 2022

First pass at low level Python for tree.num_children_array and Python API. Was mostly copy and paste as expected, but still second-guessing myself I might have missed something.
I have set the type of the num_children_array to UINT64 (see tsk_size_t). Hope this is the correct 'translation'.

@codecov
Copy link

codecov bot commented Jun 9, 2022

Codecov Report

Merging #2332 (70d7e1c) into main (7b66382) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2332      +/-   ##
==========================================
+ Coverage   93.28%   93.29%   +0.01%     
==========================================
  Files          28       28              
  Lines       26722    26730       +8     
  Branches     1222     1222              
==========================================
+ Hits        24927    24939      +12     
+ Misses       1762     1758       -4     
  Partials       33       33              
Flag Coverage Δ
c-tests 92.25% <100.00%> (ø)
lwt-tests 89.05% <ø> (ø)
python-c-tests 71.40% <92.30%> (+0.03%) ⬆️
python-tests 98.85% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
c/tskit/trees.c 95.08% <100.00%> (ø)
python/_tskitmodule.c 90.74% <100.00%> (+0.06%) ⬆️
python/tskit/trees.py 98.28% <100.00%> (+<0.01%) ⬆️

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 7b66382...70d7e1c. Read the comment docs.

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

The UINT64 thing raises an important question, which I've opened #2333 to discuss.

Copy link
Member

@benjeffery benjeffery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, we might want to add num_children to data-model.md as that documents the other arrays?

Also might be nice to simplify things like test_topology:239

I think num_children needs to be added at test_topology:1793,1825,1880,1914,2185,6482

@benjeffery
Copy link
Member

benjeffery commented Jun 10, 2022

Also, a changelog would be appreciated!

@GertjanBisschop
Copy link
Member Author

Had a go at adding in some documentation on the convenience arrays as I tentatively called them, based on @benjeffery's suggestion. Have kept them separate from the paragraph on the quintuply linked tree to split off the essential from the non-essential.

@benjeffery
Copy link
Member

benjeffery commented Jun 13, 2022

Once #2337 is merged we can modify this PR to return the 32bit type.

@benjeffery
Copy link
Member

@jeromekelleher I've added a commit here to convert to int32. Would you do a quick sanity review of that before we merge here?

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@benjeffery benjeffery added the AUTOMERGE-REQUESTED Ask Mergify to merge this PR label Jun 16, 2022
@mergify mergify bot merged commit 3e61a10 into tskit-dev:main Jun 16, 2022
@mergify mergify bot removed the AUTOMERGE-REQUESTED Ask Mergify to merge this PR label Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants