issue_comments

Thank you for this fix! It looks good to me. I would prefer a separate function for the check, but that's fine too.

I would prefer to avoid using **kwargs, the explicit list of parameters would make the code more readable. But I think it's fine that way too :)

@alexamici Could you please have a final look into this?

Thank you @kmuehlbauer! LGTM

I would try to stay as close to open_dataset as possible, which would make migrating to rioxarray's engine easier once we deprecate open_rasterio. If I understand the signature of open_dataset correctly, this is called backend_kwargs?

The idea was to deprecate in the future open_dataset backend_kwargs. Currently, in open_dataset signature, you can use either backend_kwargs or kwargs to pass the backend additional parameters. I would avoid adding backend_kwargs in open_rasterio interface.

That's perfect.

I think that It's not a bug: filename_or_obj in open_dataset can be a file, file-like, bytes, URL. The accepted inputs depend on the engine. So it doesn't make sense to raise a FileNotFoundError if the engine is not defined by the user or not automatically detected by xarray.

Me too, I was thinking about something like that to fix the error message.
Let me try to implement it. But I have really no time this week and the next one, sorry. I can do it after 23th if for you is ok.

@Illviljan I see your point, but the subclassing doesn't add too much complexity and for consistency would be better to add a check on the class. After that, I think we can merge it.

I can try to reproduce the error and fix it, but I need to know at least the xarray version.

@bcbnz could you check if this fixes also #5132?

@aurghs can you confirm that all failures are unrelated to the changes?

In my understanding, the errors are not related to the changes.

Making a backend doesn't have to be super difficult either depending if you already have a nice 3rd party module you can thinly wrap to return a Dataset instead of whatever is the default

I agree. Adding a plugin is not really very difficult, but in some cases could be discouraging especially if you are just exploring how the backends work.

At this point, probably api.normalize_path is the best choice.

LGTM. I don't know how we would test this...

I don't have any good idea about it.

python new_var = var.chunk(chunks, name=name2, lock=lock) new_var.encoding = var.encoding Here you are modifying _maybe_chunk, but _maybe_chunk is also used in Dataset.chunks. Probably would be better to change backend.api.py, here: https://github.com/pydata/xarray/blob/ddc352faa6de91f266a1749773d08ae8d6f09683/xarray/backends/api.py#L296-L307

But maybe also in this case we want to drop encoding["chunks"] if they are not compatible with dask ones.

I think that this is consequence of the refactor done by @alexamici when he has removed _normalize_path: https://github.com/pydata/xarray/pull/4701 We decided to demand the path interpretation to the backends.

I'll have a look to understand how to fix it.

Hmm. I would also be happy with explicitly deleting chunks from encoding for now. It's not adding a lot of technical debt.

I see two reasons for keeping it: - We should be able to read and write the data with the same structure on disk. - The user may be interested in this information.

But it seems to me that having two different definitions of chunks (dask one and encoded one), is not very intuitive and it's not easy to define a clear default in writing.

~rechunk~ Variable.chunk is used always when you open a data with dask, even if you are using the default chunking. So in this way, you will drop the encoding always when dask is used (≈ always).

Does it actually get called every time we load a dataset with chunks?

Yes

Should the Zarr backend be setting this?

Yes, they are already defined in zarr: preferred_chunks=chunks. We decide to separate the chunks and the preferred_chunks: - The preferred_chunks is used by the backend to define the default chunks to be used by xarray. - The chunks are the on-disk chunks.

They are not necessarily the same. Maybe we can drop the preferred_chunks after they are used.

Perhaps we could remove also overwrite_encoded_chunks, it shouldn't be any more necessary.

We could also provide a use-case in remote sensing: it would be really useful in the interferometric processing for managing Sentinel-1 IW and EW SLC data, which has multiple tiles (burts) partially overlapping in one direction (azimuth).

@alexamici and I can write the technical part of the proposal.

Unfortunately, in a previous version, there were internals plugins for the backends. Now they have been removed, but you need to re-install xarray to remove the entrypoints.
There isn't a release with the internal plugins, so the users should not have this problem.

If you want I can take care to move pynio to an external repository, but we should decide where.

Does encoding['chunks'] serve any purpose after you've loaded a Zarr store and all the variables are defined as dask arrays?

No. I run into this frequently and it is annoying. @rabernat do you remember why you chose to keep chunks around in encoding

The encodings["chunks"] is used in to_zarr. It seems to be reasonable: I expect that I should be able to read and re-write a Zarr without modifying the chunking on disk. It seems to me that dask chunks are used in writing only when the encodings["chunks"] is not defined or they are not compatible anymore with variables shapes. In the other cases encodings["chunks"] is used. So if you want to use the encoded chunks, you have to be sure that they are still compatible with variables shapes and that each Zarr chunk is contained in only one dask chunk. If you want to use the dask chunks you can: - Delite the encoded chunking as done by @eric-czech. - Use encoding when you write: ds.to_zarr('/tmp/ds3.zarr', mode='w', encoding={'x': {}}).

Maybe this interface is a little bit confusing. Probably would be better to move overwrite_encoded_chunks from open_dataset to to_zarr. open_dataset interface would be cleaner and would be clear how to use dask chunks in writing.

Concerning the different chunking per variable, I link here this related issue: https://github.com/pydata/xarray/issues/4623

I'm not sure but ... It seems to be a bug this error. There is a check on the final chunk that it seems to have the wrong direction in the inequality. The part of the code to decide what's chunking should be used in case we have defined both, dask chunking and encoded chucking, is the following: https://github.com/pydata/xarray/blob/ac234619d5471e789b0670a673084dbb01df4f9e/xarray/backends/zarr.py#L141-L173 the aims of these checks, as described in the comment, is to avoid to have multiple dask chunks in one zarr chunk. According to this logic this inequality at line 163: https://github.com/pydata/xarray/blob/ac234619d5471e789b0670a673084dbb01df4f9e/xarray/backends/zarr.py#L163 has the wrong direction. It should be in this way: if dchunks[-1] < zchunk, but this last one seems to me that it is always verified.

Btw & not for here. There are other warnings from the backends refactor. Would be nice if you could hunt them down and either fix or suppress them.

I was just doing that now. Some of them are fixed in https://github.com/pydata/xarray/pull/4728.

This cleanup involves only apiv2. I merge it.

I think this has been fixed at some point. it can be closed.

@ravwojdyla I think that currently there is no way to do this. But it would be nice to have an interface that allows defining different chunks for each variable. The main problem that I see in implementing that is to keep the ´xr.open_dataset(... chunks=)´, ´ds.chunk´ and ´ds.chunks´ interfaces backwards compatible. Probably a new issue for that would be better since this refactor is already a little bit tricky and your proposal could be implemented separately.

• I have replaced entrypoints with pkg_resources. I can't see any drawback in this change, but only advantages, the main one is that we remove an external dependency.
• I have added the tests.
• I didn't remove the signature inspection, since we have already talked about it widely with @shoyer, and in the end, we decided to keep the inspection and add the checks on the signature to be sure that neither args nor *kwargs will be used.

@weiji14 Thank you very much for your feedback. I think we should align also xr.open_mfdataset. In the case of engine == zarr and chunk == -1 there is a UserWarning also in xr.open_dataset, but I think it should be removed.

Maybe we should evaluate for the future to integrate/use dask function dask.array.core.normalize_chunks (https://docs.dask.org/en/latest/array-api.html#dask.array.core.normalize_chunks) with the key previous_chunks (see comment https://github.com/pydata/xarray/pull/2530#discussion_r247352940) It could be particularly useful for (re-)chunking taking into account the previous chunks or the on-disk chunks, especially if the on-disk chunks are small.

I think we can keep talking here about xarray chunking interface.

It seems that the interface for chunking is a tricky problem in xarray. There are involved different interfaces already implemented: - dask: da.rechunk, da.from_array - xarray: xr.open_dataset - xarray: ds.chunk - xarray-zarr: xr.open_dataset(engine="zarr") (≈ xr.open_zarr)

They are similar, but there are some inconsistencies.

dask The allowed values for chunking in dask are: - dictionary (or tuple) - integers > 0 - -1: no chunking (along this dimension) - auto: allow the chunking (in this dimension) to accommodate ideal chunk sizes (default 128MiB)

The allowed values in the dictionary are: -1, auto, None (no change to the chunking along this dimension) Note: None isn't supported outside the dictionary. Note: If chunking along some dimension is not specified then the chunking along this dimension will not change (e.g. {} is equivalent to {0: None})

xarray: xr.open_dataset for all the engines != "zarr" It works as dask but also None is supported. If chunk is None then it doesn't use dask at all.

xarray: ds.chunk It works as dask but also None is supported. None is equivalent to a dictionary with all values None (and equivalent to the empty dictionary).

xarray: xr.open_dataset(engine="zarr") It works as dask except for: - None is supported. If chunk is None then it doesn't use dask at all. - If chunking along some dimension is not specified then encoded chunks are used. - auto is equivalent to the empty dictionary, encoded chunks are used. - auto inside the dictionary is passed on to dask and behaves as in dask.

Points to be discussed:

1) auto and {} The main problem is how to uniform dask and xarray-zarr.

Option 1 Maybe the encoded chunking provided by the backend can be seen just as the current on-disk data chunking. According to dask interface, if in a dictionary the chunks for some dimension are None or not defined, then the current chunking along that dimension doesn't change. From this perspective, we would have: - with auto it uses dask auto-chunking. - with -1 it uses dask but no chunking. - with {} it uses the backend encoded chunks (when available) for on-disk data (xr.open_dataset) and the current chunking for already opened datasets (ds.chunk)

Note: ds.chunk behavior would be unchanged Note: xr.open_dataset would be unchanged, except for engine="zarr", since currently the var.encodings["chunks"] is defined only by zarr.

Option 2 We could use a different new value for the encoded chunks (e.g.encoded TBC). Something like: open_dataset(chunks="encoded") open_dataset(chunks={"x": "encoded", "y": 10,...}) Both expressions could be supported. cons: - chunks="encoded": with zarr the user probably needs to specify always to use the encoded chunks. - chunks="encoded": the user must specify explicitly in the dictionary which dimension should be chunked with the encoded chunks, that's very inconvenient (but is it really used? @weiji14 do you have some idea about it?).

2) None chunks=None should produce the same result in xr.open_dataset and ds.rechunk.

@shoyer, @alexamici, @jhamman, @dcherian, @weiji14 suggestions are welcome

Taking into account the comments in this issue and the calls, I would propose this solution: https://github.com/pydata/xarray/pull/4547

I've tried to replicate the error, but I couldn't.

• The key value auto is redundant because it has the same behavior as {}, we could remove one of them.

That's not completely true. With no dask installed auto uses chunk=None, while {} raises an error. Probably it makes sense.

I agree, open_dataset() currently has a very long signature that should be changed. The interface you proposed is obviously clearer, but a class could give a false idea that all backends support all the decoding options listed in the class. I see two other alternatives: - Instead of a class we could use a dictionary. Pros 1, 2 and 3 would still hold. - With the interface proposed by @alexamici in #4309 the pros 2 and 3 would still hold and partially 1 (since the open_dataset interface would be greatly simplified).

For both these proposals, we would lose the autocompletion with the tab but, on the other hand, the user would be relieved of managing a class. Finally, I'm not sure that for the user it would be clear the separation between backend_kwargs and decode, since they both contain arguments that will be passed to the backend. Especially if the backend needs more specific decoding options that must be set in backend_kwargs. In this sense, #4309 seems less error-prone.

Also with oversampling we have the same problem (2007-02-02 02:00:00 is out of bound):

``` python

import numpy as np import pandas as pd import xarray as xr

time = np.arange('2007-01-01 00:00:00', '2007-02-02 00:00:00', dtype='datetime64[ns]') arr = xr.DataArray( np.arange(time.size), coords=[time,], dims=('time',), name='data' ) resampler = arr.resample(time='3h', base=2, label='right')

resampler DatetimeIndex(['2007-01-01 02:00:00', '2007-01-01 05:00:00', '2007-01-01 08:00:00', '2007-01-01 11:00:00', '2007-01-01 14:00:00', '2007-01-01 17:00:00', '2007-01-01 20:00:00', '2007-01-01 23:00:00', '2007-01-02 02:00:00', '2007-01-02 05:00:00', ... '2007-01-31 23:00:00', '2007-02-01 02:00:00', '2007-02-01 05:00:00', '2007-02-01 08:00:00', '2007-02-01 11:00:00', '2007-02-01 14:00:00', '2007-02-01 17:00:00', '2007-02-01 20:00:00', '2007-02-01 23:00:00', '2007-02-02 02:00:00'], dtype='datetime64[ns]', name='time', length=257, freq='3H') ``` The fix is really very easy, I can try to make pull request.

The coordinates are grouped correctly: ```python list(arr.groupby('x'))

[(1, <xarray.DataArray (x: 3)> array([1., 1., 1.]) Coordinates: * x (x) int64 1 1 1 x2 (x) int64 1 2 3), (2, <xarray.DataArray (x: 2)> array([1., 1.]) Coordinates: * x (x) int64 2 2 x2 (x) int64 4 5)] ``` I think the grouping make sense. But once the groups are collapsed with some operation, I'm not sure that can be found a corresponding meaningful operation to apply to the grouped coordinates.

In the following cases the mean after gourpby() works as expected:

``` arr = xr.DataArray( np.ones(5), dims=('x',), coords={ 'x': ('x', np.array([1, 1, 1, 2, 2])), 'x1': ('x', np.array([1, 1, 1, 2, 2])), 'x2': ('x', np.array([1, 2, 3, 4, 5])),

}

)

arr.groupby('x1').mean('x')

<xarray.DataArray (x1: 2)> array([1., 1.]) Coordinates: * x1 (x1) int64 1 2

arr.groupby((xr.DataArray([1,1,1,2,2], dims=('x'), name='x3'))).mean('x')

<xarray.DataArray (x3: 2)> array([1., 1.]) Coordinates: * x3 (x3) int64 1 2 Also also if I try to group with an array named with as the dimension along which we perform the mean, I get the same problem:python arr.groupby(xr.DataArray([1,1,1,2,2], dims=('x'), name='x')).mean('x')

<xarray.DataArray (x: 2)> array([1., 1.]) Coordinates: * x (x) int64 1 2 x1 (x) int64 1 1 1 2 2 x2 (x) int64 1 2 3 4 5 ```

If I try to use an other dimension name we obtain again an strange behaviour: ```python arr = xr.DataArray( np.ones((5, 2)), dims=('x', 'y'), coords={ 'x': ('x', np.array([1, 1, 1, 2, 2])), 'x1': ('x', np.array([1, 1, 1, 2, 2])), 'x2': ('x', np.array([1, 2, 3, 4, 5])), } )

arr.groupby(xr.DataArray([1,1,1,2,2], dims=('x'), name='y')).mean('x')

<xarray.DataArray (y: 4)> array([1., 1., 1., 1.]) Coordinates: * y (y) int64 1 2

``` In this case probably it should raise an error.