Shallow copy parent and children in DataTree constructor

[TomNicholas]

This seems to fix the examples in #9196, but causes other tests to fail, which I think just reveals that we have existing tests that assume this in-place modification is desired. I will change those tests to match the new intended behaviour.

• Implements suggestion (1) from Creating a DataTree should not modify parent and children in-place #9196 (comment)
• Tests added
• User visible changes (including notable bug fixes) are documented in whats-new.rst
• New functions/methods are listed in api.rst

[shoyer]

Can we remove parent from the constructor instead?

It's a little weird to only be able to set the direct parent (and corresponding sibling), but not more distant relatives.

[TomNicholas]

Yeah I was going to do that in a follow-up PR, but I can do it in this one instead.

[TomNicholas]

The main delay here is just how some of the tests are written - a lot of them rely upon in-place modification to work. e.g.

xarray/xarray/tests/test_datatree.py

Line 77 in c7cb9f7

def test_path_property(self):

This change makes the constructor a somewhat unhelpful way to create trees from scratch. I could rewrite these tests either

1. The ugly way I have started in 5db25bd, which doesn't refer to copied objects,

2. Using .from_dict, which is much clearer (and the recommended user API), but relies on more complex code paths internally so doesn't isolate the behaviour of the constructor / relationship properties so much.

[flamingbear]

1. The ugly way I have started in 5db25bd, which doesn't refer to copied objects,

When you say it's ugly, it's what I was thinking. It seems closer to what you're testing.
This looks like a not super useful test.

john = DataTree.from_dict(
    {
        "/": DataTree(name="John"),
        "/Mary": DataTree(name="Mary"),
        "/Mary/Sue": DataTree(name="Sue"),
    }
)
assert john["/Mary/Sue"].path == "/Mary/Sue"
 

[shoyer]

I think using from_dict for tests is fine, as long as you thoroughly test the constructor as well.

You could also avoid all the intermediate variables by writing something like:

john = DataTree(children={"Mary": DataTree(children={"Sue": DataTree()})})

Incidentally, it would sure be nice if we could eventually support automatic unpacking of nested dictionaries in the DataTree constructor, e.g., something like:

john = DataTree({"Mary": {"Sue": {}}})

Here the nature of the node (child DataTree vs DataArray) could be inferred from the argument type.

[shoyer]

john = DataTree.from_dict(
    {
        "/": DataTree(name="John"),
        "/Mary": DataTree(name="Mary"),
        "/Mary/Sue": DataTree(name="Sue"),
    }
)
assert john["/Mary/Sue"].path == "/Mary/Sue"
 ``

Incidentally, I would try to avoid redundant name arguments, given how name is (or should be) overriden by the assigned key in the parent object. So I would wrtie this as:

john = DataTree.from_dict(
    {
        "/": DataTree(),
        "/Mary": DataTree(),
        "/Mary/Sue": DataTree(),
    }
)

or even just

john = DataTree.from_dict(
    {
        "/Mary/Sue": DataTree(),
    }
)

[TomNicholas]

I've now re-written the tests to use approach (2), i.e. to just use DataTree.from_dict everywhere that a nested tree is required.

[flamingbear]

@TomNicholas I didn't want to push up unfinished work to this PR again. But 7c530f5?diff=split&w=0
is my stab at removing the parent parameter from the DataTree Constructor. I got all but three tests to pass but it seems like the three might not make sense anymore (or I just don't fully understand them)

FAILED xarray/tests/test_datatree.py::TestFamilyTree::test_create_two_children - TypeError: DataTree.__init__() got an unexpected keyword argument 'parent'
FAILED xarray/tests/test_datatree.py::TestFamilyTree::test_dont_modify_parent_inplace - TypeError: DataTree.__init__() got an unexpected keyword argument 'parent'
FAILED xarray/tests/test_datatree.py::TestFamilyTree::test_setparent_unnamed_child_node_fails - TypeError: DataTree.__init__() got an unexpected keyword argument 'parent'

Update edited:
test_dont_modify_parent_inplace looks like might have a bug. And probably something I didn't fix when I removed the parent keyword from the constructor?
fe5ae9c

I think this is a test for test_setparent_unnamed_child_node_fails
9a004d4

still have no idea what test_create_two_children is doing

these are just 3 commits on https://github.com/flamingbear/xarray/tree/datatree_init_dont_modify_inplace

[TomNicholas]

I think these are all resolved by either:

1. Changing outdated tests
2. Also making .parent read-only.

[TomNicholas]

This PR now does a few related things:

1. Shallow copy children in the DataTree constructor (6c56b12),
2. Removes the parent argument from the DataTree constructor (7c530f5)
3. Makes the .parent attribute of DataTree read-only (897b589)
4. Changes any tests that these effect by making them pass or deleting them.

[TomNicholas]

I believe this is ready to merge. It might imply more changes to @flamingbear 's docs PR though.

[shoyer]

Looks great, thanks Tom!