Retain unary nodes with simplify()

[gtsambos]

simplify() now takes a filter_unary argument (default: True). If False, all unary nodes are retained in the simplification.

The Python implementation in python/tests/simplify.py is now more or less finished; the tests in python/tests/test_topology.py are still under heavy development.

[jeromekelleher]

This looks like very good progress @gtsambos, I think you're definitely on the right track. A few minor points:

• Can you delete the .log files please?
• It's worth while running flake8 on your code on your computer before committing, as this will save some hassle with failed tests on CircleCI (we check for style conformance automatically as part of the tests to keep the code reasonably uniform). See here for how to invoke it.

Overall: looking good!

[gtsambos]

Cool, thanks @jeromekelleher! Just to clarify: should flake8 return no warnings/errors at all? I'm only checking for line length, like this:

flake8 --max-line-length 89 tests/test_topology.py 

[jeromekelleher]

Cool, thanks @jeromekelleher! Just to clarify: should flake8 return no warnings/errors at all? I'm only checking for line length, like this:

Yep, flake8 should be perfectly happy with the code. It's useful to add the check to your git pre-commit hook. Here's the last few lines of my pre-commit:

# Bunch of other stuff from default hook.

# NOTE: the flake8 check MUST come before the git diff-index bit below!
exec python3 -m flake8 --max-line-length=89 python/tskit python/tests

# If there are whitespace errors, print the offending file names and fail.
exec git diff-index --check --cached $against --

[codecov]

Codecov Report

Merging #143 into master will decrease coverage by 0.01%.
The diff coverage is 54.54%.

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
- Coverage   85.95%   85.93%   -0.02%     
==========================================
  Files          16       16              
  Lines        8707     8718      +11     
  Branches     1670     1674       +4     
==========================================
+ Hits         7484     7492       +8     
- Misses        732      733       +1     
- Partials      491      493       +2

Flag	Coverage Δ
#c_tests	85.93% <54.54%> (-0.02%)	⬇️
#python_tests	98.26% <ø> (ø)	⬆️

Impacted Files	Coverage Δ
python/tskit/trees.py	98.59% <ø> (ø)	⬆️
python/tskit/tables.py	99.79% <ø> (ø)	⬆️
c/tskit/tables.c	77.6% <54.54%> (-0.02%)	⬇️

---

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 bebe3c1...7ca4cce. Read the comment docs.

[jeromekelleher]

This looks very good, thanks @gtsambos! My only request would be to change the visual indents to hanging indents, but it's a question of taste so I won't push too hard.

The next step is to work on the C code I think.

[jeromekelleher]

OK, I think this is ready to go! Thanks for some really excellent work @gtsambos, everything worked perfectly. The only change I had to make is to rename the feature keep_unary, which was forced by the way the C library is set up (the default needed to be "no flags", and this wasn't possible if the flag was FILTER_UNARY --- I should have thought of this originally). I've pushed through all the changes needed including documentation, which may be a useful reference later if you're adding other features to simplify.

@petrelharp, would you mind taking one last look and merging if you're happy?

[jeromekelleher]

Bumping this; @petrelharp, are you happy for us to merge?

[gtsambos]

Hey @jeromekelleher, when I run the tests in python/tests/test_topology with your latest commit, I get a lot of errors. I'll investigate further and let you know.

[gtsambos]

It looks to be something to do with the inputs you're allowed to make to the implementations of simplify in the other libraries that are compared with compare_lib.
Here's an example error:

6200D-132482-M:python gtsambos$ python -m nose -vs tests/test_topology.py:TestSimplify.test_single_tree
test_single_tree (tests.test_topology.TestSimplify) ... ERROR

======================================================================
ERROR: test_single_tree (tests.test_topology.TestSimplify)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/gtsambos/Projects/tskit/python/tests/test_topology.py", line 2736, in test_single_tree
    self.verify_no_samples(ts)
  File "/Users/gtsambos/Projects/tskit/python/tests/test_topology.py", line 2676, in verify_no_samples
    ts, samples=ts.samples(), keep_unary=keep_unary)
  File "/Users/gtsambos/Projects/tskit/python/tests/test_topology.py", line 2632, in do_simplify
    map_nodes=True)
  File "/Users/gtsambos/Projects/tskit/python/tskit/trees.py", line 2968, in simplify
    keep_unary=keep_unary)
  File "/Users/gtsambos/Projects/tskit/python/tskit/tables.py", line 1647, in simplify
    keep_unary=keep_unary)
TypeError: function takes at most 5 arguments (6 given)

----------------------------------------------------------------------

[jeromekelleher]

The issue here is that your low-level C module is out of date and needs to be rebuilt @gtsambos, as I've updated the C code and this isn't compiled automatically. Try running 'make'; this should recompile everything and resolve the problems. (You might need to 'make clean' first).

[gtsambos]

Ah, right 😅

[jeromekelleher]

Merging this; thanks @gtsambos!