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

write(fh,"A",A,"B",B) fails on 1.3.0 #599

Closed
PaulSoderlind opened this issue Nov 28, 2019 · 6 comments · Fixed by #631
Closed

write(fh,"A",A,"B",B) fails on 1.3.0 #599

PaulSoderlind opened this issue Nov 28, 2019 · 6 comments · Fixed by #631

Comments

@PaulSoderlind
Copy link

PaulSoderlind commented Nov 28, 2019

but it works in 1.2.0. In contrast,

  write(fh,"A",A)
  write(fh,"B",B)

still works.

using HDF5

A = copy(reshape(1:120,15,8))
B = 1

fh = h5open("test1.h5","w")           #works
  write(fh,"A",A)
  write(fh,"B",B)
close(fh)

fh = h5open("test2.h5","w")           #fails
  write(fh,"A",A,"B",B)
close(fh)

Edit: on Windows 10 (64)

@musm
Copy link
Member

musm commented Dec 5, 2019

sigh this was broken by https://github.com/JuliaIO/HDF5.jl/pull/558/files

I tried to see if there was a simple fix, but it's not so simple with the current API...

@musm
Copy link
Member

musm commented Dec 6, 2019

I'm not sure if @kleinhenz has some thoughts on this. It's pretty tricky to try to fix with how the api is currently setup.

@kleinhenz
Copy link
Contributor

kleinhenz commented Dec 6, 2019

Just to be sure we're talking about the same thing the error I get is

ERROR: LoadError: MethodError: write(::HDF5File, ::String, ::Array{Int64,2}, ::String, ::Int64) is ambiguous. Candidates:
  write(parent::Union{HDF5File, HDF5Group}, name1::String, val1, name2::String, val2, nameval...) in HDF5 at /Users/Joseph/Documents/code/spike/julia/hdf5/HDF5.jl/src/HDF5.jl:1662
  write(parent::Union{HDF5File, HDF5Group}, name::String, data::Union{AbstractArray{T,N} where N, T}, plists...) where T<:Union{Bool, Float32, Float64, Int16, Int32, Int64, Int8, UInt16, UInt32, UInt64, UInt8, HDF5.HDF5ReferenceObj, String, Complex{#s31} where #s31<:Union{Bool, Float32, Float64, Int16, Int32, Int64, Int8, UInt16, UInt32, UInt64, UInt8, HDF5.HDF5ReferenceObj}} in HDF5 at /Users/Joseph/Documents/code/spike/julia/hdf5/HDF5.jl/src/HDF5.jl:1750
Possible fix, define
  write(::Union{HDF5File, HDF5Group}, ::String, ::Any, ::String, ::Any, ::Vararg{Any,N} where N)

I don't think it's #558. I can reproduce at v0.12.1 (the tag before #558 was merged).

foo(name1::String, val1, name2::String, val2, nameval...) = println("foo1")
foo(name::String, data::Union{AbstractArray{T}, T}, plists...) where {T <: Number} = println("foo2")
foo("A", 1, "B", 2)

reproduces the change in behavior between 1.2 and 1.3 for me (1.2 prints foo1, 1.3 raises method ambiguity error). I'm not an expert on how the type system works so I'm not sure exactly what to make of this. Is this a regression in 1.3? Are method specificity rules expected to be stable? I guess in 1.2 exactly matching name1 and name2 'won' but now it is a tie with exactly matching data.
I agree, it's hard to see how to fix this within the current api since we use vararg name value lists for property lists and specifying datasets so in some sense the call really is ambiguous.

@kleinhenz
Copy link
Contributor

kleinhenz commented Dec 6, 2019

what about

function write(parent::Union{HDF5File, HDF5Group}, data::Vararg{Pair{String, T}}) where T
    for (name, val) in data
      write(parent, name, val)
    end
end

for the generic write function?

Then the call would be write(fh,"A"=>A,"B"=>B) and there would be no ambiguity.

The other option would be to have a wrapper type for the property lists instead of taking plist... directly, but that seems probably more disruptive.

@musm
Copy link
Member

musm commented Dec 6, 2019

apologies @kleinhenz I didn't mean to single your PR out. Clearly this wasn't related to that PR, and I mistakenly thought it did. Your right, method specificity rules seemed to have changed between 1.2 and 1.3 (this was probably intentionally to fix 'other bugs').

@musm
Copy link
Member

musm commented Dec 6, 2019

The other option would be to have a wrapper type for the property lists instead of taking plist... directly, but that seems probably more disruptive.

Yeah your two options were also the only ways I could think up to fix this issue.

write(fh,"A"=>A,"B"=>B) is certainly the simplest option.

wrapping plist is more disruptive, but probably more 'elegant'

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 a pull request may close this issue.

3 participants