-
Notifications
You must be signed in to change notification settings - Fork 339
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
Replace sphinx-gallery with nbsphinx #792
Conversation
This uses sphx_glr_python_to_jupyter.py from sphinx-gallery. Also included is an automatic pass over the notebooks.
also bring back load_style from sphinx_gallery for the new example gallery
Hello @StanczakDominik! Thanks for updating your pull request.
Comment last updated at 2020-05-20 08:38:24 UTC |
docs/conf.py
Outdated
# The version info for the project you're documenting, acts as replacement for | ||
# |version| and |release|, also used in various other places throughout the | ||
# built documents. | ||
# | ||
# The full version, including alpha/beta/rc tags. | ||
# Note: If plasmapy.__version__ can not be defined then it is set to 'unknown'. | ||
# However, release needs to be a semantic style version number, so set | ||
# the 'unknown' case to ''. | ||
from plasmapy import __version__ as release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's spooky, but it looks like black
moved this here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is definitely a PEP8 thing. I had the same warnings on my IDE. I'm torn about this though...yes having it at the top satisfies PEP8, but having it where it was is more readable IMHO.
It might be best to just move the full release
and version
declaration here.
|
||
.. topic:: Examples: | ||
|
||
* :ref:`sphx_glr_auto_examples_plot_dispersion_function.py` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This had moved away from mathematics
a while ago.
Codecov Report
@@ Coverage Diff @@
## master #792 +/- ##
=======================================
Coverage 95.87% 95.87%
=======================================
Files 56 56
Lines 5113 5113
=======================================
Hits 4902 4902
Misses 211 211 Continue to review full report at Codecov.
|
This solves the sidebar issue of examples appearing at the start of the TOC.
This helps signify that, in contrast to sphinx_gallery, all notebooks are executed at runtime. In sphinx_gallery, only notebooks with the previously configured prefix (plot_, here) would be executed and their output displayed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far. This is a review of everything, but the Jupyter Notebooks. I'm gonna go through those next.
- For the
make clean
command definition in theMakefile
, we can remove the part aboutauto_examples
since we no longer generate them.
.. raw:: html | ||
|
||
<div style="width: 30%;" class="sphx-glr-thumbcontainer" tooltip="Let's try to look at ITER plasma conditions using the physics subpackage. "> | ||
|
||
.. only:: html | ||
|
||
.. figure:: /auto_examples/images/thumb/sphx_glr_plot_physics_thumb.png | ||
|
||
:ref:`sphx_glr_auto_examples_plot_physics.py` | ||
|
||
.. raw:: html | ||
|
||
<div class="sphx-glr-thumbcontainer"> | ||
<div class="figure align-center"> | ||
<img alt="thumbnail" src="./_images/notebooks_physics_16_0.png" /> | ||
<p class="caption"> | ||
<span class="caption-text"> | ||
<a class="reference internal" href="notebooks/physics.html"> | ||
<span class="std std-ref">Analysing ITER parameters</span> | ||
</a> | ||
</span> | ||
</p> | ||
</div> | ||
|
||
.. raw:: html | ||
|
||
<div style="width: 30%;" class="sphx-glr-thumbcontainer" tooltip="Let's import some basics (and PlasmaPy!) "> | ||
|
||
.. only:: html | ||
|
||
.. figure:: /auto_examples/images/thumb/sphx_glr_plot_dispersion_function_thumb.png | ||
|
||
:ref:`sphx_glr_auto_examples_plot_dispersion_function.py` | ||
|
||
.. raw:: html | ||
|
||
</div> | ||
<div class="sphx-glr-thumbcontainer"> | ||
<div class="figure align-center"> | ||
<img alt="thumbnail" src="./_images/notebooks_dispersion_function_9_0.png" /> | ||
<p class="caption"> | ||
<span class="caption-text"> | ||
<a class="reference internal" href="notebooks/dispersion_function.html"> | ||
<span class="std std-ref">The plasma dispersion function</span> | ||
</a> | ||
</span> | ||
</p> | ||
</div> | ||
|
||
.. raw:: html | ||
|
||
<div style="width: 30%;" class="sphx-glr-thumbcontainer" tooltip="Let's analyze a few Langmuir probe characteristics using the diagnostics.langmuir subpackage. F..."> | ||
|
||
.. only:: html | ||
|
||
.. figure:: /auto_examples/images/thumb/sphx_glr_plot_langmuir_analysis_thumb.png | ||
|
||
:ref:`sphx_glr_auto_examples_plot_langmuir_analysis.py` | ||
|
||
.. raw:: html | ||
|
||
</div> | ||
<div class="sphx-glr-thumbcontainer"> | ||
<div class="figure align-center"> | ||
<img alt="thumbnail" src="./_images/notebooks_langmuir_analysis_6_1.png" /> | ||
<p class="caption"> | ||
<span class="caption-text"> | ||
<a class="reference internal" href="notebooks/langmuir_analysis.html"> | ||
<span class="std std-ref">Langmuir probe data analysis</span> | ||
</a> | ||
</span> | ||
</p> | ||
</div> | ||
</div> | ||
<div class="sphx-glr-clear"></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just use the .. nbsphinx::
directive for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried and it started populating the TOC with these three examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you happen to know a way to do a nbsphinx directive without the TOC entries, it'd be much appreciated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try :hidden
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be an option...https://sphinx-rtd-theme.readthedocs.io/en/stable/configuring.html#confval-includehidden. This would prevent hidden toctree
's from adding to the sidebar. The trade-off, we can no longer hide our toctree
's in the main index
page.
I think both ways have comparable PROs and CONs that make them equally nice and annoying. I'll leave the choice up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's tinker with it another day, if that's all right with you. :) I'd love to apply a fix that doesn't propagate elsewhere to this, but I won't lose sleep over skipping it for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did semi-thorough comparison and it looks like all the files were properly translated to notebooks. Excellent work! That must have been exhausting to do.
Do you know if there's an easy way to enlarge the plots across the board? The plots generated here look to be about 80% smaller than what sphinx_gallery
generated.
"cell_type": "markdown", | ||
"metadata": {}, | ||
"source": [ | ||
"They also change with direction with respect to the magnetic field. Here,\n", | ||
"we choose to print out, as arrays, the (parallel, perpendicular,\n", | ||
"and cross) directions. Take a look at the docs to `ClassicalTransport`\n", | ||
"for more information on these.\n", | ||
"\n" | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This becomes an interesting choice/dilemma. What do we do if we want ClassicalTransport
to link back to the documentation for the class? I currently see three choices:
- Don't link back to the class/function/package/etc. documentation?
- Use markdown style linking, like
[ClassicalTransport](../api/plasmapy.formulary.braginskii.ClassicalTransport.rst)
. However, this style of linking does not support code highlighting, e.g.[`ClassicalTransport`](...)
. - Mark the
cell_type
asraw
with a restructuredtext metadata and then we could sill use the sphinx domains (i.e.:class:`~plasmapy.formulary.braginskii.ClassicalTransport`
). In this case the cell would look something like...
"cell_type": "raw",
"metadata": {"raw_minetype": "text/restructuredtext"},
"source": [
"They also change with direction with respect to the magnetic field. Here,\n",
"we choose to print out, as arrays, the (parallel, perpendicular,\n",
"and cross) directions. Take a look at the docs to \n"
":class:`~plasmapy.formulary.braginskii.ClassicalTransport`\n",
"for more information on these.\n",
"\n"
]
Personally, I like option 3 because it keeps the style consistent in all of the documentation, but it has issues too. It takes more effort (not much) to setup. I'm not sure how this would behave if there were more advanced roles involved. Also, it might render/look a little weird if someone were to download the notebook and use it outside the documentation environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was an issue I found about it, lemme grab breakfast and I'll find it for you...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rocco8773 here's the issue I had wanted to find for you:
spatialaudio/nbsphinx#308 (comment)
I'm trying to get this implemented here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, future issue, maybe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's basically option 2. I'm fine with leaving this to another day, but we should at least open an issue about linking API in notebooks.
Not really, sphinx-gallery provides a script to do that! I don't remember it right now but it's in one of the commit messages.
We could add a matplotlib rc line to all of them... I'll look into it. Thanks for noticing, I completely missed it. |
And thank you very much for the review! :) |
Co-authored-by: Erik Everson <[email protected]>
Co-authored-by: Erik Everson <[email protected]>
Mostly done, I think. I couldn't get |
Guess I'll be merging this one, then! Thanks a lot for the feedback @rocco8773 :) ... Oh right, blocked on review approval, can I get that final green light from you? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. My only addition is my comment about adding a no execute option to tox
and mentioning the options in the Documentation Guidelines.
Nice Job! This setup will be real useful in the future.
docs/Makefile
Outdated
html-nonb: | ||
$(SPHINXBUILD) -D nbsphinx_execute='never' -b html $(ALLSPHINXOPTS) $(BUILDDIR)/html | ||
@echo | ||
@echo "Build finished. The HTML pages are in $(BUILDDIR)/html." | ||
@echo "Build finished. The HTML pages are in $(BUILDDIR)/html. Jupyter notebooks were skipped." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have a command like this in the tox
build_docs
environment as well. These options should also be mentioned in the documentation itself, probably in the Documentation Guidelines section.
Which I noted and promptly forgot about. Will add. |
The tox environment does not use `make html-nonb` because tox envs, by default, don't have `make`! We call the sphinx command directly instead. The Makefile was also broken. It's now fixed.
This pull request:
docs/notebooks
black
toconf.py
.I'd suggest reviewing this by individual commits, as I changed a good bunch of files and it probably makes more sense to look at them chronologically.
TODO