Implement efficient seeking from non-null trees using tree_pos

[duncanMR]

Description

Continuing from #2874, we want to finish moving over the tree-positioning code to use tree_pos efficiently. At the moment, tsk.tree_seek will either call tsk_tree_seek_from_null or tsk_tree_seek_linear depending on whether we are starting from the null tree or not. seek_linear repeatedly calls next or prev until it reaches the given position, with the direction being determined by which would cover the shortest distance.

As a first pass, I've implemented tsk_tree_seek_forward and tsk_tree_seek_backward and I've incorporated them into tsk_tree_seek_linear. We will need to revise some of the test_highlevel.py seek tests, because the direction we choose to seek along is different to the old approach in some cases. For example, we now seek forward to go from the first to the last tree in a sequence.

Curiously, my implementation passes all the C tests with no memory issues detected by Valgrind, and it also passes all the test_highlevel.py and test_tree_positioning tests except for the ones dependent on seeking direction. However, it has caused chaos with other Python tests, causing failures and segfaults in test_stats.py and test_divmat.py among others. The failing/crashing tests seem to be primarily be associated with LD calculations and divergence.

I'm currently trying to determine whether the problems are due to an error in my implemention (most likely) or the subtle problems with the time ordering of inserted edges, discussed in #2792.

PR Checklist:

• Tests that fully cover new/changed functionality.
• Documentation including tutorial content if appropriate.
• Changelogs, if there are API changes.

[codecov]

Codecov Report

Attention: Patch coverage is 9.21053% with 69 lines in your changes are missing coverage. Please review.

Project coverage is 89.59%. Comparing base (8b1be4f) to head (35dcda4).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2911      +/-   ##
==========================================
- Coverage   89.79%   89.59%   -0.21%     
==========================================
  Files          30       30              
  Lines       30399    30472      +73     
  Branches     5909     5922      +13     
==========================================
+ Hits        27296    27300       +4     
- Misses       1778     1846      +68     
- Partials     1325     1326       +1     

Flag	Coverage Δ
c-tests	85.79% <2.81%> (-0.42%)	⬇️
lwt-tests	80.78% <ø> (ø)
python-c-tests	67.71% <100.00%> (+<0.01%)	⬆️
python-tests	98.96% <ø> (ø)

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

Files	Coverage Δ
python/_tskitmodule.c	88.73% <100.00%> (+<0.01%)	⬆️
c/tskit/trees.c	89.05% <2.81%> (-1.38%)	⬇️

---

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 8b1be4f...35dcda4. Read the comment docs.

[jeromekelleher]

That is interesting. I can see the LD calculator causing problems as that does a lot of seeking backward and forward (was the original motivation for bidirectional seeking). I don't see anything obvious wrong, but it must be something to do with sample counts.

One thing that we can at least make progress on is that I think we should keep the option of seeking linearly. So, we make a new option to seek, TSK_SEEK_LINEAR which always uses the linear seek algorithm. We'll want to expose this to Python also, so there'll be a bit of plumbing involved.

We should specify this option in the ld_calculator, as that is definitely somewhere that linear seeking makes sense.

[duncanMR]

That is interesting. I can see the LD calculator causing problems as that does a lot of seeking backward and forward (was the original motivation for bidirectional seeking). I don't see anything obvious wrong, but it must be something to do with sample counts.

One thing that we can at least make progress on is that I think we should keep the option of seeking linearly. So, we make a new option to seek, TSK_SEEK_LINEAR which always uses the linear seek algorithm. We'll want to expose this to Python also, so there'll be a bit of plumbing involved.

We should specify this option in the ld_calculator, as that is definitely somewhere that linear seeking makes sense.

Okay; that makes sense to me. I'll revert seek_linear to its previous form and move the seek algorithm that uses tree_pos to a new function. I'll make the tree_pos method the default for seek, then see exactly which methods need to be changed back to linear in order for the tests to pass. Shall we call the new tree_pos method tsk_tree_seek_nonlinear?

[jeromekelleher]

seek_skip might be a bit more descriptive?