Set keep_unary on simplify, and document (including path compression docs)

[hyanwong]

We should also switch simplify in these cases to set keep_unary=True, and document accordingly.

[codecov]

Codecov Report

Merging #176 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #176      +/-   ##
==========================================
+ Coverage   93.93%   93.93%   +<.01%     
==========================================
  Files          11       11              
  Lines        2885     2886       +1     
  Branches      507      507              
==========================================
+ Hits         2710     2711       +1     
  Misses        145      145              
  Partials       30       30

Impacted Files	Coverage Δ
tsinfer/inference.py	99.02% <100%> (ø)	⬆️
tsinfer/eval_util.py	88.77% <100%> (+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 4d13b81...899ebf5. Read the comment docs.

[hyanwong]

@jeromekelleher - do we want to document the progress_monitor parameter too? E.g. at the moment the documentation uses something like:

def match_ancestors(
        sample_data, ancestor_data, progress_monitor=None, num_threads=0,
        path_compression=True, extended_checks=False, engine=constants.C_ENGINE):
    """
    match_ancestors(sample_data, ancestor_data, num_threads=0, path_compression=True)
    ...
    """

And (weirdly to me), the progress_monitor param, which is 3rd, is omitted from the docstring. Presumably this means that people using this function without named parameters will be misled into thinking that they can specify num_threads by setting it as a 3rd (unnamed) parameter.

[jeromekelleher]

do we want to document the progress_monitor parameter too?

No, this is too complicated, will probably change over time and is really for internal use. We don't want people using this.

And (weirdly to me), the progress_monitor param, which is 3rd, is omitted from the docstring. Presumably this means that people using this function without named parameters will be misled into thinking that they can specify num_threads by setting it as a 3rd (unnamed) parameter.

That is weird --- we should change the signature of the function to match the docs.

[hyanwong]

we should change the signature of the function to match the docs.

Fine. I'll do this in the same PR shall I? We don't mind changing the order in this case? I guess we put it either before or after the (similarly an correctly undocumented) engine param?

[jeromekelleher]

Fine. I'll do this in the same PR shall I? We don't mind changing the order in this case? I guess we put it either before or after the (similarly an correctly undocumented) engine param?

That would be good, thanks.

[hyanwong]

This now works and passes unit tests on my machine, but since keep_unary is only in tskit 0.2.0, it will probably fail CI. There's also a small change to get the tests to pass under 0.2.0, now that tskit.MISSING_DATA is output for isolated nodes, which what 2dbac3a is all about. I guess we will wait for the 0.2.0 release until merging this PR. Is that likely to be soon, @jeromekelleher ?

[jeromekelleher]

tskit 0.2.0 should be shipped in a week or so --- best to wait for it to avoid complications.

[hyanwong]

Right - perfect. Thanks.

[jeromekelleher]

Closes #146