Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for typo? in filetype docstring #208

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jbusecke
Copy link
Contributor

@jbusecke jbusecke commented Aug 3, 2024

I just ran into a minor, but confusing issue with the docstrings for the filetype input for open_virtual_dataset.

When I look at the docstring (specifically this line), it suggests that this would work:

from virtualizarr import open_virtual_dataset
vds = open_virtual_dataset(
        ...,
        filetype='netCDF3',
) 

but it raises the following error

the following works:

vds = open_virtual_dataset(
        ...,
        filetype='netcdf3',
) 

Looking at the tests, I suspect this is simply a typo in the docstring (upercase 'CDF' vs lowercase 'cdf'), but maybe I am missing something?

Happy to add more if desired.

  • Closes #xxxx
  • Tests added
  • Tests passing
  • Full type hint coverage
  • Changes are documented in docs/releases.rst
  • New functions/methods are listed in api.rst
  • New functionality has documentation

I just ran into a minor, but confusing issue with the docstrings for the `filetype` input for `open_virtual_dataset`. 

When I look at the docstring (specifically [this line](https://github.com/zarr-developers/VirtualiZarr/blob/b34a1ee47572901ed6a3623c97d340157a0fa62a/virtualizarr/xarray.py#L61)), it suggests that this would work:

```python
from virtualizarr import open_virtual_dataset
vds = open_virtual_dataset(
        ...,
        filetype='netCDF3',
) 
```

but it raises the following error

<details>

</details>

the following works:
```python
vds = open_virtual_dataset(
        ...,
        filetype='netcdf3',
) 
```

Looking at the [tests](https://github.com/zarr-developers/VirtualiZarr/blob/b34a1ee47572901ed6a3623c97d340157a0fa62a/virtualizarr/tests/test_xarray.py#L341), I suspect this is simply a typo in the docstring (upercase `'CDF'` vs lowercase `'cdf'`), but maybe I am missing something?

Happy to add more if desired.
@@ -58,7 +58,7 @@ def open_virtual_dataset(
File path to open as a set of virtualized zarr arrays.
filetype : FileType, default None
Type of file to be opened. Used to determine which kerchunk file format backend to use.
Can be one of {'netCDF3', 'netCDF4', 'HDF', 'TIFF', 'GRIB', 'FITS', 'zarr_v3'}.
Can be one of {'netcdf3', 'netcdf4', 'hdf4', 'tiff', 'grib', 'fits', 'zarr_v3'}.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not entirely shure about the other cases here, specifically 'hdf4' and 'hdf5'?

@TomNicholas
Copy link
Member

This should work, because we call str.lower() here

if filetype.name.lower() == "netcdf3":

What error did you get? You didn't actually paste it into the comment above.

@jbusecke
Copy link
Contributor Author

jbusecke commented Aug 7, 2024

Ughh I am a moron, haha. Let me change the test here to demonstrate.

@jbusecke
Copy link
Contributor Author

jbusecke commented Aug 7, 2024

If I remember correctly the error was happening when the FileType class is instantiated here. I have to admit I have 0 clue what is actually happening within that class 😆.

@jbusecke
Copy link
Contributor Author

jbusecke commented Aug 7, 2024

Hmm that is odd, I cannot reproduce this error here. I might have some time later to try to surface this again, sorry for the messyness..

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