issue_comments

Thanks @rabernat

I think this PR has received a very thorough review. I would be pleased if someone from @pydata/xarray would merge it soon.

the reason is that black released a new version yesterday, and since we don't pin black for the blackdoc entry we get the new version. If you run pre-commit clean before pre-commit run --all-files you should see this change locally, too. To avoid situations like these we could start pinning black in the blackdoc entry (and run a script to synchronize this with the black entry on autoupdate).

Hello @rabernat! 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! :beers:

Comment last updated at 2021-04-26 14:38:56 UTC

The pre-commit workflow is raising a blackdoc error I am not seeing in my local env

```diff diff --git a/doc/internals/duck-arrays-integration.rst b/doc/internals/duck-arrays-integration.rst index eb5c4d8..2bc3c1f 100644 --- a/doc/internals/duck-arrays-integration.rst +++ b/doc/internals/duck-arrays-integration.rst @@ -25,7 +25,7 @@ argument: ...

     def _repr_inline_(self, max_width):

• """ format to a single line with at most max_width characters """
• """format to a single line with at most max_width characters""" ... ```

Any further feedback on this now reduced-scope PR? Merging this would be helpful for moving forward Pangeo forge.

I have removed the controversial encoding['chunks'] stuff from the PR. Now it only contains the safe_chunks option in to_zarr.

If there are no further comments on this, I think this is good to go.

We seem to be unable to resolve the complexities around chunk encoding. I propose to remove this from the PR and reduce the scope to just the safe_chunks features. @aurghs should probably be the one to tackle the chunk encoding problem; unfortunately it exceeds my understanding, and I don't have time to dig deeper at the moment. In the meantime safe_chunks is important for pangeo-forge forward progress.

Please give a 👍 or 👎 to this idea if you have an opinion.

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.

My use for encoding.chunks is to tell Zarr what chunks to use on disk.

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.

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.

Why is chunk getting called here? Does it actually get called every time we load a dataset with chunks? If so, we will need a more sophisticated solution.

This happens specifically on this line: https://github.com/pydata/xarray/blob/ddc352faa6de91f266a1749773d08ae8d6f09683/xarray/core/dataset.py#L438

So perhaps it would make sense to copy encoding specifically in this case, e.g., python new_var = var.chunk(chunks, name=name2, lock=lock) new_var.encoding = var.encoding

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

In the long term, the whole handling of encoding should be revisited, e.g., see https://github.com/pydata/xarray/issues/5082

So any ideas how to proceed? 🧐

~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

A just pushed a new commit which deletes all encoding inside variable.chunk(). But as you will see when the CI finishes, this leads to a lot of test failures. For example:

``` =============================================================================== FAILURES ================================================================================ _______ TestNetCDF4ViaDaskData.testroundtrip_string_encoded_characters ________

self = <xarray.tests.test_backends.TestNetCDF4ViaDaskData object at 0x18cba4c40>

def test_roundtrip_string_encoded_characters(self):
    expected = Dataset({"x": ("t", ["ab", "cdef"])})
    expected["x"].encoding["dtype"] = "S1"
    with self.roundtrip(expected) as actual:
        assert_identical(expected, actual)

      assert actual["x"].encoding["_Encoding"] == "utf-8"

E KeyError: '_Encoding'

/Users/rpa/Code/xarray/xarray/tests/test_backends.py:485: KeyError ```

Why is chunk getting called here? Does it actually get called every time we load a dataset with chunks? If so, we will need a more sophisticated solution.

Replace self._encoding with None here?

Thanks! Yeah that's what I had in mind. But I was wondering if there was an example of doing that it else I could copy.

In any case, I'll give it a try now.

The problem is, I can't figure out where this happens.

Replace self._encoding with None here? https://github.com/pydata/xarray/blob/ddc352faa6de91f266a1749773d08ae8d6f09683/xarray/core/variable.py#L1084

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.

In today's dev call, we proposed to handle encoding in chunk the same way we handle it in indexing: by deleting all encoding.

The problem is, I can't figure out where this happens. Can someone point me to the place in the code where indexing operations delete encoding?

A related question: I discovered this encoding option preferred_chunks, which is treated specially: https://github.com/pydata/xarray/blob/57a4479fcd3ebc579cf00e0d6bf85007eda44b56/xarray/core/dataset.py#L396

Should the Zarr backend be setting this?

I appreciate the discussion on this PR. Does anyone have a concrete suggestion of what to do?

If we are not in agreement about the encoding stuff, perhaps I should remove that and just move forward with the safe_chunks part of this PR?

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

FWIW, I would also favor dropping encoding['chunks'] after indexing, coarsening, interpolating, etc. Basically anything that changes the array shape or chunk structure.

We already drop all of encoding after indexing. My guess is that we do the same for coarsening and interpolations as well (though I haven't checked).

Perhaps a kwarg in to_zarr like ignore_encoding_chunks?

I would argue that this is unnecessary. If you want to explicitly drop encoding, just del da.encoding['chunks'] before writing. But most users don't figure out that they should do this, because the default behavior is counterintuitive.

The problem here is with the default behavior of propagating chunk encoding through computations when it no longer makes sense. My example with the dtype encoding illustrates that we already drop encoding on certain operations, so it's not unprecedented. It's more of an implementation question: where and how to do the dropping.

FWIW, I would also favor dropping encoding['chunks'] after indexing, coarsening, interpolating, etc. Basically anything that changes the array shape or chunk structure.

Xarray knows to drop the dtype encoding after an arithmetic operation. How does that work?

There's a subtle difference. It drops all of .encoding not dtype specifically.

@shoyer's point about indexing changing chunking is a good one too. Perhaps a kwarg in to_zarr like ignore_encoding_chunks?

Xarray knows to drop the dtype encoding after an arithmetic operation. How does that work? To me .chunk feel like a similar case: an operation that invalidates any existing encoding.

To be honest, the existing convention is quite adhoc, just based on what seemed most appropriate at the time.

https://github.com/pydata/xarray/issues/1614 is most comprehensive description of the current state of things.

We were considering saying that attrs and encoding should always use the same rules, but perhaps we should be more aggressive about dropping encoding.

I see your point. I guess I don't fully understand where else in the code path encoding gets dropped. Consider this example

python import xarray as xr ds = xr.Dataset({'foo': ('time', [1, 1], {'dtype': 'int16'})}) ds = xr.decode_cf(ds).compute() assert "dtype" in ds.foo.encoding assert "dtype" not in (0.5 * ds.foo).encoding

Xarray knows to drop the dtype encoding after an arithmetic operation. How does that work? To me .chunk feel like a similar case: an operation that invalidates any existing encoding.

I'm a little conflicted about dealing with encoding['chunks'] specifically in chunk():

• On one hand, it feels inconsistent for this only this single method in xarray to modify part of encoding. Nothing else in xarray (after CF decoding) does this. Effectively encoding['chunks'] is now becoming a part of xarray's data model.
• On the other hand, this would absolutely fix a recurrent pain-point for users, and in that sense it's worth doing.

Maybe this isn't such a big deal in this particular case, especially if we don't think we would need to add such encoding specific logic to any other methods. But are we really sure about that -- what about cases like indexing?

I guess the other alternative to make chunk() and various other methods that would change chunking drop encoding entirely. I don't know if this would really be a better comprehensive solution (I know dropping attrs is much hated), but at least it's an easier mental model.

Thanks Anderson. Fixed by rebasing. Now RTD build is failing, but there is no obvious error in the logs...

Confused about the test error. It seems unrelated. In test_sparse.py:test_variable_method

E TypeError: no implementation found for 'numpy.allclose' on types that implement __array_function__: [<class 'numpy.ndarray'>, <class 'sparse._coo.core.COO'>]

Related to #5059, and it appears that @keewis came up with a fix for it in https://github.com/pydata/xarray/pull/5059#issuecomment-803620128

Confused about the test error. It seems unrelated. In test_sparse.py:test_variable_method

E TypeError: no implementation found for 'numpy.allclose' on types that implement __array_function__: [<class 'numpy.ndarray'>, <class 'sparse._coo.core.COO'>]