-
-
Notifications
You must be signed in to change notification settings - Fork 608
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 a quick start example, and change some headings #2069
Conversation
Once the build has completed, you can preview any updated documentation at this URL: https://fluxml.ai/Flux.jl/previews/PR2069/ in ~20 minutes |
Codecov ReportBase: 86.26% // Head: 86.26% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## master #2069 +/- ##
=======================================
Coverage 86.26% 86.26%
=======================================
Files 18 18
Lines 1361 1361
=======================================
Hits 1174 1174
Misses 187 187
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
I like the proposed structure here. Could the "Fitting a Line" page mention "linear regression" somewhere to contextualize the problem and help with keyword searches? Haven't looked at the other docs changes yet. |
a026e5c
to
f3c68f6
Compare
One unhappy feature is that we have a bit of a random mix of sections describing stuff in words, and sections which are more API listings. Maybe the ought to be marked somehow, something like this? Perhaps we also need to take a pass and move some of the more scattered docstrings from "descriptive" sections to "listing" sections. Edit: done in bad3011, see what you think. This also re-orders sections a bit... maybe loss functions & regularisation are part of training, right? Maybe all the sections named for packages should name a function from that package, so that you have some idea why you may want to open that section, too. |
f3c68f6
to
b7389b5
Compare
f8d5cd0
to
45735b9
Compare
docs/src/assets/flux.css
Outdated
article .docstring pre { | ||
/* cancel negative spacing inside a docstring, as it collides with outer box */ | ||
margin-left: 0em; | ||
margin-right: 0em; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm all for removing the negative spacing. At the very least, it should be opt-in instead of opt-out.
Come to think of it, is there a need for this stylesheet at all? Looking through some SciML package docs, any deviations from the default Documenter theme seem minimal and thus the maintenance overhead not very justifiable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think it's nice if distinct packages look a little different, if only to help you remember which tab is which... here this seems to mean being a bit green.
Would be better if all FluxML docs pulled the style sheet from one place though. ChainRules et al moved theirs to a package https://github.com/JuliaDiff/DocThemeIndigo.jl but I'm not volunteering to set that up.
The one big visual change which would be nice would be to unify the website and the docs, like e.g. https://turing.ml/v0.21/docs/using-turing/quick-start . Maybe we can steal things from https://github.com/TuringLang/turinglang.github.io
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
03eba13 removes all negative spacing, looks fine.
docs/src/index.md
Outdated
* **Play nicely with others**. Flux works well with Julia libraries from [data frames](https://github.com/JuliaComputing/JuliaDB.jl) and [images](https://github.com/JuliaImages/Images.jl) to [differential equation solvers](https://github.com/JuliaDiffEq/DifferentialEquations.jl), so you can easily build complex data processing pipelines that integrate Flux models. | ||
* **Play nicely with others**. Flux works well with Julia libraries from [images](https://github.com/JuliaImages/Images.jl) to [differential equation solvers](https://github.com/JuliaDiffEq/DifferentialEquations.jl), so you can easily build complex data processing pipelines that integrate Flux models. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact https://github.com/JuliaData/JuliaDB.jl is long-dead. Should anything else be highlighted?
My vote is to make this intro shorter if possible, it's not all that helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make any sense to s/diffeq/sciml/g
for this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably! Link does still work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
docs/src/models/basics.md
Outdated
## Layer helpers | ||
## Layer Helpers | ||
|
||
Flux provides a set of helpers for custom layers, which you can enable by calling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact only one helper. Maybe this section made more sense long ago. Maybe it should include create_bias
alla #2049?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 98b4cd8
Shall we do this? It's not done but seems like a step forwards. Biggish changes not mentioned in the top message are:
Note that I didn't move any files, and thus the directory structure no longer matches the doc sections. We can move them once we are happy with the arrangement. Doing so will break links, though... I have added some explicit ID tags to titles, which I think are better than referring to the title text itself or the file name. But I have not attempted to go through and update all links. I think the next steps after this are
|
da5fb4f
to
23baf23
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this, @mcabbott! I do like the idea of separating out the API reference from the guide, but this can be taken up in another PR (if this PR is getting too large). Some minor comments below -
For some more helpful tricks, including parameter freezing, please checkout the [advanced usage guide](advanced.md). | ||
```@docs | ||
Functors.@functor | ||
Flux.create_bias |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminds me of #2049. We can get rid of _create_bias
now that create_bias
is a public API. Can be handled in another PR!
Co-authored-by: Saransh Chopra <saransh0701@gmail.com>
I think the hurdles to separating Guide / Reference are
Anyway, small steps forwards... |
Brings us in line with changes on the Flux side: FluxML/Flux.jl#2069
[Replaces #2068]
This wants to change the start of the docs, so that "Getting Started" has (after the index page) two paths: A fast one (for people who know about neural nets, just not Flux) and a slow one (introducing linear regressing, parameters, etc). The slow one was called "Overview" before.
The fast one is new. I'm not sure it's the optimal example, maybe MNIST can be done about as compactly? It aims to fit on one screen & show you all the main moving parts.
Re-reading "Overview", it's nicer than I remembered. I'm not entirely sure it should live in a different section to the next one, "Basics" (which I call "Gradients and Layers" here). So I moved the second "Basics" one into "Getting Started" too. (I do think they need better titles.)
I'm not precisely sure how this intersects with #2036, #2021, #2016. Those may replace text in some sections, to make the slow path even more detailed, or to upgrade the "Basics" section. Or maybe we want a tutorials section in the docs, which replaces the one in the website?