search

JuliaIO / BSON.jl

Other
158 stars 39 forks source link

Saving array length for undef array issues #47

Open Codyk12 opened 5 years ago

Codyk12 commented 5 years ago

Changed saving format to save array length as item in the sparse dict when writing arrays. Uses the length key while reading the array to allocate the necessary length of the array. Backwards compatible with existing BSON dumps. #8

DilumAluthge commented 5 years ago

This looks fine to me.

@MikeInnes what do you think?

Codyk12 commented 5 years ago

How does this look? @MikeInnes

freddycct commented 4 years ago

can you merge this pullrequest? https://github.com/FluxML/Flux.jl/issues/737 is broken because of this.

jondeuce commented 4 years ago

I'm also running into this issue. Would be good to get this merged, if it looks good to @MikeInnes

AzamatB commented 4 years ago

Bump. Was bitten by this

AzamatB commented 4 years ago

Please merge this PR

DilumAluthge commented 4 years ago

We will need @MikeInnes to review this before it can be merged.

DilumAluthge commented 4 years ago

Also, @Codyk12 can you rebase on master?

freddycct commented 4 years ago

Flux is almost perfect, just waiting on this... @Codyk12 @MikeInnes

without this, adam optim cannot be saved https://github.com/FluxML/Flux.jl/issues/737

CarloLucibello commented 4 years ago

Since the problem with saving Adam optimizer was fixed by https://github.com/JuliaIO/BSON.jl/pull/70, do we still need this PR?

I guess the answer is yes, it would be generally nice to be able to save undef arrays. @Codyk12 could you address Mike's comment so that we can get this merged?

DhairyaLGandhi commented 4 years ago

Saving extra metadata which needs to be acted upon while reading means a departure from the specification. We should avoid that.

DilumAluthge commented 4 years ago

Since the problem with saving Adam optimizer was fixed by #70, do we still need this PR?

I guess the answer is yes, it would be generally nice to be able to save undef arrays. @Codyk12 could you address Mike's comment so that we can get this merged?

Yeah we still need this to be able to save regular Dicts.

DhairyaLGandhi commented 4 years ago

Regular dicts can be saved fine, the core issue is to be able to express undefs properly

DilumAluthge commented 4 years ago

Regular dicts can be saved fine, the core issue is to be able to express undefs properly

Ah, I see.

It is good that we are now able to save regular dicts properly.

I think that there is still value in being able to save an array that contains undefined values.

MikeInnes commented 4 years ago

Note that it's possible for someone to grab the commits here and carry on work in a new PR, which might be a good option if people need the patch soon.