Skip to content

improve to_zarr doc about chunking - #4048

Merged
shoyer merged 7 commits into
pydata:masterfrom
apatlpo:to_zarr_doc
May 20, 2020
Merged

improve to_zarr doc about chunking#4048
shoyer merged 7 commits into
pydata:masterfrom
apatlpo:to_zarr_doc

Conversation

@apatlpo

@apatlpo apatlpo commented May 8, 2020

Copy link
Copy Markdown
Contributor

I'm not sure the last point is really necessary for this PR, is it?

@pep8speaks

pep8speaks commented May 8, 2020

Copy link
Copy Markdown

Hello @apatlpo! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-05-20 13:37:26 UTC

Comment thread xarray/core/dataset.py Outdated
Comment on lines +1610 to +1612
zarr may automatically chunk a DataArray if it is not chunked or
if `chunks` is not set to -1 in its `enconding` attribute or in the
argument of the `encoding` parameter.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we instead state what does happen if a variable is already chunked or chunks can be found in encoding?

Something like

  • If chunks are found in the encoding argument or attribute corresponding to any DataArray, those chunks are used.
  • Otherwise, if the DataArray is already a dask array, it is written with those chunks.
  • Finally, if not other chunks are found, Zarr uses its own heuristics to choose automatic chunk sizes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I closely followed your suggestion.
Note that I am not sure about the docstring style for a list-like note like this one.

@apatlpo

apatlpo commented May 15, 2020

Copy link
Copy Markdown
Contributor Author

if anybody has a clue on how to fix tests this would be welcome.
thx

@rabernat

Copy link
Copy Markdown
Contributor

There are two sphinx warnings in building the docs
https://dev.azure.com/xarray/xarray/_build/results?buildId=2819&view=logs&j=7e620c85-24a8-5ffa-8b1f-642bc9b1fc36&t=68484831-0a19-5145-bfe9-6309e5f7691d&l=280

xarray/core/dataset.py:docstring of xarray.Dataset.to_zarr:44: WARNING: Definition list ends without a blank line; unexpected unindent.
xarray/core/dataset.py:docstring of xarray.Dataset.to_zarr:46: WARNING: Unexpected indentation.

These seem related to your changes. I would try to get the docs building locally without any warnings.

@keewis

keewis commented May 15, 2020

Copy link
Copy Markdown
Collaborator

the flake8 issues are fixed in #4057, so you can merge master into your branch to fix them in this branch, too.

@apatlpo

apatlpo commented May 20, 2020

Copy link
Copy Markdown
Contributor Author

ok, tests are passing.

@keewis

keewis commented May 20, 2020

Copy link
Copy Markdown
Collaborator

they do, but could you remove the saved_on_disk.nc file that was accidentally added?

Also, the items are not a list despite being formatted like one:
to_zarr
is that intentional?

@apatlpo

apatlpo commented May 20, 2020

Copy link
Copy Markdown
Contributor Author

argh ... saved_on_disk.nc has been removed.

I was unfortunately not able to create a proper list without breaking tests and have exhausted my time trying to figure it out.
If you know how to do that I am more than happy to follow your advice.

@shoyer

shoyer commented May 20, 2020

Copy link
Copy Markdown
Member

I think it's OK that it's not formatted as a list. It's still pretty readable

@shoyer
shoyer merged commit 484d1ce into pydata:master May 20, 2020
@shoyer

shoyer commented May 20, 2020

Copy link
Copy Markdown
Member

thanks @apatlpo !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants