Tree #1169

Licini · 2023-07-21T14:52:32Z

Adding the tree class. @tomvanmele for serialization strategy, I ended up opting for directed nested dictionaries. This way it uses least amount of memory and not needing any types of uuid referencing. I'm aware the docstrings is not formatted entirely correctly, but besides that let me know how does the class structure itself look like. thx!

What type of change is this?

Bug fix in a backwards-compatible manner.
New feature in a backwards-compatible manner.
Breaking change: bug fix or new feature that involve incompatible API changes.
Other (e.g. doc update, configuration, etc)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

I added a line to the CHANGELOG.md file in the Unreleased section under the most fitting heading (e.g. Added, Changed, Removed).
I ran all tests on my computer and it's all green (i.e. invoke test).
I ran lint on my computer and there are no errors (i.e. invoke lint).
I added new functions/classes and made them available on a second-level import, e.g. compas.datastructures.Mesh.
I have added tests that prove my fix is effective or that my feature works.
I have added necessary documentation (if appropriate)

src/compas/datastructures/tree/tree.py

gonzalocasas

I quickly glanced over it, I think it's a great idea to add trees in general! Here's a first pass of comments!
Other random comments would include considering whether adding a more sophisticated type of tree would make sense (red-black, for example?)

src/compas/datastructures/tree/tree.py

gonzalocasas · 2023-07-21T18:43:57Z

src/compas/datastructures/tree/tree.py

+                yield descendant
+
+    def traverse(self):
+        """Traverse the tree from this node."""


Traverse can be done many ways, is it ok to have a single top level traverse method and no alternative for choice which traversal strategy?

Added pre-order and post-order, things like in-order are for binary tree so not included here

please add parameter to the docs and document, return, raise, ...

src/compas/datastructures/tree/tree.py

Licini · 2023-07-26T13:18:14Z

I quickly glanced over it, I think it's a great idea to add trees in general! Here's a first pass of comments! Other random comments would include considering whether adding a more sophisticated type of tree would make sense (red-black, for example?)

Thx for the feedbacks! For more specific type of trees like binary tree, red black tree etc we can add them in the same folder inheriting from this class. For instance for binary tree we'd add left/right property, in-order traverse, for red black tree we add color property and corresponding algorithms, but it is very important that the base tree class does not create any limitation on later more specific implementations, as well as not creating any significant computational overhead due to the way we structure it. The primary use cases we have in mind for now are things like scene objects graph (like spatial hierarchy in ifc files), classes inheritance tree etc, rather high-level stuff.

btw I'm curious to know your opinion on the serialization strategy, is it good the way it is now as nested dicts or should we do flat node list + uuid identifiers?

tomvanmele · 2023-10-03T19:00:00Z

src/compas/datastructures/tree/tree.py

+                yield descendant
+
+    def traverse(self):
+        """Traverse the tree from this node."""


please add parameter to the docs and document, return, raise, ...

src/compas/datastructures/tree/tree.py

tomvanmele · 2023-10-03T19:03:33Z

src/compas/datastructures/tree/tree.py

+
+
+class Tree(Datastructure):
+    """A tree data structure.


perhaps a short overview of the most important characteristics of the data structure would be useful.

src/compas/datastructures/tree/tree.py

jf--- · 2023-10-06T08:42:58Z

Greenwashing compas with trees ;)

Licini · 2023-10-06T10:31:18Z

@tomvanmele How does it look now?
Besides the requested changes, one more thing:

I changed Tree.nodes attribute to be dynamically inferred. Because maintaining this list separately is a bit error-prone especially when you start to trim branches or merge trees (you would need always traverse the given branch and update the node list every-time you add or remove). The decision comes with a bit computational penalty when you print the tree. But since we are not likely to operate on super huge trees yet, I'd vote for keeping the base class simple, and later have more advanced implementations when performance issue arises.

tomvanmele

maybe you should also add a few tests :)

src/compas/datastructures/tree/tree.py

tomvanmele · 2023-10-06T14:11:21Z

src/compas/datastructures/tree/tree.py

+        if strategy == "preorder":
+            yield self
+            for child in self.children:
+                for node in child.traverse(strategy):
+                    yield node
+        elif strategy == "postorder":
+            for child in self.children:
+                for node in child.traverse(strategy):
+                    yield node
+            yield self
+        else:
+            raise ValueError("Unknown traversal strategy: {}".format(strategy))


preorder and postorder is the same as depth-first, breadth-first?

I thought they are, but just realized I got it confused too... Turned out pre and post order are both subcategory of depth-first, the difference is whether you visit current node first or not. None of them are breath-first. Will make some adjustments here

@tomvanmele ok now there are two arguments strategy and order, strategy refers to depth first of breath first, and order is pre or post order (only for depth first). This should better aligns the terminologies.

src/compas/datastructures/tree/tree.py

tomvanmele · 2023-10-06T14:13:35Z

src/compas/datastructures/tree/tree.py

+            raise TypeError("The node is not a TreeNode object.")
+
+        if node.parent:
+            raise ValueError("The node is already part of another tree, remove it from that tree first.")


can also be THIS tree, no?

Sorry for the confusion, I adjusted a bit the sentence here, the idea is that if a node is already added somewhere (a parent link already exists, no matter it is this tree or another), we can ask user to explicitly remove that link first. Another option would be to not care about it and directly overwrite this link, which one you think is better?

tomvanmele · 2023-10-09T11:08:59Z

src/compas/datastructures/tree/tree.py

-            The traversal strategy. Options are ``"preorder"`` and ``"postorder"``.
+        strategy : {"depthfirst", "breadthfirst"}, optional
+            The traversal strategy.
+            Default is ``"depthfirst"``.


in my opinion you don't have to mention the default. this is already clear from line 163

tomvanmele · 2023-10-09T11:10:36Z

src/compas/datastructures/tree/tree.py

+        ----------
+        strategy : {"depthfirst", "breadthfirst"}, optional
+            The traversal strategy.
+            Default is ``"depthfirst"``.


same here. no mention of default needed in my opinion

Licini · 2023-10-09T11:47:01Z

hey @gonzalocasas could you take another look at the current state of this PR, it needs your approval

tomvanmele · 2023-10-09T13:19:38Z

src/compas/datastructures/tree/tree.py

@@ -379,11 +377,9 @@ def traverse(self, strategy="depthfirst", order="preorder"):
        ----------
        strategy : {"depthfirst", "breadthfirst"}, optional
            The traversal strategy.
-            Default is ``"depthfirst"``.



there can't be a blank line in parameter lists

oops.... fixed

tomvanmele · 2023-10-09T13:19:45Z

src/compas/datastructures/tree/tree.py

@@ -168,11 +168,9 @@ def traverse(self, strategy="depthfirst", order="preorder"):
        ----------
        strategy : {"depthfirst", "breadthfirst"}, optional
            The traversal strategy.
-            Default is ``"depthfirst"``.



Licini · 2023-10-23T14:03:04Z

@gonzalocasas Pin.... : ) Could you approve this so it can be merged?

Licini added 3 commits July 20, 2023 13:47

Tree classes

bf37e2d

more apis

916f808

Docs

a16d86f

Licini requested review from gonzalocasas, chenkasirer and tomvanmele July 21, 2023 14:52

Licini marked this pull request as ready for review July 21, 2023 14:55

tomvanmele reviewed Jul 21, 2023

View reviewed changes

src/compas/datastructures/tree/tree.py Outdated Show resolved Hide resolved

gonzalocasas requested changes Jul 21, 2023

View reviewed changes

Licini added 2 commits July 26, 2023 15:05

adjustments based on feedback

0b772d2

remove type hints

90594eb

tomvanmele reviewed Oct 3, 2023

View reviewed changes

Licini added 3 commits October 5, 2023 14:26

Merge remote-tracking branch 'origin/main' into tree

952f2c5

docstring fixes

42ca1ea

align with new data mechanism

3441105

Licini added 3 commits October 6, 2023 11:03

adjust tree functions

98dd468

simplify node tree links and add data schemas

7666de4

make tree link dynamic

9928394

tomvanmele reviewed Oct 6, 2023

View reviewed changes

Licini added 2 commits October 9, 2023 13:02

add tests

7c12d19

lint

a0d2b4f

tomvanmele approved these changes Oct 9, 2023

View reviewed changes

Merge branch 'main' into tree

8ff0d0f

Licini requested a review from gonzalocasas October 9, 2023 11:17

Licini closed this Oct 9, 2023

Licini reopened this Oct 9, 2023

skip data validate for ipy

c0ba4de

tomvanmele reviewed Oct 9, 2023

View reviewed changes

no spaces

f3ef02b

tomvanmele approved these changes Oct 9, 2023

View reviewed changes

tomvanmele merged commit 6ff2f35 into compas-dev:main Oct 24, 2023
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tree #1169

Tree #1169

Licini commented Jul 21, 2023 •

edited

Loading

gonzalocasas left a comment

gonzalocasas Jul 21, 2023

Licini Jul 26, 2023

tomvanmele Oct 3, 2023

Licini commented Jul 26, 2023 •

edited

Loading

tomvanmele Oct 3, 2023

tomvanmele Oct 3, 2023

jf--- commented Oct 6, 2023

Licini commented Oct 6, 2023

tomvanmele left a comment

tomvanmele Oct 6, 2023

Licini Oct 9, 2023

Licini Oct 9, 2023

tomvanmele Oct 6, 2023

Licini Oct 9, 2023

tomvanmele Oct 9, 2023

tomvanmele Oct 9, 2023

Licini commented Oct 9, 2023

tomvanmele Oct 9, 2023

Licini Oct 9, 2023

tomvanmele Oct 9, 2023

Licini commented Oct 23, 2023

Tree #1169

Tree #1169

Conversation

Licini commented Jul 21, 2023 • edited Loading

What type of change is this?

Checklist

gonzalocasas 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

Licini commented Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jf--- commented Oct 6, 2023

Licini commented Oct 6, 2023

tomvanmele 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

Licini commented Oct 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Licini commented Oct 23, 2023

Licini commented Jul 21, 2023 •

edited

Loading

Licini commented Jul 26, 2023 •

edited

Loading