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.

Sign up for GitHub

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

Jump to bottom

dropout function always active #1263

Merged
merged 7 commits into from
Jul 9, 2020
Merged

dropout function always active #1263

merged 7 commits into from
Jul 9, 2020

Conversation

CarloLucibello
Copy link
Member

@CarloLucibello CarloLucibello commented Jul 2, 2020 •
edited
Loading

Fix #1084 according to the suggestions of @AzamatB and @jondeuce

PR Checklist

  • Tests are added
  • Entry in NEWS.md
  • Documentation, if applicable
  • Final review from @MikeInnes or @dhairyagandhi96 (for API changes).

@DhairyaLGandhi
Copy link
Member

I don't think we should do it this way, dropout is usually omitted at test time, this way it will make it difficult to turn it off when most cases all for it to be off

@CarloLucibello
Copy link
Member Author

I don't think we should do it this way, dropout is usually omitted at test time, this way it will make it difficult to turn it off when most cases all for it to be off

this is what the Dropout type is for. In fact, in 99% cases you should always use the Dropout layer and never the dropout function. With this PR the implementations are now consistent with the docstrings (they weren't before). I can add some additional warning in the docstring

@DhairyaLGandhi
Copy link
Member

In that case better to just remove the starting _ in dropout_kernel and document using that if anyone wants to dropout all the time. Fixing a docstring is a good idea though. Thanks!

@CarloLucibello
Copy link
Member Author

to avoid silent change of behavior, I introduced the mandatory keyword argument active, that we can possibly deprecate in the future

oxinabox
oxinabox reviewed Jul 2, 2020
View reviewed changes
NEWS.md Outdated
@@ -17,6 +17,7 @@
* Testing suite improvements now test for gradients of all layers along with GPU support.
* Functors have now moved to [Functors.jl](https://github.com/FluxML/Flux.jl/pull/1174) to allow for their use outside of Flux.
* Added [helper functions](https://github.com/FluxML/Flux.jl/pull/873) `Flux.convfilter` and `Flux.depthwiseconvfilter` to construct weight arrays for convolutions outside of layer constructors so as to not have to depend on the default layers for custom implementations.
* `dropout` function is now [always active](https://github.com/FluxML/Flux.jl/pull/1263)
Copy link
Member

@oxinabox oxinabox Jul 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is out of date with the new keyword?
also maybe mention the Dropout layer here ?

@CarloLucibello CarloLucibello added this to the v0.11 milestone Jul 2, 2020
@CarloLucibello
Copy link
Member Author

@DhairyaLGandhi merge?

DhairyaLGandhi
DhairyaLGandhi reviewed Jul 8, 2020
View reviewed changes
NEWS.md Outdated
@@ -17,6 +17,8 @@
* Testing suite improvements now test for gradients of all layers along with GPU support.
* Functors have now moved to [Functors.jl](https://github.com/FluxML/Flux.jl/pull/1174) to allow for their use outside of Flux.
* Added [helper functions](https://github.com/FluxML/Flux.jl/pull/873) `Flux.convfilter` and `Flux.depthwiseconvfilter` to construct weight arrays for convolutions outside of layer constructors so as to not have to depend on the default layers for custom implementations.
* `dropout` function now has a mandatory [active](https://github.com/FluxML/Flux.jl/pull/1263)
keyword argument. The `Dropout` struct is the recommended choice for common usage.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe say the behaviour of the layer has not changed.

@CarloLucibello
Copy link
Member Author

@DhairyaLGandhi done

@DhairyaLGandhi
Copy link
Member

bors r+

In general, we should try to remove the dependency on the active field in other structs as well, and use functional approaches instead (like we used to do earlier) and remove trainmode! but that's breaking and the matter of a separate discussion.

@bors
Copy link
Contributor

bors bot commented Jul 9, 2020

Build succeeded:

@bors bors bot merged commit f8001cf into master Jul 9, 2020
@CarloLucibello CarloLucibello deleted the cl/dropout branch January 7, 2021 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oxinabox oxinabox oxinabox left review comments

@DhairyaLGandhi DhairyaLGandhi DhairyaLGandhi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.11
Development

Successfully merging this pull request may close these issues.

dropout function is implemented as just an identity
3 participants
@CarloLucibello @DhairyaLGandhi @oxinabox