search

fmaussion / salem

Add geolocalised subsetting, masking, and plotting operations to xarray
http://salem.readthedocs.io
Other
186 stars 44 forks source link

Use center_grid when opening tiff-file #234

Closed pat-schmitt closed 9 months ago

pat-schmitt commented 10 months ago

There seems to be a bug when opening a tiff-file with ds=salem.open_xr_dataset(tiff_path) and calling ds.salem.grid afterwards. When opening the tiff-file the resulting grid is defined with corner-reference here https://github.com/fmaussion/salem/blob/master/salem/datasets.py#L288, but when you call ds.salem.grid it is assumed that the coordinates are with center-reference here https://github.com/fmaussion/salem/blob/master/salem/sio.py#L425 and here https://github.com/fmaussion/salem/blob/master/salem/sio.py#L333.

Related to https://github.com/OGGM/oggm/pull/1682

fmaussion commented 10 months ago

Thanks! This is not great - I hope not too many people used this feature

fmaussion commented 10 months ago

@pat-schmitt I have the impression that a test needs to be adapted. Can you confirm?

pat-schmitt commented 10 months ago

Yes, one test was failling. I explicitly compare now the center_grids and it is working (locally).

However, if it is more intuitive that GeoTiff returns a center-grid, we can undo all changes I made and just set the returned grid to center grid here https://github.com/fmaussion/salem/blob/master/salem/datasets.py#L292. I do not know what is the better way to go, maybe returning the center grid directly avoids unintended mistakes...

fmaussion commented 10 months ago

Thanks @pat-schmitt

This API is now quite old, and certain choices are outdated indeed. But I think it's okay to have a Grid that is closest to the internal model of the data (center or corner depending on the underlying dataset). However the mistake was when converting the dataset to the xarray datamodel, where it MUST be center based indeed. So if the tests are green I think we are good to go.

pat-schmitt commented 10 months ago

Ok.

I do not think that the other tests that are failing are related to the changes in this PR. At least I can not reproduce them locally...

fmaussion commented 10 months ago

I do not think that the other tests that are failing are related to the changes in this PR. At least I can not reproduce them locally...

No, they are probably due to changes in xarray and need investigation. Will have a look