Added support for `Float16` #341

andyferris · 2016-10-24T05:11:22Z

Saving and loading Float16 values is now supported
Float16 is no longer an "opaque" type.
Added some more wrapper functions relating to setting and getting
floating-point datatype fields
H5T_FLOAT16 needs to be created dynamically. Attempts at making the
list of natively supported types dynamically modifiable by the user
is blocked by the fact that HDF5BitsKind is used for dispatch
everywhere...
Unit test

CC @omriroames @r-chris

Note: There is a (rather small) caveat that all 2-byte floating point types will be loaded as Float16 (which may be lossy if people aren't using the IEEE conventional type).

andyferris · 2016-10-24T05:11:49Z

CC @c42f

c42f

I had a look. This all looks pretty reasonable, though I don't know a lot about the surrounding code.

c42f · 2016-10-24T05:25:53Z

src/plain.jl

+
+    # Set up Float16 (must occur at runtime)
+    eval(:(const H5T_FLOAT16 = make_float16()))
+    eval(:(hdf5_type_id(::Type{Float16}) = H5T_FLOAT16))


I guess you can use @eval here? Slightly more readable.

True! I always forget about @eval

c42f · 2016-10-24T05:28:10Z

src/plain.jl

+        error("Error getting fields of floating-point datatype")
+    end
+    return (spos[], epos[], esize[], mpos[], msize[])
+end


Was this function required for debugging? I can't see it used elsewhere.

Seemed reasonable to define getters as I define the matching setters.

And yes I did use it for debugging (same with h5t_get_ebias above)

Makes sense.

c42f · 2016-10-24T05:33:55Z

src/plain.jl

-                is_signed = h5t_get_sign(native_type)
-            else
-                is_signed = nothing
+        # First look in the type last for a match


spelling "last" ?

andyferris · 2016-10-26T05:49:09Z

<begin cranky rant>

I'm noting that there have been several sensible and minimal PRs sitting around, some since July, and others since quite recently, with not even a comment. These include #317 and #318 (@eschnett), #331 (@r-chris), and #342 (@OmriRoames - obviously this one is much too recent for complaints) (plus 2 more recent ones from myself).

Can I ping whoever maintains this package a bit of general HDF5 community attention? Apologies in advance for being pushy, but I'm going start with @timholy, @simonster and @tkelman. Not sure who else might have merge permissions.

For obvious reasons, HDF5.jl is an important package for getting real work done. I completely understand that everyone here is a volunteer and is busy - and if the package needs more maintainers to share the workload, I'm sure there's a few of us willing to help out. And again, sorry for being cranky and pushy!!!

</end rant>

c42f · 2016-10-26T06:12:20Z

@andyferris sorry about the mess on this branch, there were some crossed wires with a rebase!

I tried to fix it up - hope it looks good to you now?

andyferris · 2016-10-26T06:19:10Z

I don't think I see any problems...

c42f · 2016-10-26T06:23:37Z

src/plain.jl

-    eval(:(const H5T_FLOAT16 = make_float16()))
-    eval(:(hdf5_type_id(::Type{Float16}) = H5T_FLOAT16))
+    @eval(const H5T_FLOAT16 = make_float16())
+    @eval(hdf5_type_id(::Type{Float16}) = H5T_FLOAT16)


I don't think you even need the macro brackets here by the way.

andyferris · 2017-01-13T01:21:41Z

bump

* Saving and loading Float16 values is now supported * Float16 is no longer an "opaque" type. * Added some more wrapper functions relating to setting and getting floating-point datatype fields * `H5T_FLOAT16` needs to be created dynamically. Attempts at making the list of natively supported types dynamically modifiable by the user is blocked by the fact that `HDF5BitsKind` is used for dispatch everywhere... * Unit test

andyferris · 2017-03-16T05:04:31Z

Rebased. bump.

c42f · 2017-03-16T06:37:56Z

The CI errors here seem to be the issue in JuliaLang/julia#7669

I think you need

@eval hdf5_type_id(::Type{Float16})=H5T_FLOAT16

musm · 2017-03-21T15:07:04Z

@simonster any reason not to support float16? Do you have any reservations here. The basic implementation looks fine I believe but im not an expert with the code base

musm · 2017-03-23T15:30:04Z

tests now passing thanks @c42f for that! @andyferris hope you don't mind I pushed the fix for you

andyferris · 2017-03-23T22:47:23Z

Of course - thanks @musm and @c42f :)

tkelman · 2017-03-28T15:21:18Z

test/plain.jl

+
+    # Test Float16 support
+    arr_float16 = Float16[1.0, 0.25, 0.5, 8.0]
+    h5write("test_float16.h5", "x", arr_float16)


should clean this file up when the test is done, otherwise stale results from previous runs could give misleading results

Is it more robust to check for existence and delete it before the test?

I guess if you want it to stick around afterwards for debugging, maybe? But then it sticks around in the package dir (I think that's where Pkg.test will usually run from). It's not checked in so it wouldn't make the package dirty in the git sense, but it would be an untracked file.

Would be consistent with the earlier tests to put it under joinpath(tmpdir, "test_float16.h5") and move the rm(tmpdir, recursive=true) to after this test.

andyferris · 2017-03-30T03:27:02Z

Thanks @musm for the patches. Is this good to go?

simonster · 2017-03-30T16:01:50Z

It would be good to make sure that this doesn't change things for JLD, e.g. that Float16 values saved with JLD and the previous version of HDF5 can still be read after these changes. Otherwise we may need to tweak some things to accommodate this change.

musm · 2017-05-12T16:00:25Z

src/HDF5.jl

+    h5t_lock(FLOAT16)
+    return FLOAT16
+end
+# const H5T_FLOAT16 = make_float16()  (in `__init__()`)


I guess I don't really understand why this has to be in __init__ do you think you could please explain this to me?

This is needed for the same reason that H5open() is called inside __init__: make_float16() is allocating new data structures inside the underlying hdf5 library and must be called each time the library is loaded.

Actually it's interesting that the rest of the code assumes the value of globals like H5T_NATIVE_FLOAT_g will be consistent between different runs of the julia program. Looking at the C library, these globals are allocated inside H5open(), so it seems this isn't really guaranteed.

Thanks. I'm wondering of a way to trigger a bug to show that.
In matlab.jl we have references https://github.com/JuliaInterop/MATLAB.jl/blob/master/src/init.jl
that are filled in with the pointers in inside __init__ ref https://github.com/JuliaInterop/MATLAB.jl/blob/master/src/MATLAB.jl#L53
we might have to do the same here?

I have no idea if we can trigger a bug/make a test for this... would it happen if the HDF5 binary library was updated without this package being precompiled again? And yes, to me, filling references at run-time during __init__ seems the safest possible thing to do.

To trigger this possible bug, it looks like you'd need to first load a library which creates a custom id dynamically (through a call to H5Iregister() I guess?), before H5open() is called. I don't know whether the libhdf5 API contract prevents this from happening.

After that, load HDF5.jl into the same process and the ids may be different from when HDF5.jl was loaded in build.jl.

This seems quite possible. Another library (e.g. PETSc) could depend on HDF5_jll so that HDF5_jll is loaded and initialized before HDF5.jl is initialized. It would be quite difficult to trigger this in a test case though.

musm · 2020-03-02T19:25:59Z

It seems like a good idea to try to push this through once again.

Iddingsite · 2022-04-07T17:49:22Z

Hi, is there any news on Float16 support?

mkitti · 2022-05-31T21:13:19Z

I'm thinking about resurrecting this, but making it lazy.

Instead of invoking it on __init__, we could have a constant Ref to hold this. make_float16 would just be invoked once on demand.

musm · 2022-05-31T21:16:51Z

@mkitti that's what I was suggesting in https://github.com/JuliaIO/HDF5.jl/pull/341/files#r116400351

Iddingsite · 2024-01-17T12:32:35Z

Hi, is there some updates on that? Is there a reason that is preventing the changes from @musm to be merged? Would be really useful for one of my application.

mkitti · 2024-01-18T03:20:34Z

Mainly it needs to rebased. The pull request is quite old now.

Iddingsite · 2024-01-22T13:12:57Z

I see. Doesn't seems so much work but sadly I don't have any expertise in this. I will have to wait for someone to pick this up!

mkitti · 2024-01-22T15:13:18Z

What would be even better is if HDF Group added upstream support for Float16.

There's a related issue here:
HDFGroup/hdf5#2154

Iddingsite · 2024-01-22T17:27:22Z

I see, I was not aware of that one. Yeah, that would makes sense to wait for native support 👍 thx!

eschnett · 2024-01-22T21:00:02Z

No, it doesn't make sense to wait. It would be really cool if we had that feature now.

Iddingsite · 2024-07-02T14:25:01Z

I've noticied that the last version from the hdfgroup is now supporting Float16 (https://www.hdfgroup.org/2024/04/release-of-hdf5-1-14-4-newsletter-202/). Is that now supported in the Julia wrapper?

eschnett · 2024-07-02T18:33:38Z

Working on it JuliaPackaging/Yggdrasil#8588 ...

andyferris mentioned this pull request Oct 24, 2016

Should HDF5.jl support writing arrays of Float16 type? #301

Open

c42f reviewed Oct 24, 2016

View reviewed changes

OmriTreidel force-pushed the float16 branch from 115cea2 to 814521e Compare October 26, 2016 05:00

c42f force-pushed the float16 branch from 814521e to b64594f Compare October 26, 2016 06:11

c42f reviewed Oct 26, 2016

View reviewed changes

Andy Ferris and others added 2 commits March 16, 2017 15:03

Aesthetic fixes

b6eb8d1

andyferris force-pushed the float16 branch from df8c6a6 to b6eb8d1 Compare March 16, 2017 05:04

work around macro with parens

429bc75

musm added 2 commits March 27, 2017 16:07

Merge branch 'master' into float16

e5e12ca

Update plain.jl

e51c84e

tkelman reviewed Mar 28, 2017

View reviewed changes

Andy Ferris and others added 8 commits March 29, 2017 10:20

Put Float16 tests in temp directory and cleanup

5c7b240

rm both temporary files created during testing

33dfb18

Fix typo tempdir() to tmpdir

be6645b

Update plain.jl

bab0763

Update plain.jl

ef2d1a6

update open of nullterm_ascii file

78a260a

Update plain.jl

861cdf4

Update plain.jl

9a7788e

musm reviewed May 12, 2017

View reviewed changes

musm added 2 commits June 28, 2017 12:53

Merge branch 'master' into float16

6ec1cd6

Update plain.jl

ca674a8

kleinhenz mentioned this pull request Jul 19, 2019

complex number support #558

Merged

kleinhenz mentioned this pull request Nov 5, 2019

improve performance/ergonomics of reading compound datatypes #592

Merged

This was referenced Oct 7, 2020

An assortment of minor changes #692

Merged

RFC: Initialize runtime globals during __init__ #703

Draft

mkitti mentioned this pull request Jan 22, 2024

Improve float16 performance HDFGroup/hdf5#2154

Open

Added support for Float16 #341

Are you sure you want to change the base?

Added support for Float16 #341

Conversation

andyferris commented Oct 24, 2016

andyferris commented Oct 24, 2016

c42f left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyferris Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyferris commented Oct 26, 2016 • edited Loading

c42f commented Oct 26, 2016

andyferris commented Oct 26, 2016

Choose a reason for hiding this comment

andyferris commented Jan 13, 2017

andyferris commented Mar 16, 2017

c42f commented Mar 16, 2017

musm commented Mar 21, 2017

musm commented Mar 23, 2017

andyferris commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyferris commented Mar 30, 2017

simonster commented Mar 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

musm commented Mar 2, 2020

Iddingsite commented Apr 7, 2022

mkitti commented May 31, 2022

musm commented May 31, 2022

Iddingsite commented Jan 17, 2024

mkitti commented Jan 18, 2024

Iddingsite commented Jan 22, 2024

mkitti commented Jan 22, 2024

Iddingsite commented Jan 22, 2024

eschnett commented Jan 22, 2024

Iddingsite commented Jul 2, 2024

eschnett commented Jul 2, 2024

Added support for `Float16` #341

Added support for `Float16` #341

andyferris Oct 26, 2016 •

edited

Loading

andyferris commented Oct 26, 2016 •

edited

Loading