DAS-2067 - Migrate datatree io.py and common.py #9011
Conversation
| @@ -347,6 +347,34 @@ def _ipython_key_completions_(self) -> list[str]: | |||
| return list(items) | |||
|
|
|||
|
|
|||
| class TreeAttrAccessMixin(AttrAccessMixin): | |||
There was a problem hiding this comment.
So here's the biggest question I have... is this okay for now? (Maybe this is directed at @TomNicholas)
The use of __slots__ is a little new to me (but makes sense from reading up on it). I think to adopt __slots__ and remove __dict__ I would need to slightly rework DataTree, TreeNode and NamedNode. I wanted to check that the scope mainly seems to be self.children and self.parent attributes, and understand why those classes were implemented as they currently are before trying to alter things.
There was a problem hiding this comment.
This is going to embarrass me, but why did you stop calling init_subclass on the Parent object? everything else with this merge seems reasonable to me.
There was a problem hiding this comment.
Good catch. I accidentally commented out that last line. Will fix!
There was a problem hiding this comment.
So, my last comment was true, but... uncommenting out that line means that the same method in the parent class (AttrAccessMixin) gets called by super in addition to this method, right? That would mean we end up calling the validation logic that this child method is trying to avoid for now (checking that __dict__ is not present on the class).
Now if only I'd said that the first time around 😉
TomNicholas
added
the
topic-DataTree
| @@ -347,6 +347,34 @@ def _ipython_key_completions_(self) -> list[str]: | |||
| return list(items) | |||
|
|
|||
|
|
|||
| class TreeAttrAccessMixin(AttrAccessMixin): | |||
There was a problem hiding this comment.
This is going to embarrass me, but why did you stop calling init_subclass on the Parent object? everything else with this merge seems reasonable to me.
| @@ -347,6 +347,34 @@ def _ipython_key_completions_(self) -> list[str]: | |||
| return list(items) | |||
|
|
|||
|
|
|||
| class TreeAttrAccessMixin(AttrAccessMixin): | |||
There was a problem hiding this comment.
Why is this class necessary? Can't DataTree inherit directly from AttrAccessMixin? And override _attr_sources and so on?
I think the reason this class existed separately was just to do with hacking around limitations of datatree being in a separate repository...
There was a problem hiding this comment.
I initially tried to use AttrAccessMixin. Just dropping it in as a replacement for TreeAttrAccessMixin doesn't work because of the checking done inside the AttrAccessMixin.__init_subclass__ method. The check for if __dict__ exists fails. DataTree, TreeNode and NamedNode do all define __slots__, but I think there are attributes being defined in places like DataTree.__init__, for example DataTree.parent and DataTree.children (versus the DataTree._parent and DataTree._children attributes defined in DataTree.__slots__).
I think we talked about this a little bit in one of our calls - things like DataTree.parent and DataTree.children seemed like tricky things to change over.
|
@TomNicholas @owenlittlejohns I think we decided at the Tuesday meeting we are leaving the slots/dict as is and this is ready for approvals. Which I am about to do. |
|
@TomNicholas - just checking in over this PR. I captured the outstanding work to make |
This reverts commit cb3663d.
* upstream/main: [skip-ci] Try fixing hypothesis CI trigger (pydata#9112) Undo custom padding-top. (pydata#9107) add remaining core-dev citations [skip-ci][skip-rtd] (pydata#9110) Add user survey announcement to docs (pydata#9101) skip the `pandas` datetime roundtrip test with `pandas=3.0` (pydata#9104) Adds Matt Savoie to CITATION.cff (pydata#9103) [skip-ci] Fix skip-ci for hypothesis (pydata#9102) open_datatree performance improvement on NetCDF, H5, and Zarr files (pydata#9014) Migrate datatree io.py and common.py into xarray/core (pydata#9011) Micro optimizations to improve indexing (pydata#9002) (fix): don't handle time-dtypes as extension arrays in `from_dataframe` (pydata#9042)
This PR migrates what I believe is the last actual source code out of the
xarray/datatree_/datatreedirectory intoxarray/core. This is another piece of #8572.I made a couple of decisions that I'd love some feedback/extra eyes on:
xarray/datatree_/datatree/io.pyinto their own module, rather thanxarray/backends/api.py. It felt like these were a little separate, but I can definitely be convinced otherwise.TreeAttrAccessMixin. I really just wanted to swap over to usingAttrAccessMixin, but I ran into issues with__slots__. Looking at it, I think I would need to work on theDataTree,TreeNodeandNamedNodeclasses. I can go down that path, but thought I'd put up this PR for discussion before beginning that effort.Tests addedwhats-new.rstNew functions/methods are listed inapi.rst