Add seek_backward function with tests

[duncanMR]

Description

Addresses the first part of #2794, to create a seek_backward function which can reverse down a tree sequence (or jump to any tree from the null state). Given the symmetry between seek_forward and seek_backward, I just translated all the tests from the former to the latter. All of them pass now. In both the TreePosition and StatefulTree versions of seek_backward, I resorted to explicitly handling the null case with if statements because I couldn't see a more elegant way that works in all cases; I may have missed a neater solution there.

I included some commented-out print statements to match the ones currently in seek_forward; I can easily remove them if need be.

PR Checklist:

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

[codecov]

Codecov Report

Merging #2819 (1d533ad) into main (0ce4eac) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2819   +/-   ##
=======================================
  Coverage   89.79%   89.79%           
=======================================
  Files          30       30           
  Lines       29474    29474           
  Branches     5737     5737           
=======================================
  Hits        26465    26465           
  Misses       1726     1726           
  Partials     1283     1283           

Flag	Coverage Δ
c-tests	86.09% <ø> (ø)
lwt-tests	80.78% <ø> (ø)
python-c-tests	67.88% <ø> (ø)
python-tests	99.00% <ø> (ø)

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 0ce4eac...1d533ad. Read the comment docs.

[jeromekelleher]

Looks good @duncanMR!

This is very subtle, but I think you've hit on a good point. The out_range should really just be empty when the seeking from NULL. Do you think you could update the forward and reverse iters to account for this (and add tests to verify)?

[jeromekelleher]

Hmmm, this doesn't smell right to me. I think we need to look at the initial conditions to think about why we don't need to exclude for the null tree. I guess we should really take care of this by setting the out_range to be empty, in the TreePosition.seek_forward and TreePosition.seek_backward functions, rather than having to reason about it here. As you point out, we should have no edges to remove, so there's no point in iterating over the out_range at all.

[duncanMR]

That makes sense to me, I'll give that a try now. It worked in the seek_forward case because it was checking if e_left < old_right = 0 but I soon realised that it didn't work out so neatly when translated to seek_backward.

[duncanMR]

After some experiments, an approach I've found that works is as follows, from the TreePosition.seek_forward function as an example:

j = right_current_index
self.out_range.start = j
while j < M and right_coords[right_order[j]] <= left:
    j += 1
self.out_range.stop = j

if self.index == -1:
    #No edges, so out_range should be empty
    self.out_range.start = self.out_range.stop

The out_range.stop value still needs to be updated for later iterations to work, but setting start = stop prevents StatefulTree.seek_forward from iterating over the out_range. It works similarly in the seek_backward case. Is that what you had in mind?

[duncanMR]

I just pushed a commit with these changes. The tests are rather trivial; I couldn't think of another way to check the out_range is empty without adding code to track if StatefulTree.seek_x is looping over the out_range or not.

[jeromekelleher]

Looks good, let's review when we meet tomorrow

[duncanMR]

I'm revising some of the tests for clarity. Just posting my reasoning here since it will be hard to read from the commits. We currently have two tests of this form:

    def test_forward_to_prev(self, index):
        tree1 = StatefulTree(self.ts())
        tree1.iter_forward(index)
        tree1.prev()
        tree2 = StatefulTree(self.ts())
        tree2.iter_forward(index - 1)
        tree1.assert_equal(tree2)
        tree2 = StatefulTree(self.ts())
        tree2.iter_backward(index - 1)
        tree1.assert_equal(tree2)

    def test_seek_forward_next_prev(self, index):
        tree1 = StatefulTree(self.ts())
        tree1.iter_forward(index)
        tree1.prev()
        tree2 = StatefulTree(self.ts())
        tree2.seek_forward(index - 1)
        tree1.assert_equal(tree2)
        tree2 = StatefulTree(self.ts())
        tree2.iter_backward(index - 1)
        tree1.assert_equal(tree2)

• The second function is identical to the first, but instead of using iter_forward from null it uses seek_forward from null. This is redundant because we also have a test that ensures that iter_forward and seek_forward are the same when used from null. So we only need one of them.
• If the first assertion in the function is true, the second assertion is effectively testing if iter_backward to an index from null gives the same result as iter_forward. This isn't tested elsewhere and seems worthwhile, but I think it should be a separate test.

So this is what we are left with:

    def test_seek_forward_then_prev(self, index):
        tree1 = StatefulTree(self.ts())
        tree1.seek_forward(index)
        tree1.prev()
        tree2 = StatefulTree(self.ts())
        tree2.seek_forward(index - 1)
        tree1.assert_equal(tree2)

    def test_iter_backward_matches_iter_forward(self, index):
        ts = self.ts()
        tree1 = StatefulTree(ts)
        tree1.iter_forward(index)
        tree2 = StatefulTree(ts)
        tree2.iter_backward(index)
        tree1.assert_equal(tree2)

As discussed with @jeromekelleher, I'm also going to make generic functions for the tests that are repeated across multiple classes, to reduce unnecessary repetition. I'm still going to make separate functions for seek_forward and seek_backward for now, though. I don't think it's worth the trouble to combine them.

[jeromekelleher]

Sounds great, thanks @duncanMR!

[duncanMR]

I just committed the test changes and print statement cleanups. Is it okay if I squash the commits for merging?

[jeromekelleher]

Yep, LGTM. Let's squash and merge

[duncanMR]

I rebased and squashed, so it should be ready now.

[jeromekelleher]

Can you create an issue to track adding the new step function to the C API please, so we don't forget?