Skip to content
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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

owenlittlejohns
Copy link
Contributor

This PR migrates documentation for datatree from xarray/datatree_/docs into the main doc 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 as xarray.core.datatree.DataTree. When DataTree and friends are added to the public xarray 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?)

  • Completes documentation migration step of Track merging datatree into xarray #8572
  • Tests added
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

@keewis
Copy link
Collaborator

keewis commented May 19, 2024

I would be fine with adding DataTree and open_datatree to the top-level module in this PR: adding documentation for DataTree to me means that we're publicly exposing it.

The only alternative I can think of would be to do this in a separate PR and merge that, then sync this PR with main.

@TomNicholas
Copy link
Contributor

I agree with @keewis

The only alternative I can think of would be to do this in a separate PR and merge that, then sync this PR with main.

This might a little neater, but the result is the same either way.

@TomNicholas TomNicholas added the topic-DataTree Related to the implementation of a DataTree class label May 20, 2024
@owenlittlejohns
Copy link
Contributor Author

@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.

@owenlittlejohns
Copy link
Contributor Author

I have added open_datatree, DataTree, register_datatree_accessor, map_over_subtree and assert_isomorphic to the public API. The DataTree public API has some other exceptions in it (TreeIsomorphismError, InvalidTreeError and NotFoundInTreeError), but I was going by the things listed in #8572.

Copy link
Contributor

@flamingbear flamingbear left a 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.

doc/roadmap.rst Outdated Show resolved Hide resolved

DataTrees
---------

:py:class:`DataTree` is a tree-like container of :py:class:`xarray.DataArray` objects, organised into multiple mutually alignable groups.
:py:class:`xarray.DataTree` is a tree-like container of :py:class:`xarray.DataArray` objects, organised into multiple mutually allignable groups.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that misspelling was misspelled.

Suggested change
:py:class:`xarray.DataTree` is a tree-like container of :py:class:`xarray.DataArray` objects, organised into multiple mutually allignable groups.
:py:class:`xarray.DataTree` is a tree-like container of :py:class:`xarray.DataArray` objects, organised into multiple mutually alignable groups.

doc/user-guide/data-structures.rst Outdated Show resolved Hide resolved
doc/user-guide/data-structures.rst Outdated Show resolved Hide resolved
doc/user-guide/data-structures.rst Outdated Show resolved Hide resolved
doc/user-guide/hierarchical-data.rst Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this doc back in the datatree contrib repo before starting this and except for my comments inline, this looks fine.

doc/user-guide/io.rst Show resolved Hide resolved
doc/user-guide/io.rst Outdated Show resolved Hide resolved
doc/user-guide/terminology.rst Outdated Show resolved Hide resolved
Copy link
Contributor Author

@owenlittlejohns owenlittlejohns left a 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!

doc/user-guide/io.rst Outdated Show resolved Hide resolved

Alternatively you can also create a ``DataTree`` object from

- An ``xarray.Dataset`` using ``Dataset.to_node()`` (not yet implemented),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed it in the latest commit.

doc/user-guide/data-structures.rst Show resolved Hide resolved
doc/user-guide/io.rst Show resolved Hide resolved
@TomNicholas
Copy link
Contributor

(TreeIsomorphismError, InvalidTreeError and NotFoundInTreeError)

These should probably be added to the api.rst page after MergeError.

@owenlittlejohns
Copy link
Contributor Author

@TomNicholas - to clarify your previous comment about those exceptions: would you like those 3 exceptions added to the public xarray API (TreeIsomorphismError, InvalidTreeError, NotFoundInTreeError)? Or would you just want them added to api.rst with their full current paths (pointing to e.g., xarray.core.treenode.InvalidTreeError).

I can definitely add them to the public API, but hadn't as those exceptions were not listed in #8572.

@TomNicholas
Copy link
Contributor

TomNicholas commented May 28, 2024

@owenlittlejohns however it's already done for xarray's MergeError (which I happen to remember is listed in the public api.rst page). How exactly we do this isn't particularly important (hence why I forgot to include it in #8572), we just want to be consistent with other error types that we already have exposed publicly.

@owenlittlejohns
Copy link
Contributor Author

Thanks for the guidance. I just added those three exceptions to the public API, so they are now consistent with MergeError.

@flamingbear
Copy link
Contributor

flamingbear commented Jun 6, 2024

I approved the changes here, but I don't know how to mark this as waiting for an event. (numpy 2)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-DataTree Related to the implementation of a DataTree class topic-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants