Skip to content

Better docs for reexported packages #2046

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

Merged
merged 6 commits into from
Aug 29, 2022

Conversation

Saransh-cpp
Copy link
Member

Closes #2038

This PR -

Discussion

On the other hand, MLUtils does deserve a tutorial page on an "intro to data loading" directly in the Flux docs

There is a tutorial (or more precisely, a getting-started/how-to guide) on DataLoader available here - https://fluxml.ai/tutorials/2021/01/21/data-loader.html. Should this be shifted to the new "Getting Started" section?

Originally posted by @Saransh-cpp in #2038 (comment)

PR Checklist

  • Tests are added
  • Entry in NEWS.md
  • Documentation, if applicable

@Saransh-cpp
Copy link
Member Author

Saransh-cpp commented Aug 22, 2022

TODO:

Comment on lines 1 to 4
# Functor from Functors.jl

Flux makes use of the [Functors.jl](https://github.com/FluxML/Functors.jl) to represent many of the core functionalities it provides.

Copy link
Member

@mcabbott mcabbott Aug 22, 2022

Choose a reason for hiding this comment

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

This seems a bit vague to me... first, this use of "functor" belongs to a tiny nice of functional programming, I don't think it's worth putting any emphasis on that here, it's just some package with a weird name. (I mean the title, and the paragraph just below where I can comment.)

Second, I don't know what "many of the core functionalities" means. I wrote something but maybe giving examples (params, training, gpu) would be nice too.

Suggested change
# Functor from Functors.jl
Flux makes use of the [Functors.jl](https://github.com/FluxML/Functors.jl) to represent many of the core functionalities it provides.
# Recursive transformations from Functors.jl
Flux models are deeply nested structures, and [Functors.jl](https://github.com/FluxML/Functors.jl) provides tools needed to explore such objects, apply functions to the parameters they contain, and re-build them.

I can't make suggestions below this, but (IMO) it might also be worth separating the list of functions according to level of obscurity. Maybe @functor, functor, isleaf, fmap should be in one block, the rest in another? Or just @functor, fmap as the top? Not sure, maybe it's too messy to make such a division.

Copy link
Member Author

Choose a reason for hiding this comment

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

The number of docstrings looks too few for a further division; should I still divide them?

Copy link
Member

@ToucheSir ToucheSir Aug 24, 2022

Choose a reason for hiding this comment

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

I would think of and describe functors like you would the module system of any other library. Because while it is a general-purpose library, that's what we use it for in Flux. One way to do this could be to show practical examples of using @functor etc. to define layers, and then explain what is happening along with why things are done this way. If that last part is too much, just the examples, module system mention and a link to the advanced model building page could suffice.

@@ -1,4 +1,4 @@
# MLUtils.jl
# Working with data using MLUtils.jl
Copy link
Member

Choose a reason for hiding this comment

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

I can't comment below, but flatten appears on this page (as it should), and also here:

https://fluxml.ai/Flux.jl/latest/models/layers/#Flux.flatten

Should it be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yes! Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Correction: MLUtils.unsqueeze cross-references MLUtils.flatten, and the doctests fail if I remove MLUtils.flatten's reference from the docs.

Copy link
Member

@mcabbott mcabbott left a comment

Choose a reason for hiding this comment

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

This looks like a good step forwards, thanks!

"NNlib" => "models/nnlib.md",
"Functors" => "models/functors.md"
"Neural Network primitives from NNlib.jl" => "models/nnlib.md",
"Functor from Functors.jl" => "models/functors.md"
Copy link
Member

Choose a reason for hiding this comment

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

One more thought. Should Zygote.jl be among the packages which gets a sidebar heading?

@mcabbott
Copy link
Member

Let's merge this and refine more if we need to, later. Thanks!

@mcabbott mcabbott merged commit 276e372 into FluxML:master Aug 29, 2022
@Saransh-cpp Saransh-cpp deleted the reexported-docs branch August 29, 2022 17:00
@Saransh-cpp
Copy link
Member Author

Thanks for the review! I missed the Zygote.jl suggestion, but I can add it in another PR.

Saransh-cpp added a commit to Saransh-cpp/Flux.jl that referenced this pull request Aug 29, 2022
* remove linear_regression.jl (commited by mistake)

* @ToucheSir's comments

* Update Functor.jl's sidebar title
@Saransh-cpp Saransh-cpp mentioned this pull request Aug 29, 2022
3 tasks
ToucheSir added a commit that referenced this pull request Aug 29, 2022
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.

[Discussion]: documentation for @reexported and imported (or using) packages
3 participants