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-4 groups support on reading #1180

Merged
merged 2 commits into from
Jan 12, 2019

Conversation

jdemaria
Copy link
Contributor

@jdemaria jdemaria commented Jan 7, 2019

What does this PR do?

NetCDF-4 groups support on reading

What are related issues/pull requests?

Tasklist

  • ADD YOUR TASKS HERE
  • Add test case(s)
  • Review
  • Adjust for comments
  • All CI builds and checks have passed

Environment

Provide environment details, if relevant:

  • OS: Ubuntu 14.04.5 LTS
  • Compiler: gcc (Ubuntu 4.8.4-2ubuntu1~14.04.4) 4.8.4

@jdemaria
Copy link
Contributor Author

jdemaria commented Jan 8, 2019

The only failing Travis CI check (https://travis-ci.org/OSGeo/gdal/jobs/476739965) seems related to recent NGW commits which broke NGW autotests.
Update: I don't know why but now the failing Travis case is now passed...

Copy link
Member

@rouault rouault left a comment

Choose a reason for hiding this comment

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

Great work. I couldn't review the general logic, but seems to be well thought. Just concentrated on some coding aspects.

gdal/frmts/netcdf/frmt_netcdf.html Outdated Show resolved Hide resolved
gdal/frmts/netcdf/frmt_netcdf.html Outdated Show resolved Hide resolved
gdal/frmts/netcdf/netcdfdataset.cpp Outdated Show resolved Hide resolved
gdal/frmts/netcdf/netcdfdataset.cpp Outdated Show resolved Hide resolved
gdal/frmts/netcdf/netcdfdataset.cpp Outdated Show resolved Hide resolved
gdal/frmts/netcdf/netcdfdataset.cpp Outdated Show resolved Hide resolved
gdal/frmts/netcdf/netcdfdataset.cpp Outdated Show resolved Hide resolved
gdal/frmts/netcdf/netcdfdataset.cpp Outdated Show resolved Hide resolved
gdal/frmts/netcdf/netcdfdataset.cpp Outdated Show resolved Hide resolved
gdal/frmts/netcdf/netcdfdataset.cpp Show resolved Hide resolved
@rouault
Copy link
Member

rouault commented Jan 9, 2019

You might want to squash all commits in a single one (perhaps except 461fc8b), once you're done.

@jdemaria
Copy link
Contributor Author

You might want to squash all commits in a single one (perhaps except 461fc8b), once you're done.

Ok I have fixed all points you raised, and squashed commits but I'm not sure if it's ok because I see commits from masters in the list?

It seems checks failure are still only related to recent NGW commits which broke NGW autotests (test_ogr_ngw_14).

@rouault
Copy link
Member

rouault commented Jan 11, 2019

Hum, you didn't squash correctly. Looks like some merge instead, but with original commits modified... There are some hints there to squash: https://github.com/OSGeo/gdal/blob/master/CONTRIBUTING.md

- Explore recursively all nested groups to create the subdatasets list
- Subdatasets in nested groups use the /group1/group2/.../groupn/var standard NetCDF-4 convention, except for variables in the root group which do not have a leading slash for backward compatibility with the NetCDF-3 driver
- Global attributes of each nested group are also collected in the GDAL dataset metadata, using the same convention /group1/group2/.../groupn/NC_GLOBAL#attr_name, except for the root group which do not have a leading slash for backward compatibility
- When searching for a variable containing auxiliary information on the selected subdataset, like coordinate variables or grid_mapping, we now also search in parent groups and their childs as specified in cf-convention/cf-conventions#144
@jdemaria
Copy link
Contributor Author

Ok sorry however I tried to follow these hints but surely did something wrong...
Now I have removed extraneous commits using another "git rebase -i master" and the commits history seems better.
The only remaining failing check is test_vsicurl_test_redirect on AppVeyor Visual Studio 2017, platform=x86.

@rouault rouault merged commit 3245d24 into OSGeo:master Jan 12, 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