-
-
Notifications
You must be signed in to change notification settings - Fork 615
Print the state of Dropout etc. #2222
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
Merged
Merged
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
49d1303
print the state of Dropout etc.
mcabbott 061440e
add tests
mcabbott 1153f57
doc improvements
mcabbott a22d7d0
simpler scheme for testmode/trainmode
mcabbott a36cd59
simplify active keyword a bit
mcabbott aa38847
a bug
mcabbott fd59571
fix tests
mcabbott 5a09f69
Update test/layers/normalisation.jl
mcabbott 3720d9f
Update src/functor.jl
mcabbott e2cef3d
extend docstrings & warnings
mcabbott File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we happy with the name
active? This existed as a field name, but not previously exposed.testmode!andtrainmode!both had a positional argument calledmodewith opposite meanings. I made theseactive+inactiveto match.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the double negative a little confusing but can't come up with a better word. For a future PR, would it make sense to add a third
setactive!function and move the true/false/auto/nothing handling logic to that? Thentrainmode!andtestmode!lose their second arg and become shortcuts forsetactive!(model, <true|false>). Either way, we could even use an enum if we're feeling fancy.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's awful that they both do everything. It would be OK if either accepted
:auto, but never true/false. Maybe that's a deprecation goal?There could also be a 3rd function, but two is already a lot. Or the 3rd could replace both, but that's more churn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But more immediately, your enum suggestion could read
Dropout(0.5; mode=:test)andDropout(0.5; mode=:train). That has the advantage of always being one type. It's a little more indirect -- it tells you what the layer is intended for, not what it does.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea! Can we keep
modethen? In that case, have we considered something likeenabledinstead ofmodeor(in)active?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tricky thing is that
trainmode!(m, ::Bool)recurses to itself, and is what's overloaded by layers. Presumably some layers in packages may overload this too.Deprecating that method and changing the recursion to use something else means that we will break any packages which rely on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Maybe 3 functions isn't that bad after all then if it makes the deprecation path easier. PyTorch has
.train(),.eval()andmodule.training = {true,false}.