-
-
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
DAS-2155 - Merge datatree documentation into main docs. #9033
DAS-2155 - Merge datatree documentation into main docs. #9033
Conversation
I would be fine with adding The only alternative I can think of would be to do this in a separate PR and merge that, then sync this PR with |
I agree with @keewis
This might a little neater, but the result is the same either way. |
@TomNicholas - So the alternative here is exposing things in the public API, then updating this PR to have the correct references and merging this afterwards? (Just checking I read your last comment correctly) Maybe I'm being a bit keen, but I'm tempted to keep both pieces (adding to the main API and including DataTree in the documentation) in the same PR to keep the documentation and public API in-sync. |
I have added |
…hildfree_tree_factory.
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.
There are lots of little bitty things, but this looks great and the docs read well.
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.
@flamingbear - I've updated in most places you've suggested. Thanks for going through this so rigorously!
These should probably be added to the |
@TomNicholas - to clarify your previous comment about those exceptions: would you like those 3 exceptions added to the public I can definitely add them to the public API, but hadn't as those exceptions were not listed in #8572. |
@owenlittlejohns however it's already done for xarray's |
Thanks for the guidance. I just added those three exceptions to the public API, so they are now consistent with |
Co-authored-by: Matt Savoie <[email protected]>
I approved the changes here, but I don't know how to mark this as waiting for an event. (numpy 2) |
Co-authored-by: Eni <[email protected]>
Co-authored-by: Eni <[email protected]>
Co-authored-by: Eni <[email protected]>
To be crystal clear. We can merge this and update the docs with small tweaks when #9475 is updated? Also to add the open_goups and other additions? I'm 100% for this but only if others are ok with that. |
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.
This looks great! Well done @flamingbear
I left a handful of mostly minor suggestions. I'm totally fine leaving some of these for follow-up work if you feel ready to merge this now.
possess those dimensions, these dimensions will not be present when that | ||
file is opened as a DataTree object. | ||
Saving this DataTree object to file will therefore not preserve these | ||
"unused" dimensions. |
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.
This may also be a good place to mention xarray's non-support for coordinates that conflict between parent and child nodes, and pointing users to open_groups
as the recommended way to open such datasets.
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'm not sure this is the right place for that. This is under modifying existing zarr stores. They will always be aligned based on their data model? and this warning makes it sound like you're already changing the data groups?
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 think the point here is that somewhere in the IO docs for netCDF and Zarr there should be info or links to info about how open_datatree is not guaranteed to roundtrip but open_groups is.
Co-authored-by: Stephan Hoyer <[email protected]>
Co-authored-by: Stephan Hoyer <[email protected]>
Co-authored-by: Stephan Hoyer <[email protected]>
Co-authored-by: Stephan Hoyer <[email protected]>
Co-authored-by: Stephan Hoyer <[email protected]>
Co-authored-by: Stephan Hoyer <[email protected]>
Co-authored-by: Stephan Hoyer <[email protected]>
Last comments to address are being added here so when I merge this we can find them easily:
This is ready but for these. Let's do them off of main and merge this. |
Amazing work @flamingbear ! Hopefully we can now return to a saner branch structure for the final push! |
* main: (26 commits) Forbid modifying names of DataTree objects with parents (pydata#9494) DAS-2155 - Merge datatree documentation into main docs. (pydata#9033) Make illegal path-like variable names when constructing a DataTree from a Dataset (pydata#9378) Ensure TreeNode doesn't copy in-place (pydata#9482) `open_groups` for zarr backends (pydata#9469) Update pyproject.toml (pydata#9484) New whatsnew section (pydata#9483) Release notes for v2024.09.0 (pydata#9480) Fix `DataTree.coords.__setitem__` by adding `DataTreeCoordinates` class (pydata#9451) Rename DataTree's "ds" and "data" to "dataset" (pydata#9476) Update DataTree repr to indicate inheritance (pydata#9470) Bump pypa/gh-action-pypi-publish in the actions group (pydata#9460) Repo checker (pydata#9450) Add days_in_year and decimal_year to dt accessor (pydata#9105) remove parent argument from DataTree.__init__ (pydata#9465) Fix inheritance in DataTree.copy() (pydata#9457) Implement `DataTree.__delitem__` (pydata#9453) Add ASV for datatree.from_dict (pydata#9459) Make the first argument in DataTree.from_dict positional only (pydata#9446) Fix typos across the code, doc and comments (pydata#9443) ...
* main: Opt out of floor division for float dtype time encoding (pydata#9497) fixed formatting for whats-new (pydata#9493) Forbid modifying names of DataTree objects with parents (pydata#9494) DAS-2155 - Merge datatree documentation into main docs. (pydata#9033) Make illegal path-like variable names when constructing a DataTree from a Dataset (pydata#9378) Ensure TreeNode doesn't copy in-place (pydata#9482) `open_groups` for zarr backends (pydata#9469) Update pyproject.toml (pydata#9484) New whatsnew section (pydata#9483)
Co-authored-by: Matt Savoie <[email protected]> Co-authored-by: Eni <[email protected]> Co-authored-by: Stephan Hoyer <[email protected]> Co-authored-by: Matt Savoie <[email protected]>
This PR migrates documentation for datatree from
xarray/datatree_/docs
into the maindoc
directory.It seems a little iffy that a bunch of the references to the
DataTree
class, associated methods and other migrated functions all need to point to lower level locations (such asxarray.core.datatree.DataTree
. WhenDataTree
and friends are added to the publicxarray
API these references can be significantly simplified. This makes me wonder if we should couple the documentation change with that ability to access these classes from the main public API? (And if so, whether this PR is the time to do it?)Tests addedwhats-new.rst
api.rst