Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate get_dims and set_dims! #798

Merged
merged 12 commits into from
Jan 13, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/src/api_bindings.md
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,7 @@ h5r_get_region
- [`h5s_is_regular_hyperslab`](@ref HDF5.h5s_is_regular_hyperslab)
- [`h5s_is_simple`](@ref HDF5.h5s_is_simple)
- [`h5s_select_hyperslab`](@ref HDF5.h5s_select_hyperslab)
- [`h5s_set_extent_simple`](@ref HDF5.h5s_set_extent_simple)
```@docs
h5s_close
h5s_combine_select
Expand All @@ -380,6 +381,7 @@ h5s_get_simple_extent_type
h5s_is_regular_hyperslab
h5s_is_simple
h5s_select_hyperslab
h5s_set_extent_simple
```

---
Expand Down
1 change: 1 addition & 0 deletions gen/api_defs.jl
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@
@bind h5s_is_regular_hyperslab(space_id::hid_t)::htri_t "Error determining whether datapace is regular hyperslab"
@bind h5s_is_simple(space_id::hid_t)::htri_t "Error determining whether dataspace is simple"
@bind h5s_select_hyperslab(dspace_id::hid_t, seloper::Cint, start::Ptr{hsize_t}, stride::Ptr{hsize_t}, count::Ptr{hsize_t}, block::Ptr{hsize_t})::herr_t "Error selecting hyperslab"
@bind h5s_set_extent_simple(dspace_id::hid_t, rank::Cint, current_size::Ptr{hsize_t}, maximum_size::Ptr{hsize_t})::herr_t "Error setting dataspace size"

###
### Datatype Interface
Expand Down
32 changes: 4 additions & 28 deletions src/HDF5.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1087,7 +1087,7 @@ end
Base.isempty(dspace::Union{Dataspace,Dataset,Attribute}) = length(dspace) == 0

"""
isnull(dspace::Union{Dataspace, Dataset, Attribute})
isnull(dspace::Union{HDF5.Dataspace, HDF5.Dataset, HDF5.Attribute})

Determines whether the given object has no size (consistent with the `H5S_NULL` dataspace).

Expand All @@ -1107,14 +1107,6 @@ function isnull(obj::Union{Dataspace,Dataset,Attribute})
return ret
end

function get_dims(dspace::Dataspace)
h5_dims, h5_maxdims = h5s_get_simple_extent_dims(dspace)
# reverse dimensions since hdf5 uses C-style order
N = length(h5_dims)
dims = ntuple(i -> @inbounds(Int(h5_dims[N-i+1])), N)
maxdims = ntuple(i -> @inbounds(h5_maxdims[N-i+1]) % Int, N) # allows max_dims to be specified as -1 without triggering an overflow
return dims, maxdims
end

function get_regular_hyperslab(dspace::Dataspace)
start, stride, count, block = h5s_get_regular_hyperslab(dspace)
Expand All @@ -1123,25 +1115,6 @@ function get_regular_hyperslab(dspace::Dataspace)
return rev(start), rev(stride), rev(count), rev(block)
end

"""
HDF5.get_dims(obj::Union{HDF5.Dataset, HDF5.Attribute})

Get the array dimensions from a dataset or attribute and return a tuple of dims and maxdims.
"""
function get_dims(dset::Union{Dataset,Attribute})
dspace = dataspace(dset)
ret = get_dims(dspace)
close(dspace)
return ret
end

"""
set_dims!(dset::HDF5.Dataset, new_dims::Dims)

Change the current dimensions of a dataset to `new_dims`, limited by
`max_dims = get_dims(dset)[2]`. Reduction is possible and leads to loss of truncated data.
"""
set_dims!(dset::Dataset, new_dims::Dims) = h5d_set_extent(checkvalid(dset), hsize_t[reverse(new_dims)...])

"""
start_swmr_write(h5::HDF5.File)
Expand Down Expand Up @@ -1752,6 +1725,9 @@ end
# end of high-level interface


include("api_midlevel.jl")


### HDF5 utilities ###

function get_jl_type(obj_type::Datatype)
Expand Down
11 changes: 11 additions & 0 deletions src/api.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1610,6 +1610,17 @@ function h5s_select_hyperslab(dspace_id, seloper, start, stride, count, block)
return nothing
end

"""
h5s_set_extent_simple(dspace_id::hid_t, rank::Cint, current_size::Ptr{hsize_t}, maximum_size::Ptr{hsize_t})

See `libhdf5` documentation for [`H5Sset_extent_simple`](https://portal.hdfgroup.org/display/HDF5/H5S_SET_EXTENT_SIMPLE).
"""
function h5s_set_extent_simple(dspace_id, rank, current_size, maximum_size)
var"#status#" = ccall((:H5Sset_extent_simple, libhdf5), herr_t, (hid_t, Cint, Ptr{hsize_t}, Ptr{hsize_t}), dspace_id, rank, current_size, maximum_size)
var"#status#" < 0 && error("Error setting dataspace size")
return nothing
end

"""
h5t_array_create(basetype_id::hid_t, ndims::Cuint, sz::Ptr{hsize_t}) -> hid_t

Expand Down
47 changes: 47 additions & 0 deletions src/api_midlevel.jl
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
# This file defines midlevel api wrappers. We include name normalization for methods that are
# applicable to different hdf5 api-layers. We still try to adhere close proximity to the underlying
# method name in the hdf5-library.

"""
HDF5.set_extent_dims(dset::HDF5.Dataset, new_dims::Dims)

Change the current dimensions of a dataset to `new_dims`, limited by
`max_dims = get_simple_extent_dims(dset)[2]`. Reduction is possible and leads to loss of truncated data.
musm marked this conversation as resolved.
Show resolved Hide resolved
"""
function set_extent_dims(dset::Dataset, size::Dims)
checkvalid(dset)
h5d_set_extent(dset, hsize_t[reverse(size)...])
end

"""
HDF5.set_extent_dims(dset::HDF5.Dataspace, new_dims::Dims, max_dims::Union{Dims,Nothing} = nothing)

Change the current dimensions of a dataspace `dset` to `new_dims`, limited by
`max_dims = get_simple_extent_dims(dset)[2]`. Reduction is possible and leads to loss of truncated data.
"""
musm marked this conversation as resolved.
Show resolved Hide resolved
function set_extent_dims(dspace::Dataspace, size::Dims, max_dims::Union{Dims,Nothing} = nothing)
checkvalid(obj)
rank = length(size)
current_size = hsize_t[reverse(new_dims)...]
maximum_size = isnothing(max_dims) ? C_NULL : hsize_t[reverse(max_dims)...]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
maximum_size = isnothing(max_dims) ? C_NULL : hsize_t[reverse(max_dims)...]
maximum_size = isnothing(max_dims) ? C_NULL : [reverse(max_dims .% hsize_t)...]

to support using -1 to set unlimited max size.

(Unfortunately, using max_dims::Dims makes this incompatible with unlimited sizes given as (HDF5.H5S_UNLIMITED,) ... not sure what to do about that, but it's not a regression from what set_dims! was doing already.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think this will take some more careful thinking on how to address setting H5S_UNLIMITED more broadly.

Copy link
Member Author

@musm musm Jan 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the simplest thing is to just document the correct way to set an unlimited dimension is to set it to -1 and not use the internal HDF5.H5S_UNLIMITED constant? A little more intrusive would be to set HDF5.H5S_UNLIMITED=-1, but I think that would require enforcing truncation to which ever inter api call excepts HDF5.H5S_UNLIMITED since cconvert to hsize_t would throw an error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it's mostly a documentation issue — low-level API will want HDF5.H5S_UNLIMITED (and should remain as -1 % hsize_t to agree with the C header literal value), and everything in mid-level and high-level API will instead use -1 and do the data type conversions. I think so far we just don't have the documentation to make that clear.

h5s_set_extent_simple(obj, rank, current_size, maximum_size)
close(obj)
return nothing
end


"""
HDF5.get_extent_dims(obj::Union{HDF5.Dataspace, HDF5.Dataset, HDF5.Attribute}) -> dims, maxdims

Get the array dimensions from a dataspace, dataset, or attribute and return a tuple of `dims` and `maxdims`.
"""
function get_extent_dims(obj::Union{Dataspace,Dataset,Attribute})
dspace = obj isa Dataspace ? checkvalid(obj) : dataspace(obj)
h5_dims, h5_maxdims = h5s_get_simple_extent_dims(dspace)
# reverse dimensions since hdf5 uses C-style order
N = length(h5_dims)
dims = ntuple(i -> @inbounds(Int(h5_dims[N-i+1])), N)
maxdims = ntuple(i -> @inbounds(h5_maxdims[N-i+1]) % Int, N) # allows max_dims to be specified as -1 without triggering an overflow
obj isa Dataspace || close(dspace)
musm marked this conversation as resolved.
Show resolved Hide resolved
return dims, maxdims
end
4 changes: 4 additions & 0 deletions src/deprecated.jl
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,7 @@ function exists end
### Changed in PR#776
@deprecate create_dataset(parent::Union{File,Group}, path::AbstractString, dtype::Datatype, dspace::Dataspace,
lcpl::Properties, dcpl::Properties, dapl::Properties, dxpl::Properties) HDF5.Dataset(HDF5.h5d_create(parent, path, dtype, dspace, lcpl, dcpl, dapl), HDF5.file(parent), dxpl) false

### Changed in PR#798
@deprecate get_dims(dspace::Union{Dataspace,Dataset,Attribute}) get_extent_dims(dspace) false
@deprecate set_dims!(dset::Dataspace) set_extent_dims(dset) false
2 changes: 1 addition & 1 deletion src/show.jl
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ function Base.show(io::IO, dspace::Dataspace)
return
end
# otherwise type == H5S_SIMPLE
sz, maxsz = get_dims(dspace)
sz, maxsz = get_extent_dims(dspace)
sel = h5s_get_select_type(dspace)
if sel == H5S_SEL_HYPERSLABS && h5s_is_regular_hyperslab(dspace)
start, stride, count, _ = get_regular_hyperslab(dspace)
Expand Down
14 changes: 7 additions & 7 deletions test/dataspace.jl
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,13 @@ using Test
@test !HDF5.isnull(ds_zerosz)
@test !HDF5.isnull(ds_vector)

@test HDF5.get_dims(ds_null) === ((), ())
@test HDF5.get_dims(ds_scalar) === ((), ())
@test HDF5.get_dims(ds_zerosz) === ((0,), (0,))
@test HDF5.get_dims(ds_vector) === ((5,), (5,))
@test HDF5.get_dims(ds_matrix) === ((5, 7), (5, 7))
@test HDF5.get_dims(ds_maxdim) === ((5, 7), (20, 20))
@test HDF5.get_dims(ds_unlim) === ((1,), (-1,))
@test HDF5.get_extent_dims(ds_null) === ((), ())
@test HDF5.get_extent_dims(ds_scalar) === ((), ())
@test HDF5.get_extent_dims(ds_zerosz) === ((0,), (0,))
@test HDF5.get_extent_dims(ds_vector) === ((5,), (5,))
@test HDF5.get_extent_dims(ds_matrix) === ((5, 7), (5, 7))
@test HDF5.get_extent_dims(ds_maxdim) === ((5, 7), (20, 20))
@test HDF5.get_extent_dims(ds_unlim) === ((1,), (-1,))

# Can create new copies
ds_tmp = copy(ds_maxdim)
Expand Down
30 changes: 15 additions & 15 deletions test/extend_test.jl
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,16 @@ fn = tempname()
fid = h5open(fn, "w")
g = create_group(fid, "shoe")
d = create_dataset(g, "foo", datatype(Float64), ((10, 20), (100, 200)), chunk=(1, 1))
#println("d is size current $(map(int,HDF5.get_dims(d)[1])) max $(map(int,HDF5.get_dims(d)[2]))")
dims, max_dims = HDF5.get_dims(d)
#println("d is size current $(map(int,HDF5.get_extent_dims(d)[1])) max $(map(int,HDF5.get_extent_dims(d)[2]))")
dims, max_dims = HDF5.get_extent_dims(d)
@test dims == (UInt64(10), UInt64(20))
@test max_dims == (UInt64(100), UInt64(200))
HDF5.set_dims!(d, (100, 150))
dims, max_dims = HDF5.get_dims(d)
HDF5.set_extent_dims(d, (100, 150))
dims, max_dims = HDF5.get_extent_dims(d)
@test dims == (UInt64(100), UInt64(150))
@test max_dims == (UInt64(100), UInt64(200))
d[1, 1:5] = [1.1231, 1.313, 5.123, 2.231, 4.1231]
HDF5.set_dims!(d, (1, 5))
HDF5.set_extent_dims(d, (1, 5))
@test size(d) == (1, 5)

# Indexing returns correct array dimensions
Expand All @@ -39,18 +39,18 @@ HDF5.set_dims!(d, (1, 5))
# Test Array constructor
Array(d) == [1.1231 1.313 5.123 2.231 4.1231]

#println("d is size current $(map(int,HDF5.get_dims(d)[1])) max $(map(int,HDF5.get_dims(d)[2]))")
#println("d is size current $(map(int,HDF5.get_extent_dims(d)[1])) max $(map(int,HDF5.get_extent_dims(d)[2]))")
b = create_dataset(fid, "b", Int, ((1000,), (-1,)), chunk=(100,)) #-1 is equivalent to typemax(hsize_t) as far as I can tell
#println("b is size current $(map(int,HDF5.get_dims(b)[1])) max $(map(int,HDF5.get_dims(b)[2]))")
#println("b is size current $(map(int,HDF5.get_extent_dims(b)[1])) max $(map(int,HDF5.get_extent_dims(b)[2]))")
b[1:200] = ones(200)
dims, max_dims = HDF5.get_dims(b)
dims, max_dims = HDF5.get_extent_dims(b)
@test dims == (UInt64(1000),)
@test max_dims == (HDF5.H5S_UNLIMITED % Int,)
HDF5.set_dims!(b, (10000,))
dims, max_dims = HDF5.get_dims(b)
HDF5.set_extent_dims(b, (10000,))
dims, max_dims = HDF5.get_extent_dims(b)
@test dims == (UInt64(10000),)
@test max_dims == (HDF5.H5S_UNLIMITED % Int,)
#println("b is size current $(map(int,HDF5.get_dims(b)[1])) max $(map(int,HDF5.get_dims(b)[2]))")
#println("b is size current $(map(int,HDF5.get_extent_dims(b)[1])) max $(map(int,HDF5.get_extent_dims(b)[2]))")
# b[:] = [1:10000] # gave error no method lastindex(HDF5.Dataset{PlainHDF5File},),
# so I defined lastindex(dset::HDF5.Dataset) = length(dset), and exported lastindex
# but that didn't fix the error, despite the lastindex function working
Expand All @@ -62,17 +62,17 @@ close(fid)

fid = h5open(fn, "r")
d_again = fid["shoe/foo"]
dims, max_dims = HDF5.get_dims(d_again)
dims, max_dims = HDF5.get_extent_dims(d_again)
@test dims == (UInt64(1), UInt64(5))
@test max_dims == (UInt64(100), UInt64(200))
@test (sum(d_again[1, 1:5]) - sum([1.1231, 1.313, 5.123, 2.231, 4.1231])) == 0
#println("d is size current $(map(int,HDF5.get_dims(re_d)[1])) max $(map(int,HDF5.get_dims(re_d)[2]))")
#println("d is size current $(map(int,HDF5.get_extent_dims(re_d)[1])) max $(map(int,HDF5.get_extent_dims(re_d)[2]))")
@test fid["b"][1:10000] == [1:10000;]
b_again = fid["b"]
dims, max_dims = HDF5.get_dims(b_again)
dims, max_dims = HDF5.get_extent_dims(b_again)
@test dims == (UInt64(10000),)
@test max_dims == (HDF5.H5S_UNLIMITED % Int,)
#println("b is size current $(map(int,HDF5.get_dims(b)[1])) max $(map(int,HDF5.get_dims(b)[2]))")
#println("b is size current $(map(int,HDF5.get_extent_dims(b)[1])) max $(map(int,HDF5.get_extent_dims(b)[2]))")

close(fid)
rm(fn)
Expand Down
2 changes: 1 addition & 1 deletion test/swmr.jl
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ end
function dataset_write(d, ch_written, ch_read)
for i = 1:10
@assert take!(ch_read) == true
HDF5.set_dims!(d, (i*10,))
HDF5.set_extent_dims(d, (i*10,))
inds::UnitRange{Int} = (1:10) .+ (i - 1) * 10
d[inds] = inds
flush(d) # flush the dataset
Expand Down