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 CF-1.8 Driver Updates - Writing Support, Bug Fix, Streaming etc. #1680

Merged
merged 182 commits into from
Jul 2, 2019
Merged

netCDF CF-1.8 Driver Updates - Writing Support, Bug Fix, Streaming etc. #1680

merged 182 commits into from
Jul 2, 2019

Conversation

wchen329
Copy link
Contributor

@wchen329 wchen329 commented Jun 26, 2019

What does this PR do?

This PR is a follow up to the netCDF Reading support PR (#1541). It introduces CF-1.8 simple geometry writing support for the netCDF driver. More specifically it does the following:

  • Makes the default writing behavior of the netCDF Vector Driver to write CF-1.8 encoded geometries rather than CF-1.6 WKT netCDF. The new driver is able to write OGRPoints, OGRMultipoints, OGRLineStrings, OGRMultiLineStrings, OGRPolygons, and OGRMultipolygons (with and without interior rings) corresponding to feature types available through the CF-1.8 standard.
  • Preserves WKT writing capabilities for legacy use (see below).
  • Fixes a couple of bugs present in the netCDF Reader from (Additions to netCDF Driver to allow Reading from CF-1.8 Encoded Geometries (#1287) #1541). These bugs were found during the writing tests, and so they are also tested in these tests (specifically netcdf.py/multipolygon_write_test)
  • Reader now only retrieves one feature at a time into memory (rather than buffering them all).
  • SRS reading support for CF-1.8 simple geometries using CF-1.8 simple geometry grid mapping attribute

What about Backwards Compatibility?

Backwards compatibility once again is preserved. However, to enable writing of the legacy WKT styled netCDF files, the dataset creation option LEGACY=WKT must be presented. There is no mixing of WKT and CF-1.8 geometries in writing.

Limitations

For now, due to restrictions present in the legacy driver, only one layer can be translated into netCDF at a time (this applies to the non-legacy options). The legacy option has MULTIPLE_LAYERS, but by default it is disabled. The intent is eventually to get multiple layer support eventually working by default for CF-1.8 datasets with update support, as well as writing multiple layers to the same netCDF file. Work remaining to get multiple layer writing support includes:

  • Removing the multiple layer denial check in TestCapability of the dataset
  • Writing to more than one SRS variable name (currently the driver only writes to a variable of name "crs")
  • Setting Define mode for netCDF::Create and unsetting it at the end
  • Defining the record dimension as different names between netCDFLayers (currently all use name "record", which causes the layer translation to error out early, rightfully so)

Because of this multiple layer writing capabilities will be presented in another PR.

Testing

Unit tests are available in GDAL autotest within netcdf.py. The writing tests are in JSON format and are translated from JSON into NetCDF-3 format. The specific data is present in the following directory.

What are related issues/pull requests?

Issue #1287
PR #1541

Tasklist

  • Complete writing of simple geometries layers in CF-1.8 while preserving backwards compatibility
  • Fix SRS Setting Issue in Reading
  • Reading Buffering Updates
  • Add standard writing, translation, test cases
  • Bug Fix: Writing the same netCDF Dataset out from the source with a different destination file name
  • Add test cases demonstrating bug fixes above
  • Review
  • Adjust for comments + Changes to prevent Lat/Lon reprojection and flipping
  • All CI builds and checks have passed

Environment

  • OS: openSUSE Tumbleweed and Microsoft Windows 7 Professional
  • Compiler: GCC 9.1.1 (on openSUSE) and MSVC 14.0 [2015] (on Windows 7)

@wchen329
Copy link
Contributor Author

Hey @rouault this PR is now ready for review, when you get some time please check through what I have written here. Thanks!

gdal/frmts/netcdf/netcdfdataset.cpp Outdated Show resolved Hide resolved
gdal/frmts/netcdf/netcdfdataset.cpp Outdated Show resolved Hide resolved
gdal/frmts/netcdf/netcdflayersg.h Show resolved Hide resolved
gdal/frmts/netcdf/netcdflayer.cpp Outdated Show resolved Hide resolved
gdal/frmts/netcdf/netcdflayer.cpp Outdated Show resolved Hide resolved
gdal/frmts/netcdf/netcdfsg.cpp Outdated Show resolved Hide resolved
gdal/frmts/netcdf/netcdfdataset.h Outdated Show resolved Hide resolved
gdal/frmts/netcdf/netcdflayer.cpp Outdated Show resolved Hide resolved
gdal/frmts/netcdf/netcdflayersg.cpp Outdated Show resolved Hide resolved
gdal/frmts/netcdf/netcdfsgwriterutil.cpp Outdated Show resolved Hide resolved
@rouault
Copy link
Member

rouault commented Jun 29, 2019

@wchen329
Copy link
Contributor Author

wchen329 commented Jun 29, 2019

* There are memory leaks.  Look for `______________________________ test_netcdf_46 ____________________________` in https://api.travis-ci.com/v3/job/212032991/log.txt

* cppcheck issues: see end of https://api.travis-ci.com/v3/job/212032992/log.txt

After looking through the logs, many of the new memory leaks (I daresay, most) actually originate from using the projection setting function "SetProjectionFromVar" (given that I on purpose more or less didn't explicitly use the heap this time, this is not surprising). I'll go look through that in my "Raster by-pass" stuff. The Feature Leaks are from a mistake from trying to fix some of earlier feedback (just one of those "Vim" keystroke errors probably, probably placed the delete in some awkward position). Was not my intent to change that.

Also: there's for WGS84 Lat/Lon specifically (and maybe some other projections, not sure) there's this issue of reading a layer with said projection and having the X and Y axes flipped automatically (which, by the way through GDB I found is done after the Feature is already created in GetNextRawFeature() but before the feature is given to the per-feature driver translater. Not sure if there's a builtin way to disable this behavior?

EDIT: cppcheck stuff- not sure why that wasn't reference already... made necessary changes for that [done]

@wchen329
Copy link
Contributor Author

Actually I think I found the answer to the lat/lon v. lon/lat issue:

oSRS.SetAxisMappingStrategy(OAMS_TRADITIONAL_GIS_ORDER);

@wchen329
Copy link
Contributor Author

wchen329 commented Jul 1, 2019

By the way @rouault when is the planned release date of GDAL 3.1? Good to know so I could guage a good time to stop with so much feature updates.

UPDATE: Also changed GEOMETRY_ENCODING to a DSCO instead of an LCO to prepare for possible future work and allowing more flexibility in treating the two types of datasets differently.

@rouault
Copy link
Member

rouault commented Jul 1, 2019

@wchen329
Copy link
Contributor Author

wchen329 commented Jul 2, 2019

So looks like this time's failure was another "CI goes awkwardly MIA" moment on the OS X build according to the logs, so @rouault do you have any other feedback you would like to suggest so I can retrigger a build again? Or if not please retrigger it? Thanks!

@rouault
Copy link
Member

rouault commented Jul 2, 2019

we're probably good to go. Anything remaining on your side for this PR ?

@wchen329
Copy link
Contributor Author

wchen329 commented Jul 2, 2019

Hmm, since this PR is getting rather large, I think I'll save further improvements for future PRs. Thanks again @rouault!

@rouault rouault merged commit 3d3ef77 into OSGeo:master Jul 2, 2019
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