JuliaIO / HDF5.jl

Save and load data in the HDF5 file format from Julia
https://juliaio.github.io/HDF5.jl
MIT License
386 stars 140 forks source link

Make `begin` as the first index of a `Dataset` work #1027

Closed machakann closed 1 year ago

machakann commented 1 year ago

Consider that d is a Dataset with two dimensions. Currently, d[:, end] works, however, d[:, begin] doesn't. This PR intended to make the latter work.

using HDF5
fn = tempname()
fid = h5open(fn, "w")
g = create_group(fid, "foo")
d = create_dataset(g, "bar", Int, (1, 3))
d[1, 1:3] = 1:3

@show d[1, end]  # OK
@show d[1, begin] # Error

The error message is:

MethodError: no method matching axes(::HDF5.Dataset, ::Int64)
Closest candidates are:
  axes(::Base.Broadcast.Broadcasted{<:Any, <:Tuple{Vararg{T, N}} where T}, ::Integer) where N at broadcast.jl:227
  axes(::Number, ::Integer) at number.jl:83
  axes(::AbstractArray{T, N}, ::Any) where {T, N} at abstractarray.jl:72
  ...

Stacktrace:
 [1] firstindex(a::HDF5.Dataset, d::Int64)
   @ Base .\abstractarray.jl:402
 [2] top-level scope
   @ show.jl:1047
 [3] eval
   @ .\boot.jl:368 [inlined]
 [4] include_string(mapexpr::typeof(REPL.softscope), mod::Module, code::String, filename::String)
   @ Base .\loading.jl:1428
machakann commented 1 year ago

Additionally, this is optional but the definition of lastindex is no longer needed, probably. Base.lastindex also uses axes to work.

https://github.com/JuliaIO/HDF5.jl/blob/205518d6c1d09dc8586b577d88a2f3633a9ea061/src/datasets.jl#L134-L135

musm commented 1 year ago

Julia 1.0/1.3 have test failures

┌ Warning: Deprecated syntax `"begin" inside indexing expression` at /home/runner/work/HDF5.jl/HDF5.jl/test/extend_test.jl:26.

might need to version out the tests and or changes.

musm commented 1 year ago

Additionally, this is optional but the definition of lastindex is no longer needed, probably. Base.lastindex also uses axes to work.

If we don't need them, better to remove them for simplicity than cary uneccessary definitions.

machakann commented 1 year ago

Thank you for your comments. I will work on it. It seems I still need some tweaks for tests.

I found that the test failed without the two lines of lastindex in julia v1.4.2 and v1.5.4. So, the two lines should be kept for v1.4 and v1.5. Probably, https://github.com/JuliaLang/julia/pull/38742 is required to remove them. I will just add a comment on it. (begin just works without explicit firstindex definitions.)

musm commented 1 year ago

Does the @static need to be pulled out of the testset ? Seems like it's not being respected

machakann commented 1 year ago

Finally, all tests passed. Since begin is interpreted as a begin-end block in v1.3, I separate the file to avoid LoadError.

musm commented 1 year ago

Great thanks!