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

read_ncdf refactor and improvements #205

Merged
merged 43 commits into from
Aug 12, 2019
Merged

read_ncdf refactor and improvements #205

merged 43 commits into from
Aug 12, 2019

Conversation

dblodgett-usgs
Copy link
Contributor

This PR includes a significant chunk of work from @mdsumner that I've picked up and tried to make some progress on.

  1. A number of the commits are related to a file ncdf_tidync.R that has been temporarily moved into inst/nc I'm planning on using that for future work on ncdf.R and thought it would be worth preserving.

  2. There's work here to fix TRMM netCDF file is fliped / rotated  #199 and Handle non-canonical NetCDF axis order #89 non-canonical axis order. I'm not 100% happy with how that's all handled, but this is a good start and reasonably well tested for both 1d and 2d coordinate variables.

  3. This has not been tested with the latest RNetCDF so does not address read_ncdf regression? #183. I plan on doing that testing in the future as v2 RNetCDF gets closer to CRAN.

  4. This work is largely a refactor in preparation for work on Support for reading station data timeseries (NetCDF example) #30 and NetCDF Geometry and Subsetting Functionality #86 -- A timeseries.nc file from rasterwise reads with no errors but is not where it should be at all.

  5. There is work included here to handle bounds as requested in coordinates with bounds not correctly read for ncdf #175. Additional work is needed before we can consider that fixed.

Before I go any further I felt it was important to get this work at least reviewed if not merged. All tests pass and all rasterwise sample data read with no error -- although copious warnings!!

mdsumner and others added 30 commits April 24, 2019 00:16
Merge branch 'master' of https://github.com/dblodgett-usgs/stars into dblodgett-usgs-master

# Conflicts:
#	R/ncdf.R
read_ncdf now has dimension-time units and variable units  (still need dimension units)
@edzer
Copy link
Member

edzer commented Aug 10, 2019

Great work! Right now, units are ignored, and this causes a few errors; also, I see while checking

  1. Failure: curvilinear (@test_ncdf.R#79) 
  2. Failure: curvilinear broked (@test_ncdf.R#97) 

and a lot of not so helpful warnings of the kind of

< time    1   1     NA    NA     NA    NA   1460    
< Warning messages:
< 1: Unknown or uninitialised column: 'name'. 
< 2: Unknown or uninitialised column: 'name'. 
< 3: Unknown or uninitialised column: 'name'. 
< 4: In .get_nc_time(dimensions, make_time, coord_var, var[1], meta) :
<   ignoring units of time dimension, not able to interpret

so not ready to be merged IMO. Do you want me to help out here, or to review the PR as a whole?

@dblodgett-usgs
Copy link
Contributor Author

Happy to fix things up unless you want to take time to help out!

How are you creating the curvilinear failures? Tests all pass for me (running tests in Rstudio) but I have to admit I'm unfamiliar with the testing style you are using with .Rout.save files. Are those issues showing up during R CMD CHECK?

@dblodgett-usgs
Copy link
Contributor Author

OK -- I see the issues in the travis tests. Turns out I'm using RNetCDF 2.0 -- and I can't get 1.9 installed anymore. Will work on that install a little more and try and deal with these issues.

@edzer
Copy link
Member

edzer commented Aug 11, 2019

Things check clean locally now, with RNetCDF 1.9-1, but both travis & appveyor give the same error - any ideas where that could come from?

@dblodgett-usgs
Copy link
Contributor Author

dblodgett-usgs commented Aug 11, 2019

The errors seem to be related to a warning (not) coming from try_as_units.

Locally, with the units 0.6-3 linked to udunits 2.2.26

as_units("kg m^-2")
Error: ‘`kg` `m`^-2’ is not a unit recognized by udunits or a user-defined unit
In addition: Warning message:
Could not parse expression: ‘`kg` `m`^-2’. Returning as a single symbolic unit() 

Apparently not in CI which is on udunits 2.2.0-1?

@dblodgett-usgs
Copy link
Contributor Author

Derp... hadn't pulled down the most recent upstream main changes. The warning is suppressed now.

R/ncdf.R Outdated
out_data <- .set_nc_units(out_data, meta$attribute, make_units)

# Check if we have curvilinear data
curvilinear <- .check_curvilinear(coord_var, var[1], meta$variable)
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that parameter curvilinear is being overwritten, and hence ignored as input?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does! Will fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tracked this down and fixed for now. Lon and Lat order matter or we need to make this a named vector using "X" and "Y" to denote canonical axes. I'm asssuming Lon/Lat order for now.

R/ncdf.R Show resolved Hide resolved
R/ncdf.R Outdated Show resolved Hide resolved
inst/nc/ncdf_tidync.R Show resolved Hide resolved
tests/testthat/test_ncdf.R Show resolved Hide resolved
tests/testthat/test_ncdf.R Show resolved Hide resolved
R/ncdf.R Show resolved Hide resolved
@edzer
Copy link
Member

edzer commented Aug 12, 2019

Ready to merge?

@dblodgett-usgs
Copy link
Contributor Author

I think so. I'm starting to work on a couple other additions but those deserve a separate PR.

@edzer edzer merged commit aa392fc into r-spatial:master Aug 12, 2019
@edzer
Copy link
Member

edzer commented Aug 12, 2019

Great -- thanks so much!!

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.

TRMM netCDF file is fliped / rotated
3 participants