Skip to content

Conversation

@xhluca
Copy link

@xhluca xhluca commented Apr 3, 2019

About

Ok so this one is a more experimental PR, and the last one before 0.1.0 release (which will be a separate PR, but should be quick to review).

I've thought about this for a while and it seems to be pretty useful to have a Tree class that can be easily constructed as a nested object, then rendered into an element. For example,

>>> tree = Tree('a', [
...   Tree('b', [
...     Tree('c')
...   ]),
...   Tree('d')
... ])

running tree.get_elements() would result in:

[{'data': {'id': 'a'}},
 {'data': {'id': 'b'}},
 {'data': {'id': 'c'}},
 {'data': {'id': 'd'}},
 {'data': {'source': 'a', 'target': 'b'}},
 {'data': {'source': 'a', 'target': 'd'}},
 {'data': {'source': 'b', 'target': 'c'}}]

With a tree class, we can use useful methods to manipulate it, such as:

  • get_nodes(), get_edges(): When we only want to access one type of elements from the tree
  • find_by_id(): Using a tree traversal, retrieve a specific object in a subtree
  • create_index(): Create a dictionary to easily retrieve a node in the subtree by its id

As well as more that I didn't think of.

My thought behind creating a prototype Tree class is to have a customized data structure for easily interacting with Cytoscape, which in the long term might be more appropriate than extending a tree library. For example, we can easily assign an ID to a node directly. However, there are excellent Tree libraries out there (treelib @ 357 stars seems pretty solid), writing a wrapper would indeed be possible if everyone thinks it's a good idea. In this case, there's no need to merge this class (will close this PR instead).

Please let me know your thoughts in whether we should create this class or not. Thanks!

Description of changes

  • dash_cytoscape.utils.Tree: New Tree object
  • demos/usage-dag-edges.py: Example using the tree

Pre-Merge checklist

  • The project was correctly built with npm run build:all.
  • If there was any conflict, it was solved correctly
  • All changes were documented in CHANGELOG.md.
  • All tests on CircleCI have passed.
  • All Percy visual changes have been approved.
  • Two people have 💃'd the pull request. You can be one of these people if you are a Dash Cytoscape core contributor.

Reference Issues

Closes #[issue number]

Other comments

@vivekvs1
Copy link

vivekvs1 commented Apr 4, 2019

Wow great <3 . Quick question: how does it benefit having a tree structure when defining methods such as get_nodes(), get_edge() etc?
Right now I created callback functions to get node information, get parent node id info etc.

@xhluca
Copy link
Author

xhluca commented Apr 4, 2019

@vivekvs1 The get_nodes() methods lets you directly get the nodes of the trees in the elements format, so you can perform adhoc manipulation before returning it. In the example above, you would get

[{'data': {'id': 'a'}},
 {'data': {'id': 'b'}},
 {'data': {'id': 'c'}},
 {'data': {'id': 'd'}}]

Which would be useful if you want to temporarily modify the data field (e.g. add a different label depending on the input of the callback), but don't want to affect the Tree structure.

Overall, a Tree class will be useful if you want to represent a graph that is inherently a tree, but don't want to directly manipulate an elements list. Once you built or modified a tree, e.g. inside a callback, you can simply convert it into an elements list and output it.

@vivekvs1
Copy link

vivekvs1 commented Apr 4, 2019

Oh I see. Very nice! Thanks @xhlulu

Copy link
Contributor

@shammamah-zz shammamah-zz left a comment

Choose a reason for hiding this comment

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

Yay for using things we learn in our CS degrees! 🎉

Copy link
Author

@xhluca xhluca left a comment

Choose a reason for hiding this comment

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

Thank you for the helpful review!

Add a list of children to the current children of a Tree
:param children: List of Tree objects
"""
self.children.extend(children)
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to assert len(children) > 1.
Is add_child a special case of add_children, which would still work when len(children) == 1?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. Add child is a special case of add_children. Sometimes the programmer might not know in advance how many children the tree might have (when it's decided programmatically). So they might just use add_children() with a list all the time.

Copy link
Contributor

Choose a reason for hiding this comment

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

@xhlulu Then I would recommend removing method add_child and keep only method add_children (so there's one way to add children, however many they end up being).

@xhluca xhluca marked this pull request as ready for review April 5, 2019 15:42
@xhluca
Copy link
Author

xhluca commented Apr 5, 2019

Thank you @mkcor for you reviews!

@shammamah-zz
Copy link
Contributor

💃

@xhluca xhluca merged commit 7613b94 into master Apr 5, 2019
@xhluca xhluca deleted the dev branch April 5, 2019 18:43
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.

5 participants