-
Notifications
You must be signed in to change notification settings - Fork 140
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
Add dset_no_attrs_hint support, h5f coverage up to v1.12 #944
Conversation
Only failure is codecov on functions.jl since I just added a bunch of bindings from LibHDF5.jl |
Looks like also a failure using the system hdf5 (which uses 1.10 I think) |
Good point. We need to gate at 1.10.5 https://docs.hdfgroup.org/hdf5/v1_12/group___h5_f.html#gacbf3ba8b36750c42b49740567a9732c4 Also does anyone know how to make doxygen URLs more readable? |
Isn't this the same information as https://portal.hdfgroup.org/display/HDF5/H5F_GET_DSET_NO_ATTRS_HINT? |
It is, I'm just worried that the newer doxygen based docs will replace the older docs at some point. Doxygen is geared more towards C++ though so they hash the method name and the argument types. |
I fixed this. The remaining check fail is now truly code coverage. |
I broke and fixed this again. Main fail is due to codecov on |
@@ -496,6 +497,11 @@ function class_getproperty(::Type{DatasetCreateProperties}, p::Properties, name: | |||
name === :external ? API.h5p_get_external(p) : | |||
name === :filters ? get_filters(p) : | |||
name === :layout ? get_layout(p) : | |||
name === :no_attrs_hint ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add to the docstring for DatasetCreateProperties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in a523356
test/fileio.jl
Outdated
HDF5.API.h5f_set_dset_no_attrs_hint(f, false) | ||
@test !HDF5.API.h5f_get_dset_no_attrs_hint(f) | ||
f["test"] = 0x1 | ||
# We expect that with the hint, the offset will actually be 300 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment needs to change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you go about guessing the size ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I first figured out what the minimum size would be, and picked a round number greater than that.
https://twitter.com/markkitti/status/1530811272966586368?t=kyNK_e2Gwr87Xuv9mhYVog&s=19
@@ -169,37 +169,97 @@ h5e_walk | |||
--- | |||
|
|||
## [[`H5F`](https://portal.hdfgroup.org/display/HDF5/Files) — File Interface](@id H5F) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably these were all determined using the generator you have developed? Thus I haven't reviewed them for correctness, since I think the generator has been working without issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes they are coming from https://github.com/mkitti/LibHDF5.jl/blob/712b6e306a15de37f748727b37676aca70ea0664/src/bind.jl
That is generated by this code:
https://github.com/mkitti/LibHDF5.jl/blob/main/gen/bind_generator.jl
Co-authored-by: Mustafa M <[email protected]>
The primary focus of this pull request is to add
HDF5.API.h5f_get_dset_no_attrs_hint
HDF5.API.h5f_set_dset_no_attrs_hint
This pull request adds helpers for
HDF5.API.h5f_get_dset_no_attrs_hint
HDF5.API.h5p_get_dset_no_attrs_hint
The
DatasetCreateProperties
gains ano_attrs_hint
property.Increased support is added for H5F* functions in general, covering known functions up to HDF5 v1.12.
Apart of that effort, this pull request converts
H5F_libver_*
into an enum from a series of constants.A practical benefit of the
dset_no_attrs_hint
enhancements is a smaller HDF5 file with data: