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

Allow saving IdDict #70

Merged
merged 2 commits into from
Apr 17, 2020
Merged

Conversation

DhairyaLGandhi
Copy link
Collaborator

Another approach to solve the issues in saving parameters from Flux/ Zygote directly, for structures based on using IdDict. In the basic saving and loading test, things seem to work fine, but I would like to know if this would break any assumptions.

MWE:

julia> d = IdDict(:a => 1, :b => rand(3))
IdDict{Symbol,Any} with 2 entries:
  :a => 1
  :b => [0.819137, 0.668358, 0.611674]

# without the PR:
julia> @save "d.dict" d
# error: Access to undefined reference

cc @MikeInnes

@DhairyaLGandhi
Copy link
Collaborator Author

Ref #47, FluxML/Flux.jl#737 and sidesteps #8, which came up in this use case the most, apart from where user code was causing undefs in places where they shouldn't be.

@DhairyaLGandhi
Copy link
Collaborator Author

DhairyaLGandhi commented Apr 16, 2020

Does it make it a technically breaking change? I am not sure if it can read dumps that were made prior to the breakage since the dictionary is different now.

@MikeInnes
Copy link
Collaborator

I guess it could technically be a breaking change if anyone had managed to save an IdDict without any undefs in it, but that seems relatively unlikely.

I only feel a bit uneasy about applying this to all AbstractDicts, and suggest the more convservative Union{IdDict,Dict} for now.

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 this pull request may close these issues.

2 participants