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

NetCDF Projection Handling and EuroCordex test #208

Merged
merged 10 commits into from
Aug 16, 2019
Merged

NetCDF Projection Handling and EuroCordex test #208

merged 10 commits into from
Aug 16, 2019

Conversation

dblodgett-usgs
Copy link
Contributor

This PR includes:

  1. Some further cleanup of NetCDF dimension, coordinate variable, and canonical axis order handling.
  2. Coordinate reference system lookup and assignment.

I've made some opinionated choices in here but I think they are reasonable.

  1. Something that was already here when I showed up was using the first variable as a representative variable for the coordinate system to be inserted into the stars dimension object. I've instantiated that with the rep_var variable.
  2. In .get_nc_projection, I return WGS84 (as EPSG:4326) if no CRS is declared but the coordinate variable units are degrees. A warning is issued in this case.
  3. If a NetCDF file is found to have 1D auxiliary coordinate variables (in addition to 2D lat/lon coordinate variables) they are used in preference to the 2D variables. I should probably only do this in the case that a CRS is available. Will implement in a moment and add to this PR.

I think we can say that, with this PR, #89 is fixed.

}
} else {
ret <- st_crs(NULL)
ret <- try(st_crs(ncmeta::nc_gm_to_prj(nc_grid_mapping)))
Copy link
Member

Choose a reason for hiding this comment

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

if the try() fails here, ret will be of class try-error. Is that what you intent to return?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh. No. Will fix.

@edzer
Copy link
Member

edzer commented Aug 13, 2019

If I understand you correctly: for raster files that have a CRS that can be understood, we return the raster with appropriate CRS; for files without understandable CRS but with long/lat rasters, we return them as curvilinear grids.

My request: can we also make this optional, so that in the first case we can get the curviilinear long/lat raster returned? This may be helpful to understand the intention of the creators. Or is this already optional right now with using the curvilinear argument?

@dblodgett-usgs
Copy link
Contributor Author

I think this is a good idea. There are potentially three options here.

  1. use curvilinear (2D) coordinate variables because a user specifies them with the curvilinear argument.
  2. use curvilinear coordinate variables but have stars figure out what the coordinate variable names are.
  3. use 1d auxiliary coordinate variables and have stars figure out what the coordinate variables names are.

I'll make sure that option 1 occurs if the curvilinear input is supplied.

As an unrelated side note - I expect much of the functionality that is being factored out of the main read_stars function to find a home in ncmeta with specific unit tests. It's just more convenient to do all the refactoring here.

@edzer
Copy link
Member

edzer commented Aug 13, 2019

I'll work on rewriting read_stars to spawn, by default, into read_ncdf if the object(s) are NetCDF files, with an option to override this & go the GDAL path.

@dblodgett-usgs
Copy link
Contributor Author

I'll add more test coverage and clean this up a bit later on. Don't merge yet.

@dblodgett-usgs
Copy link
Contributor Author

@edzer and @mdsumner I think this is ready to merge if you are happy with it.

@edzer edzer merged commit 1087b7a into r-spatial:master Aug 16, 2019
@edzer
Copy link
Member

edzer commented Aug 16, 2019

Great work!

@adrfantini
Copy link

Awesome step forward, cant wait to have some time to test this out!

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.

3 participants