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

Sphinx documentation #155

Merged
merged 1 commit into from
Jan 19, 2022
Merged

Sphinx documentation #155

merged 1 commit into from
Jan 19, 2022

Conversation

horaceg
Copy link
Contributor

@horaceg horaceg commented Dec 31, 2021

Hello,

I don't know much about Blackjax but thought I could help setup a sphinx doc seing #154

Build with e.g. sphinx-build -b html docs docs/_build/html and then open docs/_build/html/index.html

Current problems:

  • The modules structure is a bit large, so I only documented part of it for now (see docs/api.rst).
  • NamedTuple: the parameters are thrice in the documentation: once as properties, once as parameters, and once for type hints
  • References don't work
  • The HierarchicalBNN notebook contains base64-encoded images that don't play well with myst-nb. I thus removed it from the doc for now, but it is still parsed so it slows the process down.
  • The use_with_pymc3 notebook has incorrect header sections, so it messes with the notebook stuff
  • The Github README is included but with the name Blackjax, so it is not very explicit

@rlouf
Copy link
Member

rlouf commented Dec 31, 2021

Wow that's amazing! Is there any chance you could add a github workflow to build the doc automatically whenever something is pushed on main? There should be a recipe already in mcx

@rlouf
Copy link
Member

rlouf commented Jan 1, 2022

I see you are using myst-nb to include notebooks in the documentation. Is there anyway we could do the same for notebooks in a jupytext format?

I eventually want to get rid of .ipynb files in the the repo but want to keep a rendered version of the example notebooks somewhere.

@horaceg
Copy link
Contributor Author

horaceg commented Jan 1, 2022

I see you are using myst-nb to include notebooks in the documentation. Is there anyway we could do the same for notebooks in a jupytext format?

I eventually want to get rid of .ipynb files in the the repo but want to keep a rendered version of the example notebooks somewhere.

I think it is possible given https://myst-nb.readthedocs.io/en/latest/examples/custom-formats.html#using-jupytext

However in jupytext format there is no output so they will need to be run every time the documentation is generated, which I guess is not very convenient if you need to run them on beefy machines or if the execution time is large. Is it the case ?

There are some options to do caching of this stuff, but I guess it would be hard to share the cache across machines : https://myst-nb.readthedocs.io/en/latest/use/execute.html#triggering-notebook-execution

It seems that JAX is doing this, but it adds a layer of complexity to the process https://jax.readthedocs.io/en/latest/developer.html#update-documentation

@rlouf
Copy link
Member

rlouf commented Jan 1, 2022

It does add a layer of complexity, but having .ipynb in the repo is already hard to manage collaboration-wise. I suggest we do it in another PR as this is a separate concern. I was just curious.

Let me know when I can review the PR and/or if I can help you with anything!

@horaceg horaceg marked this pull request as ready for review January 2, 2022 20:15
@rlouf
Copy link
Member

rlouf commented Jan 19, 2022

@horaceg I finally had time to look at your PR in details. This looks great for a first pass! Since I am a bit more familiar woth BlackJAX I'll take it from there for the content of the API reference and the front page. I'll have a look at the outstanding issues as well.

Thank you for your help!

@codecov
Copy link

codecov bot commented Jan 19, 2022

Codecov Report

Merging #155 (3f51177) into main (6f728a9) will not change coverage.
The diff coverage is n/a.

❗ Current head 3f51177 differs from pull request most recent head 094748a. Consider uploading reports for the commit 094748a to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main     #155   +/-   ##
=======================================
  Coverage   98.56%   98.56%           
=======================================
  Files          25       25           
  Lines         977      977           
=======================================
  Hits          963      963           
  Misses         14       14           
Impacted Files Coverage Δ
blackjax/adaptation/step_size.py 100.00% <ø> (ø)
blackjax/diagnostics.py 100.00% <ø> (ø)
blackjax/nuts.py 100.00% <ø> (ø)
blackjax/stan_warmup.py 96.11% <ø> (ø)
blackjax/tempered_smc.py 100.00% <ø> (ø)
blackjax/types.py 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f728a9...094748a. Read the comment docs.

@rlouf rlouf force-pushed the sphinx branch 2 times, most recently from 098d344 to 094748a Compare January 19, 2022 17:02
@rlouf rlouf merged commit 98fc6b0 into blackjax-devs:main Jan 19, 2022
@rlouf
Copy link
Member

rlouf commented Jan 19, 2022

It is not perfect but it is already a net improvement to the library, so merging. Thank you again for your help.

rlouf pushed a commit that referenced this pull request Jan 19, 2022
@rlouf rlouf mentioned this pull request Feb 4, 2022
@rlouf
Copy link
Member

rlouf commented Sep 22, 2022

Just a quick note to say that I've played with the documentation recently, and MyST-NB was a very good pick. Thank you again!

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.

2 participants