Conversation
michaelmckinsey1
added
area-thicket
michaelmckinsey1
force-pushed
the
add_root_node
branch
from
65c9c5e to
64ade20
Compare
michaelmckinsey1
added
status-ready-for-review
ilumsden
left a comment
ilumsden
left a comment
There was a problem hiding this comment.
Mostly looks good.
The only change that really needs to be made is the one about pop(). The other two are just suggestions.
thicket/thicket.py
Outdated
|
|
||
| new_node = hatchet.node.Node( | ||
| frame_obj=frame.Frame(attrs=attrs), hnid=len(self.graph) | ||
| ) |
There was a problem hiding this comment.
If you're going to do frame.Frame, why not also do node.Node? Or, better yet, just do the imports in a way that let's you do Node and Frame.
There was a problem hiding this comment.
I was using node.Node but we use node as a variable name in squash and I'm using it in get_node(). Not a big deal, so I'll just import Node and Frame
thicket/thicket.py
Outdated
| warnings.warn(f'More than one node with name "{name}". Returning a list') | ||
| return node | ||
|
|
||
| return node.pop() |
There was a problem hiding this comment.
It would be better to just return node[0]. pop will do things like resizing (see the source code), which isn't needed.
michaelmckinsey1
added
status-revisions-needed
michaelmckinsey1
force-pushed
the
add_root_node
branch
from
3ecea5b to
7294aac
Compare
ilumsden
left a comment
ilumsden
left a comment
There was a problem hiding this comment.
This mostly looks good at this point. Just a couple of minor changes.
thicket/tests/test_add_root_node.py
Outdated
|
|
||
|
|
||
| def test_add_root_node(literal_thickets): | ||
| tk, tk2, tk3 = literal_thickets |
There was a problem hiding this comment.
Since you're not using tk2 or tk3, change this line to the following:
tk, _, _ = literal_thickets
thicket/tests/test_get_node.py
Outdated
|
|
||
|
|
||
| def test_get_node(literal_thickets): | ||
| tk, tk2, tk3 = literal_thickets |
There was a problem hiding this comment.
See previous comment about use of underscores here
thicket/thicket.py
Outdated
| warnings.warn(f'More than one node with name "{name}". Returning a list') | ||
| return node | ||
| elif len(node) == 0: | ||
| raise ValueError(f'Node with name "{name}" not found.') |
There was a problem hiding this comment.
I would raise a KeyError here instead.
There was a problem hiding this comment.
thicket/thicket.py
Outdated
| node = [n for n in self.graph.traverse() if n.frame["name"] == name] | ||
|
|
||
| if len(node) > 1: | ||
| warnings.warn(f'More than one node with name "{name}". Returning a list') |
There was a problem hiding this comment.
I don't think this warning is necessary as long as we document the return type correctly. Regardless if this warning is kept or removed, the docstring needs to be updated to indicate that it can return either Node or List[Node].
4 participants
Examples of
add_root_nodeandget_nodecan be found in the notebook in thicket-tutorial/48add_root_nodeenables adding a root node to theThicket.graphand correspondingThicket.statsframeandThicket.dataframeobjects.get_nodeprovides a simpler interface for grabbing a node object by its name in the Thicket instead of using something like[n for n in self.graph.traverse() if n.frame["name"] == name][0]. This function is optional for this PR or could be its own PR; I am proposing it here.