Saving array length for undef array issues #47
Conversation
… when writing arrays. Uses the lengthkey while reading the array to allocate the neccessary length of the array. Backwards compatible with existing BSON dumps.
|
This looks fine to me. @MikeInnes what do you think? |
|
How does this look? @MikeInnes |
|
can you merge this pullrequest? FluxML/Flux.jl#737 is broken because of this. |
|
I'm also running into this issue. Would be good to get this merged, if it looks good to @MikeInnes |
|
Bump. Was bitten by this |
|
Please merge this PR |
|
We will need @MikeInnes to review this before it can be merged. |
|
Also, @Codyk12 can you rebase on master? |
There was a problem hiding this comment.
Nice patch, thanks!
Is the length storage just a performance optimisation? I don't think we should do that this way because the low-level read and write code is meant to deal with the pure BSON spec. As it stands the change means that BSON.jl will disagree with other BSON readers about what's in a file.
| @@ -2,6 +2,29 @@ using BSON | |||
| using Test | |||
|
|
|||
| roundtrip_equal(x) = BSON.roundtrip(x) == x | |||
| function roundtrip_equal(x::AbstractArray) | |||
There was a problem hiding this comment.
Special casing roundtrip_equal for just these couple of types seems like it could lead to confusion. I think it'd be better to define an undef_equal(x, y) for arrays and explicitly test undef_equal(BSON.roundtrip(x), x) or similar.
There was a problem hiding this comment.
Bump @Codyk12 can you address this comment?
|
Flux is almost perfect, just waiting on this... @Codyk12 @MikeInnes without this, adam optim cannot be saved FluxML/Flux.jl#737 |
|
Saving extra metadata which needs to be acted upon while reading means a departure from the specification. We should avoid that. |
Yeah we still need this to be able to save regular |
|
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. |
|
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. |
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