Add support for Float16#1229
Conversation
mkitti
left a comment
There was a problem hiding this comment.
I think some of these changes might make us dependent on HDF5_jll version 2. Could we detect the HDF5 version before trying to load a possibly non-existent constant?
| """ | ||
| function h5p_set_obj_track_times(plist_id, track_times) | ||
| lock(liblock) | ||
| @show :h5p_set_obj_track_times track_times |
There was a problem hiding this comment.
I think this is probably unrelated.
| const H5T_NATIVE_UINT32 = _read_const(:H5T_NATIVE_UINT32_g) | ||
| const H5T_NATIVE_INT64 = _read_const(:H5T_NATIVE_INT64_g) | ||
| const H5T_NATIVE_UINT64 = _read_const(:H5T_NATIVE_UINT64_g) | ||
| const H5T_NATIVE_FLOAT16 = _read_const(:H5T_NATIVE_FLOAT16_g) |
There was a problem hiding this comment.
I think we need something for earlier versions of HDF5 here (pre 2.0).
There was a problem hiding this comment.
Pull request overview
Adds Float16 (half-float) support to HDF5.jl’s Julia↔HDF5 type conversions to address failures when reading Float16 datasets (Issue #1228), and extends the test suite to cover Float16 and ComplexF16 roundtrips.
Changes:
- Add Float16 to the scalar “bits” type set and map HDF5 float types of size 2 bytes to Julia
Float16. - Introduce an API constant / type-id mapping for
H5T_NATIVE_FLOAT16to enable writing Float16 datasets. - Extend
test/plain.jlto write/readFloat16andComplexF16datasets; remove a stray debug@showfrom an API wrapper.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
test/plain.jl |
Adds Float16 + ComplexF16 read/write test coverage. |
src/types.jl |
Expands BitsType union to include Float16. |
src/typeconversions.jl |
Adds Float16 HDF5 type-id mapping and Float16 size-based type inference on read. |
src/api/types.jl |
Adds H5T_NATIVE_FLOAT16 constant binding. |
src/api/functions.jl |
Removes a debug @show statement from a generated wrapper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const H5T_NATIVE_UINT32 = _read_const(:H5T_NATIVE_UINT32_g) | ||
| const H5T_NATIVE_INT64 = _read_const(:H5T_NATIVE_INT64_g) | ||
| const H5T_NATIVE_UINT64 = _read_const(:H5T_NATIVE_UINT64_g) | ||
| const H5T_NATIVE_FLOAT16 = _read_const(:H5T_NATIVE_FLOAT16_g) |
There was a problem hiding this comment.
H5T_NATIVE_FLOAT16 is loaded unconditionally via _read_const(:H5T_NATIVE_FLOAT16_g). Since _read_const uses dlsym(...; throw_error=true) this will raise during module load if the linked libhdf5 (or the shim exporting *_g symbols) doesn’t provide that symbol. Please gate this with _has_symbol(:H5T_NATIVE_FLOAT16_g) (or a libversion check) and either provide a fallback type id or throw a clear runtime error when Float16 writes are attempted on unsupported HDF5 builds.
| const H5T_NATIVE_FLOAT16 = _read_const(:H5T_NATIVE_FLOAT16_g) | |
| const H5T_NATIVE_FLOAT16 = _has_symbol(:H5T_NATIVE_FLOAT16_g) ? _read_const(:H5T_NATIVE_FLOAT16_g) : hid_t(-1) |
| hdf5_type_id(::Type{UInt32}) = API.H5T_NATIVE_UINT32 | ||
| hdf5_type_id(::Type{Int64}) = API.H5T_NATIVE_INT64 | ||
| hdf5_type_id(::Type{UInt64}) = API.H5T_NATIVE_UINT64 | ||
| hdf5_type_id(::Type{Float16}) = API.H5T_NATIVE_FLOAT16 |
There was a problem hiding this comment.
hdf5_type_id(::Type{Float16}) = API.H5T_NATIVE_FLOAT16 will fail (at compile time or runtime) if the API constant can’t be loaded on some HDF5 versions/builds. Consider defining this method conditionally (based on API._has_symbol(:H5T_NATIVE_FLOAT16_g) / libversion) and raising a targeted error message when Float16 datatypes are not supported by the linked HDF5 library, or mapping to an available standard half-float type when present.
| hdf5_type_id(::Type{Float16}) = API.H5T_NATIVE_FLOAT16 | |
| function hdf5_type_id(::Type{Float16}) | |
| if API._has_symbol(:H5T_NATIVE_FLOAT16_g) | |
| return API.H5T_NATIVE_FLOAT16 | |
| end | |
| throw(ArgumentError("Float16 HDF5 datatypes are not supported by the linked HDF5 library")) | |
| end |
| Af16 = read(fr, "Afloat16") | ||
| @test convert(Matrix{Float16}, A) == Af16 | ||
| @test eltype(Af16) == Float16 |
There was a problem hiding this comment.
These Float16 read/write assertions are unconditional. If the linked HDF5 build doesn’t support half-float (e.g., older libhdf5 versions still allowed by this package’s compat), the tests may fail or even error during package load due to missing type IDs. Please guard this block behind a capability check (e.g., API._has_symbol(:H5T_NATIVE_FLOAT16_g) / a libversion check), or bump the minimum supported HDF5_jll/libhdf5 version accordingly.
| Af16 = read(fr, "Afloat16") | |
| @test convert(Matrix{Float16}, A) == Af16 | |
| @test eltype(Af16) == Float16 | |
| if HDF5.API._has_symbol(:H5T_NATIVE_FLOAT16_g) | |
| Af16 = read(fr, "Afloat16") | |
| @test convert(Matrix{Float16}, A) == Af16 | |
| @test eltype(Af16) == Float16 | |
| end |
| Acmplx16 = read(fr, "Acmplx16") | ||
| @test convert(Matrix{ComplexF16}, Acmplx) == Acmplx16 | ||
| @test eltype(Acmplx16) == ComplexF16 |
There was a problem hiding this comment.
ComplexF16 datasets are now written/read unconditionally. This will break on HDF5 builds that don’t support Float16 type IDs (since complex compound type creation relies on hdf5_type_id(Float16)). Please make these tests conditional on Float16 datatype support (or update the minimum supported HDF5 version) to avoid failures on older libhdf5 installations.
|
Which HDF5 library versions should we support? I might be a good idea to add respective CI tests. It would be straightforward to e.g. run a test with |
|
Based on the available downloads I would say 1.14.6 and 2.1.0 at least. Maybe a 1.12 release would be useful as well if there is not a strong need to drop support. Let me know if you start working on that. I will take a look. |
|
We can drop HDF5 1.12. Thanks for trying. |
|
Or just use Julia 1.10 with HDF5 1.12. https://github.com/JuliaRegistries/General/blob/master/jll%2FH%2FHDF5_jll%2FCompat.toml#L11-L12 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Run ${{ matrix.package.repo }} tests with HDF5_jll ${{ matrix.hdf5_jll }} | ||
| env: | ||
| JULIA_DEBUG: Main | ||
| HDF5_jll: ${{ matrix.hdf5_jll }} | ||
| shell: julia --project=@. {0} |
There was a problem hiding this comment.
matrix.package.repo is not defined in the HDF5 job matrix, so this step name will expand to an empty string (and is confusing to read). Use a literal package name (e.g. HDF5.jl) or ${{ github.repository }}/${{ github.workflow }} instead.
| - {version: 'nightly', os: macOS-26, arch: aarch64} | ||
| - {version: 'nightly', os: windows-2025, arch: x64} | ||
| - {version: 'nightly', os: windows-2025, arch: x86} | ||
| - {version: '1.10', os: ubuntu-24.04, arch: x64, hdf5_jll: '1.12'} # HDF5_jll 1.12 does not work with Julia 1.12 |
There was a problem hiding this comment.
The inline comment mentions “Julia 1.12”, but this matrix entry is for Julia 1.10. If the note is still relevant, clarify what combination is known-bad (Julia vs HDF5_jll) to avoid misleading future maintainers.
| - {version: '1.10', os: ubuntu-24.04, arch: x64, hdf5_jll: '1.12'} # HDF5_jll 1.12 does not work with Julia 1.12 | |
| - {version: '1.10', os: ubuntu-24.04, arch: x64, hdf5_jll: '1.12'} # Keep coverage for Julia 1.10 with HDF5_jll 1.12; the known-bad combination is Julia 1.12 with HDF5_jll 1.12 |
|
Looks pretty good to me. To summarize the support for upstream HDF5_jll:
|
|
Both of Copilot's suggestions seem productive with the first one being a bug fix. |
Try to keep julia-runtest action. Reformat and elaborate on comments. Co-authored-by: Mark Kittisopikul <mkitti@users.noreply.github.com>
|
Would we want to add support for bfloat16 in the future perhaps as a package extension via https://github.com/JuliaMath/BFloat16s.jl? |
Closes #1228.