corteva / rioxarray

geospatial xarray extension powered by rasterio
https://corteva.github.io/rioxarray
Other
504 stars 80 forks source link

ENH: Pass on on-disk chunk sizes as preferred chunk sizes to the xarray backend #678

Closed mraspaud closed 11 months ago

mraspaud commented 1 year ago

This PR proposes a removes the (unused) chunks and cache argument from the open_dataset plugin function and sends values to open_rasterio to get a result inline with what xarray expects. Moreover, the preferred_chunks encoding attribute is populated for xarray to be able to do optimized automated chunking.

The fact that chunks and cache are ignored by xarray is documented in the example here: https://docs.xarray.dev/en/stable/internals/how-to-add-new-backend.html#backendentrypoint-subclassing And the recommended usage of preferred_chunks is documented here: https://docs.xarray.dev/en/stable/internals/how-to-add-new-backend.html#preferred-chunk-sizes

Finally, I wasn't really sure how/where to put the tests, what do you recommend?

snowman2 commented 1 year ago

chunks is not used by xarray, however it is used by rioxarray. See: https://github.com/corteva/rioxarray/blob/33c0059ae97f108d631dad2ede721f5a0d4c18dc/rioxarray/_io.py#L896-L929

mraspaud commented 1 year ago

chunks is not used by xarray, however it is used by rioxarray. See:

Indeed! my objective here is to return the LazilyIndexedOuterArrays to xarray directly when the data is requested through the xarray backend to let xarray deal with the chunking and create dask arrays itself.

mraspaud commented 1 year ago

Anything else I can do to get this PR ready to merge?

snowman2 commented 1 year ago

Mind adding a note in docs/history.rst?

mraspaud commented 1 year ago

Mind adding a note in docs/history.rst?

Done in 1808857

snowman2 commented 11 months ago

@mraspaud, the only thing left to do in this PR is to remove the changes in the .pre-commit-config.yaml file and resolve the conflicts. The optimal chunk size can be updated separately.

mraspaud commented 11 months ago

Done. Sorry for the delay, I was on holidays...

snowman2 commented 11 months ago

This looks good from my perspective, Any other changes you would like to add before merging?

mraspaud commented 11 months ago

Nothing more from my side, thanks for all the support!

snowman2 commented 11 months ago

Thanks @mraspaud 👍