BSON.jl icon indicating copy to clipboard operation
BSON.jl copied to clipboard

Published 20 hours ago • verified publisher icon JuliaIO

Saving array length for undef array issues

Open Codyk12 opened this issue 5 years ago • 15 comments

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

Codyk12 avatar Jun 13 '19 22:06 Codyk12

This looks fine to me.

@MikeInnes what do you think?

DilumAluthge avatar Jun 17 '19 15:06 DilumAluthge

How does this look? @MikeInnes

Codyk12 avatar Jun 25 '19 21:06 Codyk12

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

freddycct avatar Aug 23 '19 22:08 freddycct

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

jondeuce avatar Sep 30 '19 20:09 jondeuce

Bump. Was bitten by this

AzamatB avatar Feb 01 '20 18:02 AzamatB

Please merge this PR

AzamatB avatar Feb 01 '20 20:02 AzamatB

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

DilumAluthge avatar Feb 01 '20 20:02 DilumAluthge

Also, @Codyk12 can you rebase on master?

DilumAluthge avatar Feb 01 '20 20:02 DilumAluthge

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

freddycct avatar Mar 04 '20 18:03 freddycct

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?

CarloLucibello avatar Apr 21 '20 10:04 CarloLucibello

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

DhairyaLGandhi avatar Apr 21 '20 12:04 DhairyaLGandhi

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.

DilumAluthge avatar Apr 21 '20 18:04 DilumAluthge

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

DhairyaLGandhi avatar Apr 21 '20 18:04 DhairyaLGandhi

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.

DilumAluthge avatar Apr 21 '20 19:04 DilumAluthge

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.

MikeInnes avatar Apr 22 '20 10:04 MikeInnes