Fix mutation ordering

[benjeffery]

Addressing #3253

This PR changes the mutation sort order from site->time->ID to site->time->node_time->num_descendants->ID

This is required as if mutations have unknown times, and no parent info, we need to use the node time to have parents before children. For mutations at the same node, we assume the existing order is parents before children. We need to use num descendants to use parent info if it is present for mutations at the same node.

I've updated a few of the breaking Python tests to give a flavour of the impact. This PR still needs tests for the new order, and the rest of the broken tests fixing, but thought this was a good point for feedback.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.75%. Comparing base (0b89f97) to head (84a2f4f).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3257      +/-   ##
==========================================
+ Coverage   89.73%   89.75%   +0.01%     
==========================================
  Files          29       29              
  Lines       32544    32499      -45     
  Branches     5958     5949       -9     
==========================================
- Hits        29203    29169      -34     
+ Misses       1894     1888       -6     
+ Partials     1447     1442       -5     

Flag	Coverage Δ
c-tests	86.74% <100.00%> (+0.03%)	⬆️
lwt-tests	80.38% <ø> (ø)
python-c-tests	88.24% <ø> (ø)
python-tests	98.79% <ø> (ø)
python-tests-no-jit	33.02% <ø> (ø)
python-tests-numpy1	50.71% <ø> (ø)

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

Files with missing lines	Coverage Δ
c/tskit/tables.c	83.06% <100.00%> (+0.04%)	⬆️
python/tskit/tables.py	99.26% <ø> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jeromekelleher]

What's the difference now between sort_mutations and sort_canonical? Would a simpler way of thinking about this be to say that we're changing things by making sort_mutations simply call sort_canonical? Presumably we thought hard about what sort_canonical had to do, so we're just swapping in that definition now as a stricter ordering. I'd imagine we only kept the original sorting to avoid breakage at the time.

[benjeffery]

The only difference now is that canonical has a sort by node id, which I think is still needed in the case of nodes with equal times?

[jeromekelleher]

Let's just use sort_canonical for now, and see how we get on with test suite breakage? I don't see much point in having another stricter sort if we're already changing the semantics.

[benjeffery]

Sorting is now identical to canonical - only a couple of python tests failing. Will fix those and add some new tests for the new sort order.

[benjeffery]

I've fixed up the tests - the only one I wasn't sure about was tests/test_extend_haplotypes.py::TestExamples::test_suite_examples_defaults[back_mutations] where mutations didn't match between the python and C implementations any more. Digging both methods do a sort during haplotype extension iterations, but the C version does this skipping a mutation sort. Modifying the Python to match fixes the test, but I'm not sure if this has impact on result of extension. Just doing a sort on both at the end didn't fix the error.

[jeromekelleher]

Looks great - I think we need @petrelharp's input on the haplotype extender stuff

[benjeffery]

Whoops - I missed two topology tests. These got a bit more involved - I've had to switch from checking for exact mutational equality to mutational functional equivalence in the simplify tests.

[benjeffery]

I think this is ready for final review. I've added @hyanwong's initial issue failing code as a test.

[petrelharp]

Hm - we should benchmark the change? - recall that sort is a bottleneck for SLiM for instance. To be clear, this is only making sort match canonical for mutations, not other things, right?

Re: the extend haplotypes - thanks for digging into that! For reference, here's the error:

=========================== short test summary info ============================
FAILED tests/test_extend_haplotypes.py::TestExamples::test_suite_examples_defaults[back_mutations] - AssertionError: MutationTable row 1222 differs:
self.node=15 other.node=9
self.metadata=b'1186' other.metadata=b'1188'

The error is fine, though - it's because the tables end up in different sorted order, since the node below a mutation can change after extending haplotypes, and hence the mutation's position in the table after the new sorting.

[benjeffery]

On already sorted tables (French-Canadian dataset) this branch adds about 40% to the sort time. I can do some more digging with shuffled tables if you think it is worth it.

[petrelharp]

I should really just try it with SLiM. I can just copy over the code in this branch to try it, right?

[benjeffery]

I should really just try it with SLiM. I can just copy over the code in this branch to try it, right?

Yep, no changes to external interfaces. Just copying tables.c should do it.

[petrelharp]

I'm getting no measurable* difference with this change in SLiM's behavior with some fairly mutation-sort-intensive sims. So I think we're good.

*(with admittedly not a terribly sensitive measurement)

[benjeffery]

Rebased and squashed - I think we just need your green tick here @jeromekelleher