Add kwargs to tree accessors

[hyanwong]

Fixes #847

No tests. What, if any, would we like?

[petrelharp]

It should be easy to put in a quick test that a few of those kwargs are actually having the desired effect? e.g. that the sample lists are there.

[benjeffery]

I think this is ok - I want to be very careful that we don't creep to an API full of **kwargs to the point where finding the docs on what you can actually pass in is hard. (Looking at you, matplotlib!)

[petrelharp]

I want to be very careful that we don't creep to an API full of **kwargs to the point where finding the docs on what you can actually pass in is hard.

Oh, god, good point. Can we make sure to stick in something that will produce a link to where the kwargs are actually listed? (is this :class:.Tree or :meth:.TreeSequence.trees?)

[hyanwong]

Oh, god, good point. Can we make sure to stick in something that will produce a link to where the kwargs are actually listed? (is this :class:.Tree or :meth:.TreeSequence.trees?)

Yes, I think this is in the original PR. I linked thew **kwargs info to :class:``Tree`` which is where the init params are listed, I think.

[hyanwong]

I just realised that, actually, the main trees() method should also pass **kwargs to the Trees constructor, instead of explicitly listing the parameters in the definition, because then we don't miss any parameters. For example, the Trees constructor has a new parameter, root_threshold, which you currently can't set when running trees(), and I guess we might want to allow that?

That's independent of the actual documentation of the trees iterator, though. We might well want to leave the parameters listed there, as it's such a commonly used method. Or if not, I guess we would want to give examples?

[benjeffery]

In my view trees() should explicitly have all arguments listed. There should be a test to check that the two methods have the same signature so when things are added to Tree we add it to trees().

[hyanwong]

In my view trees() should explicitly have all arguments listed. There should be a test to check that the two methods have the same signature so when things are added to Tree we add it to trees().

If we can test for it, that seems a good approach.

[hyanwong]

In my recent push, I've added the missing root_threshold param to .trees() and a test to check that all the args used to initialise a Tree are present as the first args of ts.tree(). Also added to the tests are some (rather hacky IMO) checks that tracked_samples is passed though correctly. It's hard to test sample_lists, as setting this to True or False simply activates different algorithmic approaches, of different speeds, to the same end result.

I think #851 should be merged first, though, so that we can properly match up the order of parameters in .tree()

[benjeffery]

Signature test was almost exactly what I was thinking. I've force pushed a small change to make it more explicit. Thanks @hyanwong!

[hyanwong]

Thanks @benjeffery - you were working late!