Skip to content
This repository has been archived by the owner on Oct 24, 2024. It is now read-only.

Should the parent of the root node be None or itself? #288

Closed
TomNicholas opened this issue Dec 3, 2023 · 3 comments
Closed

Should the parent of the root node be None or itself? #288

TomNicholas opened this issue Dec 3, 2023 · 3 comments

Comments

@TomNicholas
Copy link
Member

Currently the .parent attribute of a node can point to another DataTree object, or just None if that node is the root node.

@etienneschalk pointed out in #286 (comment) that in pathlib (and filesystems in general) instead the parent of the root node is just the root node.

I can't decide if this is something worth changing. Either could be made to work with DataTree I think, but changing the behaviour of .parent would be a breaking change.

@etienneschalk
Copy link
Contributor

A root being defined as a node having its parent to None is at least in my opinion more intuitive than a self-referencing node.

Code-wise, I assume the definition of a node being a root would change from node.parent is None to node.parent is node. For potential bugs, having the first means potential access errors on None, while having the second means potential infinite recursion.

However, root nodes with a None parents mean, they implicitly all belong to the same tree. So the checks like "do the nodes belong in different trees" might be less verbose with self-referencing roots. Indeed, node.root.parent is not other.root.parent, can be used to determine if node and other belong to the same tree, but cannot be used when the roots share the None parent.

@marcel-goldschen-ohm
Copy link

I completely agree with @etienneschalk point that a None parent of the root is far more intuitive than a self-referencing parent, which will lead to nightmares.

Regarding checks for belonging to the same tree, why not just node.root is not other.root? I don't see why the None parent is a problem here.

@TomNicholas
Copy link
Member Author

Regarding checks for belonging to the same tree, why not just node.root is not other.root? I don't see why the None parent is a problem here.

That's literally exactly what we currently do

return self.root is other.root

I completely agree with @etienneschalk point that a None parent of the root is far more intuitive than a self-referencing parent, which will lead to nightmares.

The consensus here seems to be that a self-referencing root is a bad idea, so I will close this issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants