Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DataTree.from_dict to be insensitive to insertion order #9292

Merged
merged 5 commits into from
Jul 31, 2024
Merged

Fix DataTree.from_dict to be insensitive to insertion order #9292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
97b5aa9
regression test
TomNicholas Jul 30, 2024
f32d262
fix by sorting before insertion
TomNicholas Jul 30, 2024
d41a78f
whatsnew
TomNicholas Jul 30, 2024
3e06608
test nodes retain insertion order within a group
TomNicholas Jul 31, 2024
8a59da4
make test pass by sorting by depth
TomNicholas Jul 31, 2024
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
2 changes: 2 additions & 0 deletions doc/whats-new.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ Deprecations
Bug fixes
~~~~~~~~~

- Fix bug causing `DataTree.from_dict` to be sensitive to insertion order (:issue:`9276`, :pull:`9292`).
By `Tom Nicholas <https://github.com/TomNicholas>`_.

Documentation
~~~~~~~~~~~~~
Expand Down
7 changes: 6 additions & 1 deletion xarray/core/datatree.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1102,9 +1102,14 @@ def from_dict(
else:
obj = cls(name=name, data=root_data, parent=None, children=None)

def depth(item) -> int:
pathstr, _ = item
return len(NodePath(pathstr).parts)

if d:
# Populate tree with children determined from data_objects mapping
for path, data in d.items():
# Sort keys by depth so as to insert nodes from root first (see GH issue #9276)
for path, data in sorted(d.items(), key=depth):
# Create and set new node
node_name = NodePath(path).name
if isinstance(data, DataTree):
Expand Down
24 changes: 24 additions & 0 deletions xarray/tests/test_datatree.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,30 @@ def test_roundtrip_unnamed_root(self, simple_datatree):
roundtrip = DataTree.from_dict(dt.to_dict())
assert roundtrip.equals(dt)

def test_insertion_order(self):
# regression test for GH issue #9276
reversed = DataTree.from_dict(
{
"/Homer/Lisa": xr.Dataset({"age": 8}),
"/Homer/Bart": xr.Dataset({"age": 10}),
"/Homer": xr.Dataset({"age": 39}),
"/": xr.Dataset({"age": 83}),
}
)
expected = DataTree.from_dict(
{
"/": xr.Dataset({"age": 83}),
"/Homer": xr.Dataset({"age": 39}),
"/Homer/Lisa": xr.Dataset({"age": 8}),
"/Homer/Bart": xr.Dataset({"age": 10}),
}
)
assert reversed.equals(expected)

# Check that Bart and Lisa's order is still preserved within the group,
# despite 'Bart' coming before 'Lisa' when sorted alphabetically
assert list(reversed["Homer"].children.keys()) == ["Lisa", "Bart"]
Copy link
Member Author

@TomNicholas TomNicholas Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This assertion fails unless sorted sorts by depth.

Copy link
Member

Choose a reason for hiding this comment

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

I had to think on this, but I like the way it works.



class TestDatasetView:
def test_view_contents(self):
Expand Down
Loading