Opened 14 years ago

Closed 10 years ago

Last modified 8 years ago

#1926 closed defect (fixed)

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

Reported by: jluis Owned by: chaitanya
Priority: high Milestone:
Component: GDAL_Raster Version: svn-trunk
Severity: normal Keywords: netcdf
Cc: stensby, dnadeau, warmerdam, Kyle Shannon

Description

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.

Attachments (4)

netcdfdataset.cpp (83.7 KB) - added by jluis 14 years ago.
Patches Y order issues on header description
netcdfdataset.diff (3.1 KB) - added by jluis 14 years ago.
Updat patch that fixes only wrong info on files created by GMT
netcdf.patch (4.7 KB) - added by warmerdam 14 years ago.
Revised patch (applicable with patch) - ifdefed out flipping.
netcdf_1.5.1_actual_range_fix.diff (2.2 KB) - added by stensby 14 years ago.
Patch to netcdfdataset.cpp version 1.5.1

Download all attachments as: .zip

Change History (20)

Changed 14 years ago by jluis

Attachment: netcdfdataset.cpp added

Patches Y order issues on header description

comment:1 Changed 14 years ago by warmerdam

Keywords: netcdf added
Milestone: 1.5.0
Status: newassigned

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.

comment:2 Changed 14 years ago by 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

comment:3 Changed 14 years ago by 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

Changed 14 years ago by jluis

Attachment: netcdfdataset.diff added

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

comment:4 Changed 14 years ago by warmerdam

Priority: normalhigh

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

comment:5 Changed 14 years ago by jluis

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

comment:6 Changed 14 years ago by warmerdam

Milestone: 1.5.01.5.1

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.

Changed 14 years ago by warmerdam

Attachment: netcdf.patch added

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

comment:7 Changed 14 years ago by 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.

comment:8 Changed 14 years ago by 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?

comment:9 Changed 14 years ago by 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

Changed 14 years ago by stensby

Patch to netcdfdataset.cpp version 1.5.1

comment:10 Changed 14 years ago by 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.

comment:11 Changed 13 years ago by stensby

Cc: stensby added

comment:12 Changed 13 years ago by warmerdam

Cc: dnadeau added

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

comment:13 Changed 13 years ago by warmerdam

Cc: warmerdam added
Milestone: 1.5.41.6.1
Owner: changed from warmerdam to chaitanya
Status: assignednew

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.

comment:14 Changed 11 years ago by Kyle Shannon

Cc: Kyle Shannon added

comment:15 Changed 10 years ago by etourigny

Resolution: fixed
Status: newclosed

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.

comment:16 Changed 8 years ago by Even Rouault

Milestone: 1.6.4

Milestone 1.6.4 deleted

Note: See TracTickets for help on using tickets.