RFC 100 text: Add RFC for float16 support #10146
Conversation
|
@eschnett Once you're happy with the RFC, it would also be appropriate to mention it by an email to the gdal-dev mailing list (https://lists.osgeo.org/pipermail/gdal-dev/). Cf https://lists.osgeo.org/pipermail/gdal-dev/2024-February/058548.html for an example.
|
Co-authored-by: Even Rouault <[email protected]>
|
Thinking about automatically converting float16 to float32 a bit more: I think it would be confusing to choose GDAL's behaviour in this respect on which compiler flags and versions were used to build GDAL. It would probably be better to make GDAL be consistent – it should either always automatically convert to float32, or never do that automatically. And given the current behaviour, it would be quite inconvenient if one was suddenly presented with a float16 dataset. Maybe there should be a new flag, passed to GDAL when opening a dataset, that specifies whether float16 data should be automatically converted to float32 (the default setting), or should be preserved as float16? |
That's a good point. To give some perspective: in the past when we have introduced GDT_Int64 and GDT_UInt64, before their addition, the Zarr driver automatically exposed arrays with those types as Float64, as that was the closest (somewhat) compatible available data type. And we didn't add an option to allow int64/uint64 to be presented when we added those types. That was documented as a change with some breaking potential in the MIGRATION_GUIDE.TXT file. Similarly when GDT_Int8 has been introduced, such data type was handled in a very clumsy way before (it was presented as unsigned byte + a metadata item saying "actually it is signed"). After the change, those datasets were returned with the new type. So the practice up to now has been to adopt the new data type. I think it would be fine to do the same here, that is when the GDAL build has _Float16 support and the dataset is Float16, then use GDT_Float16 as the declared data type. Float16 datasets are sufficiently esoteric (at least that's my perspective) that it is likely acceptable to do so. |
|
@eschnett not sure what your plans are, but at that point, I believe that working on a candidate implementation would be the appropriate step forward |
|
@rouault I have an implementation but got stuck writing tests. It turns out to be difficult to test from Python because SWIG doesn't support Float16. I was looking into some C++ tests but got sidetracked by another project. I'll push my current state. Any preference whether this should be a separate PR from the one with the RFC? |
|
Thanks for the update. No rush, just wanted to know how things were progressing
is it itself an issue? I don't anticipate a lot of GDAL API methods to actually return Float16 scalars. Most Float16 specific tests should be around RasterIO(), and RasterIO() at the SWIG level is handle by the swig/include/python specifics (and swig/include/gdal_array.i) with dedicate C wrapping code and Python code.
yes, the candidate implementation would be better into a separate PR |
|
The GDAL project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 28 days and is being automatically marked as "stale". If you think this pull request should be merged, please check
|
|
While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 6 weeks. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the GDAL project can do to help push this PR forward please let us know how we can assist. |
|
Just leaving a comment here – yes, I am still interested in this, and I am still working on it, but there's another higher-priority project taking up my time at the moment. |
|
The GDAL project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 28 days and is being automatically marked as "stale". If you think this pull request should be merged, please check
|
|
While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 6 weeks. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the GDAL project can do to help push this PR forward please let us know how we can assist. |
|
(keep alive) |
|
The GDAL project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 28 days and is being automatically marked as "stale". If you think this pull request should be merged, please check
|
|
(keep alive) |
rouault
changed the title
| ------- | ||
|
|
||
| This RFC adds support for the IEEE 16-bit floating point data type | ||
| (aka ``half``, ``float16``). It adds new pixel data types |
There was a problem hiding this comment.
What is this two back quote style?
There was a problem hiding this comment.
A double backquote is rendered as "code" style, similar to a single backquote in Github markdown. I've taken it from RFC 99.
|
For the remaining spelling issues (once you've americanized behaviour->behavior), you can add suppressions like in Line 135 in 0defeab |
| routines. This type should be supported without converting to | ||
| float32. | ||
|
|
||
| - C++23 will introduce native support for ``std::float16_t``. However, |
There was a problem hiding this comment.
eschnett#1 is an attempt of aliasing GFloat16 on std::float16_t if it is available
There was a problem hiding this comment.
Thanks! I'll probably merge it and then hash out the details.
There was a problem hiding this comment.
Once the RFC text has been updated taking into account above to reflect how C++23 std::float16_t will be handled, I'd be happy to give my +1 to the RFC.
There was a problem hiding this comment.
Woohoo! Thank you both! I'm the same as Even for a +1.
See #10144.