I've converted to draft for now, as I'm still evaluating solutions for the CF encoding/decoding.

As explained I've created two PR (#7719 and #7720) for the "easy" changes from this PR. Would be great, if those could go in fast. Thanks!

@dcherian Just a heads-up: I find this PR getting more and more involved at different parts of the machinery and hard to follow for reviewers. I'll split this up and start with the more or less undisputed changes.

Still hunting for corner cases and issues inside encode_cf_variable/decode_cf_variable.

It looks like I already see some light again. Not sure, if this is the last iteration, but the testsuite is still running green with added and enhanced tests, which is not that bad.

Unfortunately https://github.com/pydata/xarray/issues/2304 is still an issue for now. I'll clarify that later with an added test.

While trying to create a test which specifically tests _choose_float_dtype I've found some issues with checking for availability of scale_factor/add_offset. Now testing for None.

This is now ready for another round of reviews, @dcherian, @Illviljan and @mankoff.

As @mankoff already pointed out, xarray is very generous to try to encode/decode non CF conforming data. This makes things a bit complicated as some issues only surface in rare corner cases.

I've tried to be as explicit in _choose_float_dtype, also added comments/tests where needed.

I'm finding the typing a bit hard. It seems that mypy can't derive the correct types from return types in certain cases.

If at first you don't succeed... It looks like we have something working here.

Some more typing and maybe some more tests covering the cases with scale_factor/add_offset/_FillValue non-conforming CF and we should be good to go. Or do I miss something?

The latest changes brake #1840 again. We have two contradicting forces here, which need to be aligned.

@Illviljan I'm not able to figure out the typing if I want to use Data-types as functions to convert python numbers to array scalars. If you have any suggestion how to fix this, please let me know.

@dcherian @Illviljan Thanks for the first round of review. I've rebased everything on latest main. Now the code moving from conventions.py to coding.variable.py is correct. I've also removed the functions which have been converted to VariableCoders and adapted the tests.

To sum up this PR, it does:

• convert functions to VariableCoders along @shoyer's TODO: https://github.com/pydata/xarray/blob/1c81162755457b3f4dc1f551f0321c75ec9daf6c/xarray/conventions.py#L298-L302 https://github.com/pydata/xarray/blob/1c81162755457b3f4dc1f551f0321c75ec9daf6c/xarray/conventions.py#L393-L405
• preserve boolean dtype within encoding: https://github.com/pydata/xarray/issues/7652#issuecomment-1476956975
• deterrmine cf packed dtype from scale_factor/add_offset

7691, #2304

@dcherian @basnijholt

After the dev-meeting I've taken a step back and first implemented the coders as mentioned in @shoyer's ToDo.

I've fixed the one bool->int issue and it now derives the dtype for ScaleOffset coding from scale_factor add_offset.

I've improved some test with regard to the scale/offset issue.

I'll concentrate on the string fillvalue issues in a follow up PR.

Just leaving a note here. I would expect that the datatype that was saved, is the datatype that is loaded. So preferably if I save a string array of e.g., type <U5, I expect it would still be <U5 when loaded, not suddenly HDF5 VLEN types.

Thanks again @kmuehlbauer for digging into this problem and all your work! 😄

make

That's 15 UTC, or 17 CEST (my local time). Should work for me. I'll try to collect all available information on that topic and the current status.

Any comments appreciated.

Let's discuss at next week's meeting. @kmuehlbauer can you make it? 9.30am MT Wed 29 Mar 2023

And yet another related issue: https://github.com/pydata/xarray/issues/1647

Currently both netcdf/h5netcdf are able to set a _FillValue for VLEN strings, but for the numpy-side "NaN" can only be handled with dtype=object. Maybe it's time to consolidate string handling in xarray. But that should be taken care of in a separate issue / feature branch.

XRef: https://github.com/pydata/xarray/issues/2040#issuecomment-379418732

Citing @shoyer from above comment:

The main reason why we don't do any special handling for object arrays currently in xarray is that our conventions coding/decoding system has no way of marking variable length string arrays. We should probably handle this by making a custom dtype like h5py that marks variables length strings using dtype metadata: http://docs.h5py.org/en/latest/special.html#variable-length-strings

Another interesting issue by @shoyer: https://github.com/pydata/xarray/issues/2059

I'm really uncertain if using .astype here is the right way to go. Any comments appreciated.

But not int64 -> int32, and <U1 -> O

@basnijholt I've explained the issues whereabouts over at your issue https://github.com/pydata/xarray/issues/7652#issuecomment-1479080036.

@basnijholt Thanks for testing. I can't reproduce this. Here everything works as expected. But I've a slightly different environment (full netcdf4-stack, latest versions of everything).

The tests which check for dtype equality (vlen-case) do not raise in this PR for any backend and array container, so I'm assuming this should work as expected (at least for bool->int8, and <U1 -> O.

As stated over at #7652 I can't reproduce the int64->int32 conversion in any environments I tested so far. I'll have another look at your environment.

Thanks a lot for the quick PR!

I can confirm that this fixes - https://github.com/pydata/xarray/issues/7652#issuecomment-1476956975 (bool -> int8)

But not int64 -> int32, and <U1 -> O - https://github.com/pydata/xarray/issues/7652#issuecomment-1476967312 - https://github.com/pydata/xarray/issues/7652#issue-1632718954

The one failed run might be spurious. Maybe a re-run of this would work.

I'll add to whats-new.rst if an updated version is merged.