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

Fix Saving and loading model output example #1746

Merged
merged 2 commits into from
Oct 15, 2021
Merged

Conversation

logankilpatrick
Copy link
Member

Right now, the example here: https://fluxml.ai/Flux.jl/stable/saving/#Saving-and-Loading-Models is incorrect.

julia> model = Chain(Dense(10,5,relu),Dense(5,2),softmax)
Chain(Dense(10, 5, NNlib.relu), Dense(5, 2), NNlib.softmax)

is not what happens and the loading model example at the bottom expects Chain(Dense(10, 5, NNlib.relu), Dense(5, 2), NNlib.softmax) so this fixes this inconsistency.

@ToucheSir
Copy link
Member

What version of Flux are you on? Both of those have been reexported for as long as I can remember:

julia> using Flux

julia> softmax
softmax (generic function with 2 methods)

julia> relu
relu (generic function with 2 methods)

@logankilpatrick
Copy link
Member Author

I guess I am on a very old version Flux v0.8.3 but when I do up Flux, it does not change.

@logankilpatrick
Copy link
Member Author

Okay I updated to Flux v0.11.6 and still get:

julia> model = Chain(Dense(10,5,relu),Dense(5,2),softmax)
Chain(Dense(10, 5, relu), Dense(5, 2), softmax)

@logankilpatrick
Copy link
Member Author

Even on the newest version, it is still wrong:

julia> using Flux

julia> model = Chain(Dense(10,5,relu),Dense(5,2),softmax)
Chain(Dense(10, 5, relu), Dense(5, 2), softmax)

(@v1.6) pkg> st
      Status `~/.julia/environments/v1.6/Project.toml`
  [fbb218c0] BSON v0.3.4
  [8f4d0f93] Conda v1.5.2
  [587475ba] Flux v0.12.7
  [5fb14364] OhMyREPL v0.5.10
  [438e738f] PyCall v1.92.3

@logankilpatrick
Copy link
Member Author

But maybe I have the PR backwards, I should instead switch it to Chain(Dense(10,5,relu),Dense(5,2),softmax) in both cases?

@ToucheSir
Copy link
Member

This is what I get:

julia> using Flux

julia> using BSON: @save, @load

julia> model = Chain(Dense(10,5,relu),Dense(5,2),softmax)
Chain(
  Dense(10, 5, relu),                   # 55 parameters
  Dense(5, 2),                          # 12 parameters
  NNlib.softmax,
)                   # Total: 4 arrays, 67 parameters, 524 bytes.

julia> @save "mymodel.bson" model

julia> model = nothing

julia> @load "mymodel.bson" model

julia> model
Chain(
  Dense(10, 5, relu),                   # 55 parameters
  Dense(5, 2),                          # 12 parameters
  NNlib.softmax,
)                   # Total: 4 arrays, 67 parameters, 524 bytes.

@logankilpatrick
Copy link
Member Author

@ToucheSir exactly! The docs right now show the output of:

julia> model = Chain(Dense(10,5,relu),Dense(5,2),softmax)

as

Chain(Dense(10, 5, NNlib.relu), Dense(5, 2), NNlib.softmax)

but we both do not get that, we get:

Chain(Dense(10,5,relu),Dense(5,2),softmax)

Copy link
Member

@darsnack darsnack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! This probably affects more than just the saving/loading docs? @logankilpatrick do you want to merge as is, or would you like to tackle the other cases too?

@logankilpatrick
Copy link
Member Author

I'll hunt down the others when I have time (hopefully this weekend).

@darsnack
Copy link
Member

Okay let's merge this for now then.

bors r+

bors bot added a commit that referenced this pull request Oct 15, 2021
1746: Fix Saving and loading model output example r=darsnack a=logankilpatrick

Right now, the example here: https://fluxml.ai/Flux.jl/stable/saving/#Saving-and-Loading-Models is incorrect.

```Julia
julia> model = Chain(Dense(10,5,relu),Dense(5,2),softmax)
Chain(Dense(10, 5, NNlib.relu), Dense(5, 2), NNlib.softmax)
```

is not what happens and the loading model example at the bottom expects `Chain(Dense(10, 5, NNlib.relu), Dense(5, 2), NNlib.softmax)` so this fixes this inconsistency.

Co-authored-by: Logan Kilpatrick <[email protected]>
@darsnack
Copy link
Member

bors r-

@bors
Copy link
Contributor

bors bot commented Oct 15, 2021

Canceled.

@darsnack
Copy link
Member

Sorry for the spam notifications. Confused myself for a second.

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 15, 2021

Build succeeded:

@bors bors bot merged commit df9d08d into master Oct 15, 2021
@CarloLucibello CarloLucibello deleted the logankilpatrick-patch-1 branch April 7, 2022 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants