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

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     

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

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.

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?

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

@duncanMR good to get your eyes on this also please

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

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

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?

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.

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

@Mergifyio rebase

rebase

✅ Branch has been successfully rebased

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

Sure, will do!