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

Conversation

@keewis
Copy link
Member

@keewis keewis commented Nov 9, 2022

The docstring of DataTree.from_dict claims that it allows having DataTree objects as values, but both the type hints and the code disagree (the code fails because DataTree(DataTree()) does not work).

The implementation I chose in this PR is to special-case DataTree objects to copy and orphan the node instead of passing it to the DataTree constructor.

Apparently this does not please mypy because while DataTree has a copy method, that is inherited from Dataset. The copy is only necessary because orphan works in-place (but then again you'd probably need copy to implement a side-effect free orphan).

  • Tests added
  • Passes pre-commit run --all-files
  • New functions/methods are listed in api.rst
  • Changes are summarized in docs/source/whats-new.rst

For reference, I'm currently working around this limitation using

import posixpath

def normalize_tree_dict(mapping):
    def _normalize(mapping):
        for path, item in mapping.items():
            if not isinstance(item, datatree.DataTree):
                yield path, item
            else:
                root = item.copy()
                root.orphan()
                yield from ((posixpath.join(path, node.path), node.ds) for node in root.subtree)
            
    return dict(_normalize(mapping))

d = {"/path/to/node1": node1, "/path/to/node2": node2}
datatree.DataTree.from_dict(normalize_tree_dict(d))

@TomNicholas TomNicholas added the bug Something isn't working label Nov 9, 2022
Comment on lines 793 to 797
if isinstance(data, cls):
new_node = data.copy()
new_node.orphan()
else:
new_node = cls(name=node_name, data=data)
Copy link
Member

Choose a reason for hiding this comment

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

This is a neat and sensible solution!

@TomNicholas
Copy link
Member

Thanks for catching this @keewis ! Looks like I forgot to write a unit test for this intended behaviour and so never implemented it 😅 I think your solution is neat though.

Apparently this does not please mypy because while DataTree has a copy method, that is inherited from Dataset

So we could just explicitly write out the copy method on DataTree? I think I originally had that in some branch but it caused a recursion problem, which was why I ended up just applying Dataset.copy with map_over_subtree. It would be better to have the method written out explicitly though.

@TomNicholas TomNicholas enabled auto-merge (squash) December 7, 2022 19:24
@TomNicholas TomNicholas merged commit d8e7d38 into xarray-contrib:main Dec 7, 2022
@keewis keewis deleted the from_dict-datatree branch December 20, 2022 09:45
@TomNicholas TomNicholas mentioned this pull request Dec 30, 2022
5 tasks
flamingbear pushed a commit to flamingbear/rewritten-datatree that referenced this pull request Jan 19, 2024
…trib/datatree#159

* allow passing `DataTree` objects as `dict` values

* add a test verifying that DataTree objects are actually allowed

* ignore mypy error with copied copy method

* whatsnew

Co-authored-by: Tom Nicholas <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants