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

[Discussion]: doctests, docstrings, documentation manual, and unclear internal API (for newcomers) #1990

Closed
Saransh-cpp opened this issue Jun 6, 2022 · 2 comments · Fixed by #1998

Comments

@Saransh-cpp
Copy link
Member

Hello! I am opening this issue for further discussions on docstrings, doctests, making internal API visible (by prepending _), and updating the manual. Before I open a PR, I think it would be best to discuss the documentation here. I hope this is okay as I couldn't find a discussions tab in Flux's repository.

onhot.jl

onehot.jl misses doctests for the structs returned by onehotbatch and onehot. I am not sure if these structs are supposed to be user-facing? The docstring of the struct Embedding uses OneHotMatrix directly, but other than that, I couldn't find any other use cases. Should I add some doctests to these structs? I can also add them to the manual if they are supposed to be user-facing.

Additionally, right now if a user tries to access the documentation of OneHotVector and OneHotMatrix, they are presented with the documentation (with the signature type) of OneHotArray. After reading Flux's code, the reason becomes obvious, but I don't think every user will want to dive into Flux's code. Should these structs have their independent docstrings?

utils.jl

utils.jl has a few functions with no doctests, but it isn't clear if they are user-facing or not. From my previous PRs I have learned that the internal API doesn't need doctests, so should I prepend a _ to these functions, or should I add doctests to them (if they are user-facing)? The functions -

Not present in the manual -

  • rng_from_array
  • ones32
  • zeros32
  • create_bias

Present in the manual -

  • rand32
  • randn32
  • throttle

functor.jl

The exact same case as that of utils.jl -

Present in the manual -

  • testmode!
  • trainmode!
  • f32
  • f64

Additionally, I think cpu and gpu are user-facing, but their APIs aren't documented in the manual.

optimisers.jl

Currently, none of the optimisers have doctests, and I am not sure how to add them without excessive repetition. For example, I came up with the following doctest for the Descent optimiser -

julia> f(x) = 4x + 2;

julia> x = hcat(0:5...); y = f.(x);

julia> model = Dense(1 => 1);

julia> opt = Descent(0.01);

julia> loss(x, y) = Flux.Losses.mse(model(x), y);

julia> ps = Flux.params(model)  # initialised parameters
Params([Float32[-1.1358223;;], Float32[0.0]])

julia> loss(x, y)  # loss with initial parameters
297.14438f0

julia> gs = gradient(ps) do
                  loss(x, y)
              end;

julia> Flux.Optimise.update!(opt, ps, gs) # the parameters will be updated, and the loss should ideally go down

julia> ps
Params([Float32[-0.09425485;;], Float32[0.29679117]])

julia> loss(x, y)
191.4279f0

This does look a bit extra, but I am not sure how else I can document it properly. My main concern is that this would have to be repeated for all the optimisers, if it is added. A workaround can be initializing the same variables only once -

"""
    Descent(η = 0.1)
...
\```jldoctest optimisers
julia> f(x) = 4x + 2;

julia> x = hcat(0:5...); y = f.(x);

julia> model = Dense(1 => 1);

julia> opt = Descent(0.01);

julia> loss(x, y) = Flux.Losses.mse(model(x), y);

julia> ps = Flux.params(model)  # initialised parameters
Params([Float32[-1.1358223;;], Float32[0.0]])

julia> loss(x, y)  # loss with initial parameters
297.14438f0

julia> gs = gradient(ps) do
                  loss(x, y)
              end;

julia> Flux.Optimise.update!(opt, ps, gs) # the parameters will be updated, and the loss should ideally go down

julia> ps
Params([Float32[-0.09425485;;], Float32[0.29679117]])

julia> loss(x, y)
191.4279f0
\```
"""
mutable struct Descent <: AbstractOptimiser
   ...
"""
    Momentum(η = 0.01, ρ = 0.9)
...
\```jldoctest optimisers
julia> opt = Momentum(0.03, 0.99);

julia> ps = Flux.params(model)
Params([Float32[-0.09425485;;], Float32[0.29679117]])

julia> loss(x, y)
191.4279f0

julia> gs = gradient(ps) do
                  loss(x, y)
       end;

julia> Flux.Optimise.update!(opt, ps, gs) # the parameters will be updated

julia> ps  # updated parameters
Params([Float32[2.4130664;;], Float32[1.013122]])

julia> loss(x, y)
31.88943f0
\```
"""
mutable struct Momentum <: AbstractOptimiser
   ...

I am not sure if I should proceed ahead with this.

Suggestions from everyone are welcome :)

@mcabbott
Copy link
Member

mcabbott commented Jun 6, 2022

@Saransh-cpp
Copy link
Member Author

Thank you for the suggestion! I will create a PR for updating the docstrings. I will leave Optimise.jl and onehot.jl as it is for now. I'll definitely pick up some documentation work in the new packages!

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

Successfully merging a pull request may close this issue.

2 participants