Corrects for the wrong header info printed by gdalinfo on GMT grids and the ReadAttributes method issue

[jluis]

Patches for both the header decoding of GMT produced grids and the bug reported by Mario Cruz, where "The ReadAttributes? method was using a fixed-length array to store attribute data, crashing when presented with an attribute whose data size was larger than MAX_STR_LEN".

However, there is still one problem that requires a more extended solution. The way the driver works, it expects files written "upside down" (the first row is the top row) which is the contrary of what GMT does (it writes the first row as the bottom row). That is why GMT grids are imported flipped up-down. Both ways of writing the grids are valid, but the driver must detect it and act accordingly.

[jluis]

Patches Y order issues on header description

[warmerdam]

jluis,

I'm rather confused by your submitting a change to netcdfdataset.cpp but with the text talking about GMT written grids. Aren't grids written by GMT interpreted by gmtdataset.cpp?

It would be preferrable if you could provide a patch instead of a "whole file". I've made non-trivial changes to the netcdf driver as recently as last night so it may be hard to know what changes are what.

[jluis]

Frank,

At GMT 4.1 version released at 1-Jan-2006 started to create COARDS compliant netCDF grids by default, so since than grids created by GMT are interpreted by netcdfdataset.cpp and not by gmtdataset.cpp. That is why I talk about GMT grids (the ones I use regularly) and the netcdfdataset.cpp issues.

I'll upload a patch file created against the "netcdfdataset.cpp 12892 2007-11-20 16:30:13Z" version.

Joaquim Luis

[jluis]

Update patch to account only to "my own" changes, which deal with error reporting limits of grids created by GMT. The previous version also corrected for the bug of reading attributes reported by Mario Cruz but since he said that his patch was incomplete I'm retiring my implementation of his patch. I'm sorry for not providing pointers to mail messages or track tickets but I couldn't find neither of them.

 http://lists.maptools.org/mailman/listinfo/gdal-dev/ says "No such list gdal-dev"

and I couldn't find a ticket mentioned on Mario Cruz's mail "Re: [Gdal-dev] NetCDF driver bug fix" of 23-Nov-2007

[jluis]

Updat patch that fixes only wrong info on files created by GMT

[warmerdam]

Can someone provide a file, and demonstration of the issue being fixed? I hate to apply patches blind.

[jluis]

Frank, Ticket #1595 has associated a GMT grid that can be used for testing this patch

[warmerdam]

I have reviewed the patch, and come to the determination that the Y flipping logic is wrong in at least some cases (such as my test file IDY02001.nc - not GMT generated). The node_offset and actual_range logic looks ok, but I'm not sure why we need the actual_range logic. Does this produce more accurate results than the existing map lookup?

Given the confusion and uncertainty, I'm not applying this patch for 1.5 release. I am attaching my version of the patch (with the flipping ifdefed out) for future review and possibly application.

[warmerdam]

Revised patch (applicable with patch) - ifdefed out flipping.

[jluis]

I don't know why the flipping logic failed on your test file or what you find wrong with it. It is only meant to assure that yMinMax[0] = yMin; and yMinMax[1] = yMax; I found a IDY02001.nc at  ftp://ftp2.bom.gov.au/anon/sample/IDBYA904/ and using a patched version, or not, works equally.

As for the actual_range maybe it is overzealous but I used it to double check when we are dealing with a GMT generated file. If not, node_offset is set to 0 which I think is a variable name used only by GMT.

[warmerdam]

jluis,

The problem with the flipping logic is that there are datasets which are "south up" and flipping the geotranforms results in them being incorrectly georeferenced. It is normal for adfGeoTransform[5] to be negative (ie. north up) but it isn't required, so I think it is better not to have that logic. Unless you can point to a dataset where it is critical.

The actual_range is used to override other information - not as a double check. In a sense this just complicates the logic without adding any apparent value. I'm inclined to scale back to just use the node_offset logic to determine whether to offset the origin by a half pixel.

Does that seem reasonable?

[jluis]

Frank,

Ah, so that is the issue with the flipping logic. I'm afraid it extends further. We need to know (even if not forcedly reverse) the Y direction because, as I mentioned on the ticket creation test:

"However, there is still one problem that requires a more extended solution. The way the driver works, it expects files written "upside down" (the first row is the top row) which is the contrary of what GMT does (it writes the first row as the bottom row). That is why GMT grids are imported flipped up-down. Both ways of writing the grids are valid, but the driver must detect it and act accordingly."

I think I remember better now why used the actual_range. I simple was not sure on what happens to node_offset when no attribute exists with that name in the call: nc_get_att_int (cdfid, NC_GLOBAL, "node_offset", &node_offset); So if node_offset is not changed above from its default value of zero, than in fact there is no need for the code that tests on the actual_range.

Joaquim

[stensby]

Patch to netcdfdataset.cpp version 1.5.1

[stensby]

The way GDAL 1.5.1 uses "actual_range" to set the geotransform is wrong. The "actual_range" values for a south-to-north and a north-to-south grid can be the same. GMT is able to operate on both types of grid and when writing a netCDF file the difference between the two will be reflected in the values of the pdfYCoord array used in netcdfdataset.cpp.

The older GDAL 1.5.0 version handled these grids correct (with respect to flipping). However the 1.5.0 version did not use the "node_offset" value to distinguish between node grids and pixel grids.

The patch netcdf_1.5.1_actual_range_fix.diff can be used to patch the 1.5.1 version of netcdfdataset.cpp so that both pixel/grid registration and "flipping" (geotransform) is correct.

[warmerdam]

Denis, any thoughts on this? Does Trond Stensby's patch look appropriate?

[warmerdam]

Chaitanya,

I would like you to review the patch, and attempt to assemble a variety of netcdf files to test against (which can be stockpiled on  http://download.osgeo.org/gdal/data (under a new netcdf directory).

My concern with the patch is that it may just break things for people for whom the existing code is working fine. But I don't have a respectible understanding of what is typical in netcdf and so I have trouble judging whether this is the case. I also haven't reviewed the patch or existing code carefully.

Denis Nadeau is monitoring this ticket, and if you note your findings and plans he may have some feedback for you. His preliminary concern is similar to mine, that we may just be fixing it for some people and breaking it for others.

Please seek feedback and sample data pointers on the mailing list.

Preliminary work on this ticket should be done in trunk, and if we are satisfied with the results we can look to backport to the 1.6 branch.

[etourigny]

It seems that the y-axis issue has been resolved, as part of bugs #2654 and #3575.

The other issue mentioned in this bug have been implemented in r13698 (bug #2196).

Closing this issue.