Tuple support #1060
Merged
Tuple support #1060
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mkitti
reviewed
Apr 25, 2023
src/typeconversions.jl
Outdated
| @@ -64,14 +64,15 @@ end | |||
|
|
|||
| # These will use finalizers. Close them eagerly to avoid issues. | |||
| datatype(::T) where {T} = Datatype(hdf5_type_id(T), true) | |||
| datatype(T::Type) = Datatype(hdf5_type_id(T), true) | |||
There was a problem hiding this comment.
I believe we actually want this.
Suggested change
| datatype(T::Type) = Datatype(hdf5_type_id(T), true) | |
| datatype(::Type{T}) where T = Datatype(hdf5_type_id(T), true) |
There was a problem hiding this comment.
Yes, I agree that your change fixes several issues
mkitti
reviewed
Apr 25, 2023
| datatype(x::AbstractArray{T}) where {T} = Datatype(hdf5_type_id(T), true) | ||
|
|
||
| hdf5_type_id(::Type{T}) where {T} = hdf5_type_id(T, Val(isstructtype(T))) | ||
| function hdf5_type_id(::Type{T}, isstruct::Val{true}) where {T} | ||
| dtype = API.h5t_create(API.H5T_COMPOUND, sizeof(T)) | ||
| for (idx, fn) in enumerate(fieldnames(T)) | ||
| ftype = fieldtype(T, idx) | ||
| API.h5t_insert(dtype, fn, fieldoffset(T, idx), hdf5_type_id(ftype)) | ||
| API.h5t_insert(dtype, Symbol(fn), fieldoffset(T, idx), hdf5_type_id(ftype)) |
There was a problem hiding this comment.
This is the line that adds support for tuples to be saved as hdf5 compound data types. The field names of tuples are integers whereas other structs have field names that are symbols, which is why this conversion is necessary.
|
I'm going to try to get this formatted and merged soon to do a release. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since there is already support for reading/writing compound types, I wanted to try adding support for
Tuples as well, since sometimes saving tuple data is convenient, or compound types have tuple data (e.g.SArray). This pr writes tuples to compound types with member names corresponding to the indices of each element, i.e.var"1", var"2", var"3",.... I understand that there is no native HDF5 type corresponding to a tuple, however this pr is non-breaking and since tuples will be read back as named tuples, which inherit the majority of behavior from tuples, this only provides additional features. Please advise.I also made the following changes:
dset[i] = eforea compound type by loosening method signaturesdatatype(::Type{T})so thatcreate_dataset(parent, path, T, dataspace)works for compound typesTI also prepared a follow-up pr showing that we can save StaticArrays as compound types (https://github.com/lxvm/HDF5.jl/tree/pr_sarray), although one method would change in a breaking way.