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

cleanup subtree assignment in io open functions #96

Closed
wants to merge 1 commit into from
Closed

cleanup subtree assignment in io open functions #96

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 3 additions & 18 deletions datatree/io.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,16 +68,8 @@ def _open_datatree_netcdf(filename: str, **kwargs) -> DataTree:
tree_root = DataTree.from_dict({"/": ds})
for path in _iter_nc_groups(ncds):
subgroup_ds = open_dataset(filename, group=path, **kwargs)
tree_root[path] = DataTree(name=path, data=subgroup_ds)
Copy link
Member

@TomNicholas TomNicholas May 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Using __setitem__ instead of _set_item here can behave differently, for instance if a group appeared twice one method would allow overwriting by default and the other wouldn't.

Another difference is that __setitem__ creates intermediate nodes, i.e. if dt only has one node, then calling dt['/root/a/b/c/d/e/foo'] = DataTree() would create nodes /root/a, /root/a/b/, /root/a/b/c ... etc. whereas _set_item would raise.

Copy link
Member

Choose a reason for hiding this comment

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

The reason I made the _set_item_ method was for internal developer API so that this kind of behaviour could be controlled more explicitly.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the explanation.


# TODO refactor to use __setitem__ once creation of new nodes by assigning Dataset works again
Copy link
Member

Choose a reason for hiding this comment

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

This comment was actually referring to the difference between

dt['folder'] = DataTree(data=ds)
and
dt['folder'] = ds,

the latter of which currently doesn't work, but also perhaps doesn't need to.

node_name = NodePath(path).name
new_node: DataTree = DataTree(name=node_name, data=subgroup_ds)
Comment on lines -73 to -74
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure your new code behaves the same way as this old code? Because previously we were naming new nodes as NodePath(path).name, which would take the last part of a long unix-like path (e.g. /a/b/c -> c), whereas with your changes it looks like it would name the node /a/b/c?

tree_root._set_item(
path,
new_node,
allow_overwrite=False,
new_nodes_along_path=True,
)
return tree_root


Expand All @@ -93,15 +85,8 @@ def _open_datatree_zarr(store, **kwargs) -> DataTree:
except zarr.errors.PathNotFoundError:
subgroup_ds = Dataset()

# TODO refactor to use __setitem__ once creation of new nodes by assigning Dataset works again
node_name = NodePath(path).name
new_node: DataTree = DataTree(name=node_name, data=subgroup_ds)
tree_root._set_item(
path,
new_node,
allow_overwrite=False,
new_nodes_along_path=True,
)
tree_root[path] = DataTree(name=path, data=subgroup_ds)

return tree_root


Expand Down