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

added auto gen documentation and tutorials #27

Merged
merged 11 commits into from
Sep 21, 2022
Merged

added auto gen documentation and tutorials #27

merged 11 commits into from
Sep 21, 2022

Conversation

weidel-p
Copy link
Contributor

Added autogenerted documentation for Lava and adapted the "Getting started" page to display tutorials directly.

This PR requires lava-nc/lava#318 to be merged.

Copy link
Contributor

@bamsumit bamsumit left a comment

Choose a reason for hiding this comment

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

Looks awesome Phillip. Great work.

Just one comment. It looks like something is wrong with latex renderer. The equations are not rendered somehow.
image

@bamsumit
Copy link
Contributor

I haven't tried to build it myself. Will try that sometime later and let you know if there are any feedback.

@weidel-p
Copy link
Contributor Author

weidel-p commented Sep 5, 2022

In the last commit I addressed the issue with rendering the Latex equation.
Additionally, I added the auto-generated documentation for lava-optimization.

@bamsumit
Copy link
Contributor

bamsumit commented Sep 7, 2022

In the last commit I addressed the issue with rendering the Latex equation. Additionally, I added the auto-generated documentation for lava-optimization.

Great. Looks good to me then.

Copy link
Contributor

@mathisrichter mathisrichter left a comment

Choose a reason for hiding this comment

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

So far, I only browsed the changes on Github. Will have a look at the built documentation once available.

Now I looked through the generated website:

  • Under "Lava's foundational concepts", remove the colons after "1. Processes:, 2. Behavioral implementations via ProcessModels: ..."
  • More white-space below figures.
  • The capitalization of headings is inconsistent. Either go with all words capitalized "Getting Started With Lava", only beginning and names capitalized "Getting started with Lava" or only "helper words" (I'm sure that has some fancy name) capitalized "Getting Started with Lava".
  • Under "Getting started" -> "Fundamental concepts" -> Installing Lava, below "2. Getting started", this should not be a heading: When running tests if you see ‘OSError: [Errno 24] Too many open files’
  • I don't think "Installing Lava" should be under "Fundamental concepts" but rather it's own heading under "Getting started".
  • I suggest to not duplicate the instruction on how to clone and setup Lava here but rather point to the README.md, where this is explained. Or pull in that information here. If we have to maintain multiple places, it will get out of sync.
  • The instructions on how to set up Lava on Loihi should similarly be available in the private Lava repository and pulled in from there, possibly.
  • The instructions for Lava on Loihi seem to have too little detail. Rather than referring to an old email, why not put that information in the README.md or the website? What are the "Intel external vlab Machines"? How do I log in. The instructions seem to suggest to just install the release artifact with pip. Does this not require that the Lava virtual environment is set up and activated and poetry installed?
  • Under "Application examples" there is the MNIST tutorial, which is a bit of a stretch already, but okay. But the "Walk through Lava" should not be listed here. It is too basic to be called an "application".
  • Under "Dynamic neural fields" there are links in markdown syntax: (e.g., using cedar or cosivina) that are just copied in.
  • Can we get rid of the "Development roadmap" and the table under "Initial release" entirely? In its current outdated form, it does not distinguish between what is currently available and what we will develop in the future.
  • The list of Project committers is out of date. jlakness-intel no longer works at NCL.
  • Get rid of the extra 'Table of content' in the Developers guide.
  • There is a missing white space in the "will come soon" notice for the lava-dnf API documentation: API documentation will arrive shortly. In the mean time, please reviewdocstrings in code.

getting_started/end_to_end_tutorials.rst Outdated Show resolved Hide resolved
getting_started/in_depth_tutorials.rst Outdated Show resolved Hide resolved
lava/lava.rst Outdated Show resolved Hide resolved
lava/lava.rst Outdated Show resolved Hide resolved
@GaboFGuerra
Copy link
Contributor

Thanks Philipp, this is looking great!

I just noticed the module_path in sync_notebook.py does not take into account the "src/lava-nc" when looking for Lava tutorials under .venv/src/lava-nc, this is the resulting folder structure when you do poetry install under the inner-source Lava repo.

Also had to run "pip install nbsphinx" not sure if it is already on the requirements somewhere.

README.md Show resolved Hide resolved
@weidel-p
Copy link
Contributor Author

I worked on your comments and implemented them as it was possible.

So far, I only browsed the changes on Github. Will have a look at the built documentation once available.

Now I looked through the generated website:

  • Under "Lava's foundational concepts", remove the colons after "1. Processes:, 2. Behavioral implementations via ProcessModels: ..."

done.

  • More white-space below figures.

I tried to change from image to figure, which apparently adds some white space below.

  • The capitalization of headings is inconsistent. Either go with all words capitalized "Getting Started With Lava", only beginning and names capitalized "Getting started with Lava" or only "helper words" (I'm sure that has some fancy name) capitalized "Getting Started with Lava".

Done.

  • Under "Getting started" -> "Fundamental concepts" -> Installing Lava, below "2. Getting started", this should not be a heading: When running tests if you see ‘OSError: [Errno 24] Too many open files’

Done.

  • I don't think "Installing Lava" should be under "Fundamental concepts" but rather it's own heading under "Getting started".

I flattened the hierarchy there. There is no distinguishment between in-depth and end-to-end tutorials.

  • I suggest to not duplicate the instruction on how to clone and setup Lava here but rather point to the README.md, where this is explained. Or pull in that information here. If we have to maintain multiple places, it will get out of sync.

I adapted them such that they are not getting outdated that fast anymore. But I hesitate to delete a complete tutorial.

  • The instructions on how to set up Lava on Loihi should similarly be available in the private Lava repository and pulled in from there, possibly.

The private lava repo is not covered here.

  • The instructions for Lava on Loihi seem to have too little detail. Rather than referring to an old email, why not put that information in the README.md or the website? What are the "Intel external vlab Machines"? How do I log in. The instructions seem to suggest to just install the release artifact with pip. Does this not require that the Lava virtual environment is set up and activated and poetry installed?

See above.

  • Under "Application examples" there is the MNIST tutorial, which is a bit of a stretch already, but okay. But the "Walk through Lava" should not be listed here. It is too basic to be called an "application".

See above.

  • Under "Dynamic neural fields" there are links in markdown syntax: (e.g., using cedar or cosivina) that are just copied in.

Done.

  • Can we get rid of the "Development roadmap" and the table under "Initial release" entirely? In its current outdated form, it does not distinguish between what is currently available and what we will develop in the future.

I need more detailed feedback on how it should look like,

  • The list of Project committers is out of date. jlakness-intel no longer works at NCL.

I removed jlakness-intel but need more feedback on the current status, maybe @mgkwill can update that list?

  • Get rid of the extra 'Table of content' in the Developers guide.

Why?

  • There is a missing white space in the "will come soon" notice for the lava-dnf API documentation: API documentation will arrive shortly. In the mean time, please reviewdocstrings in code.

Done.

@mgkwill mgkwill merged commit 9917a3e into main Sep 21, 2022
@mgkwill mgkwill deleted the doc/auto-api branch September 21, 2022 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants