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

Re-write "basics" page of docs #2535

Merged
merged 15 commits into from
Dec 8, 2024
Merged

Re-write "basics" page of docs #2535

merged 15 commits into from
Dec 8, 2024

Conversation

mcabbott
Copy link
Member

@mcabbott mcabbott commented Dec 3, 2024

A while ago (when we first discussed making Functors.jl automatic, rather than opt-in) I tried re-writing basics.md (the 2nd page of the "guide") to stress the idea that neural networks are parameterised functions. Almost any callable Julia object which contains parameter arrays is now a valid Flux model, and this encapsulation is the key idea to understanding Flux.

RFC, I guess? It goes with 0.15 but doesn't strictly have to hold that up.

It's much the same material as the existing page, but I think these points are quite hard to get from reading what's there, if you don't already know. The downside is that this is longer, maybe it needs a pass looking for things to shorten.

Formatted link in case the robot fails. Current text to compare.

Copy link
Contributor

github-actions bot commented Dec 3, 2024

Once the build has completed, you can preview any updated documentation at this URL: https://fluxml.ai/Flux.jl/previews/PR2535/ in ~20 minutes

@mcabbott mcabbott marked this pull request as ready for review December 3, 2024 16:03
Copy link
Contributor

@christiangnrd christiangnrd left a comment

Choose a reason for hiding this comment

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

Generally looks good. I left a few inline comments.

Other comments are all nits, like checking to make sure all code blocks are properly annotated with jldoctest, julia, or julia-repl as appropriate (should it all be julia-repl?)

There should probably be a using Flux and using Functors before the first use of each example. I know I always find it much more enjoyable when I can run a tutorial from start to finish without having to look anything up. That said, should there be a note at the very beginning telling the user how to activate a temp environment and add Flux and Functors?

docs/src/guide/models/basics.md Outdated Show resolved Hide resolved
docs/src/guide/models/basics.md Outdated Show resolved Hide resolved
docs/src/guide/models/basics.md Outdated Show resolved Hide resolved
docs/src/guide/models/basics.md Outdated Show resolved Hide resolved
docs/src/guide/models/basics.md Outdated Show resolved Hide resolved
using Flux
julia> using Enzyme

julia> grad3e = Flux.gradient((x,p) -> p(x), Const(5.0), Duplicated(poly3s))
Copy link
Contributor

Choose a reason for hiding this comment

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

Are Const and Duplicated from Enzyme? What does Duplicated do? What would happen if I called the gradient function exactly how I would when used with Zygote but after loading Enzyme?

(I don't actually know the answer to these questions)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the useful comments above!

Yes these types are from Enzyme, which easy to make clear via using Enzyme: Const, Duplicated.

The full description is its own page https://fluxml.ai/Flux.jl/previews/PR2535/reference/training/enzyme/
That should be linked here, but maybe we don't want too much detail -- except to say clearly that these act as flags to use Enzyme.

The order of loading should not matter, but I agree it would be nice to make that 100% obvious too.

@mcabbott
Copy link
Member Author

mcabbott commented Dec 4, 2024

make sure all code blocks are properly annotated with jldoctest

Anyone recall how to make a regex which matches all floats, positive & negative? (For documenter's filter.) Not like this, as it needs to ignore all the random numbers printed out:

julia> pat = r"-?[0-9]\d*(\.\d+)";

julia> match(ans, "Layer(Float32[-1.4319131 -0.09954874 1.9806088; -2.2309866 1.5175914 -0.98129755], Float32[0.0, 0.0], sigmoid)")
RegexMatch("-1.4319131", 1=".4319131")

@christiangnrd
Copy link
Contributor

I don't know if this is an option in this situation, but when I wrote the doctests for Metal rand functionality, I specified a seed so the results would match.

https://github.com/JuliaGPU/Metal.jl/blob/3fd6c06456dfdea8fa73483d396178a3202db666/docs/src/usage/array.md?plain=1#L3-L13

@mcabbott mcabbott force-pushed the intro_docs branch 3 times, most recently from e730716 to baeee9c Compare December 4, 2024 21:36
Copy link

codecov bot commented Dec 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 32.69%. Comparing base (74c3a63) to head (3c842e0).
Report is 4 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2535   +/-   ##
=======================================
  Coverage   32.69%   32.69%           
=======================================
  Files          34       34           
  Lines        1976     1976           
=======================================
  Hits          646      646           
  Misses       1330     1330           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mcabbott
Copy link
Member Author

mcabbott commented Dec 8, 2024

Ok let's do this. With apologies to Mike for deleting more of his very concise text.

Thanks for comments above, and help trying to dig up regex. (I copied some line from elsewhere in these docs, in the end.)

@mcabbott mcabbott merged commit 8c60006 into master Dec 8, 2024
9 of 11 checks passed
@mcabbott mcabbott deleted the intro_docs branch December 8, 2024 17:41
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.

3 participants