Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
molpopgen opened this issue Jun 9, 2021 · 7 comments
Open

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

molpopgen opened this issue Jun 9, 2021 · 7 comments
Milestone

Comments

@molpopgen
Copy link
Member

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
Copy link
Member

benjeffery commented Jun 9, 2021

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
Copy link
Member Author

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
Copy link
Member

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

@molpopgen
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

Shall we clarify the doc string a bit then?

@jeromekelleher jeromekelleher added this to the 1.0.2 milestone Jun 11, 2021
@jeromekelleher
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants