Add feature for searching upper layer parent node #24

DechinPhy · 2020-05-20T10:48:20Z

This is a new feature implemented.

flake8 passed with nothing return.
unit test passed with 100% coverage.

joowani

Thanks for the PR. The concept of using wtree seems very hacky. So I suggest exposing your new API as a public, importable function get_parent(root, node). The function itself could be simplified greatly (see comments).

Also, please squash all the commits into one, and change the commit messsage to something like "Add get_parent function".

Thanks!

joowani · 2020-05-20T21:06:48Z

binarytree/__init__.py

+    node_height = node.height
+    if root_height == node_height:
+        return None
+    node_level_list = root.levels[root_height - node_height]


Using Node.levels here is unnecessary and inefficient. You could search for parent using BFS instead.

I will try this but I think BFS maybe also not the most efficient way. If you agree with this, I can add BFS as the first stage searching algorithm and later update it if I found a more efficient way.

joowani · 2020-05-20T21:08:29Z

binarytree/__init__.py

+    start_index = node_index // 2
+    for pnode in parent_level_list[start_index:]:
+        try:
+            if pnode.left.value == node.value or\


What if there are multiple nodes with same values? Best way to check for object equality is using Python's is keyword.

That's ok to use is keyword and I will modify it. In my cases all nodes are with different values so I didn't consider about this.

joowani · 2020-05-20T21:10:24Z

binarytree/__init__.py

@@ -314,6 +315,38 @@ def _get_tree_properties(root):
    }


+def _get_parent_node(root, node):


Instead of using wtree (which feels very hacky to be honest), I suggest providing the functionality as a public function with signature get_parent(root, node).

What do you think about add the property up or I used upper for the Node object? Still using the function get_parent(root, node).

I don't think it can work cleanly as a property since you need access to the root and because we should not be mutating objects. I think a public function might be the only way.

joowani · 2020-05-20T21:10:43Z

binarytree/__init__.py

+        self._root = tree
+
+    @property
+    def upper(self):


Why not just call this parent?

Because when you call left you mean go to the left node and right to the right one. I just want to be the same style with these properties. eg. You can call up or upper to go to the upper node.

The most common convention is "parent". I've never seen the term "upper node" used in context of binary trees so it may confuse the users.

coveralls · 2020-05-21T06:42:34Z

Coverage remained the same at 100.0% when pulling 12455b3 on DechinPhy:master into 1d384dd on joowani:dev.

coveralls · 2020-05-21T06:42:34Z

Coverage remained the same at 100.0% when pulling 7c1e41a on DechinPhy:master into 1d384dd on joowani:dev.

DechinPhy

@joowani Thank you for your advice! According to what we talked about, the only change I commit is adding a function get_parent_node and correspond unittests. The rebase stuff can be done after review.

DechinPhy · 2020-05-21T06:44:56Z

binarytree/__init__.py

@@ -315,7 +314,7 @@ def _get_tree_properties(root):
    }


-def _get_parent_node(root, node):
+def get_parent_node(root, node):


The properties in Node class is removed and the function get_parent_node has been changed into a public function

binarytree/__init__.py

DechinPhy · 2020-05-21T06:47:45Z

binarytree/__init__.py

@@ -1064,57 +1082,6 @@ def size(self):
        """
        return _get_tree_properties(self)['size']

-    @property


Wtree and upper property removed.

DechinPhy · 2020-05-21T06:48:22Z

binarytree/exceptions.py

@@ -31,7 +31,3 @@ class NodeValueError(BinaryTreeError):

 class TreeHeightError(BinaryTreeError):
    """Tree height was invalid."""
-
-
-class WtreeUnsetError(BinaryTreeError):


Since we do not need a Wtree, the Error class should also be removed.

tests/test_tree.py

DechinPhy

git rebase works done.

joowani

Hi @DechinPhy,

Looking good. Just a few minor fixes and I think it will be ready for merging. Please squash the commit again. Thanks.

joowani · 2020-05-21T18:00:49Z

binarytree/__init__.py

+                 /
+                3
+    """
+    if root is node or root is None or node is None:


Is this if statement necessary?

This if just make it clear what will be the return when situations happens. I deleted this in new push.

joowani · 2020-05-21T18:02:42Z

tests/test_tree.py

+
+
+@pytest.mark.order14
+def test_get_parent_node():


You are missing some test cases (e.g. when root is None).

The test case root is None and node is None cases are added.

joowani · 2020-05-21T18:03:56Z

binarytree/__init__.py

+    if root is node or root is None or node is None:
+        return None
+    node_stack = []
+    while True:


I like this solution 👍

DechinPhy

Thank you for you advice again :)

DechinPhy · 2020-05-22T02:06:32Z

binarytree/__init__.py

+                 /
+                3
+    """
+    if node is None:


node == None case remained and necessary.

DechinPhy · 2020-05-22T02:07:08Z

tests/test_tree.py

+    assert get_parent_node(root, root.right.right) == root.right
+    assert get_parent_node(root, root.right) == root
+    assert get_parent_node(root, Node(5)) is None
+    assert get_parent_node(None, root.left) is None


Both test case of None node input is added.

joowani

Everything looks good. Thanks for your contribution!

joowani force-pushed the dev branch 3 times, most recently from 628e909 to 1d384dd Compare May 20, 2020 20:43

joowani requested changes May 20, 2020

View reviewed changes

DechinPhy commented May 21, 2020

View reviewed changes

joowani requested changes May 21, 2020

View reviewed changes

Add feature get_parent_node.

7c1e41a

DechinPhy commented May 22, 2020

View reviewed changes

joowani approved these changes May 22, 2020

View reviewed changes

joowani merged commit 821f433 into joowani:dev May 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add feature for searching upper layer parent node #24

Add feature for searching upper layer parent node #24

DechinPhy commented May 20, 2020

joowani left a comment

joowani May 20, 2020

DechinPhy May 21, 2020

joowani May 20, 2020

DechinPhy May 21, 2020

joowani May 20, 2020

DechinPhy May 21, 2020

joowani May 21, 2020

joowani May 20, 2020

DechinPhy May 21, 2020

joowani May 21, 2020

coveralls commented May 21, 2020

coveralls commented May 21, 2020 •

edited

DechinPhy left a comment

DechinPhy May 21, 2020

DechinPhy May 21, 2020

DechinPhy May 21, 2020

DechinPhy left a comment

joowani left a comment •

edited

joowani May 21, 2020

DechinPhy May 22, 2020

joowani May 21, 2020

DechinPhy May 22, 2020

joowani May 21, 2020

DechinPhy left a comment

DechinPhy May 22, 2020

DechinPhy May 22, 2020

joowani left a comment

		@@ -314,6 +315,38 @@ def _get_tree_properties(root):
		}


		def _get_parent_node(root, node):

Add feature for searching upper layer parent node #24

Add feature for searching upper layer parent node #24

Conversation

DechinPhy commented May 20, 2020

joowani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 21, 2020

coveralls commented May 21, 2020 • edited

DechinPhy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DechinPhy left a comment

Choose a reason for hiding this comment

joowani left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DechinPhy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joowani left a comment

Choose a reason for hiding this comment

coveralls commented May 21, 2020 •

edited

joowani left a comment •

edited