CF encoding should preserve vlen dtype for empty arrays

[tomwhite]

• Closes Zarr store array dtype changes for empty object string #7328
• Tests added

The idea is that it enables a workaround for #7328, where the dtype metadata can be set to a vlen string so that it doesn't get changed to float.

The line that is changed in the example in #7328 is:

ds = xr.Dataset({"a": np.array([], dtype=coding.strings.create_vlen_dtype(str))})

[kmuehlbauer]

Thanks @tomwhite for the PR. I've only quickly checked the approach, which looks reasonable. But those changes have implications on several locations of the backend code, which we would have to sort out.

Considering this example:

import numpy as np
import xarray as xr
print(f"creating dataset with empty string array")
print("-----------------------------------------")
dtype = xr.coding.strings.create_vlen_dtype(str)
ds = xr.Dataset({"a": np.array([], dtype=dtype)})
print(f"dtype: {ds['a'].dtype}")
print(f"metadata: {ds['a'].dtype.metadata}")
ds.to_netcdf("a.nc", engine="netcdf4")

print("\nncdump")
print("-------")
!ncdump a.nc

engines = ["netcdf4", "h5netcdf"]
for engine in engines:
    with xr.open_dataset("a.nc", engine=engine) as ds:
        print(f"\nloading with {engine}")
        print("-------------------")
        print(f"dtype: {ds['a'].dtype}")
        print(f"metadata: {ds['a'].dtype.metadata}")

creating dataset with empty string array
-----------------------------------------
dtype: object
metadata: {'element_type': <class 'str'>}

ncdump
-------
netcdf a {
dimensions:
	a = UNLIMITED ; // (0 currently)
variables:
	string a(a) ;
data:
}

loading with netcdf4
-------------------
dtype: object
metadata: None

loading with h5netcdf
-------------------
dtype: object
metadata: {'vlen': <class 'str'>}

Engine netcdf4 does not roundtrip here, losing the dtype metadata information. There is special casing for h5netcdf backend, though.

The source is actually located in open_store_variable of netcdf4 backend, when the underlying data is converted to Variable (which does some object dtype twiddling).

Unfortunately I do not have an immediate solution here.

[tomwhite]

Thanks for taking a look @kmuehlbauer and for the useful example code. I hadn't considered the netcdf cases, so thanks for pointing those out.

Engine netcdf4 does not roundtrip here, losing the dtype metadata information. There is special casing for h5netcdf backend, though.

Could netcdf4 do the same special-casing as h5netcdf?

[kmuehlbauer]

@tomwhite Special casing on netcdf4 backend should be possible, too.

But it might need fixing at zarr backend, too:

ds = xr.Dataset({"a": np.array([], dtype=xr.coding.strings.create_vlen_dtype(str))})
print(f"dtype: {ds['a'].dtype}")
print(f"metadata: {ds['a'].dtype.metadata}")
ds.to_zarr("a.zarr")
print("\n### Loading ###")
with xr.open_dataset("a.zarr", engine="zarr") as ds:
    print(f"dtype: {ds['a'].dtype}")
    print(f"metadata: {ds['a'].dtype.metadata}")

dtype: object
metadata: {'element_type': <class 'str'>}

### Loading ###
dtype: object
metadata: None

Could you verify the above example, please? I'm relatively new to zarr 😬

[kmuehlbauer]

@tomwhite I've put a commit with changes to zarr/netcdf4-backends which should preserve the dtype metadata here: https://github.com/kmuehlbauer/xarray/tree/preserve-vlen-string-dtype.

I'm not really sure if that is the right location, but as it was already present that location at netcdf4-backend I think it will do.

[tomwhite]

Could you verify the above example, please?

The code looks fine, and I get the same result when I run it with this PR.

Your fix in https://github.com/kmuehlbauer/xarray/tree/preserve-vlen-string-dtype changes the metadata so it is correctly preserved as metadata: {'element_type': <class 'str'>}.

I feel less qualified to evaluate the impact of the netcdf4 fix.

[kmuehlbauer]

Thanks for trying. I can't think of any downsides for the netcdf4-fix, as it just adds the needed metadata to the object-dtype. But you never know, so it would be good to get another set of eyes on it.

So it looks like the changes here with the fix in my branch will get your issue resolved @tomwhite, right?

I'm a bit worried, that this might break other users workflows, if they depend on the current conversion to floating point for some reason. Also other backends might rely on this feature. Especially because this has been there since the early days when xarray was known as xray.

@dcherian What would be the way to go here?

There is also a somehow contradicting issue in #7868.

[tomwhite]

So it looks like the changes here with the fix in my branch will get your issue resolved @tomwhite, right?

Yes - thanks!

I'm a bit worried, that this might break other users workflows, if they depend on the current conversion to floating point for some reason.

The floating point default is preserved if you do e.g. xr.Dataset({"a": np.array([], dtype=object)}). The change here will only convert to string if there is extra metadata present that says it is a string.

[kmuehlbauer]

After discussion with @tomwhite this PR should now preserve the vlen dtype for empty arrays (on encoding) and preserve the vlen dtype metadata for zarr/netcdf4 backends.

Tests would have to be added for the latter and a zarr expert might want to have another look at that part.

[kmuehlbauer]

@tomwhite I've added tests to check the backend code for vlen string dtype metadadata. Also had to add specific check for the h5py vlen string metadata. I think we've covered everything for the proposed change to allow empty vlen strings dtype metadata.

I'm looking at the mypy error and do not have the slightest clue what and where to change. Any help appreciated.

[tomwhite]

@kmuehlbauer thanks for adding tests! I'm not sure what the mypy error is either, I'm afraid...

[dcherian]

xarray/tests/test_coding_strings.py:36: error: No overload variant of "dtype" matches argument types "str", "Dict[str, Type[str]]" [call-overload]

cc @Illviljan @headtr1ck

[headtr1ck]

This seems to be a numpy issue, mypy thinks that you cannot call np.dtype like you do.

Might be worth an issue over at numpy with the example from the test.

For now we can simply ignore this error.

[kmuehlbauer]

Might be worth an issue over at numpy with the example from the test.

numpy/numpy#23886

[kmuehlbauer]

Might be worth an issue over at numpy with the example from the test.

numpy/numpy#23886

The issue is already resolved over at numpy which is really great! It was also marked as backport. @headtr1ck How are these issues resolved currently or how do we track removing the ignore?

[headtr1ck]

If you want you can leave a comment
But the mypy CI should fail on unused ignores, so we will notice :)

[kmuehlbauer]

But the mypy CI should fail on unused ignores, so we will notice :)

I've added an ignore for now.

[jhamman]

Overall, this looks approach seems fine to me. I left one comment that should be easy to address. Thanks @tomwhite and @kmuehlbauer for making this work.

[kmuehlbauer]

Thanks @tomwhite and all others for getting this in.

[tomwhite]

Thank you @kmuehlbauer for your work on this!