test for time with keep_roots

[petrelharp]

There appears to be a bug related to mutation times and simplify(keep_input_roots=True). This PR modifies a test so that it triggers the problem: simplify changes the node the mutation is on, if that node is retained as an input root. Here's a simple example:

import tskit

t = tskit.TableCollection(sequence_length=1)
t.nodes.add_row(time=0, flags=1)
t.nodes.add_row(time=1, flags=0)
t.nodes.add_row(time=2, flags=0)
t.edges.add_row(parent=1, child=0, left=0, right=1)
t.edges.add_row(parent=2, child=1, left=0, right=1)
t.sites.add_row(position=0, ancestral_state='0')
t.mutations.add_row(site=0, derived_state='1', time=2, node=2)

print(t.mutations)
# id	site	node	time	derived_state	parent	metadata
# 0	0	2	2.0	1	-1	

t.simplify(keep_input_roots=True)
print(t.mutations)
# id	site	node	time	derived_state	parent	metadata
# 0	0	0	2.0	1	-1	

The node should be 1 afterwards, not 0, and doing t.tree_sequence() throws

_tskit.LibraryError: A mutation's time must be < the parent node of the edge on which it occurs, or be marked as 'unknown'

I'll see if I can track this down now...

[petrelharp]

doh!

[jeromekelleher]

Double Doh!

[petrelharp]

I guess this slipped through the cracks because assigning a mutation to the root node instead of the node-above-the-root doesn't change anything except for the genotype of the node-above-the-root-node.I only saw it because mutation.time caused it to throw an error!

[codecov]

Codecov Report

Merging #823 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #823   +/-   ##
=======================================
  Coverage   93.57%   93.57%           
=======================================
  Files          24       24           
  Lines       19545    19545           
  Branches      789      789           
=======================================
  Hits        18290    18290           
  Misses       1223     1223           
  Partials       32       32           

Impacted Files	Coverage Δ
c/tskit/tables.c	90.60% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 961c941...6778639. Read the comment docs.

[jeromekelleher]

Dammit! Great catch, thanks @petrelharp. Looks like we need to get a bugfix release out for this.

Can you add your example above in as an explicit standalone test? I think it's good to keep simple tests around that show these bugs, just in case we ever regress.

[petrelharp]

I've fixed up the python version of simplify to agree, and added tests for above-the-root mutations here (and, elsewhere, since I've added them to tsutil.insert_branch_mutations, miraculously not breaking anything!). So... maybe this is ready to go? If others and/or CI don't spot anything?

[petrelharp]

Can you add your example above in as an explicit standalone test?

Well, I was going to, but instead I modified this test to do pretty much exactly that. I think it's a simple and stand-along enough test?

[petrelharp]

Gah, as noted elsewhere my pre-commit is broken...

[jeromekelleher]

Well, I was going to, but instead I modified this test to do pretty much exactly that. I think it's a simple and stand-along enough test?

Sure, sounds good. The tests are pretty strong now you've added mutations over the root to the insert mutations method.

[jeromekelleher]

Ready to go, if you could squash, please

[benjeffery]

Added to 0.99.6 which is shaping up to be a bugfix release.

[petrelharp]

Squashed; should be ready.

[jeromekelleher]

Thanks for cleaning up my mess @petrelharp! We'll get this bundled into a C release in the next few days so you can pull it into SLiM.