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

Add freeze!/thaw! #112

Merged
merged 8 commits into from
Nov 25, 2022
Merged

Add freeze!/thaw! #112

merged 8 commits into from
Nov 25, 2022

Conversation

mcabbott
Copy link
Member

@mcabbott mcabbott commented Oct 13, 2022

Replaces #49 with something much simpler: now that Leaf is mutable, you can simply pass it the appropriate limb of the state tree, rather than having some explicit address mechanism.

Addresses some of #107

PR Checklist

  • Tests are added
  • Documentation, if applicable

@mcabbott mcabbott added the enhancement New feature or request label Oct 13, 2022
src/adjust.jl Outdated
Comment on lines 25 to 26
julia> s # Leaf(..., true) means frozen
(x = (Leaf(Momentum{Float32}(0.01, 0.9), [0.0], true), ()), y = Leaf(Momentum{Float32}(0.01, 0.9), [3.14159]))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just a clear show would be nice here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Like make it a keyword, you mean?

I'd like to make them colourful too... #53

@mcabbott mcabbott marked this pull request as ready for review October 14, 2022 01:30
docs/src/index.md Outdated Show resolved Hide resolved
docs/src/index.md Show resolved Hide resolved
src/adjust.jl Outdated Show resolved Hide resolved
@mcabbott
Copy link
Member Author

Shall we do this?

Would be nice to include in FluxML/Flux.jl#2114 while sorting that out.

Copy link
Member

@ToucheSir ToucheSir left a comment

Choose a reason for hiding this comment

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

Let's. One quick thought on top of @darsnack's comment suggestion.


Optimisers.update!(opt, net, gradient(m -> sum(m(x)), net)...);

opt # bias = Leaf(Momentum{Float32}(0.01, 0.9), Float32[0.0, 0.0], frozen = true)
Copy link
Member

Choose a reason for hiding this comment

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

This would be nice as a doctest.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried & now everything is broken locally, I don't understand Documenter.

Note that the tests at present don't load Flux at all. And that I think testing the output of this block probably means testing a bunch of Flux printing which I'm not sure this package should ever break if that changes.

Copy link
Member

Choose a reason for hiding this comment

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

Fair point. In that case, this line could be eliminated entirely and the comment moved to directly before/after the freeze! call. Also duplicating it after the thaw! call to show frozen = false.

opt_state = Optimisers.setup(Optimisers.Momentum(), net);

...

Optimisers.freeze!(opt_state.layers[3])
# opt_state.layers[3].bias = Leaf(Momentum{Float32}(0.01, 0.9), Float32[0.0, 0.0], frozen = true)

...

Optimisers.thaw!(opt)
# opt_state.layers[3].bias = Leaf(Momentum{Float32}(0.01, 0.9), Float32[0.0, 0.0], frozen = false)
...

Copy link
Member Author

Choose a reason for hiding this comment

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

Last commit does something like this

@mcabbott mcabbott merged commit a25b681 into FluxML:master Nov 25, 2022
@mcabbott mcabbott deleted the freeze2 branch November 25, 2022 04:48
using Flux, Optimisers

x = randn(Float32, 28, 28, 1, 1);
net = @autosize (size(x)...,) Chain(
Copy link
Member

Choose a reason for hiding this comment

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

what's the point of splatting here instead of just using size(x)?

Copy link
Member Author

Choose a reason for hiding this comment

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

The macro at present wants to see a tuple expression, not a call. It could be made to accept anything.

@mcabbott mcabbott mentioned this pull request Nov 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants