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

Use center_grid when opening tiff-file #234

Merged
merged 2 commits into from
Jan 31, 2024

Conversation

pat-schmitt
Copy link
Contributor

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 OGGM/oggm#1682

  • closes #xxxx
  • tests added / passed
  • passes git diff upstream/master | flake8 --diff
  • whatsnew entry

@fmaussion
Copy link
Owner

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

@fmaussion
Copy link
Owner

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

@pat-schmitt
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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

@fmaussion fmaussion merged commit f5e7e09 into fmaussion:master Jan 31, 2024
5 of 8 checks passed
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