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 a quick start example, and change some headings #2069

Merged
merged 21 commits into from
Oct 11, 2022
Merged

Conversation

mcabbott
Copy link
Member

@mcabbott mcabbott commented Sep 27, 2022

[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?

@github-actions
Copy link
Contributor

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-commenter
Copy link

codecov-commenter commented Sep 27, 2022

Codecov Report

Base: 86.26% // Head: 86.26% // No change to project coverage 👍

Coverage data is based on head (da5fb4f) compared to base (dfc5a7e).
Patch has no changes to coverable lines.

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           
Impacted Files Coverage Δ
src/outputsize.jl 87.17% <ø> (ø)

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ToucheSir
Copy link
Member

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.

@mcabbott
Copy link
Member Author

mcabbott commented Sep 27, 2022

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?

Screenshot 2022-09-27 at 10 02 01

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.

article .docstring pre {
/* cancel negative spacing inside a docstring, as it collides with outer box */
margin-left: 0em;
margin-right: 0em;
Copy link
Member Author

Choose a reason for hiding this comment

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

Note the overlapping boxes here, on master:

Screenshot 2022-09-28 at 21 41 34

Maybe the negative space should just be removed entirely.

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member Author

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.

Comment on lines 8 to 7
* **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.
Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 214 to 216
## Layer helpers
## Layer Helpers

Flux provides a set of helpers for custom layers, which you can enable by calling
Copy link
Member Author

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 98b4cd8

@mcabbott
Copy link
Member Author

mcabbott commented Oct 10, 2022

Shall we do this? It's not done but seems like a step forwards.

Biggish changes not mentioned in the top message are:

  • The "advanced.md" page about custom layers is moved to the tutorials. It's a bit of a random selection of examples, and perhaps could be tidied up, but I didn't try.
  • New page for destructure, plus Flux.modules, as they didn't fit well elsewhere
  • Adding 📚 to mark sections which contain API listings. I think moving text / docstrings to make every page more cleanly one or the other class would make this better.

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

  • to re-organise the training / regularization / optimisers docs. But these are coupled to the move away from implicit parameters.
  • to start moving other tutorials here from the website. Coupled to work on the website.

@mcabbott
Copy link
Member Author

Adding 📚 to mark sections which contain API listings.

An alternative here would be to re-order sections into Guide / Reference. Maybe better? But right now the division isn't quite clean enough, IMO... the optimisers / training sections especially are a bit tangled.

Screenshot 2022-10-10 at 20 27 26

Copy link
Member

@Saransh-cpp Saransh-cpp left a 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 -

docs/src/data/mlutils.md Outdated Show resolved Hide resolved
For some more helpful tricks, including parameter freezing, please checkout the [advanced usage guide](advanced.md).
```@docs
Functors.@functor
Flux.create_bias
Copy link
Member

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!

docs/src/models/quickstart.md Outdated Show resolved Hide resolved
docs/src/models/quickstart.md Outdated Show resolved Hide resolved
docs/src/models/quickstart.md Outdated Show resolved Hide resolved
mcabbott and others added 2 commits October 11, 2022 10:50
Co-authored-by: Saransh Chopra <saransh0701@gmail.com>
Co-authored-by: Saransh Chopra <saransh0701@gmail.com>
@mcabbott
Copy link
Member Author

mcabbott commented Oct 11, 2022

I think the hurdles to separating Guide / Reference are

  • that the order of reference starts to seem completely random, is there a better way? maybe moving all the "foreign" pages to "Reference, II" section would help?
  • that quite a few sections currently are half in each camp, training / optimisers etc. But these will change for 0.14 anyway.

Anyway, small steps forwards...

@mcabbott mcabbott merged commit b08cb67 into master Oct 11, 2022
@mcabbott mcabbott deleted the doc_sections2 branch October 11, 2022 14:58
ToucheSir added a commit to FluxML/NNlib.jl that referenced this pull request Oct 22, 2022
Brings us in line with changes on the Flux side: FluxML/Flux.jl#2069
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.

4 participants