Correctly handle HDF5 fillvalue for string dtype arrays.

[sharkinsspatial]

What I did

Updated the HDF5 parser to support string dtype arrays with a bytes or str fillvalue.

Acceptance criteria:

• Closes Failure to read HDF5 data with data arrays of dtype='<U1' #878
• Tests added
• Tests passing
• No test coverage regression

[codecov]

Codecov Report

❌ Patch coverage is 86.36364% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.30%. Comparing base (fbe8afc) to head (dc0e05d).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
virtualizarr/parsers/hdf/hdf.py	83.33%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #988      +/-   ##
==========================================
- Coverage   90.36%   90.30%   -0.06%     
==========================================
  Files          37       37              
  Lines        2242     2260      +18     
==========================================
+ Hits         2026     2041      +15     
- Misses        216      219       +3     

Files with missing lines	Coverage Δ
virtualizarr/codecs.py	93.44% <100.00%> (+0.22%)	⬆️
virtualizarr/parsers/hdf/hdf.py	94.91% <83.33%> (-2.15%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TomNicholas]

Good to merge now so I can include it in the release?

[maxrjones]

Good to merge now so I can include it in the release?

let me take a quick look at the codecs changes. Assigning vlen bytes to all object dtypes seems a bit weird to me.

[abarciauskas-bgse]

I'm wondering if this could be introducing a different type of bug where now we are introducing a value for the zarr metadata fill_value that could differ from what is in the dataset's original metadata. I realized today that for GPM IMERG HH precipitation the attrs from the h5py Dataset include both _FillValue and CodeMissingValue which both are -9999.0:

h5ds = h5py.File(s3fs.open(gpm_imerg_s3_url))
h5ds['/Grid/precipitation'].attrs['CodeMissingValue']
# np.bytes_(b'-9999.9')
h5ds['/Grid/precipitation'].attrs['_FillValue']
# np.float32(-9999.9)

Whereas this fallback returns 1e+20:

np.ma.default_fill_value(h5ds['/Grid/precipitation'].dtype)
# 1e+20

I recognize that CodeMissingValue is not a standard attribute (as far as I can tell), but should the zarr fill_value attribute fall back to the _FillValue attribute picked up by h5py?

Looking at NASA-IMPACT/veda-odd#371 would this be handled by the more general purpose phase 3 "fill_value_defined() distinction: stop propagating h5py-default fills to zarr storage."

[abarciauskas-bgse]

@maxrjones curious what you think

[sharkinsspatial]

@abarciauskas-bgse For clarification, fill_value and _FillValue are distinct semantic concepts https://virtualizarr.readthedocs.io/en/stable/custom_parsers.html#fill-values. fill_value is always set by HDF5 but as you noted above, not all types of fill_value that HDF5 can produce are supported by Zarr v3.

[abarciauskas-bgse]

Right ok, thanks for reminding me about that distinction -- I recalled there were 2 semantic concepts but forgot about the details and that excellent documentation. In that case, since this function is for the first fill value concept -- Value for uninitialized chunks - (e.g., Zarr fill_value) -- then probably this fallback is reasonable?

And I recognize that the h5ds['/Grid/precipitation'].attrs['_FillValue'] is for the 2nd concept Sentinel value - (e.g., CF _FillValue ))

[maxrjones]

The CodeMissingValue is a different issue, but this is a good flag since Zarr's default fill values could differ from numpy or support dtypes not defined in np.ma.default_fill_value

@sharkinsspatial why did you chose to use numpy's default fill values here?

[sharkinsspatial]

@maxrjones @abarciauskas-bgse I'm just returning to this now (I mistakenly thought this had been altered and merged). I just did some investigation and found that

• Very old versions of h5py would throw on fillvalue for vlen strings and object types but this was fixed by Test default fill values for some common types h5py/h5py#2132.
• I ran tests with Claude for a wide range of dtypes and was unable to force an exception.
• It is theoretically possible for a non-HDF5 library (Matlab, netcdf-4) to create a non-compliant HDF dataset that has no fillvalue but I think the likelihood of encountering this is very low.
• As @maxrjones note it might be best for us to avoid injecting our own default fillvalue here in favor of just erroring on this actually exceptional case.

If you're ok I will revert the addition from your PR.

[abarciauskas-bgse]

FWIW I encountered this issue (where the fillvalue attribute was absent on an HDF5 dataset, causing the runtime error) while working on the virtual icechunk store for the GPM IMERG HH dataset (see https://github.com/virtual-zarr/gpmimerghh-virtualizarr-data-pipeline/blob/main/lambda/virtualizarr-processor/pyproject.toml#L10).

I don't know about the likelihood of encountering other HDF datasets with no fillvalue attribute. But just to be clear I wasn't trying to handle cases 1 and 2 from the bullets above.

It's fine with me if you need to revert the change, but I am curious what the path is for HDF datasets which have no fillvalue attribute.

[sharkinsspatial]

Only HDF datasets that are variable length arrays, object references or region references will return None for fillvalue. In h5py these are all represented as the numpy kind O with no way to distinguish between them. @maxrjones 's suggestion added a check first for string dtypes (because unfortunately vlen strings are also represented as O) and the throws NotImplementedError for any other type of dataset.

Theoretically we can expose vlen_dtype and ref_dtype through v3's VariableLengthBytes but this will likely be pretty complex and we can defer for a separate PR.

[abarciauskas-bgse]

I think what I encountered was different, where the fillvalue attribute was missing entirely, i.e.:

# when fillvalue is `None`
hasattr(dataset, 'fillvalue') => True
# when fillvalue is missing
hasattr(dataset, 'fillvalue') => False

So one workaround could be to collapse both scenarios to None:

raw = getattr(dataset, 'fillvalue', None)

however I realize the missing attribute scenario may not be likely or the purview of this PR (and like you both mentioned we may not want to inject the numpy default in this case) so fine with me if you want to revert this change.

[sharkinsspatial]

@abarciauskas-bgse In this case we are unconcerned with the arbitrary attrs and only concerned with the HDF 5 dataset's fillvalue property. In the attrs case fillvalue is semantically meaningless as the correct CF convention attr for sentinel masking would be _FillValue https://virtualizarr.readthedocs.io/en/stable/explanation/custom_parsers.html#fill-values The parser has separate logic for handling and encoding the _FillValue attr so that it can be used by xarray and other downstream libs for sentinel masking https://github.com/zarr-developers/VirtualiZarr/blob/main/virtualizarr/parsers/hdf/hdf.py#L74-L76

[sharkinsspatial]

@TomNicholas This is ready for final review.