OSGeo
Add RFC for float16 support
@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. Typically the RFC lifecycle is:
- draft a preliminary version as you did, make it known on gdal-dev to get early feedback on it
- work on a candidate implementation, revise the RFC text
- ask for any remaining comments
- call for a vote to officially endorse it (cf https://lists.osgeo.org/pipermail/gdal-dev/2024-March/058635.html)
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?
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
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. Having a specific flag to allow/disable Float16 adds some long term complications in GDAL internals (also probably minor given that drivers having Float16 capabilities still need to handle both exposing as Float16 or promotion to Float32 depending on if the GDAL build as Float16 capabilities). Also to be noted that the HDF5 library, when built with Float16 support, automatically uses it by default (which broke GDAL which didn't expect this new data type, but I found that acceptable)
@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
because SWIG doesn't support Float16.
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.
Any preference whether this should be a separate PR from the one with the RFC?
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
-
that all unit tests are passing
-
that all comments by reviewers have been addressed
-
that there is enough information for reviewers, in particular link to any issues which this pull request fixes
-
that you have written unit tests where possible In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request. If there is no further activity on this pull request, it will be closed in 2 weeks.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
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
-
that all unit tests are passing
-
that all comments by reviewers have been addressed
-
that there is enough information for reviewers, in particular link to any issues which this pull request fixes
-
that you have written unit tests where possible In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request. If there is no further activity on this pull request, it will be closed in 2 weeks.
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.
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
-
that all unit tests are passing
-
that all comments by reviewers have been addressed
-
that there is enough information for reviewers, in particular link to any issues which this pull request fixes
-
that you have written unit tests where possible In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request. If there is no further activity on this pull request, it will be closed in 2 weeks.
For the remaining spelling issues (once you've americanized behaviour->behavior), you can add suppressions like in https://github.com/OSGeo/gdal/blob/0defeabb0aab96baf70e65a46bfcb2dc2e3af6f3/doc/source/thanks.rst?plain=1#L135
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
-
that all unit tests are passing
-
that all comments by reviewers have been addressed
-
that there is enough information for reviewers, in particular link to any issues which this pull request fixes
-
that you have written unit tests where possible In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request. If there is no further activity on this pull request, it will be closed in 2 weeks.
Hi Erik,
This RFC is great and so close to done. Any chance you could push it the last little bit?
Thanks! -kurt
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
-
that all unit tests are passing
-
that all comments by reviewers have been addressed
-
that there is enough information for reviewers, in particular link to any issues which this pull request fixes
-
that you have written unit tests where possible In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request. If there is no further activity on this pull request, it will be closed in 2 weeks.
A few spelling issue:
source/development/rfc/rfc100_float16_support.rst:8: : Spell check: Schnetter: Erik Schnetter.
source/development/rfc/rfc100_float16_support.rst:9: : Spell check: eschnetter: eschnetter @ perimeterinstitute.ca.
source/development/rfc/rfc100_float16_support.rst:9: : Spell check: perimeterinstitute: eschnetter @ perimeterinstitute.ca.
source/development/rfc/rfc100_float16_support.rst:71: : Spell check: behaviour: behaviour (transparently to the user), and operations will be.
source/development/rfc/rfc100_float16_support.rst:86: : Spell check: behaviour: float32. In other drivers, the current behaviour is retained, which.
you can add an allow-list like in https://github.com/OSGeo/gdal/pull/11708/files#diff-2eeb78eed715cc4f2f9371e677d223b3a3621f843d570e5eb2f20cd4a054f91fR194
@schwehr Would you mind updating your vote in the gdal-dev mailing list thread so we can formally adopt the RFC ?