Better way of saving Dicts with non-Symbol keys - fixes #3

[DilumAluthge]

This is a better way of fixing #3 that isn't hacky.

[DilumAluthge]

cc: @MikeInnes

[DilumAluthge]

Bump @MikeInnes

[aviks]

I can confirm that this fixes the issue around writing Dicts.

[aviks]

Unfortunately, even with this, Dicts do not read back correctly. The keys are visible, but the indexing fails. Which probably means the hashes are incorrect.

[DilumAluthge]

Hmmmm can you post an example where the Dicts do not read back correctly? They do in the tests.

On Thu, Jun 21, 2018 at 19:36 Avik Sengupta ***@***.***> wrote: Unfortunately, even with this, Dicts do not read back correctly. The keys are visible, but the indexing fails. Which probably means the hashes are incorrect. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#7 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFXArXPRcLCJV96pbYPn38SYbNECcBnYks5t_C4TgaJpZM4UV2PW> .

-- *Dilum Aluthge* *[email protected] <[email protected]>* *https://www.aluthge.com <https://www.aluthge.com>*

[aviks]

If you download the file at https://github.com/JuliaText/TextAnalysis.jl/releases/download/v0.3.0/sentiment.tar.gz (15.4MB) and unzip it, you'll find a file called model/sentiment-analysis-weights.bson

julia> w = BSON.load("model/sentiment-analysis-weights.bson")
Dict{Symbol,Any} with 4 entries:
  :dense_1     => Dict{String,Any}(Pair{String,Any}("dense_1", Dict{String,Any}(Pair{String,Any}("bias:0", Float32[-0.000456777, -…
  :dense_2     => Dict{String,Any}(Pair{String,Any}("dense_2", Dict{String,Any}(Pair{String,Any}("bias:0", Float32[-0.00026646]),P…
  :flatten_1   => Dict{String,Any}()
  :embedding_1 => Dict{String,Any}(Pair{String,Any}("embedding_1", Dict{String,Any}(Pair{String,Any}("embeddings:0", Float32[0.004…

julia> x=w[:dense_1]["dense_1"]
Dict{String,Any} with 2 entries:
  "bias:0"   => Float32[-0.000456777, -0.00394565, 0.0167285, 0.00663774, 0.00539125, -0.00755487, 0.00562309, 0.0169308, 0.000605…
  "kernel:0" => Float32[-0.0138779 0.0164853 … 0.0222927 -0.0899643; -0.01212 0.00812849 … 0.00999072 -0.0178676; … ; 0.00156742 0…

julia> x["bias:0"]
ERROR: KeyError: key "bias:0" not found
Stacktrace:
 [1] getindex(::Dict{String,Any}, ::String) at ./dict.jl:474

julia> collect(keys(x))[1] == "bias:0"
true

julia> x.slots
2-element Array{UInt8,1}:
 0x01
 0x01

julia> Base.rehash!(x);

julia> x["bias:0"]
250-element Array{Float32,1}:
 -0.000456777
  ⋮

julia> x.slots
16-element Array{UInt8,1}:
 0x00
 0x00
 0x00
 0x00
 0x00
 0x00
 0x00
 0x00
 0x00
 0x01
 0x01
 0x00
 0x00
 0x00
 0x00
 0x00

Hope this helps. This file was written and read using this branch of BSON.jl

[DilumAluthge]

Thanks! I’ll work on this today.

[DilumAluthge]

@aviks I believe I've fixed this. Basically I just rehash every dictionary at load-time. It's an easy fix, and now your example seems to work. Can you confirm that it works for you?

[aviks]

Yes, this does work for me, thanks! I'll let Mike chime in on whether rehashing on load is a valid strategy.

[MikeInnes]

Good stuff, but we can make this a bit simpler. Rather than trying to preserve the internal structure of the dict, we really just need to save a Vector{K} and Vector{V} for the keys and values respectively, and we can reconstruct the dict from scratch on read (avoiding the need for rehash, too). How does that sound?

[DilumAluthge]

Hmmm. So on read, we would create a new Dict{K,V}(), and then we would iterate through the key/value vectors and add the key/value pairs to the Dict one by one? That seems reasonable to me, but I’m not sure how to implement it. I think I’d have to create a new method of newstruct_raw for T<:Dict{K, V}. I’ll work on it this weekend!

On Mon, Jun 25, 2018 at 06:26 Mike J Innes ***@***.***> wrote: Good stuff, but we can make this a bit simpler. Rather than trying to preserve the internal structure of the dict, we really just need to save a Vector{K} and Vector{V} for the keys and values respectively, and we can reconstruct the dict from scratch on read (avoiding the need for rehash, too). How does that sound? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#7 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFXArV-q2C3YTzvdcjaHO_YL5U8hrPPSks5uALrqgaJpZM4UV2PW> .

-- *Dilum Aluthge* *[email protected] <[email protected]>* *https://www.aluthge.com <https://www.aluthge.com>*

[MikeInnes]

I think you just need structdata, initstruct and newstruct!.

[DilumAluthge]

Sounds good. I’ll work on it this weekend. Should I GitHub ping you when I’m done, or no?

On Mon, Jun 25, 2018 at 09:01 Mike J Innes ***@***.***> wrote: I think you just need structdata, initstruct and newstruct!. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#7 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFXArY4sJpIS2z9TWuVaJ7U3Bs6GOwoeks5uAN8qgaJpZM4UV2PW> .

-- *Dilum Aluthge* *[email protected] <[email protected]>* *https://www.aluthge.com <https://www.aluthge.com>*

[MikeInnes]

Sure, feel free to bump this thread.

[DilumAluthge]

I worked on this for a bit, but I reached the conclusion that it will actually be better to get #8 to work and use that. That way we are covered for any structs with undefined values.

[DilumAluthge]

Closing in favor of #8

[MikeInnes]

I disagree actually – we will end up having to do tricks like rehash anyway, and this just makes things a lot more robust to changes to Dict's internal representation. Ideally we'd have both fixes.

The undefined values patch will also be bigger / involve some design decisions, so it'll also be easier to get this in quickly.