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

Add dset_no_attrs_hint support, h5f coverage up to v1.12 #944

Merged
merged 7 commits into from
May 31, 2022

Conversation

mkitti
Copy link
Member

@mkitti mkitti commented May 29, 2022

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 a no_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:

julia> h5open("test.h5", "w"; libver_bounds = :latest, meta_block_size = 294) do f
           HDF5.API.h5f_set_dset_no_attrs_hint(f, true)
           f["test"] = 0x1
           f["test"] |> HDF5.API.h5d_get_offset |> Int
       end
294

julia> filesize("test.h5")
295

julia> h5open("test.h5", "w"; libver_bounds = :latest, meta_block_size = 294) do f
           HDF5.API.h5f_set_dset_no_attrs_hint(f, false)
           f["test"] = 0x1
           f["test"] |> HDF5.API.h5d_get_offset |> Int
       end
479

julia> filesize("test.h5")
480

@mkitti
Copy link
Member Author

mkitti commented May 29, 2022

Only failure is codecov on functions.jl since I just added a bunch of bindings from LibHDF5.jl

@simonbyrne
Copy link
Collaborator

Looks like also a failure using the system hdf5 (which uses 1.10 I think)

@mkitti
Copy link
Member Author

mkitti commented May 29, 2022

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?

@simonbyrne
Copy link
Collaborator

Isn't this the same information as https://portal.hdfgroup.org/display/HDF5/H5F_GET_DSET_NO_ATTRS_HINT?

@mkitti
Copy link
Member Author

mkitti commented May 29, 2022

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.

@mkitti
Copy link
Member Author

mkitti commented May 29, 2022

Looks like also a failure using the system hdf5 (which uses 1.10 I think)

I fixed this. The remaining check fail is now truly code coverage.

@mkitti
Copy link
Member Author

mkitti commented May 29, 2022

I broke and fixed this again. Main fail is due to codecov on src/api/functions.jl since I added the h5f bindings.

@@ -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 ?
Copy link
Collaborator

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

Copy link
Member Author

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
Copy link
Member

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

Copy link
Member

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 ?

Copy link
Member Author

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)
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

test/fileio.jl Outdated Show resolved Hide resolved
Co-authored-by: Mustafa M <[email protected]>
@musm musm merged commit 6dd3b56 into master May 31, 2022
@musm musm deleted the mkitti/dset_no_attrs_hint branch May 31, 2022 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants