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

Docstring and documentation improvement for the Dataset class #8973

Merged
merged 8 commits into from
Apr 30, 2024

Conversation

noahbenson
Copy link
Contributor

The example in the doc-string of the Dataset class prior to this commit uses an example array whose size is 2 x 2 x 3 with the first two dimensions labeled "x" and "y" and the final dimension labeled "time". This was confusing due to the fact that "x" and "y" are just arbitrary names for these axes and that no reason is given for the data to be organized in a 2x2x3 array instead of a 2x2 matrix. This commit clarifies the example.

Additionally, this PR contains updates to the documentation, specifically the user-guide/data-structures.rst file; the updates bring the documentation examples into alignment with the doc-string change. Unfortunately, I wasn't able to build the documentation, so this will need to be checked. (I followed the instructions here, but despite cfgrib working fine, I got an error about how it wasn't a valid engine.)

See issue #8970 for more information.

…arer.

The example in the doc-string of the `Dataset` class prior to this commit
uses an example array whose size is `2 x 2 x 3` with the first two dimensions
labeled `"x"` and `"y"` and the final dimension labeled `"time"`. This was
confusing due to the fact that `"x"` and `"y"` are just arbitrary names for
these axes and that no reason is given for the data to be organized in a `2x2x3`
array instead of a `2x2` matrix. This commit clarifies the example.

See issue pydata#8970 for more information.
These changes to the documentation bring it into alignment with the
changes to the `Dataset` doc-string committed previously.

See issue pydata#8970 for more information.
Copy link

welcome bot commented Apr 25, 2024

Thank you for opening this pull request! It may take us a few days to respond here, so thank you for being patient.
If you have questions, some answers may be found in our contributing guidelines.

@max-sixty
Copy link
Collaborator

Thanks!

We do get an error in the doc build — TypeError: Variable 'temperature': Using a DataArray object to construct a variable is ambiguous, please extract the data using the .data property.. Check out https://readthedocs.org/projects/xray/builds/24178355/ for more. I also struggle to build the docs...

For the doctests, you may be on a slightly older version of xarray which doesn't print sizes, so worth updating. If you want to update the output automatically I have a project https://github.com/max-sixty/pytest-accept which does this automatically!

@noahbenson
Copy link
Contributor Author

Thanks. That error does not make sense to me—I have not materially changed it from what currently exists in the docs here where the line is ds["temperature"] = (("x", "y", "time"), temp). In fact, I think the error should have been that temp is not a declared symbol? (I changed the temp in the example to temperature but did not change it in this line.) That said, I don't use this tool for documentation, so I'm probably missing something.

Anyway, assuming that the temp versus temperature is fundamentally the problem, my last push should have fixed those issues... 🤞

@max-sixty max-sixty added the plan to merge Final call for comments label Apr 25, 2024
@max-sixty
Copy link
Collaborator

Great — if anyone has any thoughts, feel free to comment; otherwise let's merge.

Thanks @noahbenson , nice improvement to a core part of the docs...

@max-sixty
Copy link
Collaborator

Any other thoughts? There's some discussion in #8970. I think it's reasonable to merge tomorrow without other suggestions.

Give this a 👎 for disagreement (which is welcome ofc)

@max-sixty max-sixty merged commit 71372c1 into pydata:main Apr 30, 2024
28 checks passed
Copy link

welcome bot commented Apr 30, 2024

Congratulations on completing your first pull request! Welcome to Xarray! We are proud of you, and hope to see you again! celebration gif

@max-sixty
Copy link
Collaborator

Thanks @noahbenson ! I think this was a v nice improvement, thanks for pushing through!

andersy005 added a commit that referenced this pull request Apr 30, 2024
* origin/main:
  clean up the upstream-dev setup script (#8986)
  Skip flaky `test_open_mfdataset_manyfiles` test (#8989)
  Remove `.drop` warning allow (#8988)
  Add notes on when to add ignores to warnings (#8987)
  Docstring and documentation improvement for the Dataset class (#8973)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plan to merge Final call for comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Example code in the documentation for Dataset is not clear
2 participants