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

Deprecate get_dims and set_dims! #798

merged 12 commits into from
Jan 13, 2021

Conversation

musm
Copy link
Member

@musm musm commented Jan 5, 2021

Base.size is what people will likely want instead of these methods.

I figured it would be a good idea to either remove these methods or rename them. Internally, we don't use them after upgrading Base.size

@jmert thoughts on this change (also open to other suggestions you might have in leu of these changes)

src/deprecated.jl Outdated Show resolved Hide resolved
src/HDF5.jl Outdated Show resolved Hide resolved
@musm
Copy link
Member Author

musm commented Jan 8, 2021

we might want to move these to api_helpers ?

@jmert
Copy link
Contributor

jmert commented Jan 10, 2021

I agree with the general idea, but a few thoughts:

  1. The asymmetry in get_simple_extent_dims versus set_extent seems off to me. I know the h5d_set_extent interface is what's being used, but there's also the h5s_set_extent_simple which is closer (though still annoyingly swaps the order of "simple" and "extent"...). Maybe we should just provide both setters (first acting on dataspace and second acting on actual datasets)? As mid-level routines, we could choose to normalize naming a bit, too — provide get_simple_extent_dims and set_simple_extent_dims for the dataspace operations?
  2. I'd say rather than moving to api_helpers.jl which right now is pretty focused on just low-level wrappings of the low-level api, a new midlevel.jl (name TBD) would be nice — I'd still like to see src/HDF5.jl broken up more to more clearly separate the high-level interface from the mid-level interface, so maybe this will just be the first set of functions to make the move. (?)

@musm
Copy link
Member Author

musm commented Jan 12, 2021

@jmert Updated the PR as per comment. Is that what you were more thinking of?

src/api_midlevel.jl Outdated Show resolved Hide resolved
src/api_midlevel.jl Outdated Show resolved Hide resolved
checkvalid(dspace)
rank = length(size)
current_size = hsize_t[reverse(size)...]
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.

test/extend_test.jl Outdated Show resolved Hide resolved
test/extend_test.jl Outdated Show resolved Hide resolved
@jmert
Copy link
Contributor

jmert commented Jan 13, 2021

@jmert Updated the PR as per comment. Is that what you were more thinking of?

Yeah, this is the direction I was thinking of. Just some minor comments, but I agree with the approach as a whole.

@musm musm merged commit ef192dc into JuliaIO:master Jan 13, 2021
@musm musm deleted the dims branch January 13, 2021 04:17
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.

2 participants