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

Tutorial & documentation #5

Open
psteinb opened this issue Feb 8, 2021 · 15 comments
Open

Tutorial & documentation #5

psteinb opened this issue Feb 8, 2021 · 15 comments
Assignees
Labels
help wanted Extra attention is needed

Comments

@psteinb
Copy link
Collaborator

psteinb commented Feb 8, 2021

What?

The auto-generated docs for the API and modules are old, and not regularly refreshed. It's also very hard to navigate, and all the inherited torch.nn.Module methods are also included. The tutorial is just a long README, but would be better in separate files on different topics.

How?

  • Fix the auto-generated docs (maybe sphinx would work better?)
  • Split the README into multiple .md or .rst files for the tutorial
  • Improve the tutorial by including new modules (e.g. AllInOneBlock),
    and especially ReversibleSequential which is fine for 80% of cases and much simpler to understand.
  • Add the missing sections about how to write your own modules.
  • For the final section of tips, we have better knowledge on a lot of things now
  • Perhaps the tutorial can be included into the online documentation

Difficulty? 2/5

@psteinb psteinb added the help wanted Extra attention is needed label Feb 8, 2021
@wapu wapu self-assigned this Feb 8, 2021
@wapu
Copy link
Collaborator

wapu commented Feb 9, 2021

If we're thinking about restructuring some the modules and the way Jacobian logdets are passed along the structure, the documentation should probably wait for that decision to be made.

@tbung
Copy link
Collaborator

tbung commented Feb 9, 2021

I'd suggest we decide on a docstring format that sphinx understands relatively early, so we can start writing docstrings as we make changes (see also the discussion in vislearn/FrEIA#54).

As far as I can tell the most common formats are:

The last two are also contrasted on the sphinx extension page. Pytorch uses the Google format.

@psteinb
Copy link
Collaborator Author

psteinb commented Feb 10, 2021

needless to say, @tbung brings up a good point. The docstring format should be chosen in light of the HTML rendering engine that will display the tutorial and potentially a API reference.

  • sphinx was already mentioned
  • mkdocs has seen some popularity too

if no decision between can be reached, you can always go with vanilla jekyll offered by github pages. How code is rendered by this engine, is something beyond my knowledge.

@ardizzone
Copy link
Member

To me, the Google docstring format is most legible.
https://google.github.io/styleguide/pyguide.html#Comments
I would just go ahead and start using it (I promise I will go through and change everything if we decide otherwise)

And for both sphinx and mkdocs there are ways of parsing them (https://github.com/mkdocstrings/mkdocstrings)

@wapu
Copy link
Collaborator

wapu commented Feb 10, 2021

I also like the Google format.
Any preference between sphinx and mkdocs? Seems like mkdocs is easier to use and sphinx has more features, but both claim to have plugins which replicate the advantages of the other...

@psorrenson
Copy link

psorrenson commented Feb 10, 2021

I am beginning work on fixing up the tutorial. Here is my rough plan:

  • section on how to write your own module
  • add example using ReversibleSequential (replace current 'Simple GLOW-based INN' example)
  • simplify Conditional INN example to use ReversibleSequential
  • add example using AllInOneBlock
  • add Quick Start section/guide at beginning
  • remake INN diagram with arrows and using names of actually implemented transformations
  • rewrite corresponding code to match and make it runable
  • fix typos/grammar
  • restructure: new .rst for Quick Start and new .rst for full tutorial (or maybe Quick Start can stay on the readme?)
  • at end of hackathon: fix any broken demo code

Anything else or something I should do differently?

@ardizzone
Copy link
Member

That sounds perfect! I think it's definitely safe to split it up into multiple .rst or .md files (e.g. one on writing your own InvertibleModule, etc.), so it becomes easier to navigate once it's online.

Also perhaps a small chapter on the basic concept of invertible computation graphs, and that the Reversible* graph wraps such a graph in a torch.nn.Module, and you can access the condition, input, output nodes by calling .forward()?
(I don't know if that would be useful or not)

@wapu
Copy link
Collaborator

wapu commented Feb 10, 2021

We should definitely provide a clean code example with the full training loop, both unsupervised and conditional, with explanation where adaptations to custom data sets/problems need to be made. But that can be added in the end.

@tbung
Copy link
Collaborator

tbung commented Feb 11, 2021

I decided on Sphinx because it seems more stable and battle-tested, also Sphinx allows interdoc links, where we can define some external documentations we want to reference (for example PyTorch) and then easily link to entries in those docs. I also found more projects using Sphinx to draw inspiration from.

We can still use markdown instead of RST, if you prefer.

@tbung
Copy link
Collaborator

tbung commented Feb 11, 2021

Do we want a github action to generate and publish docs? If yes, do we also want that to run on every change or do we want to do some kind of release cycle and only update docs on release?

In my opinion automatic docs on releases are the best idea, as long as the project is relatively small and we actually do releases.

@ardizzone
Copy link
Member

Sounds good! I think we were planning to do a release once we're done with everything (that would be "0.3" I guess)

@tbung
Copy link
Collaborator

tbung commented Feb 11, 2021

I have finished a basic setup (see vislearn/FrEIA#68). Points up for discussion are:

  • Should docs be automatically rebuilt on every push
  • Should built docs be stored in and served from the master branch or a separate branch/repo
  • The theme
  • The structure

@ardizzone
Copy link
Member

🤩 ooh, that looks very very nice!
One thing I noticed: For the modules, the __init__() and the class itself both have a docstring (the former explaining the constructor args, the latter explaining what the module does, giving paper references and so son)
Right now, in the docs, only the class docstring is given.
Is it better if I put the constructer arguments under the class docstring?
Or is there a way for the docs to list both?

It's fine if we collect opinions on the theme, structure, how to serve, but I would give you the mandate to just decide it in the end if no-one has strong thoughts on it.

@tbung
Copy link
Collaborator

tbung commented Feb 12, 2021

I have included __init__ functions now, might take a moment to update on Github pages.

Yes, I think I picked sane defaults for our situation, so if nobody has any opinion on those points we can go ahead and merge the PR in a bit.

@psorrenson
Copy link

psorrenson commented Feb 15, 2021

Update on tutorial:

  • I am pretty happy with where it stands and will stop working on it for the time being
  • still requires restructuring into separate files / incorporation into docs if desired
  • final section on tips and tricks needs updating @ardizzone @wapu
  • someone else should proofread for mistakes or improvements
  • it might be good to advertise SequenceINN more prominently, since it will be enough for most new users
  • code example based on complicated INN diagram runs but there are a lot of NaNs in output, so the assert fails. NaNs seem to be coming from the output of the coupling block. Does someone want to check this out?
  • final code example using custom modules does not invert as expected. I have checked both custom modules and as far as I can see both are inverting as expected, so the problem may be with the GraphINN algorithm itself. Someone should check this out @fdraxler @wapu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants