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

Dem gridded_data shifted coordinates #1682

Merged
merged 6 commits into from
Jan 30, 2024

Conversation

pat-schmitt
Copy link
Member

This fixes a coordinate shift between dem.tif and the gridded_data.nc.

However, there seems to be a bug in salem when opening a .tif-file using ds = salem.open_xr_dataset(tiff_path) and calling ds.salem.grid afterwards. I will open a PR for salem and link it here.

Due to this bug, one test is currently not working and I commented it out. After salem is fixed the test can be uncommented again (and maybe adapted due to floating point problems...).

Closes #1679

  • Tests added/passed
  • Fully documented
  • Entry in whats-new.rst

@pat-schmitt
Copy link
Member Author

Here the PR for salem fmaussion/salem#234

assert np.allclose(gridded_topo.data, gtiff_ds.data)

# compare coordinates of topo.tif with dem.tif
demtiff_ds = salem.open_xr_dataset(gdir.get_filepath('dem'))
Copy link
Member

Choose a reason for hiding this comment

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

Can we try to avoid using salem here, but use rioxarray instead? It might be possible to use rioxarray to open but still use salem's accessor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I can use rioxarray to open the tiff files and compare the coordinates, I will change this. But I can not use salem's accessor because their is no variable .pyproj_srs added to the dataset when opening with rioxarray.

Copy link
Member

Choose a reason for hiding this comment

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

OK yes I hoped that the coords would be enough, but indeed the proj info is missing - no worries, that's enough as is for now.

@pat-schmitt
Copy link
Member Author

Ok, now the tiff-files are opened with rioxarray and this PR is now independent of the bug in salem and can be merged if all tests pass.
I will add this to whats-new now.

@pat-schmitt
Copy link
Member Author

It seems that for the minimal test rioxarray is not available. How can I solve this?

@fmaussion
Copy link
Member

Sorry for the mess @pat-schmitt - just ignore and keep it simple, as long as it worked locally we should be good to go

@pat-schmitt
Copy link
Member Author

Yes, it also worked with the other containers, so it is ready to merge.

However, we should add rioxarray to the container used in the minimal test or exclude this test. Otherwise, it will never turn green...

@fmaussion
Copy link
Member

Yes sorry I was not clear: adding rioxarray to minimal defies the purpose of the minimal environment (which is to have no GIS library in it). To this test should rather be moved to tests_prepro, or altered/commented for simplicity.

@pat-schmitt
Copy link
Member Author

I used now rioxr = pytest.importorskip('rioxarray') in test_prepro, so all tests using rioxarray are skipped if it can not be imported. The same is done for the other gis packages like salem, rasterio and geopandas.

@fmaussion fmaussion merged commit 5f8a8c5 into OGGM:master Jan 30, 2024
27 checks passed
@fmaussion
Copy link
Member

Thanks @pat-schmitt ! And sorry for the confusion

@pat-schmitt pat-schmitt deleted the dem_gridded_shift branch January 31, 2024 07:45
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.

Shift in coordinates of gridded_data and dem.tif
2 participants