Skip to content

Conversation

@abarciauskas-bgse
Copy link

@abarciauskas-bgse abarciauskas-bgse commented Jan 31, 2025

This PR fixes tests and some warnings. Would like to continue to fix as many warnings as possible.

2 of the failed tests relate to how we're handling codecs. I'm not sure it's 100% correct but may be worth merging to work on refactoring our zarr array representation in a separate set of changes. Related PR: zarr-developers#175 (haven't looked closely at this yet)

@jsignell

- name: Running Tests
run: |
python -m pytest --run-network-tests --verbose --cov=virtualizarr --cov-report=xml
python -m pytest --run-network-tests --verbose --cov-report=xml
Copy link
Author

@abarciauskas-bgse abarciauskas-bgse Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ This fixes a warning CoverageWarning: --include is ignored because --source is set (include-ignored) self.warn("--include is ignored because --source is set", slug="include-ignored")

- nodefaults
dependencies:
- xarray>=2024.10.0,<2025.0.0
- xarray>=2025.1.1
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ This fixes the Group.create_array() got an unexpected keyword argument 'exists_ok' test failure

conftest.py Outdated
# Save it to disk as netCDF (in temporary directory)
ds.to_netcdf(filepath, format="NETCDF4")
air_encoding = ds["air"].encoding
air_encoding["_FillValue"] = -9999
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes a warning SerializationWarning: saving variable air with floating point data as an integer dtype without any _FillValue to use for NaNs

group=group,
decode_times=decode_times,
)
) as ds:
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ This fixes a RuntimeWarning: deallocating CachingFileManager ... but file is not already closed. This may indicate a bug warning

assert index_mappings_equal(vds.xindexes, ds.xindexes)
assert list(ds.coords) == list(vds.coords)
assert vds.dims == ds.dims
assert vds.sizes == ds.sizes
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Fixes FutureWarning: The return type of Dataset.dims will be changed to return a set of dimension names in future, in order to be more consistent with DataArray.dims. To access a mapping from dimension names to lengths, please use Dataset.sizes.

assert (
isinstance(metadata["codecs"], list)
and len(metadata["codecs"]) > 1
and len(metadata["codecs"]) == 1
Copy link
Author

@abarciauskas-bgse abarciauskas-bgse Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Fixes failing test

# Get codecs and verify
actual_codecs = get_codecs(manifest_array, normalize_to_zarr_v3=True)
expected_codecs = manifest_array.zarray._v3_codec_pipeline()
expected_codecs = (
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Fixes failing test

@abarciauskas-bgse
Copy link
Author

I decided to reset some of the warnings so this PR is just changes to fix the tests. I'll work on a separate branch to fix warnings.

@jsignell jsignell merged commit 77b1088 into jsignell:main-kerchunk Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants