Add include_terminal param

[hyanwong]

Fixes #783

[hyanwong]

Exciting segfault here. Will investigate!

[hyanwong]

So I've been fiddling with this for a good while, and it doesn't segfault on my machine, so I can't work out why it does on circleCI, especially as it fails on an old test, and I have defaulted to unchanged behaviour. Any help spotting my mistake here would be vrey gratefully accepted @benjeffery or @jeromekelleher. It would be helpful to have this PR working so that I can sort out the KC polytomy-breaking stuff for Wilder.

[benjeffery]

I can look at this - hopefully will be able to recreate!

[hyanwong]

Thanks so much. I wouldn't normally pester, but I've been banging my head against it for a few hours now!

[benjeffery]

My previous traumatic experiences with the Python C API have helped here. A close reading of https://docs.python.org/3/c-api/arg.html#other-objects shows that an int is needed to receive a bool from python. I've committed a fix to your branch.

[hyanwong]

Thanks. I'd never have caught that!

[jeromekelleher]

Looks good, but needs more tests. Definitely on the C side we want to exercise this on a few different topology types - what happens on an empty ts? An ordinary ts with several trees? We have to think through all the corner cases.

[hyanwong]

Looks good, but needs more tests. Definitely on the C side we want to exercise this on a few different topology types - what happens on an empty ts? An ordinary ts with several trees? We have to think through all the corner cases.

Can we not test an empty ts, multi-tree ts etc at the python level? We have a function get_example_tree_sequences() in test_highlevel.py which can return various pathological tree sequences like this. Actually, having looked at it, I see it doesn't test a ts with no edges, so I'll open a PR for that. (edit - now in #792 )

[jeromekelleher]

Can we not test an empty ts, multi-tree ts etc at the python level? We have a function get_example_tree_sequences() in test_highlevel.py which can return various pathological tree sequences like this.

No, because we can't run valgrind when Python is running and would therefore not detect memory access issues (which are non-deterministic and differ across platforms, so you can't just say "it doesn't segfault, so it's fine").

There's loads of tests checking for empty tree sequences across the suite - the test_highlevel.py approach isn't how we're doing things any more.

[hyanwong]

No, because we can't run valgrind when Python is running and would therefore not detect memory access issues (which are non-deterministic and differ across platforms, so you can't just say "it doesn't segfault, so it's fine").

Ah, OK. Apart from your suggestion above for all parent[j] == -1, I'm not familiar with the way you might check memory issues here. But now that I have implemented that, perhaps I can just add test_multiroot_diff_iter and test_empty_diff_iter functions to test_trees.c?

There's loads of tests checking for empty tree sequences across the suite - the test_highlevel.py approach isn't how we're doing things any more.

Added anyway: I think there's no harm, and a function to produce a whole set of different semi-pathological tree sequences seems a useful thing to have around.

[jeromekelleher]

Ah, OK. Apart from your suggestion above for all parent[j] == -1, I'm not familiar with the way you might check memory issues here. But now that I have implemented that, perhaps I can just add test_multiroot_diff_iter and test_empty_diff_iter functions to test_trees.c?

Once the tests are implemented valgrind is run on them automatically, checking for these errors.

[benjeffery]

Think I found am incorrect variable usage in the python C layer, and one comment about test style.

[benjeffery]

Looking good, thanks @hyanwong! We just need a changelog before merge.

[hyanwong]

Looking good, thanks @hyanwong! We just need a changelog before merge.

Done, thanks. Note that this includes the change in #788 as the first commit in the PR, so that can be PR can be closed if this is merged.

[jeromekelleher]

Let's hold off on this until our bugfix releases have been shipped.

[hyanwong]

I think this is ready to merge @benjeffery ?

[benjeffery]

LGTM, lets merge.

[benjeffery]

@hyanwong I've just force pushed to resolve the conflict and add the change to the C changelog too as this is a change to both APIs.