Initial implementation of float16 support #10342
Conversation
There was a problem hiding this comment.
Thanks for tackling this. One major adjustment will be to introduce #ifdef at all places where we reference _Float16
There will be also the question on how to deal with Float16 datasets on environments where this is not a native data type. We should probably continue to expose them as Float32 as done for example by the GTiff, HDF5 or Zarr driver in current master. Otherwise we'll have to make sure that RasterIO() / GDALCopyWords() can work for Float16 <--> Float32 conversions even if _Float16 is not a native data type (using the functions of port/cpl_float.h)
Same issue with the VRT driver: what if a user creates/reads a VRT file referencing Float16 on a system without _Float16 support.
If you could rebase on top of latest master to solve a conflict, that would enable to have CI to trigger
gcore/gdal_priv.h
Outdated
| } | ||
|
|
||
| // The generic function above does not work for _Float16 because | ||
| // std::numeric_limits<float>::epsilon() is the wrong choice | ||
| #ifndef SWIGPYTHON |
There was a problem hiding this comment.
Why testing for SWIGPYTHON outside of swig/include? That doesn't seem appropriate
There was a problem hiding this comment.
SWIG examines this file and is getting confused because it does not know _Float16. Is there a different way to tell SWIG to ignore this function? (I'm not a SWIG expert.)
There was a problem hiding this comment.
ok I see. Perhaps use #ifndef SWIG to be more general then
There was a problem hiding this comment.
The error is
In file included from extensions/gdal_array_wrap.cpp:3395:
/localhome/eschnett/src/gdal/gcore/gdal_priv.h:4414:28: error: ‘bool ARE_REAL_EQUAL’ redeclared as different kind of entity
4414 | inline bool ARE_REAL_EQUAL(_Float16 hfVal1, _Float16 hfVal2, int ulp = 2)
| ^~~~~~~~
gcore/gdalmultidim.cpp
Outdated
| @@ -1692,6 +1695,9 @@ bool GDALExtendedDataType::CopyValue(const void *pSrc, | |||
| static_cast<GIntBig>( | |||
| *static_cast<const std::int64_t *>(pSrc))); | |||
| break; | |||
| case GDT_Float16: | |||
| str = CPLSPrintf("%.9g", *static_cast<const _Float16 *>(pSrc)); | |||
There was a problem hiding this comment.
I would expect float16 requires less than 9 significant digits. Same below for CFloat16
There was a problem hiding this comment.
Thanks, it requires 5 digits.
I notice that double values are output with 18 digits. 17 would suffice.
| @@ -1155,9 +1155,11 @@ static GDALExtendedDataType ParseDtype(const CPLJSONObject &obj, | |||
| } | |||
| else if (chType == 'f' && nBytes == 2) | |||
| { | |||
| // elt.nativeType = DtypeElt::NativeType::IEEEFP; | |||
There was a problem hiding this comment.
exposing has Float32 will still be needed for environments without native Float16 support
| @@ -375,6 +375,10 @@ CPLErr GTIFFBuildOverviewsEx(const char *pszFilename, int nBands, | |||
| nBandFormat = SAMPLEFORMAT_INT; | |||
| break; | |||
|
|
|||
| case GDT_Float16: | |||
| CPLAssert(false); | |||
There was a problem hiding this comment.
The assertion is inappropriate, as this could happen. We should error out with a proper CPLError(CE_Failure, CPLE_NotSupported, ...) error. Same remark for CFloat16 below. Actually we could potentially support Float16. There's for example support for it in frmts/gtiff/gtiffoddbitsband.cpp
| @@ -105,6 +105,9 @@ static PyObject *GDALCreateNumpyArray(PyObject *pCreateArray, void *pBuffer, | |||
| case GDT_UInt64: | |||
| pszDataType = "uint64"; | |||
| break; | |||
| case GDT_Float16: | |||
| CPLAssert(FALSE); | |||
There was a problem hiding this comment.
this assert should be converted to a CPLError(), or implemented
| @@ -59,6 +59,8 @@ inline double GetSrcVal(const void *pSource, GDALDataType eSrcType, T ii) | |||
| case GDT_Int64: | |||
| return static_cast<double>( | |||
| static_cast<const int64_t *>(pSource)[ii]); | |||
| case GDT_Float16: | |||
There was a problem hiding this comment.
The data type enumeration in frmts/vrt/data/gdalvrt.xsd will need to be extended
There was a problem hiding this comment.
Done. I assume you refer to DataTypeType. I assume the datatypes there are essentially unordered and do not have to have the same order as enum GDALDataType. I also added Int8 which seemed to be missing.
|
I am planning to use |
that sounds reasonable. We should make sure that this is set in cmake/template/cpl_config.h.in so that code like out-of-tree drivers has also access to that define when building against GDAL headers |
|
Should |
This implements support for the
float16datatype as proposed in #10146.