Decapitate2

[jeromekelleher]

Stacked on #2302.

Finally, decapitate fully implemented!

The testing is perhaps a bit overboard, but I had implemented all of this in #2240 and it seemed a waste to just throw it away.

[jeromekelleher]

Will update once #2302 iis merged.

TODO: update changelog.

[codecov]

Codecov Report

Merging #2331 (55c4def) into main (830c9e7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2331   +/-   ##
=======================================
  Coverage   93.26%   93.26%           
=======================================
  Files          28       28           
  Lines       26636    26642    +6     
  Branches     1213     1213           
=======================================
+ Hits        24842    24848    +6     
  Misses       1762     1762           
  Partials       32       32           

Flag	Coverage Δ
c-tests	92.25% <ø> (ø)
lwt-tests	89.05% <ø> (ø)
python-c-tests	71.35% <16.66%> (-0.03%)	⬇️
python-tests	98.87% <100.00%> (+<0.01%)	⬆️

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

Impacted Files	Coverage Δ
python/tskit/tables.py	98.89% <ø> (ø)
python/tskit/trees.py	98.18% <100.00%> (+<0.01%)	⬆️

---

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 830c9e7...55c4def. Read the comment docs.

[petrelharp]

Whew! Looks good!

[jeromekelleher]

Ready for final review @benjeffery, if you'd like to take a look.

[benjeffery]

LGTM. Really nice testing here. I tried for a while to think of a case that wasn't covered and failed!

As a thought about the flags/population/metadata of new nodes, how come population is the only one that can inherit from the retained node? Sorry if you've explained elsewhere, but I don't see why that wouldn't be useful for flags and metadata. I also wonder why it is the child information that gets used, would it not be the deleted parent?

[jeromekelleher]

As a thought about the flags/population/metadata of new nodes, how come population is the only one that can inherit from the retained node? Sorry if you've explained elsewhere, but I don't see why that wouldn't be useful for flags and metadata. I also wonder why it is the child information that gets used, would it not be the deleted parent?

Good questions! Population is different here I think because, unless there is other information in the model, we'd assume that any genome (node) that exists along the edge will be in the same population as the child. Flags and metadata are different in that we're probably not making these kinds of assumptions about processes.

Why it inherits from the child rather than the parent is because of the generally half-openness of intervals.

The more I try to explain this, the weaker the reasoning is though...

Maybe we should also default the population to -1, and file an issue to track adding functionality later to allow copying from the child or parent, like maybe something like:

def decapitate, time, *, flags=None, population=None, metadata=None, copy_parent=False, copy_child=False):
    """
    If flags, population or metadata are not specified and ``copy_parent`` is True, copy the value from the edge's 
    parent node (likewise for copy_child). If values are explicitly provided for any of the node fields, the field 
    in question will **not** be copied from the child/parent.
    """

@petrelharp, any thoughts?

[petrelharp]

I think you're right - defaulting to -1 is the right way to go. I don't see a strong use case for copy_parent now, either.

[jeromekelleher]

OK, opened a new issue to track this before 0.5 (#2334) as I can't bear updating this PR any more!