Skip to content

Conversation

@daniel-goldstein
Copy link
Member

@daniel-goldstein daniel-goldstein commented Apr 23, 2020

Now t.depth(u) returns the depth of node u in tree t, where depth is defined as the number of nodes on the path from u to the root, not including u.

@daniel-goldstein
Copy link
Member Author

@jeromekelleher Should I be using a uint32_t (or some other int type) for depth and cast it to an unsigned int when building the python value?

@codecov
Copy link

codecov bot commented Apr 23, 2020

Codecov Report

Merging #554 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #554      +/-   ##
==========================================
+ Coverage   87.18%   87.20%   +0.01%     
==========================================
  Files          21       21              
  Lines       16233    16251      +18     
  Branches     3184     3187       +3     
==========================================
+ Hits        14153    14171      +18     
  Misses       1020     1020              
  Partials     1060     1060              
Flag Coverage Δ
#c_tests 88.37% <100.00%> (+0.01%) ⬆️
#python_c_tests 90.30% <100.00%> (+0.01%) ⬆️
#python_tests 99.19% <100.00%> (+<0.01%) ⬆️
Impacted Files Coverage Δ
c/tskit/trees.c 90.89% <100.00%> (+0.03%) ⬆️
python/_tskitmodule.c 83.95% <100.00%> (+0.02%) ⬆️
python/tskit/trees.py 98.64% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad50fdf...176218e. Read the comment docs.

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent! Just a few very minor comments.

@jeromekelleher
Copy link
Member

@petrelharp, do you agree with this definition of Tree.depth(u):

Returns the number of nodes on the path from u to root.

I think the PR is uncontroversial otherwise, no need to look over it.

@jeromekelleher
Copy link
Member

Actually, now that I look at it that definition is wrong! Maybe "Returns the number of nodes on the path from u to root, not including u. Thus, the depth of u is 0 if u is a root."

@daniel-goldstein
Copy link
Member Author

Good catch! In the code I used the wording "above u" but I think it's better to just be more explicit and say not including u.

@jeromekelleher
Copy link
Member

Good catch! In the code I used the wording "above u" but I think it's better to just be more explicit and say not including u.

"Above" is tricky, because a lot of people think of trees with the root at the bottom (I don't know where they got that from!), so it's good to avoid it if you can. Sometimes it just makes things unreadable if you don't choose a direction though.

@petrelharp
Copy link
Contributor

Returns the number of nodes on the path from u to root, not including u. Thus, the depth of u is 0 if u is a root.

I agree with this, except that it should say "from u to a root". (probably what you meant to write).

@jeromekelleher jeromekelleher merged commit c7d3a13 into tskit-dev:master Apr 24, 2020
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

Successfully merging this pull request may close these issues.

3 participants