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

Replace sphinx-gallery with nbsphinx #792

Merged
merged 32 commits into from
May 24, 2020

Conversation

StanczakDominik
Copy link
Member

@StanczakDominik StanczakDominik commented Apr 25, 2020

This pull request:

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

  • Can nbsphinx decide on pre-execution based on prefix?

@pep8speaks
Copy link

pep8speaks commented Apr 25, 2020

Hello @StanczakDominik! Thanks for updating your pull request.

Line 197:100: E501 line too long (113 > 99 characters)
Line 206:100: E501 line too long (150 > 99 characters)
Line 208:100: E501 line too long (221 > 99 characters)

Comment last updated at 2020-05-20 08:38:24 UTC

docs/conf.py Outdated
Comment on lines 24 to 32
# 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
Copy link
Member Author

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.

Copy link
Member

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.

Comment on lines -9 to -13

.. topic:: Examples:

* :ref:`sphx_glr_auto_examples_plot_dispersion_function.py`

Copy link
Member Author

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.

docs/index.rst Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 25, 2020

Codecov Report

Merging #792 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           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.

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

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.
@rocco8773 rocco8773 added this to the v0.4.0 milestone May 15, 2020
@rocco8773 rocco8773 added the docs PlasmaPy Docs at http://docs.plasmapy.org label May 15, 2020
@StanczakDominik StanczakDominik added the status: ready for review PRs that are ready for code review label May 17, 2020
Copy link
Member

@rocco8773 rocco8773 left a 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 the Makefile, we can remove the part about auto_examples since we no longer generate them.

.circleci/config.yml Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
docs/conf.py Show resolved Hide resolved
docs/development/code_guide.rst Outdated Show resolved Hide resolved
docs/formulary/index.rst Outdated Show resolved Hide resolved
docs/formulary/parameters.rst Outdated Show resolved Hide resolved
Comment on lines 9 to +47
.. raw:: html

<div style="width: 30%;" class="sphx-glr-thumbcontainer" tooltip="Let&#x27;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&#x27;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&#x27;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>
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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!

Copy link
Member Author

Choose a reason for hiding this comment

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

Try :hidden

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out :hidden: shows its contents in the TOC and hides the display on the main page, see the screenshot:

image

There's an extra duplicated TOC in there that's not seen in the main page.

Copy link
Member

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.

Copy link
Member Author

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.

docs/index.rst Outdated Show resolved Hide resolved
docs/simulation/particletracker.rst Outdated Show resolved Hide resolved
Copy link
Member

@rocco8773 rocco8773 left a 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.

Comment on lines +184 to +192
"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"
]
Copy link
Member

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:

  1. Don't link back to the class/function/package/etc. documentation?
  2. 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`](...).
  3. Mark the cell_type as raw 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.

Copy link
Member Author

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...

Copy link
Member Author

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.

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, future issue, maybe.

Copy link
Member

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.

plasmapy/formulary/braginskii.py Outdated Show resolved Hide resolved
@StanczakDominik
Copy link
Member Author

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.

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.

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.

We could add a matplotlib rc line to all of them... I'll look into it. Thanks for noticing, I completely missed it.

@StanczakDominik
Copy link
Member Author

And thank you very much for the review! :)

@StanczakDominik
Copy link
Member Author

Mostly done, I think. I couldn't get nbgallery to work with the :hidden: attr, and I'd prefer to come back to adding links to existing docs later. Could be a good issue for new contributors, too.

@StanczakDominik
Copy link
Member Author

StanczakDominik commented May 23, 2020

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?

Copy link
Member

@rocco8773 rocco8773 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 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
Comment on lines 26 to 29
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."
Copy link
Member

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.

@StanczakDominik
Copy link
Member Author

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.

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.
@StanczakDominik StanczakDominik merged commit caf1905 into PlasmaPy:master May 24, 2020
@StanczakDominik StanczakDominik deleted the nbsphinx branch May 24, 2020 12:03
@namurphy namurphy removed the status: ready for review PRs that are ready for code review label Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs PlasmaPy Docs at http://docs.plasmapy.org
Projects
None yet
4 participants