Re-haul and implement a more generic read

[kleinhenz]

This adds a new generic (not type constrained) read method

read(obj::DatasetOrAttribute, ::Type{T}, I...) where T

which reads a hyperslab of a dataset or an attribute into an array of type T.

It also adds convenience methods

read(parent::Union{HDF5File, HDF5Group}, name_type_pair::Pair{String, DataType}; pv...)
h5read(filename, name_type_pair::Pair{String, DataType}; pv...)

which can be used like v_read = read(h5f, "data"=>Simple) or v_read = h5read(fn, "data"=>Simple) to read
data into an array with element type Simple (see test/custom.jl). This provides the functionality of #559 in a way that is more integrated with the rest of the library than adding a new separate h5read_compound method and closes #169.

Although the function does not have a type constraint I do check that the type is the correct size so this should still prevent you from shooting yourself in the foot too easily. The behavior of the generic read can be customized by defining do_reclaim, do_normalize and normalize_types on your custom type. If we decide to do this, I will write up some documentation for customization.

Internally, I was able to replace a bunch of separate read methods for different types with this one generic method. This works by separating reading from hdf5 into a memory compatible datatype from mapping that data into a native julia type. The first part can be handled generically (HDF5Scalar types are not special) and the second part can be handled by the normalize_types methods which we have been using with compound types and can be extended. I think this is a win in terms of simplification and reducing the number of lines of code. The only type that requires special handling is now HDF5Opaque.

As a side-effect of this hyperslab indexing works generically for everything besides HDF5Opaque. Closes #625.
As another side-effect, vlen strings are now handled uniformly via unsafe_string so this should hopefully close #627.

The rules for mapping from hdf5 to julia types now uniformly follow the conventions we have been using for reading compound types which shouldn't be too breaking but does cause a few changes in behavior. In particular the data

HDF5 "output.h5" {
DATASET "/salut_vlen" {
   DATATYPE  H5T_VLEN { H5T_STRING {
      STRSIZE 1;
      STRPAD H5T_STR_NULLTERM;
      CSET H5T_CSET_ASCII;
      CTYPE H5T_C_S1;
   }}
   DATASPACE  SIMPLE { ( 2 ) / ( 2 ) }
   DATA {
   (0): ("H", "i"), ("t", "h", "e", "r", "e")
   }
}
}

is now read as salut_vlenr = [["H", "i"], ["t", "h", "e", "r", "e"]] instead of ["Hi", "There"]. However, I think the new behavior is actually a more correct interpretation of the datatype since STRSIZE=1. Also now hdf5 strings are read into julia strings even if they are length 1 whereas previously these would have been read in character types. This could be changed if people think that it is important. Other than the salut_vlenr example all other tests pass without modification.

[kleinhenz]

@tknopp, @tamasgal would this work for your usecases?

[tamasgal]

Looks good to me!

[musm]

As a side-effect of this hyperslab indexing works generically for everything besides HDF5Opaque. Closes #625.
As another side-effect, vlen strings are now handled uniformly via unsafe_string so this should hopefully close #627.

👍

is now read as salut_vlenr = [["H", "i"], ["t", "h", "e", "r", "e"]] instead of ["Hi", "There"]. However, I think the new behavior is actually a more correct interpretation of the datatype since STRSIZE=1. Also now hdf5 strings are read into julia strings even if they are length 1 whereas previously these would have been read in character types. This could be changed if people think that it is important.

I don't have a strong opinion whether it should a string or character, but I tend to agree with you. Perhaps there is some efficiency lost by it not being a character, when STRSIZE=1. But wouldn't that mean we would need to special case STRSIZE=1, which seems cumbersome/inelegant.

[musm]

Not sure I understand the point of this method.

The hdf5_to_julia_eltype method
already has defined class_id == H5T_OPAQUE

HDF5.jl/src/HDF5.jl

Line 1808 in ece0dcd

elseif class_id == H5T_OPAQUE

So it seems that without this method things should just work already.

Edit: NVM that's for hdf5_to_julia_eltype

[kleinhenz]

Yeah this mostly duplicates hdf5_to_julia_eltype but changes the string handling part so there are no special cases for the length=1 case. I wrote a new function instead of changing the original because I wanted to only touch the read part of the library. There is probably some consolidation/cleanup possible.

[musm]

maybe add a comment that this is temporarily special cased for H5T_OPAQUE

[musm]

massive improvement in simplicity and readability.

[musm]

feel free to merge whenever

[musm]

What should we do with the UTF8Char and ASCIIChar types

[kleinhenz]

Cool, if you are overall happy with this approach I can clean things up a bit and write some documentation. I left some stuff that can probably be removed like the hdf5_to_julia_eltype function which mostly but not quite duplicates get_jl_type because I was trying to only touch the read part of the library and some of that is used in the write part. However there is probably at least some cleanup/consolidation possible.

I was actually wrong about the string reading behavior. Fixed length strings with length 1 are already read as strings not characters. The only place where the character types are actually used to change behavior is for reading H5T_VLEN datatypes with fixed string element types. In that case, if STRSIZE=1 then the data is read as an array of strings instead of an array of arrays of characters. This is what is going on in the salut_vlenr example. We could replicate the current behavior by adding a method for normalize_types(x::VariableArray{FixedString}). This is a fairly weird example though since the natural thing to do would be to just use a variable length string type. I need to check if the character types are important for any of the write methods but otherwise I think they can be removed.

[musm]

Cool, if you are overall happy with this approach I can clean things up a bit and write some documentation. I left some stuff that can probably be removed like the hdf5_to_julia_eltype function which mostly but not quite duplicates get_jl_type because I was trying to only touch the read part of the library and some of that is used in the write part. However there is probably at least some cleanup/consolidation possible.

Right now I think is the appropriate time to make such changes in the upcoming breaking release. It would certainly be good to clean up as much of the old cruft and any ugly parts of the code that we can, so I'm in support.

[tknopp]

@kleinhenz: I have switched to the regular compound reading with named tuples: MagneticResonanceImaging/MRIReco.jl@9b1a219

works fine although there was some drop in performance.

[musm]

@kleinhenz LMK if this is g2g, I'm happy with the changes here.

[kleinhenz]

yeah I think this is good to go. There is some more cleanup of internals possible but it is only sort of related to the changes here and I don't want to bloat this pull request too much.

[musm]

#739