zarr-developers / VirtualiZarr

Create virtual Zarr stores from archival data files using xarray syntax
https://virtualizarr.readthedocs.io/en/latest/
Apache License 2.0
95 stars 18 forks source link

Search for coord_names in separate_coords #191

Open ayushnag opened 2 months ago

ayushnag commented 2 months ago
ayushnag commented 2 months ago

@TomNicholas this closes the issue however I think there is some existing functionality that can be refactored. My understanding is that the current logic is trying to find coordinates within attrs here. However this is accessing the dataset attributes whereas coordinates is within the attrs of each variable. When I debug the current code, coord_names is always empty. I can refactor the code and update this PR to remove the coord_names param from separate_coords and simply use the newly added search within separate_coords

TomNicholas commented 2 months ago

I can refactor the code and update this PR to remove the coord_names param from separate_coords and simply use the newly added search within separate_coords

That would be great @ayushnag. I think the presence of the coord_names kwarg comes from a time when I didn't actually understand where I was supposed to get the information about which variable was a coordinate, so I had left it general. But it seems we don't need it, so let's get rid of it.

We should also do a few other things:

TomNicholas commented 2 months ago

@ayushnag I would like to merge this important bugfix and issue a release of a new version of virtualizarr. Are you likely to have time to come back to this or should I merge this PR and open another?

ayushnag commented 2 months ago

@TomNicholas I was looking into this some more and it seems that the coord_names param is needed for non dimension coordinate cases (there is coordinate information in global dataset .zattrs) and we can't eliminate that function param. When I tried to remove it, the kerchunk roundtrip integration test failed

I unfortunately don't have time to implement your new suggestions but you can feel free to merge this or make an updated PR.