Tree array access.

[jeromekelleher]

WIP for #1299

Trickier than I thought because of the circular references between the array and the Tree object, which owns the memory.

[codecov]

Codecov Report

Merging #1320 (8750471) into main (38ec5ae) will increase coverage by 0.01%.
The diff coverage is 98.48%.

@@            Coverage Diff             @@
##             main    #1320      +/-   ##
==========================================
+ Coverage   93.80%   93.82%   +0.01%     
==========================================
  Files          26       26              
  Lines       22143    22209      +66     
  Branches     1013     1013              
==========================================
+ Hits        20772    20837      +65     
- Misses       1338     1339       +1     
  Partials       33       33              

Flag	Coverage Δ
c-tests	92.44% <ø> (ø)
lwt-tests	92.97% <ø> (ø)
python-c-tests	95.15% <98.48%> (+0.01%)	⬆️
python-tests	98.86% <100.00%> (+<0.01%)	⬆️

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

Impacted Files	Coverage Δ
python/_tskitmodule.c	91.51% <97.91%> (+0.06%)	⬆️
python/tskit/trees.py	97.93% <100.00%> (+0.02%)	⬆️

---

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 38ec5ae...8750471. Read the comment docs.

[jeromekelleher]

Looks like this is working well. I think it's probably a good idea to add the flags_array and time_array to the Tree class also, since these are things we'll often be using in low-level algorithms, and it's a faff (and currently copy-ful, semantics-wise) to do this via the tree sequence tables arrays.

Should we go ahead and merge this much, and try it out for a bit before documenting, or go ahead and document it now and be done with it? I doubt we can change much here, it's pretty much a done-deal I think.

@benjeffery, any thoughts?

[benjeffery]

This is awesome stuff, couple of small comments.

I wonder about the _array naming though. Would left_children, parents etc. fit more with our existing names as we don't put type information in names normally?

[benjeffery]

Should we go ahead and merge this much, and try it out for a bit before documenting, or go ahead and document it now and be done with it? I doubt we can change much here, it's pretty much a done-deal I think.

@benjeffery, any thoughts?

Lets merge this, then file issues under 0.3.6 for time_array and flags_array and the docs. I'm happy to pick those up if I get to them before you.

[jeromekelleher]

I wonder about the _array naming though. Would left_children, parents etc. fit more with our existing names as we don't put type information in names normally?

It's a bit awkward, I agree, but I'm not sure there's much choice. If we were starting again I'd get rid of the parent(u) etc functions and use these arrays instead. But, that ship has sailed, so parent_array seems like the least bad option. I don't want to use parents because this will be break with the convention that array names are singular. Also, I think it would be confusing if we had tree.left_children[u] and tree.left_child(u) giving the same thing.

[jeromekelleher]

(Thanks for the review btw, will update later)

[jeromekelleher]

OK, updated and documented. @petrelharp, fancy taking a quick look?

(I finally figured out how to make repeated bits of text in RST: this would come in handy for the stats API!)

[petrelharp]

woah nice

[jeromekelleher]

Yeah!

[petrelharp]

I did look at this, btw - looks good.