-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
Remove unvetted DataTree methods #9585
Conversation
As [discussed](https://docs.google.com/presentation/d/1zBjEsihBhK_U972jxHwaAZBbzS1-hd3aDLnO9uu2Ob4/edit#slide=id.g3087b787633_13_0) in the last DataTree meeting, this PR deletes the many Dataset methods that were copied onto DataTree without unit tests, along with a few that are not implemented properly yet, e.g., 1. Arithmetic methods were removed, because `DataTree + Dataset` should probably raise an error. 2. Indexing and aggregation methods were removed, because these should allow for dimensions that are missing only on some nodes. 3. The untested `map_over_subtree_inplace` and `render` methods were removed. 3. A few other methods (e.g., `merge` and `plot`) that were only implemented by raising `NotImplementedError`` are entirely removed instead.
I have an implementation of |
Reimplemented aggregations also coming soon... |
I've commented out the parts of the doc that reference removed DataTree methods. This is ready for review now. |
xref #9365 (comment)
xref #8949
👍
|
|
||
.. DataTree.assign_coords | ||
.. DataTree.merge | ||
.. DataTree.rename | ||
.. DataTree.rename_vars | ||
.. DataTree.rename_dims | ||
.. DataTree.swap_dims | ||
.. DataTree.expand_dims | ||
.. DataTree.drop_vars | ||
.. DataTree.drop_dims | ||
.. DataTree.set_coords | ||
.. DataTree.reset_coords |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was the decision here that these methods should act locally rather than mapping over the subtree? And the distinction being between methods which manipulate structure versus methods which manipulate data - the latter map over the whole tree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had not thought about how these methods should work, I was just removing them from the docs because they no longer have an implementation!
That said, my inclination is that mapping over the subtree is probably appropriate for all of these. But many will probably need some special handling so they don't raise errors about missing variables/dimensions.
.. DataTree.reindex | ||
.. DataTree.reindex_like | ||
.. DataTree.set_index | ||
.. DataTree.reset_index |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unclear to me whether these methods would count as "manipulating structure" or "manipulating data"...
This PR deletes the many Dataset methods that were copied onto DataTree without unit tests, along with a few that are not implemented properly yet, e.g.,
DataTree + Dataset
should probably raise an error.map_over_subtree_inplace
andrender
methods were removed.merge
andplot
) that were only implemented by raising `NotImplementedError`` are entirely removed instead.