Bug in simplify with KEEP_UNARY_IN_INDIVIDUALS

[hyanwong]

Yes, this really is a simplify bug, which is my own fault. In f2d906d I inserted a check to see if the node being simplified has an individual:

tskit/c/tskit/tables.c

Line 6783 in 6c3fdc4

&& (self->tables->nodes.individual[input_id] != TSK_NULL)) {

But it seems that input_id here does not correspond to the node number in the current self->tables->nodes - I'm assuming it's been changed already by some previous node jiggling in the simplification algorithm, but the tables haven't been updated. (see below: I'm not sure this is the problem, actually)

This is hard to test in python, because keep_unary_in_individuals is not an option to the simplify command yet: it's only in the C interface. But hopefully someone who knows about simplify can verify that my triaging is correct. Otherwise I can post a tree sequence where this causes a problem, and link to my branch where I've implemented keep_unary_in_individuals in python.

[hyanwong]

The TS file is at keep_unary_in_individuals_test.trees.zip, which is a ts which has been slimmed down to 1 tree. Node 3 is a unary node within individual 1, and it is on the path from node 25 to a root (node 15), so it should be kept in. When I try

ts.simplify([n.id for n in ts.nodes() if n.time==0], keep_unary_in_individuals=True).num_individuals

With my modified python tskit, that node disappears. In fact, when we get into simplifier_merge_ancestors(simplifier_t *self, tsk_id_t input_id) for input_id 3, self->tables->nodes.individual[3] equals TSK_NULL. Perhaps that means I'm wrong in my guess above. It looks like node 3 doesn't have an individual in self->tables->nodes.individual after all. So the node table itself has changed by this point.

[hyanwong]

Aha!

I think it should be

self->input_tables.nodes[input_id]

(not self->tables->nodes[input_id])

[benjeffery]

@hyanwong Can we get a PR for the python implementation before the bug fix? Would be good to separate those.

[hyanwong]

@hyanwong Can we get a PR for the python implementation before the bug fix? Would be good to separate those.

You mean the implementation complete with failing tests. Then the bug fix later to check the tests pass?

[benjeffery]

Just the implementation of keep_unary_in_individuals in python, without the tests for this bug.

[hyanwong]

Oh, OK. I have these. Will PR in 45 mins,.

[hyanwong]

@benjeffery do you also want a separate PR with the (trivial) fix?

[benjeffery]

Yep, fix and tests please! Thanks!

[hyanwong]

Separate PRs, or together? Together will take some time, as I actually have to write the tests :) Oh, and re-implement in the python version of simplify(), I guess?

[benjeffery]

All three in one PR is fine! 😁