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

Expand RNN/LSTM/GRU docs #1772

Merged
merged 6 commits into from
Nov 23, 2021
Merged

Expand RNN/LSTM/GRU docs #1772

merged 6 commits into from
Nov 23, 2021

Conversation

mcognetta
Copy link
Contributor

This PR adds expanded documentation to the RNN/LSTM/GRU/GRUv3 docs, resolving #1696.

It addresses the in and out parameter meanings and adds a warning about a common gotcha (not calling reset when batch sizes are changed).

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.

This is great! My only significant request would be to switch over the output array summaries to |> size like you have for [recur].state. That would save some vertical space and keep consistency with other layers (e.g.

julia> layer(xs) |> size
(96, 96, 7, 50)
julia> Conv((5,5), 3 => 7; stride = 2)(xs) |> size
(48, 48, 7, 50)
julia> Conv((5,5), 3 => 7; stride = 2, pad = SamePad())(xs) |> size
(50, 50, 7, 50)
julia> Conv((1,1), 3 => 7; pad = (20,10,0,0))(xs) |> size
(130, 100, 7, 50)
julia> Conv((5,5), 3 => 7; stride = 2, dilation = 4)(xs) |> size
(42, 42, 7, 50)
).

@mcognetta
Copy link
Contributor Author

Thanks for the comments, I condensed the examples as you suggested.

@ToucheSir
Copy link
Member

Doctests appear to be failing because the previous line comments of # A demonstration of not using reset! when the batch size changes. are interpreted as expected outputs. Can you shorten them a bit so that they can become end of line comments? Otherwise everything looks good to me.

@mcognetta
Copy link
Contributor Author

I split it into two code-blocks. The first one is a jldoctest and shows the expected usages, the second is just a julia block and shows the gotcha behavior regarding batch sizes. I figured it was not a good idea to make the second one a doctest block because that behavior is "bad" and not something we want to include in tests.

@ToucheSir
Copy link
Member

Apologies for yet more revisions, but WDYT about extracting the "bad" behaviour as a shared block under the Recur docstring? It seems to me that the only difference between the various RNN layers is the actual type name, so deduplicating that and inserting backrefs (with Documenter.jl warning style if it helps) in each layer's docstring would reduce the repetition.

@mcognetta
Copy link
Contributor Author

Apologies for yet more revisions, but WDYT about extracting the "bad" behaviour as a shared block under the Recur docstring? It seems to me that the only difference between the various RNN layers is the actual type name, so deduplicating that and inserting backrefs (with Documenter.jl warning style if it helps) in each layer's docstring would reduce the repetition.

I changed all but the RNN docstring to have a reference to RNN in a warning about the batch size changing. The RNN docstring has the original example of the behavior.

I wasn't able to check that the docs rendered correctly though, do you mind taking a look?

@ToucheSir
Copy link
Member

Let me just check if docs previews can be made to work here. If not, I'll pull locally and confirm (if or you can, that would be swell).

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.

Ok, double-checked the syntax against https://github.com/FluxML/Flux.jl/blob/master/docs/src/models/recurrence.md?plain=1#L111-L123 and everything looks fine. Thanks for the PR!

@ToucheSir
Copy link
Member

bors r+

@mcognetta
Copy link
Contributor Author

I was able to build them locally (JuliaDocs/Documenter.jl#1413 (comment)) and it looked good.

Thanks for the comments.

@bors
Copy link
Contributor

bors bot commented Nov 23, 2021

Build succeeded:

@bors bors bot merged commit 66a84ef into FluxML:master Nov 23, 2021
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.

None yet

2 participants