Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#6879 closed defect (fixed)

Behavior changes with GDALDataTypeUnion from r38182

Reported by: Kurt Schwehr Owned by: warmerdam
Priority: normal Milestone:
Component: default Version: unspecified
Severity: normal Keywords:
Cc:

Description

On applying r38182, I see changes with GdalMiscTest.GdalDataTypeUnion in https://github.com/schwehr/gdal-autotest2/blob/master/cpp/gcore/gdal_misc_test.cc

Where these behavior changes intended by this?

https://github.com/OSGeo/gdal/pull/215/files#diff-d71cafa6afd78f5a5b978d8920b85633

third_party/gdal/autotest2/cpp/gcore/gdal_misc_test.cc:40
Expected equality of these values:
  GDT_Int16
    Which is: 3
  GDALDataTypeUnion(GDT_Int16, GDT_UInt16)
    Which is: 5

third_party/gdal/autotest2/cpp/gcore/gdal_misc_test.cc:41
Expected equality of these values:
  GDT_Int32
    Which is: 5
  GDALDataTypeUnion(GDT_Int16, GDT_UInt32)
    Which is: 7

third_party/gdal/autotest2/cpp/gcore/gdal_misc_test.cc:42
Expected equality of these values:
  GDT_Int32
    Which is: 5
  GDALDataTypeUnion(GDT_UInt32, GDT_Int16)
    Which is: 7

third_party/gdal/autotest2/cpp/gcore/gdal_misc_test.cc:43
Expected equality of these values:
  GDT_CInt32
    Which is: 9
  GDALDataTypeUnion(GDT_UInt32, GDT_CInt16)
    Which is: 11

third_party/gdal/autotest2/cpp/gcore/gdal_misc_test.cc:46
Expected equality of these values:
  GDT_CFloat32
    Which is: 10
  GDALDataTypeUnion(GDT_Float32, GDT_CInt32)
    Which is: 11

third_party/gdal/autotest2/cpp/gcore/gdal_misc_test.cc:48
Expected equality of these values:
  GDT_CInt32
    Which is: 9
  GDALDataTypeUnion(GDT_CInt16, GDT_UInt32)
    Which is: 11

third_party/gdal/autotest2/cpp/gcore/gdal_misc_test.cc:50
Expected equality of these values:
  GDT_CFloat32
    Which is: 10
  GDALDataTypeUnion(GDT_CInt16, GDT_CFloat32)
    Which is: 11

third_party/gdal/autotest2/cpp/gcore/gdal_misc_test.cc:52
Expected equality of these values:
  GDT_CInt32
    Which is: 9
  GDALDataTypeUnion(GDT_CInt32, GDT_Byte)
    Which is: 11

third_party/gdal/autotest2/cpp/gcore/gdal_misc_test.cc:53
Expected equality of these values:
  GDT_CInt32
    Which is: 9
  GDALDataTypeUnion(GDT_CInt32, GDT_UInt16)
    Which is: 11

third_party/gdal/autotest2/cpp/gcore/gdal_misc_test.cc:54
Expected equality of these values:
  GDT_CInt32
    Which is: 9
  GDALDataTypeUnion(GDT_CInt32, GDT_Int16)
    Which is: 11

third_party/gdal/autotest2/cpp/gcore/gdal_misc_test.cc:55
Expected equality of these values:
  GDT_CInt32
    Which is: 9
  GDALDataTypeUnion(GDT_CInt32, GDT_UInt32)
    Which is: 11

third_party/gdal/autotest2/cpp/gcore/gdal_misc_test.cc:56
Expected equality of these values:
  GDT_CInt32
    Which is: 9
  GDALDataTypeUnion(GDT_CInt32, GDT_Int32)
    Which is: 11

third_party/gdal/autotest2/cpp/gcore/gdal_misc_test.cc:57
Expected equality of these values:
  GDT_CFloat32
    Which is: 10
  GDALDataTypeUnion(GDT_CInt32, GDT_Float32)
    Which is: 11

third_party/gdal/autotest2/cpp/gcore/gdal_misc_test.cc:58
Expected equality of these values:
  GDT_CInt32
    Which is: 9
  GDALDataTypeUnion(GDT_CInt32, GDT_CInt16)
    Which is: 11

third_party/gdal/autotest2/cpp/gcore/gdal_misc_test.cc:59
Expected equality of these values:
  GDT_CFloat32
    Which is: 10
  GDALDataTypeUnion(GDT_CInt32, GDT_CFloat32)
    Which is: 11

third_party/gdal/autotest2/cpp/gcore/gdal_misc_test.cc:61
Expected equality of these values:
  GDT_CFloat32
    Which is: 10
  GDALDataTypeUnion(GDT_CFloat32, GDT_Byte)
    Which is: 11

third_party/gdal/autotest2/cpp/gcore/gdal_misc_test.cc:62
Expected equality of these values:
  GDT_CFloat32
    Which is: 10
  GDALDataTypeUnion(GDT_CFloat32, GDT_UInt16)
    Which is: 11

third_party/gdal/autotest2/cpp/gcore/gdal_misc_test.cc:63
Expected equality of these values:
  GDT_CFloat32
    Which is: 10
  GDALDataTypeUnion(GDT_CFloat32, GDT_Int16)
    Which is: 11

Expected equality of these values:
  GDT_CFloat32
    Which is: 10
  GDALDataTypeUnion(GDT_CFloat32, GDT_UInt32)
    Which is: 11

third_party/gdal/autotest2/cpp/gcore/gdal_misc_test.cc:65
Expected equality of these values:
  GDT_CFloat32
    Which is: 10
  GDALDataTypeUnion(GDT_CFloat32, GDT_Int32)
    Which is: 11

third_party/gdal/autotest2/cpp/gcore/gdal_misc_test.cc:66
Expected equality of these values:
  GDT_CFloat32
    Which is: 10
  GDALDataTypeUnion(GDT_CFloat32, GDT_Float32)
    Which is: 11

third_party/gdal/autotest2/cpp/gcore/gdal_misc_test.cc:68
Expected equality of these values:
  GDT_CFloat32
    Which is: 10
  GDALDataTypeUnion(GDT_CFloat32, GDT_CInt16)
    Which is: 11

third_party/gdal/autotest2/cpp/gcore/gdal_misc_test.cc:69
Expected equality of these values:
  GDT_CFloat32
    Which is: 10
  GDALDataTypeUnion(GDT_CFloat32, GDT_CInt32)
    Which is: 11

Change History (14)

comment:1 by Even Rouault, 5 years ago

GDALDataTypeUnion(GDT_Int16, GDT_UInt16) now returning GDT_Int32 seems to be an improvement (since GDT_Int16 cannot hold the full range of uint16)

But GDALDataTypeUnion(GDT_CInt32, GDT_Byte) now returning GDT_CFloat64 instead of GDT_CInt32 seems to be a regression.

Note: I didn't check everything in the above log

comment:2 by Even Rouault, 5 years ago

@kurt Such tests would be more than welcome in the upstream testing infrastructure...

comment:3 by Kurt Schwehr, 5 years ago

I presume that the community isn't interested in having both tut and gunit based tests in the tree. I'd be happy if anyone wants to port any of the autotest2 C++ tests to tut.

comment:4 by Even Rouault, 5 years ago

Resolution: fixed
Status: newclosed

In 38201:

Fix GDALDataTypeUnion() changes (trunk only, patch by Josh Wittner, fixes #6879, github #217)

comment:5 by Kurt Schwehr, 5 years ago

Resolution: fixed
Status: closedreopened

I think we are still in the land of some regressions (bummer) and some fixes (woohoo!).

Discussion of why those changes are the way they are w.r.t. IEEE 754 would be good. e.g. https://en.wikipedia.org/wiki/IEEE_floating_point#Basic_and_interchange_formats would be good.

Here is the overall diff I had to apply to get my autotest2 gdal_misc_test.cc to pass:

  • .cc

    diff -u gdal_misc_test.cc gdal_misc_test_new.cc
    old new  
    3636  EXPECT_EQ(GDT_Unknown, GDALDataTypeUnion(GDT_Unknown, GDT_CFloat64));
    3737  EXPECT_EQ(GDT_UInt16, GDALDataTypeUnion(GDT_Byte, GDT_UInt16));
    3838  EXPECT_EQ(GDT_Int16, GDALDataTypeUnion(GDT_Byte, GDT_Int16));
    39   EXPECT_EQ(GDT_Int16, GDALDataTypeUnion(GDT_Int16, GDT_UInt16));
    40   EXPECT_EQ(GDT_Int32, GDALDataTypeUnion(GDT_Int16, GDT_UInt32));
    41   EXPECT_EQ(GDT_Int32, GDALDataTypeUnion(GDT_UInt32, GDT_Int16));
    42   EXPECT_EQ(GDT_CInt32, GDALDataTypeUnion(GDT_UInt32, GDT_CInt16));
     39  EXPECT_EQ(GDT_Int32, GDALDataTypeUnion(GDT_Int16, GDT_UInt16));
     40  EXPECT_EQ(GDT_Float64, GDALDataTypeUnion(GDT_Int16, GDT_UInt32));
     41  EXPECT_EQ(GDT_Float64, GDALDataTypeUnion(GDT_UInt32, GDT_Int16));
     42  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_UInt32, GDT_CInt16));
    4343  EXPECT_EQ(GDT_CInt32, GDALDataTypeUnion(GDT_Int32, GDT_CInt16));
    4444  EXPECT_EQ(GDT_CFloat32, GDALDataTypeUnion(GDT_Float32, GDT_CInt16));
    45   EXPECT_EQ(GDT_CFloat32, GDALDataTypeUnion(GDT_Float32, GDT_CInt32));
     45  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_Float32, GDT_CInt32));
    4646  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_Float64, GDT_CInt32));
    47   EXPECT_EQ(GDT_CInt32, GDALDataTypeUnion(GDT_CInt16, GDT_UInt32));
     47  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_CInt16, GDT_UInt32));
    4848  EXPECT_EQ(GDT_CInt32, GDALDataTypeUnion(GDT_CInt16, GDT_Int32));
    4949  EXPECT_EQ(GDT_CFloat32, GDALDataTypeUnion(GDT_CInt16, GDT_CFloat32));
    5050  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_CInt16, GDT_CFloat64));
    5151  EXPECT_EQ(GDT_CInt32, GDALDataTypeUnion(GDT_CInt32, GDT_Byte));
    5252  EXPECT_EQ(GDT_CInt32, GDALDataTypeUnion(GDT_CInt32, GDT_UInt16));
    5353  EXPECT_EQ(GDT_CInt32, GDALDataTypeUnion(GDT_CInt32, GDT_Int16));
    54   EXPECT_EQ(GDT_CInt32, GDALDataTypeUnion(GDT_CInt32, GDT_UInt32));
     54  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_CInt32, GDT_UInt32));
    5555  EXPECT_EQ(GDT_CInt32, GDALDataTypeUnion(GDT_CInt32, GDT_Int32));
    56   EXPECT_EQ(GDT_CFloat32, GDALDataTypeUnion(GDT_CInt32, GDT_Float32));
     56  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_CInt32, GDT_Float32));
    5757  EXPECT_EQ(GDT_CInt32, GDALDataTypeUnion(GDT_CInt32, GDT_CInt16));
    58   EXPECT_EQ(GDT_CFloat32, GDALDataTypeUnion(GDT_CInt32, GDT_CFloat32));
     58  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_CInt32, GDT_CFloat32));
    5959  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_CInt32, GDT_CFloat64));
    6060  EXPECT_EQ(GDT_CFloat32, GDALDataTypeUnion(GDT_CFloat32, GDT_Byte));
    6161  EXPECT_EQ(GDT_CFloat32, GDALDataTypeUnion(GDT_CFloat32, GDT_UInt16));
    6262  EXPECT_EQ(GDT_CFloat32, GDALDataTypeUnion(GDT_CFloat32, GDT_Int16));
    63   EXPECT_EQ(GDT_CFloat32, GDALDataTypeUnion(GDT_CFloat32, GDT_UInt32));
    64   EXPECT_EQ(GDT_CFloat32, GDALDataTypeUnion(GDT_CFloat32, GDT_Int32));
     63  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_CFloat32, GDT_UInt32));
     64  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_CFloat32, GDT_Int32));
    6565  EXPECT_EQ(GDT_CFloat32, GDALDataTypeUnion(GDT_CFloat32, GDT_Float32));
    6666  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_CFloat32, GDT_Float64));
    6767  EXPECT_EQ(GDT_CFloat32, GDALDataTypeUnion(GDT_CFloat32, GDT_CInt16));
    68   EXPECT_EQ(GDT_CFloat32, GDALDataTypeUnion(GDT_CFloat32, GDT_CInt32));
     68  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_CFloat32, GDT_CInt32));
    6969  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_CFloat64, GDT_Int32));
    7070  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_CFloat64, GDT_Float32));
    7171  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_CFloat64, GDT_Float64));

The things that I see that are not well packed:

Why are these now floats? I think these are incorrect changes

-  EXPECT_EQ(GDT_Int32,   GDALDataTypeUnion(GDT_Int16, GDT_UInt32));
+  EXPECT_EQ(GDT_Float64, GDALDataTypeUnion(GDT_Int16, GDT_UInt32));
-  EXPECT_EQ(GDT_Int32,   GDALDataTypeUnion(GDT_UInt32, GDT_Int16));
+  EXPECT_EQ(GDT_Float64, GDALDataTypeUnion(GDT_UInt32, GDT_Int16));
-  EXPECT_EQ(GDT_CInt32,   GDALDataTypeUnion(GDT_UInt32, GDT_CInt16));
+  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_UInt32, GDT_CInt16));

GDT_CFloat32 too small to handle int32, so I think this is okay.

-  EXPECT_EQ(GDT_CFloat32, GDALDataTypeUnion(GDT_Float32, GDT_CInt32));
+  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_Float32, GDT_CInt32));

Neither is correct. I think these should be GDT_CInt64. Why float?

-  EXPECT_EQ(GDT_CInt32,   GDALDataTypeUnion(GDT_CInt16, GDT_UInt32));
+  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_CInt16, GDT_UInt32));
-  EXPECT_EQ(GDT_CInt32,   GDALDataTypeUnion(GDT_CInt32, GDT_UInt32));
+  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_CInt32, GDT_UInt32));

Ok? We need 64bit float with it's 253 mantissa space as float32 only has 224 for the mantisa.

-  EXPECT_EQ(GDT_CFloat32, GDALDataTypeUnion(GDT_CInt32, GDT_Float32));
+  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_CInt32, GDT_Float32));
-  EXPECT_EQ(GDT_CFloat32, GDALDataTypeUnion(GDT_CFloat32, GDT_UInt32));
+  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_CFloat32, GDT_UInt32));
-  EXPECT_EQ(GDT_CFloat32, GDALDataTypeUnion(GDT_CFloat32, GDT_Int32));
+  EXPECT_EQ(GDT_CFloat64, GDALDataTypeUnion(GDT_CFloat32, GDT_Int32));

comment:6 by Even Rouault, 5 years ago

GDT_Int32 is not large enough to hold GDT_Int16, GDT_UInt32. If we had GDT_Int64 it would be the best fit, but in the absence of it GDT_Float64 seems the more appropriate.

Same for GDT_CInt64.

So it seems to me that the new results are OK

comment:7 by Kurt Schwehr, 5 years ago

Errr... sorry. I didn't realize we don't have a GDT_Int64 or GDT_CInt64. If a platform has the capability, should be have that pixel type available? If not, a comment as to why would be helpful.

in reply to:  7 comment:8 by Even Rouault, 5 years ago

Errr... sorry. I didn't realize we don't have a GDT_Int64 or GDT_CInt64. If a platform has the capability, should be have that pixel type available?

That could be likely worth a RFC (since it impacts API behaviour), and an update in some drivers like netCDF to use it

comment:9 by Kurt Schwehr, 5 years ago

Can someone who knows why, add a comment as to why there it doesn't have GDT_Int64, GDT_CInt64, GDT_UInt64?

in reply to:  9 comment:10 by Even Rouault, 5 years ago

Replying to Kurt Schwehr:

Can someone who knows why, add a comment as to why there it doesn't have GDT_Int64, GDT_CInt64, GDT_UInt64?

I'm not sure in which place exactly you'd want that comment: in the tests ? And is your question "why isn't there GDT_Int64, GDT_CInt64, GDT_UInt64?" If so, I guess the answer is "because nobody care enough yet to add support for them".

comment:11 by Kurt Schwehr, 5 years ago

I was thinking in the enum...

https://trac.osgeo.org/gdal/browser/trunk/gdal/gcore/gdal.h?rev=38182#L70

e.g. maybe something like this or take your pick as long as it's somewhat easy to figure out that those types are known to not be there.

/*! Pixel data types */
typedef enum {
    /*! Unknown or unspecified type */          GDT_Unknown = 0,
    /*! Eight bit unsigned integer */           GDT_Byte = 1,
    /*! Sixteen bit unsigned integer */         GDT_UInt16 = 2,
    /*! Sixteen bit signed integer */           GDT_Int16 = 3,
    /*! Thirty two bit unsigned integer */      GDT_UInt32 = 4,
    /*! Thirty two bit signed integer */        GDT_Int32 = 5,
    /* TODO(#6879): GDT_Int64 */
    /* TODO(#6879): GDT_UInt64 */
    /*! Thirty two bit floating point */        GDT_Float32 = 6,
    /*! Sixty four bit floating point */        GDT_Float64 = 7,
    /*! Complex Int16 */                        GDT_CInt16 = 8,
    /*! Complex Int32 */                        GDT_CInt32 = 9,
    /* TODO(#6879): GDT_CInt64 */
    /*! Complex Float32 */                      GDT_CFloat32 = 10,
    /*! Complex Float64 */                      GDT_CFloat64 = 11,
    GDT_TypeCount = 12          /* maximum type # + 1 */
} GDALDataType;

comment:12 by Even Rouault, 5 years ago

Resolution: fixed
Status: reopenedclosed

In 38204:

Mention potential future GDT_ constants (fixes #6879)

comment:13 by Kurt Schwehr, 5 years ago

This week, I hit my first HDF5 64-bit integer value with a JAXA file. There isn't a rush to add GDT_Int64 and GDT_UInt64 as it's possible to load the data from python with numpy and only the low order bits are set.

gdalinfo -mm -stats -listmdd GPMMRG_MAP_0007021800_H_L3S_MCH_03E.h5 

Subdatasets:
  SUBDATASET_1_NAME=HDF5:"GPMMRG_MAP_0007021800_H_L3S_MCH_03E.h5"://Grid/gaugeQualityInfo
  SUBDATASET_1_DESC=[3600x1800] //Grid/gaugeQualityInfo (16-bit integer)
  SUBDATASET_2_NAME=HDF5:"GPMMRG_MAP_0007021800_H_L3S_MCH_03E.h5"://Grid/hourlyPrecipRate
  SUBDATASET_2_DESC=[3600x1800] //Grid/hourlyPrecipRate (32-bit floating-point)
  SUBDATASET_3_NAME=HDF5:"GPMMRG_MAP_0007021800_H_L3S_MCH_03E.h5"://Grid/hourlyPrecipRateGC
  SUBDATASET_3_DESC=[3600x1800] //Grid/hourlyPrecipRateGC (32-bit floating-point)
  SUBDATASET_4_NAME=HDF5:"GPMMRG_MAP_0007021800_H_L3S_MCH_03E.h5"://Grid/observationTimeFlag
  SUBDATASET_4_DESC=[3600x1800] //Grid/observationTimeFlag (32-bit floating-point)

satelliteInfoFlag is missing. The tail end of h5dump -H:

     DATASET "satelliteInfoFlag" {
         DATATYPE  H5T_STD_I64LE
         DATASPACE  SIMPLE { ( 3600, 1800 ) / ( 3600, 1800 ) }
         ATTRIBUTE "CodeMissingValue" {
            DATATYPE  H5T_STRING {
               STRSIZE 6;
               STRPAD H5T_STR_NULLTERM;
               CSET H5T_CSET_ASCII;
               CTYPE H5T_C_S1;
            }
            DATASPACE  SCALAR
         }
         ATTRIBUTE "DimensionNames" {
            DATATYPE  H5T_STRING {
               STRSIZE 10;
               STRPAD H5T_STR_NULLTERM;
               CSET H5T_CSET_ASCII;
               CTYPE H5T_C_S1;
            }
            DATASPACE  SCALAR
         }
         ATTRIBUTE "_FillValue" {
            DATATYPE  H5T_STD_I64LE
            DATASPACE  SCALAR
         }
      }

From somewhere in https://www.gportal.jaxa.jp/gp/top.html

Note: See TracTickets for help on using tickets.