Add docstring for `params` #1786

logankilpatrick · 2021-11-26T15:25:11Z

params is used all over the model zoo and tutorials in Flux. There should be a docstring or we should not publicly use it. Not sure if the proposed docstring gets the job done 100% but this seems like a step in the right direction.

src/functor.jl

Co-authored-by: Michael Abbott <[email protected]>

ToucheSir · 2021-11-27T03:16:58Z

src/functor.jl

+
+This can be used with [`gradient`](@ref), or as input to the [`Flux.train!`](@ref Flux.train!) function.
+
+The behaviour of `params` on custom types can be customized using [`Functor.@functor`](@ref) or [`Flux.trainable`](@ref).


Suggested change

The behaviour of `params` on custom types can be customized using [`Functor.@functor`](@ref) or [`Flux.trainable`](@ref).

The behaviour of `params` on custom types can be customized using [`Functors.@functor`](@ref) or [`Flux.trainable`](@ref).

Unfortunately Functors.@functor won't resolve because it's a cross-package link. Not sure what we can do about that, perhaps a manual link to the latest version of the Functors docs?

According to CI there are many such failures, although they do not cause it to fail overall here.

Yes, I saw a couple searching for other examples of this. Short of a solution in Documenter.jl, I guess our best option is a full link?

Since the page this wants to link to is here https://fluxml.ai/Flux.jl/stable/models/functors/#Functors.jl I think it ought to be possible at least to have an automatic link to the heading of the page. Maybe that's what you mean? Like https://raw.githubusercontent.com/JuliaLang/julia/master/doc/src/manual/arrays.md it seems you can write e.g.
## [Functors.jl](@id man-h2-functors) and then refer to that.

https://juliadocs.github.io/Documenter.jl/stable/man/guide/#Cross-Referencing makes it seem like we can do [Functors.jl](@ref) in the doc string and it will just work (assuming there is only one heading like that).

Worth a try. Also not in scope for this PR, but Functors.@functor needs to be rendered correctly in the docs: https://fluxml.ai/Functors.jl/stable/api/

Also trainable isn't documented either. Looks like we'll need a follow-up PR :)

Many PR's to come : )

src/functor.jl

ToucheSir · 2021-11-27T03:22:23Z

src/functor.jl

+julia> params(Chain(Dense(ones(2,3))), softmax)
+Params([[1.0 1.0 1.0; 1.0 1.0 1.0], [0.0, 0.0]])
+
+julia> params(BatchNorm(2, relu))


We should have an example and/or note that most built-in container types will be unpacked if passed into params. This has bitten people frequently when they try to use it like Zygote.Params and pass in a couple of arrays (Flux.params(array1) will unpack every element of array1 instead of including the array itself...)

Or the other way around, it's Params(array) which is surprising? Maybe something like:

julia> params([1, 2, 3]) Params([[1, 2, 3]]) julia> params(1, [2 2], (alpha=[3,3,3], beta=Ref(4), gamma=sin)) Params([[2 2], [3, 3, 3]]) julia> keys(ans.params.dict) KeySet for a IdDict{Any, Nothing} with 2 entries. Keys: [2 2] [3, 3, 3]

Not that I'm aware of?

julia> Flux.Params([1, [2 2], [3,3,3], Ref(4), sin]) Params([1, [2 2], [3, 3, 3], Base.RefValue{Int64}(4), sin])

The docstring for Params is reasonably clear too (explicitly asks for an array).

What I mean is by surprise is this difference, you can't give Params just one naked array:

julia> x = [1,2,3]; gradient(() -> sum(x), Params(x))[x] ERROR: KeyError: key [1, 2, 3] not found julia> x = [1,2,3]; gradient(() -> sum(x), params(x))[x] 3-element Fill{Int64}, with entries equal to 1

If that's not what you meant, what did you mean people get bitten by?

Giving params an array of arrays instead is OK:

julia> x = [1,2,3]; gradient(() -> sum(x), params([x, x]))[x] 3-element Fill{Int64}, with entries equal to 1

For the life of me I can't seem to find it the GH issue or discourse discussions for it, but I think it was an issue with not expecting arrays of non-numbers (or was it tuples?) to be unpacked. Whereas Params documents this unpacking, params doesn't and certainly doesn't document that it unpacks recursively when given a nested structure.

Co-authored-by: Brian Chen <[email protected]>

logankilpatrick · 2021-11-27T04:58:24Z

Also, which file or section should we add the params docs too? I am not quite sure where it fits in best.

ToucheSir · 2021-11-27T16:11:33Z

I'm not sure either. Co-locating it with https://fluxml.ai/Flux.jl/stable/models/basics/#Building-Simple-Models is one option, but a little out of the way. WDYT about creating a new page under the "Training Models" section for parameter handling? It could just be the docstring for now, but that would give us an opportunity to expand later.

ToucheSir · 2021-11-27T16:16:33Z

Actually, https://fluxml.ai/Flux.jl/stable/training/training/#Model-parameters doesn't look too bad.

logankilpatrick · 2021-11-27T16:35:14Z

Actually, https://fluxml.ai/Flux.jl/stable/training/training/#Model-parameters doesn't look too bad.

Agreed

logankilpatrick · 2021-11-27T16:59:02Z

Preview the PR here: https://fluxml.ai/Flux.jl/previews/PR1786/training/training/#Flux.params

darsnack · 2021-11-27T18:33:53Z

Is it worth mentioning that a vector of arrays can also be passed to params? This allows very manual, fine-grained control over collecting parameters.

ToucheSir · 2021-11-27T18:47:27Z

I think that would fit with my note in #1786 (comment) that any vector of arrays passed to params will be unpacked, even if they are nested within another functor-able value.

logankilpatrick · 2021-11-30T16:44:00Z

@ToucheSir Perhaps we can merge this and have one sweeping PR to try and get all the ref issues fixed in the docs? There are a lot that should be resolved.

DhairyaLGandhi · 2021-11-30T17:02:19Z

Since params([1,23]) would resolve correctly and the remaining issue is with Params I believe we can safely merge this. Params explicitly requires an array and that is documented too, but if one feels that an extra note that says numerical arrays need to be wrapped in a "container" array is required, that's fine too but that is an orthogonal change to this docstring.

mcabbott

Looks like a good step forwards. Figuring out Functors links will involve touching many things, can be another PR.

src/functor.jl

mcabbott

bors r+

But it may not listen, Brian you may need to approve?

1786: Add docstring for `params` r=mcabbott a=logankilpatrick `params` is used all over the model zoo and tutorials in Flux. There should be a docstring or we should not publicly use it. Not sure if the proposed docstring gets the job done 100% but this seems like a step in the right direction. Co-authored-by: Logan Kilpatrick <[email protected]> Co-authored-by: Michael Abbott <[email protected]>

bors · 2021-11-30T18:07:57Z

This PR was included in a batch that successfully built, but then failed to merge into master. It will not be retried.

Additional information:

{"message":"1 review requesting changes and 1 approving review by reviewers with write access.","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

ToucheSir

Done.

mcabbott · 2021-11-30T19:12:26Z

bors r+

bors · 2021-11-30T19:36:28Z

Build succeeded:

buildkite/flux-dot-jl

Update functor.jl

2540027

mcabbott reviewed Nov 26, 2021

View reviewed changes

src/functor.jl Outdated Show resolved Hide resolved

logankilpatrick and others added 2 commits November 26, 2021 08:27

Update src/functor.jl

a12569a

Co-authored-by: Michael Abbott <[email protected]>

Add note on custom behavior

4d56e75

logankilpatrick requested review from CarloLucibello and mcabbott November 26, 2021 17:04

ToucheSir mentioned this pull request Nov 26, 2021

Update train.jl to add a more detailed train! docstring #1785

Merged

logankilpatrick requested a review from ToucheSir November 27, 2021 01:14

ToucheSir requested changes Nov 27, 2021

View reviewed changes

Update src/functor.jl

be50da0

Co-authored-by: Brian Chen <[email protected]>

Add params to the Model parameters section

428b1d9

Fix link to Gradient

e21b094

mcabbott previously approved these changes Nov 30, 2021

View reviewed changes

src/functor.jl Outdated Show resolved Hide resolved

Update src/functor.jl

73351c7

mcabbott dismissed their stale review via 73351c7 November 30, 2021 17:43

mcabbott approved these changes Nov 30, 2021

View reviewed changes

ToucheSir approved these changes Nov 30, 2021

View reviewed changes

bors bot merged commit 8fa4317 into master Nov 30, 2021

mcabbott deleted the logankilpatrick-patch-7 branch November 30, 2021 19:50

This was referenced Nov 30, 2021

missing docstring for Flux.params and trainable #1732

Closed

v0.13 deprecations #1751

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add docstring for `params` #1786

Add docstring for `params` #1786

logankilpatrick commented Nov 26, 2021 •

edited

Loading

ToucheSir Nov 27, 2021

mcabbott Nov 27, 2021

ToucheSir Nov 27, 2021 •

edited

Loading

mcabbott Nov 27, 2021

logankilpatrick Nov 27, 2021 •

edited

Loading

ToucheSir Nov 27, 2021

ToucheSir Nov 27, 2021

logankilpatrick Nov 27, 2021

ToucheSir Nov 27, 2021

mcabbott Nov 27, 2021

ToucheSir Nov 27, 2021

mcabbott Nov 27, 2021

ToucheSir Nov 27, 2021 •

edited

Loading

logankilpatrick commented Nov 27, 2021

ToucheSir commented Nov 27, 2021

ToucheSir commented Nov 27, 2021

logankilpatrick commented Nov 27, 2021

logankilpatrick commented Nov 27, 2021

darsnack commented Nov 27, 2021

ToucheSir commented Nov 27, 2021

logankilpatrick commented Nov 30, 2021

DhairyaLGandhi commented Nov 30, 2021

mcabbott left a comment

mcabbott left a comment

bors bot commented Nov 30, 2021

ToucheSir left a comment

mcabbott commented Nov 30, 2021

bors bot commented Nov 30, 2021


		This can be used with [`gradient`](@ref), or as input to the [`Flux.train!`](@ref Flux.train!) function.

		The behaviour of `params` on custom types can be customized using [`Functor.@functor`](@ref) or [`Flux.trainable`](@ref).

Add docstring for params #1786

Add docstring for params #1786

Conversation

logankilpatrick commented Nov 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ToucheSir Nov 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logankilpatrick Nov 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ToucheSir Nov 27, 2021 • edited Loading

Choose a reason for hiding this comment

logankilpatrick commented Nov 27, 2021

ToucheSir commented Nov 27, 2021

ToucheSir commented Nov 27, 2021

logankilpatrick commented Nov 27, 2021

logankilpatrick commented Nov 27, 2021

darsnack commented Nov 27, 2021

ToucheSir commented Nov 27, 2021

logankilpatrick commented Nov 30, 2021

DhairyaLGandhi commented Nov 30, 2021

mcabbott left a comment

Choose a reason for hiding this comment

mcabbott left a comment

Choose a reason for hiding this comment

bors bot commented Nov 30, 2021

ToucheSir left a comment

Choose a reason for hiding this comment

mcabbott commented Nov 30, 2021

bors bot commented Nov 30, 2021

Add docstring for `params` #1786

Add docstring for `params` #1786

logankilpatrick commented Nov 26, 2021 •

edited

Loading

ToucheSir Nov 27, 2021 •

edited

Loading

logankilpatrick Nov 27, 2021 •

edited

Loading

ToucheSir Nov 27, 2021 •

edited

Loading