tskit-dev / msprime

Simulate genealogical trees and genomic sequence data using population genetic models
GNU General Public License v3.0
172 stars 84 forks source link

sim_ancestry docs/defaults for "ploidy" are confusing #1717

Open molpopgen opened 3 years ago

molpopgen commented 3 years ago

The docs for msprime.sim_ancestry say that ploidy defaults to 2, yet the function defaults to None.

It would be preferable if the function default were acutally 2, especially for people using language servers, iPython/Jupyter.

It seems that this is the line to change:

    ploidy = 2 if ploidy is None else ploidy
benjeffery commented 3 years ago

We have this pattern a lot throughout msprime/tskit. The default is set to None to differentiate whether the user intended to specify "use the default value" or "use this specific value that happens to be the default".

As an example, the ploidy recorded in provenance is None if not specified currently. If we changed the default to 2 then the recorded ploidy in the provenance would be 2 and the information that this was selected by default is lost.

I agree this is less than ideal when it comes to docs and tooling though.

molpopgen commented 3 years ago

Then I'd say that it is the docs that are wrong here: the default is None, not 2, and the behavior is to use 2 if the default is passed through.

benjeffery commented 3 years ago

Yes the docstring could be clearer here, some thing like "if no ploidy is specified 2 is used" rather than "(Default=2)"

molpopgen commented 3 years ago

Yes the docstring could be clearer here, some thing like "if no ploidy is specified 2 is used" rather than "(Default=2)"

Yes, that would be better. Personally, I see this as violating the Python mantra of explicit being better than implicit.

jeromekelleher commented 3 years ago

I see where you're coming from here @molpopgen, but this is a hard-learned lesson for me. Unless you're certain a function is never going to get any more parameters or has really simple semantics, it's a mistake to embed the default in the signature. Certainly neither of these is true for sim_ancestry.

molpopgen commented 3 years ago

Shall we clarify the doc string a bit then?

jeromekelleher commented 3 years ago

Sure, I've added it to the next point release.