search

zarr-developers / VirtualiZarr

Create virtual Zarr stores from archival data files using xarray syntax
https://virtualizarr.readthedocs.io/en/stable/api.html
Apache License 2.0
124 stars 24 forks source link

Add virtual_backend_kwargs argument to open_virtual_dataset #315

Closed TomNicholas closed 1 day ago

TomNicholas commented 5 days ago

For https://github.com/zarr-developers/VirtualiZarr/pull/243#issuecomment-2492341326 we need to pass a fs_root kwarg down to some but not all readers. We will have other reader-specific kwargs like this come up in future too. To be in keeping with xarray.open_dataset's API these should be passed as a dict called reader_kwargs or backend_kwargs.

However there is an existing reader_options kwarg, which is meant to be specifically for fsspec options. Having both reader_options and reader_kwargs would be confusing, so should we a) rename reader_options to reader_kwargs, having it include fsspec options too, b) have reader_kwargs and fsspec_kwargs, deprecating reader_options?

So far this PR just adds a reader_kwargs option to all backends, but raises if it is used.

cc @norlandrhagen @sharkinsspatial

codecov[bot] commented 5 days ago

Codecov Report

Attention: Patch coverage is 37.50000% with 10 lines in your changes missing coverage. Please review.

Project coverage is 93.30%. Comparing base (3348670) to head (41ec08f).

Files with missing lines Patch % Lines
virtualizarr/readers/dmrpp.py 0.00% 2 Missing :warning:
virtualizarr/readers/tiff.py 0.00% 2 Missing :warning:
virtualizarr/readers/fits.py 50.00% 1 Missing :warning:
virtualizarr/readers/hdf/hdf.py 50.00% 1 Missing :warning:
virtualizarr/readers/hdf5.py 50.00% 1 Missing :warning:
virtualizarr/readers/kerchunk.py 50.00% 1 Missing :warning:
virtualizarr/readers/netcdf3.py 50.00% 1 Missing :warning:
virtualizarr/readers/zarr_v3.py 50.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #315 +/- ## ========================================== + Coverage 84.62% 93.30% +8.68% ========================================== Files 48 48 Lines 3362 3378 +16 ========================================== + Hits 2845 3152 +307 + Misses 517 226 -291 ``` | [Flag](https://app.codecov.io/gh/zarr-developers/VirtualiZarr/pull/315/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zarr-developers) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/zarr-developers/VirtualiZarr/pull/315/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zarr-developers) | `93.30% <37.50%> (+8.68%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zarr-developers#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

🚨 Try these New Features:

TomNicholas commented 1 day ago

I've changed this PR to use virtual_backend_kwargs instead of reader_kwargs, which is closer to xarray's equivalent backend_kwargs, and also makes it less similar to reader_options. We should discuss this choice before implementing #245 though (as then it will become harder to change).