JuliaIO / HDF5.jl

Save and load data in the HDF5 file format from Julia
https://juliaio.github.io/HDF5.jl
MIT License
387 stars 140 forks source link

Initialize properties on get. #993

Closed mkitti closed 2 years ago

mkitti commented 2 years ago

Also add H5P_ATTRIBUTE_ACCESS constant.

mkitti commented 2 years ago

Currently, trying to access a property of a property list without calling HDF5.init! will fail with an error.

julia> using HDF5

julia> dcpl = HDF5.DatasetCreateProperties()
HDF5.DatasetCreateProperties()

julia> HDF5.API.SHORT_ERROR[] = false
false

julia> dcpl.alloc_time
ERROR: HDF5.API.H5Error: Error getting allocation timing
libhdf5 Stacktrace:
 [1] H5P_isa_class: Invalid arguments to routine/Inappropriate type
     not a property list
 [2] H5P_object_verify: Property lists/Unable to register new atom
     property list is not a member of the class
 [3] H5Pget_alloc_time: Object atom/Unable to find atom information (already closed?)
     can't find object for ID
Stacktrace:
 [1] macro expansion
   @ C:\Users\kittisopikulm\.julia\packages\HDF5\7zvRl\src\api\error.jl:18 [inlined]
 [2] h5p_get_alloc_time(plist_id::HDF5.DatasetCreateProperties, alloc_time::Base.RefValue{Int32})
   @ HDF5.API C:\Users\kittisopikulm\.julia\packages\HDF5\7zvRl\src\api\functions.jl:1579
 [3] h5p_get_alloc_time(plist_id::HDF5.DatasetCreateProperties)
   @ HDF5.API C:\Users\kittisopikulm\.julia\packages\HDF5\7zvRl\src\api\helpers.jl:425
 [4] get_alloc_time(p::HDF5.DatasetCreateProperties)
   @ HDF5 C:\Users\kittisopikulm\.julia\packages\HDF5\7zvRl\src\properties.jl:185
 [5] class_getproperty(#unused#::Type{HDF5.DatasetCreateProperties}, p::HDF5.DatasetCreateProperties, name::Symbol)
   @ HDF5 C:\Users\kittisopikulm\.julia\packages\HDF5\7zvRl\src\properties.jl:498
 [6] getproperty(p::HDF5.DatasetCreateProperties, name::Symbol)
   @ HDF5 C:\Users\kittisopikulm\.julia\packages\HDF5\7zvRl\src\properties.jl:38
 [7] top-level scope
   @ REPL[28]:1

This change will ensure that property lists are initialized when someone tries to retrieve a property.

julia> using HDF5
[ Info: Precompiling HDF5 [f67ccb44-e63f-5c2f-98bd-6dc0ccc4ba2f]

julia> dcpl = HDF5.DatasetCreateProperties()
HDF5.DatasetCreateProperties()

julia> dcpl.id
0

julia> dcpl.alloc_time
:late

julia> dcpl.id
792633534417207321

By incorporating init! into getproperty we do introduce a isvalid check on each property access call. https://github.com/JuliaIO/HDF5.jl/blob/df76df6fcdf53f774134012f83357506ad060775/src/properties.jl#L22-L27

mkitti commented 2 years ago

I also noticed that we were missing the H5P_ATTRIBUTE_ACCESS property constant value.

mkitti commented 2 years ago

A shorter version of isvalid would be to check if the id field is -1 or 0 rather than possibly calling HDF5.API.h5i_is_valid on each check.

Given that this is a high-level API, I think the additional auto-initialization is worth while. Performance sensitive applications can use the mid-level or low-level API to get around such checks.

musm commented 2 years ago

I don't think the isvalid is that big an issue. Correctness is certainly preferred, plus we have those isvalid checks throughout the codebase in most accessors.