Re: [gdal-commits] r40282 - trunk/gdal/frmts/aaigrid

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [gdal-commits] r40282 - trunk/gdal/frmts/aaigrid

Kurt Schwehr-2
+gdal-dev

Even,  

Looking back at the changes to aaigrid, I think I've messed things up with nodata.  Before I submit anything else, what do you think?

The trouble is that it is no longer doing a cast to float for in range values.  That means for large values, it's not getting rounded to the nearest float value any more.  I'm seeing this in one on my local tests.  I dump the json out of gdalinfo and compare.  For float64.asc.json:

I was getting: -1.234567880630493164
Now seeing: -1.234567890123

The new value matches the text in the file, but it was a behavior change.  Leave it be or put in an else that does the rounding to nearest float value by casting?

What do you think?

-kurt


On Sat, Sep 30, 2017 at 10:59 AM, Even Rouault <[hidden email]> wrote:

On samedi 30 septembre 2017 10:47:16 CEST [hidden email] wrote:

> Author: goatbar

> Date: 2017-09-30 10:47:16 -0700 (Sat, 30 Sep 2017)

> New Revision: 40282

 

Kurt,

 

This will break the following test case with a infinite null value

 

{{{

north: 250.000000

south: 0.000000

east: 150.000000

west: -100.000000

rows: 2

cols: 2

null: inf

0 0

0 0

}}}

 

Peviously gdalinfo reported

NoData Value=inf

now its reports

NoData Value=3.4028234663852886e+38

 

so you should make a special case for pos and neg infinities.

 

Note: in the same file, in the AAIGRID driver, there's a similar code at line 537

 

Evn

 

>

> Modified:

> trunk/gdal/frmts/aaigrid/aaigriddataset.cpp

> Log:

> Change double -> float -> double cast to clamping by numeric limits in

> GRASSASCIIDataset::ParseHeader

>

> Found by autofuzz

>

>

> Modified: trunk/gdal/frmts/aaigrid/aaigriddataset.cpp

> ===================================================================

> --- trunk/gdal/frmts/aaigrid/aaigriddataset.cpp 2017-09-30 16:56:10 UTC (rev

> 40281) +++ trunk/gdal/frmts/aaigrid/aaigriddataset.cpp 2017-09-30 17:47:16

> UTC (rev 40282) @@ -633,9 +633,10 @@

> }

> if( eDataType == GDT_Float32 )

> {

> - // TODO(schwehr): Is this really what we want?

> - dfNoDataValue =

> - static_cast<double>(static_cast<float>(dfNoDataValue));

> + if( dfNoDataValue >= std::numeric_limits<float>::max() )

> + dfNoDataValue = std::numeric_limits<float>::max();

> + if( dfNoDataValue <= -std::numeric_limits<float>::max() )

> + dfNoDataValue = -std::numeric_limits<float>::max();

> }

> }

>

>

> _______________________________________________

> gdal-commits mailing list

> [hidden email]

> https://lists.osgeo.org/mailman/listinfo/gdal-commits

 

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com




--

_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: [gdal-commits] r40282 - trunk/gdal/frmts/aaigrid

Even Rouault-2

On lundi 2 octobre 2017 08:04:30 CEST Kurt Schwehr wrote:

> +gdal-dev

>

> Even,

>

> Looking back at the changes to aaigrid, I think I've messed things up with

> nodata. Before I submit anything else, what do you think?

>

> The trouble is that it is no longer doing a cast to float for in range

> values. That means for large values, it's not getting rounded to the

> nearest float value any more. I'm seeing this in one on my local tests. I

> dump the json out of gdalinfo and compare. For float64.asc.json:

>

> I was getting: -1.234567880630493164

> Now seeing: -1.234567890123

>

> The new value matches the text in the file, but it was a behavior change.

> Leave it be or put in an else that does the rounding to nearest float value

> by casting?

>

> What do you think?

 

That's actually a general issue with GetNoDataValue() returning a double. For a Float32 band, the return only makes sense after being cast to float.

 

In the past, we have had issues in GDAL core regarding that in statistics, min-max or histogram computation functions, but I think that now we compare pixel values to the float-casted nodata value. So whether you pre-cast to float in the driver shouldn't make a difference. You can try putting nodata values as pixel values in the .asc file, and check whether gdalinfo -stats / -mm / -hist ignore them.

 

That said, it wouldn't probably be a bad idea to keep the behaviour we had before your changes (ie double->float->double cast), so as to avoid breaking external code/tests that would depend (erroneously) on the exact value.

 

Even

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev