Document numpy traversals by benjeffery · Pull Request #1952 · tskit-dev/tskit

benjeffery · 2021-11-26T12:43:46Z

Added for follow up work tskit-dev/tutorials#150

codecov · 2021-11-26T13:02:48Z

Codecov Report

Merging #1952 (7ff7a17) into main (8bb1b5d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1952   +/-   ##
=======================================
  Coverage   93.32%   93.32%           
=======================================
  Files          27       27           
  Lines       25212    25212           
  Branches     1108     1108           
=======================================
  Hits        23530    23530           
  Misses       1648     1648           
  Partials       34       34

Flag	Coverage Δ
c-tests	`92.32% <ø> (ø)`
lwt-tests	`89.14% <ø> (ø)`
python-c-tests	`71.68% <ø> (ø)`
python-tests	`98.80% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
python/tskit/trees.py	`97.79% <ø> (ø)`

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 8bb1b5d...7ff7a17. Read the comment docs.

hyanwong · 2021-11-26T14:01:19Z

Yay, I was just about to do this @benjeffery but you beat me to it!

A couple of things:

you might be able to save some repetition by nuking some of the text in the descriptions of orders in the Tree.nodes() docketing, and simply pointing to these new methods, e.g.

... The available orders are:

        - 'preorder': Standard preorder traversal, see :meth:`Tree.preorder`.
        - ...

The docs don't say what we do if there are multiple roots in a tree. I think we do everything from one root, then everything from the next root, etc, but don't guarantee the order in which we iterate over the roots, right? This could also do with clarifying for e.g. inorder and levelorder in the Tree.nodes() docketing, but that's a different issue I guess.

jeromekelleher

Looks good, needs some tweaks as the semantics aren't quite right.

jeromekelleher · 2021-11-26T15:10:17Z

+        Also see :ref:`tutorials:sec_analysing_trees_traversals` for examples of how
+        to use traversals.
+
+        :param int u: The node to start from, defaults to the virtual root.


This isn't quite right. If we start from the virtual root we return the virtual root in the arrays, but this isn't what we usually want. It's more like "if specified, return all nodes in the subtree rooted at u (including u) in traversal order".

jeromekelleher · 2021-11-26T15:14:34Z

-    # https://github.com/tskit-dev/tskit/issues/1788
    def preorder(self, u=NULL):
+        """
+        Returns a numpy array of node ids. Starting at `u`, yield the current node,


This isn't quite right I think. How about

Returns a number array of node ids in preorder <https://en.wikipedia.org/wiki/Tree_traversal#Pre-order_(NLR)>. If the node u the traversal is rooted at this node (and it will be the first element in the returned array). Otherwise, all nodes reachable from the tree roots will be returned. See :ref:tutorials:sec_analysing_trees_traversals for examples.

I don't think need to describe what "preorder" is, the wikipedia link is fine. I think talking about "yielding" here is more confusing, since this isn't a generator.

benjeffery · 2021-12-01T12:20:53Z

Should be good to go now

hyanwong · 2021-12-01T12:44:00Z

+        <https://en.wikipedia.org/wiki/Tree_traversal##Post-order_(LRN)>. If the node u
+        is specified the traversal is rooted at this node (and it will be the first
+        element in the returned array). Otherwise, all nodes reachable from the tree
+        roots will be returned. See :ref:tutorials:sec_analysing_trees_traversals for


Does this need backticks:

:ref:`tutorials:sec_analysing_trees_traversals`

hyanwong · 2021-12-01T12:44:40Z

+        <https://en.wikipedia.org/wiki/Tree_traversal#Pre-order_(NLR)>. If the node u
+        is specified the traversal is rooted at this node (and it will be the first
+        element in the returned array). Otherwise, all nodes reachable from the tree
+        roots will be returned. See :ref:tutorials:sec_analysing_trees_traversals for


Does this need backticks:

:ref:`tutorials:sec_analysing_trees_traversals`

Yes it does

hyanwong

Trivial nitpicks

hyanwong · 2021-12-01T12:51:02Z

+        """
+        Returns a number array of node ids in postorder
+        <https://en.wikipedia.org/wiki/Tree_traversal##Post-order_(LRN)>. If the node u
+        is specified the traversal is rooted at this node (and it will be the first


"last element"?

hyanwong · 2021-12-01T12:56:28Z

Should be good to go now

Great. It might be good to add something about the order of traversal if there are multiple roots. Something like:

Returns a 1D numpy array of node ids in preorder
<https://en.wikipedia.org/wiki/Tree_traversal#Pre-order_(NLR)>. If the node u
is specified the traversal is rooted at this node (and it will be the first
element in the returned array). Otherwise, return an array in which the node
in preorder reachable from each tree root have been concatenated together.

but I can add that as a further PR if you like. It would be good to merge this, then I can push my doc PR properly.

hyanwong

More nitpicks, sorry!

benjeffery · 2021-12-01T14:47:26Z

Thanks @hyanwong should be good now

jeromekelleher · 2021-12-01T14:52:43Z

Needs another pass @benjeffery

jeromekelleher · 2021-12-01T14:54:18Z

We should discuss the semantics of the virtual root too - but, possibly simpler to merge this once the cross references have been fixed so that @hyanwong can merge in his version?

benjeffery · 2021-12-01T15:30:36Z

Fixed

benjeffery · 2021-12-01T16:33:24Z

@Mergifyio rebase

mergify · 2021-12-01T16:35:42Z

rebase

✅ Branch has been successfully rebased

benjeffery · 2021-12-01T16:44:16Z

Ok, hopefully that does it. FML.

hyanwong · 2021-12-01T17:44:10Z

Do you want me to merge stuff into this before it's merged into main, or simply make a follow-up PR? I've never done the former, but happy to try.

jeromekelleher · 2021-12-01T19:05:58Z

Let's do a follow-up @hyanwong

hyanwong reviewed Nov 26, 2021

View reviewed changes

Comment thread python/tskit/trees.py Outdated

hyanwong approved these changes Nov 26, 2021

View reviewed changes

jeromekelleher reviewed Nov 26, 2021

View reviewed changes

benjeffery force-pushed the doc_traversals branch 2 times, most recently from e6e24b7 to 2f60e98 Compare December 1, 2021 12:12

hyanwong reviewed Dec 1, 2021

View reviewed changes

Comment thread python/tskit/trees.py Outdated

hyanwong reviewed Dec 1, 2021

View reviewed changes

Comment thread python/tskit/trees.py Outdated

hyanwong reviewed Dec 1, 2021

View reviewed changes

benjeffery force-pushed the doc_traversals branch from 2f60e98 to b6364f8 Compare December 1, 2021 14:46

jeromekelleher reviewed Dec 1, 2021

View reviewed changes

Comment thread python/tskit/trees.py Outdated

benjeffery force-pushed the doc_traversals branch from b6364f8 to e2f3424 Compare December 1, 2021 15:30

benjeffery added the AUTOMERGE-REQUESTED label Dec 1, 2021

benjeffery force-pushed the doc_traversals branch from e2f3424 to c9abc72 Compare December 1, 2021 16:35

benjeffery force-pushed the doc_traversals branch from c9abc72 to c943632 Compare December 1, 2021 16:43

jeromekelleher approved these changes Dec 1, 2021

View reviewed changes

Document numpy traversals

7ff7a17

benjeffery force-pushed the doc_traversals branch from c943632 to 7ff7a17 Compare December 1, 2021 19:07

mergify Bot merged commit 1d3f6de into tskit-dev:main Dec 1, 2021

mergify Bot removed the AUTOMERGE-REQUESTED label Dec 1, 2021

Conversation

benjeffery commented Nov 26, 2021

Uh oh!

codecov Bot commented Nov 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hyanwong commented Nov 26, 2021

Uh oh!

Uh oh!

jeromekelleher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjeffery commented Dec 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hyanwong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hyanwong commented Dec 1, 2021

Uh oh!

Uh oh!

Uh oh!

hyanwong left a comment

Choose a reason for hiding this comment

Uh oh!

benjeffery commented Dec 1, 2021

Uh oh!

Uh oh!

jeromekelleher commented Dec 1, 2021

Uh oh!

jeromekelleher commented Dec 1, 2021

Uh oh!

benjeffery commented Dec 1, 2021

Uh oh!

benjeffery commented Dec 1, 2021

Uh oh!

mergify Bot commented Dec 1, 2021

✅ Branch has been successfully rebased

Uh oh!

benjeffery commented Dec 1, 2021

Uh oh!

hyanwong commented Dec 1, 2021

Uh oh!

jeromekelleher commented Dec 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Nov 26, 2021 •

edited

Loading