xarray-contrib / datatree

WIP implementation of a tree-like hierarchical data structure for xarray.
https://xarray-datatree.readthedocs.io
Apache License 2.0
161 stars 43 forks source link

Setting node name keeps tree linkage #310

Open etienneschalk opened 4 months ago

etienneschalk commented 4 months ago

Small bugfix. I added a test reproducing the example in #309, and tests are not broken

Note: In _post_attach, I set the private _name instead of setting the name. Indeed, it can lead to infinite recursions when a setter is used inside of a class. I assume the _post_attach method is like a "runtime assertion"

etienneschalk commented 4 months ago

I remained conservative and forbid the renaming of a node to None if it has a parent

Edit: according to a comment in the datatree design document mentioned in https://github.com/pydata/xarray/issues/8747, section 4) Are nodes named? in practice, only the root node would remain anonmyous. Hence it makes sense to only authorize a node without a parent (root) to be renamed to None?

See my comment https://github.com/xarray-contrib/datatree/issues/309#issuecomment-1949940406

Edit: build is failing because of the new change in printing sizes of DataArrays: https://github.com/pydata/xarray/pull/8702/

Whether build or dev-build pass, they are mutually exclusive. What is the one that should be favoured? Current version of xarray of next version?

marcel-goldschen-ohm commented 4 months ago

I see I got to this pretty late, so it may be solved already (if so, please ignore this). But I couldn't quite tell from this conversation if everything was working or not. If not, I have a solution which passes all pytests (except for a format error that was not passing prior to my changes) at https://github.com/marcel-goldschen-ohm/datatree/tree/namelinkbugfix. Changes are in datatree.py in name.setter and _attach methods, and I added a test_namechange method to test_datatree.py. Again, if this is already solved, please ignore.

marcel-goldschen-ohm commented 1 week ago

Anyone know what is still keeping this from being incorporated into the pip distribution of datatree?