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

Deprecate Tree.num_nodes? #1966

Closed
hyanwong opened this issue Nov 29, 2021 · 9 comments · Fixed by #1968
Closed

Deprecate Tree.num_nodes? #1966

hyanwong opened this issue Nov 29, 2021 · 9 comments · Fixed by #1968
Labels
Python API Issue is about the Python API

Comments

@hyanwong
Copy link
Member

Even though it's documented, I think it's super confusing that Tree.num_nodes returns the number of nodes in the entire tree sequence, not the number of nodes in the tree itself. I suggest we might want to deprecate it and say that the user should use Tree.tree_sequence.num_nodes instead, which is much clearer.

@jeromekelleher jeromekelleher added the Python API Issue is about the Python API label Nov 29, 2021
@jeromekelleher
Copy link
Member

That is confusing, I agree. Happy to mark it as deprecated/undocument it, etc.

@hyanwong
Copy link
Member Author

My vote would be to deliberately mark it for deprecation, rather than simply undocument it, because there's a good chance that it's actually an error if it's being used.

@jeromekelleher
Copy link
Member

OK. I don't think we have a formal deprecation process yet. Let's use the sphinx directive for now, "deprecated since 0.4.0"

@hyanwong
Copy link
Member Author

Cool. I was going to issue a warnings.warn on use too, like we do if you use the impute_missing_data parameter for haplotypes. Reasonable?

@benjeffery
Copy link
Member

Might be worth using https://deprecation.readthedocs.io/en/latest/ to check we are emiting the right kind of warning.

hyanwong added a commit to hyanwong/tskit that referenced this issue Nov 29, 2021
hyanwong added a commit to hyanwong/tskit that referenced this issue Nov 29, 2021
@molpopgen
Copy link
Member

OK. I don't think we have a formal deprecation process yet. Let's use the sphinx directive for now, "deprecated since 0.4.0"

Raising a FutureWarning when accessing it may also be handy??

hyanwong added a commit to hyanwong/tskit that referenced this issue Nov 29, 2021
@hyanwong
Copy link
Member Author

FutureWarning is a great idea, thanks @molpopgen - I've just incorporated it.

@molpopgen
Copy link
Member

Great. This is one of python's great gotchas. DeprecationWarning seems like what you want, but it is suppressed by default because it is only for developers.

@mergify mergify bot closed this as completed in 64cb77d Nov 30, 2021
hyanwong added a commit to hyanwong/tskit that referenced this issue Nov 30, 2021
@hyanwong
Copy link
Member Author

Wrongly tagged by me as fixed by 64cb77d

@hyanwong hyanwong reopened this Nov 30, 2021
hyanwong added a commit to hyanwong/tskit that referenced this issue Nov 30, 2021
hyanwong added a commit to hyanwong/tskit that referenced this issue Nov 30, 2021
hyanwong added a commit to hyanwong/tskit that referenced this issue Nov 30, 2021
hyanwong added a commit to hyanwong/tskit that referenced this issue Nov 30, 2021
hyanwong added a commit to hyanwong/tskit that referenced this issue Nov 30, 2021
hyanwong added a commit to hyanwong/tskit that referenced this issue Dec 1, 2021
hyanwong added a commit to hyanwong/tskit that referenced this issue Dec 1, 2021
jeromekelleher pushed a commit to hyanwong/tskit that referenced this issue Dec 2, 2021
@mergify mergify bot closed this as completed in #1968 Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Python API Issue is about the Python API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants