Modular incremental trees

[jeromekelleher]

Try to centralise our tree positioning code by bringing the logic into TreePosition class. See #2778 for discussion.

@molpopgen - the idea here is to try and pull out all the of the code that we use in the tsk_tree_t class for tree positioning into this class, so that it can then be shared by incremental algorithms as well. We should then be able to do efficient skipping to a particular tree, which will be useful for parallel algorithms.

WIP

[molpopgen]

Sounds great. IMO, it is important to have that core logic separated from updating all those tree arrays!

[codecov]

Codecov Report

Merging #2786 (2c264be) into main (67f2ef3) will increase coverage by 0.02%.
The diff coverage is 96.25%.

@@            Coverage Diff             @@
##             main    #2786      +/-   ##
==========================================
+ Coverage   89.93%   89.96%   +0.02%     
==========================================
  Files          30       30              
  Lines       29171    29274     +103     
  Branches     5697     5706       +9     
==========================================
+ Hits        26236    26335      +99     
- Misses       1666     1669       +3     
- Partials     1269     1270       +1     

Flag	Coverage Δ
c-tests	86.49% <96.25%> (+0.07%)	⬆️
lwt-tests	80.13% <ø> (ø)
python-c-tests	67.12% <ø> (ø)
python-tests	99.02% <ø> (ø)

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

Impacted Files	Coverage Δ
c/tskit/trees.c	90.88% <95.48%> (+0.08%)	⬆️
c/tskit/haplotype_matching.c	90.67% <100.00%> (+0.26%)	⬆️

---

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 67f2ef3...2c264be. Read the comment docs.

[jeromekelleher]

I think this is ready for review and merging now.

The idea is to add a new class TreePosition (or tsk_tree_position_t in C) which takes care of the details of keeping track of which edges you need to insert and remove from the current tree as you move along the sequence. This is already a substantial improvement over what we have now because it allows us to reuse code in incremental algorithms, and we can also now define incremental algorithms that go backwards as well as forwards.

I've implemented the basic next/prev operations here in Python and C, and used the core code to replace the bits of the library code that use the tsk_tree_diff_iterator. There's lots more that can be done with this, which I'll open some issues to track.

One key thing that needs to be mentioned here is that I've incorporated quite a clean way of seeking to arbitrary locations along the sequence while touching only the edges that need to be changed in order to transform the current tree into the target tree. However this has raise this issue that these edges are not necessarily inserted in time-increasing order. This needs to be thought about a bit, which is why I haven't gone ahead and switched over to using this code for the tsk_tree_t class itself.

@molpopgen - any chance you could take a look and see what you think?

[molpopgen]

I'll take a look, but it'll be next week.

[jeromekelleher]

@duncanMR good to get your eyes on this also please

[duncanMR]

@jeromekelleher I will start examining the code today. Have you found a test case where the edges aren't inserted in time-decreasing order?

[jeromekelleher]

Excellent. I haven't picked out specific examples where the edges aren't time sorted in seek_forward, and that would be a great thing to do in a follow up.

I stopped working on the random seek aspect once I realised the sortedness problem, and decided to focus on getting the basic code merged so we could start using it. So it's in a half finished state at the moment

[molpopgen]

Let's see if I got this:

1. The new type book-keeps index ranges of output/input edges. This is essentially the edge diffs for the new tree.
2. Nodes can get out of (time) order because the input/output index vectors are sorted first by position and then time, and input/output do time in different orders from one another?

[jeromekelleher]

The new type book-keeps index ranges of output/input edges. This is essentially the edge diffs for the new tree.

Yep - but without having to allocate any extra memory because we're using the existing index arrays to represent the ranges.

Nodes can get out of (time) order because the input/output index vectors are sorted first by position and then time, and input/output do time in different orders from one another?

This is subtle. The input/output arrays are sorted by position and time, which guarantees that we insert/remove the edges in the correct order for adjacent trees. However, when we're seeking to non-adjacent trees (or randomly into the middle of the sequence, like seek_from_null) we lose this guarantee.

[jeromekelleher]

Can I get a review here please @benjeffery? I want to merge this so downstream stuff can go in too

[jeromekelleher]

@Mergifyio rebase

[mergify]

rebase

✅ Branch has been successfully rebased

[benjeffery]

Very nice, just a couple of small comments.

[benjeffery]

Why not use the existing Interval class?

[jeromekelleher]

Just simpler to keep track of what it does for this kind of prototype/test code, and not have to worry about external APIs and so on

[benjeffery]

Some missing coverage here and below.

[jeromekelleher]

Whoops, didn't spot these missing coverage bits. @duncanMR could you try and get these covered in C tests as part of #2794 please?

[duncanMR]

Sure, will do!