complex number support #558

kleinhenz · 2019-07-18T17:33:49Z

This closes #180. The approach mirrors what is done by h5py. Complex types are stored as a compound datatype with field names "r" and "i". This compound type is recognized during reading and loaded as the correct complex julia type. Real and complex field names are configurable via HDF5.set_complex_field_names(real::String, imag::String). Complex support can be enabled/disabled via HDF5.enable/disable_complex_support(). All previous tests pass and I have added new tests for complex support specifically.

Although this is not part of the HDF5 standard I think it makes sense to include this here since it seems to be fairly basic and universally useful functionality that people coming from python would expect to just work. Also h5py has had this functionality for a while and doesn’t seem to have run into any major problems with their approach as far as I can tell.

@tknopp @musm what do you think of this?

tknopp · 2019-07-19T09:01:18Z

Looks good, thank you! @musm could you do the final decision / review?

kleinhenz · 2019-07-19T16:18:41Z

One downside of this approach is that the hdf5 datatype for complex numbers is created/destroyed on every read/write. I think this is necessary in order to enable dynamically enabling/disabling complex support and updating the field names. Another possibility is to create the types once at library initialization like in #341. I have also implemented this approach here although I am seeing some intermittent segfaults which I am not sure are related.

I'm not sure which approach is better. From a compatibility perspective it is nice to be able to dynamically update complex number handling but creating the type repeatedly will have at least a little performance impact. What do people think is better?

benchislett

Looks good to me.
I like the dynamic nature, and I don't think the dynamic creation will be that big of a performance hit.
The alternative of creating at init-time for every subtype of HDF5Scalar and looking them up at runtime seems like a bit much to me. I'm in favor of merging as-is

benchislett · 2019-08-07T04:41:48Z

src/HDF5.jl

@@ -323,6 +323,13 @@ cset(::Type{ASCIIChar}) = H5T_CSET_ASCII

 hdf5_type_id(::Type{C}) where {C<:CharType} = H5T_C_S1

+# global configuration for complex support
+complex_support = true
+complex_field_names = ("r", "i")


Would ("re", "im") be better here, since that's what Julia uses?

I would stick with the default convention h5py uses.

@tknopp why do you think it is better to stick to h5py convention?

The goal of HDF5 is to be language agnostic. It should not matter what programming language you use to access a file. Unfortunately, HDF5 itself defines no standard for complex numbers. But there is a very popular package (h5py) that proposes or uses a convention. Why should this convention be different for Julia, when we can stick to the convention h5py uses?

Sounds reasonable. MATLAB does not have a convention. So we can use the h5py precedent.

We should document this however.

Yes the idea was to seamlessly compatible with h5py by default. If people are happy with this approach and want to merge I can add some documentation explaining the choice.

I'm in favor of sticking to the h5py precedent

tknopp · 2019-08-21T15:55:48Z

ping @musm: Can this be merged after fixing the conflicts?

musm · 2019-08-21T15:58:39Z

src/HDF5.jl

+enable_complex_support() = global complex_support = true
+disable_complex_support() = global complex_support = false
+set_complex_field_names(real::String, imag::String) = global complex_field_names = (real, imag)
+


do we really need these global setter functions?

What's the rationale for being able to enable/disable complex support? Same with set_complex_field_names?

In any case a better way to deal with such global constants is to use Ref's with appropriate setters.

I implemented it this way following what is done by h5py and for compatibility with existing practice/workflows. Since there is no actual hdf5 standard for complex numbers allowing the fieldnames to be set dynamically lets you interface more easily with existing conventions. For example I think that octave uses real/imag and I have also seen re/im. Also it's conceivable that somebody is storing a compound type with these fieldnames that doesn't represent a complex number. I'm not sure if this flexibility is necessary/worth it but I decided to follow the h5py precedent in allowing it.

Not sure why they would want to do that, but even if they did couldn't they just change the field names?

@kleinhenz that makes sense

Can we please update all the global constants to use refs (it's precompile friendly, as opposed to globals)

const COMPLEX_SUPPORT = Ref(true) const COMPLEX_FIELD_NAMES = Ref(("r", "i")) enable_complex_support() = COMPLEX_SUPPORT[] = true disable_complex_support() = COMPLEX_SUPPORT[] = false set_complex_field_names(real::AbstractString, imag::AbstractString) = COMPLEX_FIELD_NAMES[] = ((real, imag))

And update everywhere else to use COMPLEX_SUPPORT[] instead of complex_support

I can see the rational of being able to disable this feature. It allows to to read the complex numbers in its "raw" representation, which otherwise would not be possible. It also does not really hurt making this optional.

Looking at h5py they don't offer a seperate way to enable/disable complex support.

I kinda agree with @benchislett that even if they would want to do that they could just change the field names.

I'm not 100% convinced which approach is best. I'm leaning towards no option to enable disable support since that seems a little overboard. OTOH we can always remove this non-user facing functions in the future if deemed overkill.

I think you are correct that h5py doesn't provide a mechanism for completely disabling complex support. I don't have a strong preference on this either. I lean towards including it just to be the least disruptive to existing workflows but I could see it the other way also.

One point in favor is that changing the field names will prevent you from reading data as complex when you don't want to but it can't prevent you from accidentally writing complex data in an unwanted format where previously it would have thrown an exception so I think including the enable/disable thing is somewhat safer for people with a preexisting way of handling complex data.

Ok that's convincing enough 👍

Manifest.toml

kleinhenz · 2019-08-21T16:46:13Z

@musm I've updated the global constants to use refs and fixed the merge conflicts. I will add some documentation later today.

src/HDF5.jl

kleinhenz · 2019-08-21T20:11:37Z

is there anything more that I should do here?

musm · 2019-08-21T20:14:57Z

I guess the only thing is

#558 (comment)

Do you know if h5py has an option for this as well? I looked but it seems like they only allow you to change the field names? I could be wrong though.

musm · 2019-08-21T20:29:48Z

I'll go ahead and merge this since this has been open for a while without comment

@tknopp @kleinhenz do you all approve as well?

kleinhenz · 2019-08-21T20:31:55Z

sounds good to me

tknopp · 2019-08-21T21:15:47Z

looks good!

musm · 2019-08-22T04:45:00Z

@kleinhenz thanks for sticking to this PR and seeing it through. It's great to have others improve this package

kleinhenz · 2019-08-22T13:38:54Z

@musm thanks! glad we were able to get it merged!

lazarusA · 2019-10-30T21:37:53Z

Why this does not work? what is the appropriate syntax ?

f = h5open("file.h5", "w")
dset_f = d_create(f, "psi", datatype(Complex{Float64}), dataspace(5, 5, 10))
for i in 1:10
    dset_f[:, :, i] = zeros(Complex{Float64}, 5,5)
end
flush(f)
close(f)

kleinhenz · 2019-10-30T22:15:57Z

@lazarusA does #591 fix your issue?

lazarusA · 2019-10-30T23:02:57Z

I did add HDF5#master and my code still doesn't work.

using HDF5
f = h5open("file.h5", "w")
dset_f = d_create(f, "psi", datatype(Complex{Float64}), dataspace(5, 5, 10))
for i in 1:10
    dset_f[:, :, i] = zeros(Complex{Float64}, 5,5)
end
flush(f)
close(f)

kleinhenz · 2019-10-30T23:12:58Z

It's not merged yet so you have to add it with add https://github.com/kleinhenz/HDF5.jl#complex_hyperslab

lazarusA · 2019-10-31T07:31:47Z

Nice! now it has been merged. And it works just beautiful.

bhawkins · 2019-11-14T22:25:11Z

Thanks for adding this feature! I just thought I'd mention that this supports MATLAB MAT files, too:

>> x = 1:10;
>> y = x + 10;
>> z = x + 1j * y;
>> save('test.mat', 'z', '-v7.3')

julia> using HDF5

julia> HDF5.set_complex_field_names("real", "imag");

julia> z = h5read("/tmp/test.mat", "z")
1×10 Array{Complex{Float64},2}:
 1.0+11.0im  2.0+12.0im  3.0+13.0im  …  8.0+18.0im  9.0+19.0im  10.0+20.0im

musm · 2020-10-15T16:16:11Z

@kleinhenz I just noticed this but is it really allowed for Complex{<:HDF5Scalar}, specifically the case with HDF5Reference, or should it be restricted to Complex{<:HDF5BitsKind} ?

kleinhenz · 2020-10-17T18:51:46Z

yeah this probably doesn't make much sense for HDF5Reference. OTOH I think in general we want to move more in the direction of removing dispatch restrictions rather than tightening them since this could conceivably make sense for types not in HDF5Scalar, e.g. BigFloat.

musm · 2020-10-17T20:21:44Z

Totally agreed, and actually recent commits on master have started to remove some unnecessary type restrictions for the high level interfaces

benchislett approved these changes Aug 6, 2019

View reviewed changes

benchislett reviewed Aug 7, 2019

View reviewed changes

musm reviewed Aug 21, 2019

View reviewed changes

Manifest.toml Outdated Show resolved Hide resolved

kleinhenz added 2 commits August 21, 2019 12:34

complex support

d81e86c

update global constants to use refs

6d090c9

kleinhenz force-pushed the complex branch from 48185d9 to 6d090c9 Compare August 21, 2019 16:44

musm reviewed Aug 21, 2019

View reviewed changes

src/HDF5.jl Outdated Show resolved Hide resolved

musm reviewed Aug 21, 2019

View reviewed changes

src/HDF5.jl Outdated Show resolved Hide resolved

cleanup + documentation

49f7695

musm approved these changes Aug 21, 2019

View reviewed changes

musm merged commit ac3570d into JuliaIO:master Aug 21, 2019

kleinhenz deleted the complex branch August 22, 2019 13:38

shoyer mentioned this pull request Sep 10, 2019

A convention for complex numbers? cf-convention/discuss#369

Closed

kleinhenz mentioned this pull request Oct 30, 2019

fix hyperslab support for complex datasets #591

Merged

kleinhenz mentioned this pull request Dec 6, 2019

write(fh,"A",A,"B",B) fails on 1.3.0 #599

Closed

roflmaostc mentioned this pull request Aug 20, 2021

Cannot mmap datasets of type ComplexF32 #863

Closed

olof3 mentioned this pull request Jan 2, 2024

Inconsistent writing of complex data inside compound type #1135

Open

complex number support #558

complex number support #558

Conversation

kleinhenz commented Jul 18, 2019

tknopp commented Jul 19, 2019

kleinhenz commented Jul 19, 2019

benchislett 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tknopp commented Aug 21, 2019 • edited Loading

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 Aug 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kleinhenz commented Aug 21, 2019

kleinhenz commented Aug 21, 2019

musm commented Aug 21, 2019

musm commented Aug 21, 2019

kleinhenz commented Aug 21, 2019

tknopp commented Aug 21, 2019

musm commented Aug 22, 2019

kleinhenz commented Aug 22, 2019

lazarusA commented Oct 30, 2019

kleinhenz commented Oct 30, 2019

lazarusA commented Oct 30, 2019 • edited Loading

kleinhenz commented Oct 30, 2019

lazarusA commented Oct 31, 2019

bhawkins commented Nov 14, 2019

musm commented Oct 15, 2020

kleinhenz commented Oct 17, 2020

musm commented Oct 17, 2020

tknopp commented Aug 21, 2019 •

edited

Loading

musm Aug 21, 2019 •

edited

Loading

lazarusA commented Oct 30, 2019 •

edited

Loading