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 minimal infrastructure for the docs #431

Merged
merged 4 commits into from
Aug 29, 2022

Conversation

Saransh-cpp
Copy link
Member

This currently does not add any new doctests or improve the existing docs, as that would make this PR massive.

Here are the docstrings that are missing from the manual at the moment (I think most of them are a part of the internal API) -

NNlib.predilated_size :: Union{Tuple{M}, Tuple{N}, Tuple{Tuple{Vararg{T, N}} where T, Tuple{Vararg{T, M}} where T}} where {N, M}
NNlib.logσ :: Tuple{Any}
NNlib.transpose_swapbatch :: Tuple{AbstractArray}
NNlib.∇depthwiseconv_data_im2col!
NNlib.glu :: Union{Tuple{Any}, Tuple{Any, Any}}
NNlib.reverse_indices :: Union{Tuple{N}, Tuple{AbstractArray{<:Any, N}}} where N
NNlib.im2col_dims :: Tuple{ConvDims}
NNlib.conv_im2col! :: Union{Tuple{T}, Tuple{AbstractArray{T, 5}, AbstractArray{T, 5}, AbstractArray{T, 5}, DenseConvDims}} where T
NNlib.BatchedTranspose
NNlib.∇conv_data_im2col! :: Union{Tuple{T}, Tuple{AbstractArray{T, 5}, AbstractArray{T, 5}, AbstractArray{T, 5}, DenseConvDims}} where T
NNlib.storage_type :: Tuple{AbstractArray}
NNlib.add_blanks :: Tuple{Any, Any}
NNlib.∇conv_filter_direct!
NNlib.is_strided :: Tuple{StridedArray}
NNlib.depthwiseconv_direct! :: Union{Tuple{wT}, Tuple{xT}, Tuple{yT}, Tuple{AbstractArray{yT, 5}, AbstractArray{xT, 5}, AbstractArray{wT, 5}, DepthwiseConvDims}} where {yT, xT, wT}
NNlib.maximum_dims :: Tuple{AbstractArray{<:Integer}}
NNlib.σ :: Tuple{Any}
NNlib.scatter_dims :: Union{Tuple{Nidx}, Tuple{Ny}, Tuple{Nx}, Tuple{Tidx}, Tuple{Ty}, Tuple{Tx}, Tuple{AbstractArray{Tx, Nx}, AbstractArray{Ty, Ny}, AbstractArray{Tidx, Nidx}}} where {Tx, Ty, Tidx, Nx, Ny, Nidx}
NNlib.hardσ :: Tuple{Any}
NNlib.depthwiseconv_im2col!
NNlib.transpose_pad :: Tuple{ConvDims}
NNlib.conv_direct!
NNlib.∇conv_data_direct!
NNlib.∇depthwiseconv_filter_im2col!
NNlib.col2im!
NNlib.∇conv_filter_im2col! :: Union{Tuple{T}, Tuple{AbstractArray{T, 5}, AbstractArray{T, 5}, AbstractArray{T, 5}, DenseConvDims}} where T
NNlib.predilate :: Union{Tuple{M}, Tuple{N}, Tuple{T}, Tuple{AbstractArray{T, N}, Tuple{Vararg{T, M}} where T}} where {T, N, M}
NNlib.logaddexp :: Tuple{Any, Any}
NNlib.insert_singleton_spatial_dimension :: Tuple{C} where C<:ConvDims
NNlib.safe_div :: Tuple{Any, Any}
NNlib.gemm!
NNlib.∇depthwiseconv_filter_direct!
NNlib.BatchedAdjoint
NNlib.storage_typejoin :: Tuple{Any, Vararg{Any}}
NNlib.fast_act :: Union{Tuple{F}, Tuple{F, AbstractArray}} where F<:Function
NNlib.calc_padding_regions :: Tuple{Any}
NNlib.∇depthwiseconv_data_direct!
NNlib.im2col! :: Union{Tuple{T}, Tuple{AbstractMatrix{T}, AbstractArray{T, 4}, ConvDims}} where T

@Saransh-cpp Saransh-cpp marked this pull request as draft August 6, 2022 19:32
@ToucheSir
Copy link
Member

σ and glu should definitely be public. The rest can stay private for now, I think.

@Saransh-cpp
Copy link
Member Author

σ and glu should definitely be public. The rest can stay private for now, I think.

σ is present in the references as sigmoid. Adding glu!


## Activation Functions

Non-linearities that go between layers of your model. Note that, unless otherwise stated, activation functions operate on scalars. To apply them to an array you can call `σ.(xs)`, `relu.(xs)` and so on.
Copy link
Member

Choose a reason for hiding this comment

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

Is this true? I think a PR gave them all broadcasting behaviour:

julia> relu(-2:2)
5-element Vector{Int64}:
 0
 0
 0
 1
 2

This is slightly weird, and won't work for tanh. I think the logic was that this was less ugly than introducing a special Activate layer, or writing x -> relu.(x) in your Chain.

Copy link
Member Author

Choose a reason for hiding this comment

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

Out of curiosity, why is the behavior implemented differently for tanh?

Copy link
Member

Choose a reason for hiding this comment

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

Because some functions already have matrix input definitions that NNlib does not override.

Copy link
Member

Choose a reason for hiding this comment

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

I don't have an issue with the current wording because it's not clear whether/how much we want to advertise this behaviour. We can always relax it later on.

Comment on lines 70 to 81
## Upsampling

```@docs
upsample_nearest
Copy link
Member

Choose a reason for hiding this comment

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

For many of these sections, it might be worth pointing briefly to what Flux layers use these functions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should it be something like this -

## Softmax

\```@docs
softmax
logsoftmax
\```

### Relevant Flux functionality

\```@docs
Flux.xyz
\```

Or should I simply point it out in text -

NNlib.softmax is used by Flux.xyz(Redirect to the reference in Flux's documentation, can't use @ref as Flux.xyz is not present in NNlib's docs) ...

Copy link
Member

@darsnack darsnack Aug 17, 2022

Choose a reason for hiding this comment

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

Maybe it suffices to summarize at the top of section:

## Upsampling

Used by `Flux.x`, `Flux.y`, ...

```@docs
upsample_nearest
...
```

Copy link
Member

Choose a reason for hiding this comment

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

Yes something like this was what I pictured. "There's an Upsample layer in Flux for which this is the backend"

I don't think we should repeat the Flux docstrings here, too much duplication. This API page might end up included entire in Flux's docs, too.

@Saransh-cpp Saransh-cpp requested review from ToucheSir, darsnack and mcabbott and removed request for ToucheSir, darsnack and mcabbott August 22, 2022 04:07
@Saransh-cpp
Copy link
Member Author

For some reason I can request a review from only 1 person at a time :(

@Saransh-cpp
Copy link
Member Author

Is this ready for merging?

@mcabbott mcabbott merged commit 806b0ef into FluxML:master Aug 29, 2022
@mcabbott
Copy link
Member

What's next, should these show up at https://fluxml.ai/NNlib.jl/dev/ ? If so, should this package's readme link to that?

@Saransh-cpp
Copy link
Member Author

The documentation should show up on the given link, but I think the GH pages are not enabled for NNlib? Could you please enable the pages from NNlib's settings? Additionally, a DOCUMENTER_KEY should be added as a repository secret in NNlib.jl.

Right now the deployment is failing -

┌ Info: Deployment criteria for deploying devbranch build from GitHub Actions:-ENV["GITHUB_REPOSITORY"]="FluxML/NNlib.jl" occurs in repo="github.com/FluxML/NNlib.jl.git"-ENV["GITHUB_EVENT_NAME"]="push" is "push"-ENV["GITHUB_REF"] matches devbranch="master"-ENV["GITHUB_ACTOR"] exists
│ -ENV["DOCUMENTER_KEY"] exists exists
└ Deploying: ✔
hint: Using 'master' as the name for the initial branch. This default branch name
hint: is subject to change. To configure the initial branch name to use in all
hint: of your new repositories, which will suppress this warning, call:
hint: 
hint: 	git config --global init.defaultBranch <name>
hint: 
hint: Names commonly chosen instead of 'master' are 'main', 'trunk' and
hint: 'development'. The just-created branch can be renamed via this command:
hint: 
hint: 	git branch -m <name>
Initialized empty Git repository in /tmp/jl_wxIzo9/.git/
Failed to add the ECDSA host key for IP address '192.30.255.112' to the list of known hosts (/home/runner/.ssh/known_hosts).
Load key "/home/runner/work/NNlib.jl/NNlib.jl/docs/.documenter": invalid format
git@github.com: Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
┌ Error: Git failed to fetch git@github.com:FluxML/NNlib.jl.git
│ This can be caused by a DOCUMENTER_KEY variable that is not correctly set up.
│ Make sure that the environment variable is properly set up as a Base64-encoded string
│ of the SSH private key. You may need to re-generate the keys with DocumenterTools.
└ @ Documenter ~/.julia/packages/Documenter/PLD7m/src/Documenter.jl:502
┌ Error: Failed to push:
│   exception =
│    failed process: Process(`git fetch upstream`, ProcessExited(128)) [128]
│    
│    Stacktrace:
│      [1] pipeline_error
│        @ ./process.jl:538 [inlined]
│      [2] run(::Cmd; wait::Bool)
│        @ Base ./process.jl:453
│      [3] run
│        @ ./process.jl:451 [inlined]
│      [4] (::Documenter.var"#git_commands#22"{String, String, SubString{String}, String, Vector{Any}, Bool, String, String})(sshconfig::String)
│        @ Documenter ~/.julia/packages/Documenter/PLD7m/src/Documenter.jl:500
│      [5] (::Documenter.var"#21#27"{String})()
│        @ Documenter ~/.julia/packages/Documenter/PLD7m/src/Documenter.jl:598
│      [6] cd(f::Documenter.var"#21#27"{String}, dir::String)
│        @ Base.Filesystem ./file.jl:106
│      [7] #20
│        @ ~/.julia/packages/Documenter/PLD7m/src/Documenter.jl:598 [inlined]
│      [8] withenv(f::Documenter.var"#20#26"{String, String}, keyvals::Pair{String, String})
│        @ Base ./env.jl:161
│      [9] (::Documenter.var"#19#25"{String, String, SubString{String}})(sshconfig::String, io::IOStream)
│        @ Documenter ~/.julia/packages/Documenter/PLD7m/src/Documenter.jl:597
│     [10] mktemp(fn::Documenter.var"#19#25"{String, String, SubString{String}}, parent::String)
│        @ Base.Filesystem ./file.jl:[70](https://github.com/FluxML/NNlib.jl/runs/8074414339?check_suite_focus=true#step:6:71)3
│     [11] mktemp(fn::Function)
│        @ Base.Filesystem ./file.jl:701
│     [12] git_push(root::String, temp::String, repo::String; branch::String, dirname::String, target::String, sha::SubString{String}, devurl::String, versions::Vector{Any}, forcepush::Bool, deploy_config::Documenter.GitHubActions, subfolder::String)
│        @ Documenter ~/.julia/packages/Documenter/PLD7m/src/Documenter.jl:582
│     [13] #13
│        @ ~/.julia/packages/Documenter/PLD7m/src/Documenter.jl:456 [inlined]
│     [14] mktempdir(fn::Documenter.var"#13#15"{SubString{String}, String, String, String, String, String, String, Vector{Any}, Bool, Documenter.GitHubActions, String}, parent::String; prefix::String)
│        @ Base.Filesystem ./file.jl:[72](https://github.com/FluxML/NNlib.jl/runs/8074414339?check_suite_focus=true#step:6:73)9
│     [15] mktempdir(fn::Function, parent::String) (repeats 2 times)
│        @ Base.Filesystem ./file.jl:727
│     [16] (::Documenter.var"#12#14"{String, String, String, String, String, Nothing, String, Vector{Any}, Bool, Documenter.GitHubActions, String})()
│        @ Documenter ~/.julia/packages/Documenter/PLD7m/src/Documenter.jl:455
│     [17] cd(f::Documenter.var"#12#14"{String, String, String, String, String, Nothing, String, Vector{Any}, Bool, Documenter.GitHubActions, String}, dir::String)
│        @ Base.Filesystem ./file.jl:106
│     [18] deploydocs(; root::String, target::String, dirname::String, repo::String, branch::String, deps::Nothing, make::Nothing, devbranch::String, devurl::String, versions::Vector{Any}, forcepush::Bool, deploy_config::Documenter.GitHubActions, push_preview::Bool)
│        @ Documenter ~/.julia/packages/Documenter/PLD7m/src/Documenter.jl:432
│     [19] top-level scope
│        @ ~/work/NNlib.jl/NNlib.jl/docs/make.jl:17
│     [20] include(mod::Module, _path::String)
│        @ Base ./Base.jl:3[84](https://github.com/FluxML/NNlib.jl/runs/8074414339?check_suite_focus=true#step:6:85)
│     [21] exec_options(opts::Base.JLOptions)
│        @ Base ./client.jl:2[85](https://github.com/FluxML/NNlib.jl/runs/8074414339?check_suite_focus=true#step:6:86)
│     [22] _start()
│        @ Base ./client.jl:485
└ @ Documenter ~/.julia/packages/Documenter/PLD7m/src/Documenter.jl:603

@Saransh-cpp Saransh-cpp deleted the independent-docs branch August 29, 2022 17:38
@darsnack
Copy link
Member

Btw the docs issue is now taken care of. Do we want the link in the README to point to the dedicated docs?

@Saransh-cpp
Copy link
Member Author

And we should also add the URL in GitHub's "About" section

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants