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 gallery #902

Closed
wants to merge 6 commits into from
Closed

Sphinx gallery #902

wants to merge 6 commits into from

Conversation

choldgraf
Copy link

I was making my way through the cartopy documentation in preparation for a class I'm working on, and thought that it would be helpful if the gallery were built with Sphinx-Gallery. This lets you include text / short descriptions of examples, it's also a more stable / faster way of building galleries with sphinx.

I figured it would be worth putting in some time to do the conversion, contained in this PR. The only main changes should be some renaming etc to clean up broken links, as well as setting up the machinery to get sphinx-gallery building. Here's an example of the gallery hosted on my fork:

http://predictablynoisy.com/cartopy/gallery/index.html

This PR focuses only on getting the gallery built with sphinx-gallery, but for future reference it could be extended to auto-link functions with API stub pages, or to (one day) include mybinder.org links for jupyter notebooks that are generated by sphinx-gallery. I didn't implement this stuff so that the PR doesn't become overwhelming. LMK what you think.

@@ -0,0 +1,3 @@
netCDF4
Copy link
Author

Choose a reason for hiding this comment

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

I included an extra .txt file because getting the docs to build required several extra requirements I didn't have. I can either clean this up / add to it if folks have suggestions, or remove it if people prefer not to have it.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't there be sphinx and sphinx-gallery in here at least?

@@ -21,6 +21,8 @@ lib/cartopy/tests/mpl/output/
docs/source/cartopy_outline.rst
docs/source/examples
docs/source/gallery.rst
docs/source/gallery
docs/source/api
Copy link
Author

Choose a reason for hiding this comment

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

this api section is auto-generated by sphinx-gallery and can be used to build mini-galleries for specific functions / classes if desired

# Sphinx gallery configuration
sphinx_gallery_conf = {
'examples_dirs': ['../../lib/cartopy/examples'],
'filename_pattern': '^((?!sgskip).)*$',
Copy link
Author

Choose a reason for hiding this comment

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

Any file that includes sgskip in its title will not be run by sphinx-gallery. Right now this is only the geostationary example because it kept hanging on my computer and I couldn't debug it.

Copy link
Member

Choose a reason for hiding this comment

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

It just takes a long time to reproject things, about 2 minutes for me.

Copy link
Author

Choose a reason for hiding this comment

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

ah I see - I wonder if this is just because I'm working on a relatively underpowered laptop...I'll remove the sgskip and will add some print statements so it doesn't look like it's hanging

Copy link
Member

Choose a reason for hiding this comment

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

So we can now remove this line, right?

Copy link
Author

Choose a reason for hiding this comment

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

this line should stay in case you ever want to have an example that isn't run by sphinx-gallery...it's a configuration option so won't change the behavior of the docs unless "sgskip" is in a file name

@@ -363,10 +367,10 @@
'figure.subplot.top': 0.96,
'figure.subplot.left': 0.04,
'figure.subplot.right': 0.96}
plot_formats = (('thumb.png', 20),
plot_formats = [('thumb.png', 20),
Copy link
Author

Choose a reason for hiding this comment

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

This kept giving me a warning when building the site saying it should be a list, so I updated it

@@ -65,23 +66,23 @@ outlines recent changes, new features, and future development plans.
Getting involved
================

Cartopy was originally developed at the UK Met Office to allow scientists to visualise
their data on maps quickly, easily and most importantly, accurately.
Cartopy was originally developed at the UK Met Office to allow scientists to visualise
Copy link
Author

Choose a reason for hiding this comment

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

I believe all of these are just trailing whitespace updates that my editor apparently auto-corrected. LMK if you want me to revert these for some reason

@@ -139,16 +139,18 @@ visualising vector fields:
:meth:`streamplots <cartopy.mpl.geoaxes.GeoAxes.streamplot>` (:ref:`example <examples-streamplot>`)
each with their own benefits for displaying certain vector field forms.

.. literalinclude:: /examples/arrows.py
.. plot:: examples/arrows.py
.. figure:: ../gallery/vector_data/images/sphx_glr_arrows_001.png
Copy link
Author

Choose a reason for hiding this comment

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

These are updated so that old figures generated with the plot directive are now linking to the sphinx-gallery image (the end result is basically the same)

@@ -0,0 +1,4 @@
Lines and polygons
Copy link
Author

@choldgraf choldgraf Jun 27, 2017

Choose a reason for hiding this comment

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

All of these docstring changes are just to add a title + description (sometimes the description is an empty string), which gets rendered by sphinx-gallery

Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

Looks good to me; cross linking in the example code is already a huge benefit. Hope that can be added for Cartopy code as well.

The deleted __init__.py needs to be restored for tests to work though.

# Sphinx gallery configuration
sphinx_gallery_conf = {
'examples_dirs': ['../../lib/cartopy/examples'],
'filename_pattern': '^((?!sgskip).)*$',
Copy link
Member

Choose a reason for hiding this comment

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

It just takes a long time to reproject things, about 2 minutes for me.

'doc_module': ('cartopy'),
'reference_url': {'matplotlib': 'http://matplotlib.org',
'numpy': 'http://docs.scipy.org/doc/numpy/reference',
'scipy': 'http://docs.scipy.org/doc/scipy/reference'},
Copy link
Member

Choose a reason for hiding this comment

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

https: on all links should work.

Copy link
Member

Choose a reason for hiding this comment

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

Might be a problem due to sphinx-gallery/sphinx-gallery#236, but it should be fixed with sphinx-gallery/sphinx-gallery#258.

.. figure:: ../gallery/vector_data/images/sphx_glr_arrows_001.png
:target: ../gallery/vector_data/arrows.html
:align: center
:scale: 50
Copy link
Member

Choose a reason for hiding this comment

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

Seems too small.

.. figure:: ../gallery/vector_data/images/sphx_glr_regridding_arrows_001.png
:target: ../gallery/vector_data/regridding_arrows.html
:align: center
:scale: 50
Copy link
Member

Choose a reason for hiding this comment

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

Also a bit small.

.. figure:: ../gallery/lines_and_polygons/images/sphx_glr_feature_creation_001.png
:target: ../gallery/lines_and_polygons/feature_creation.html
:align: center
:scale: 50
Copy link
Member

Choose a reason for hiding this comment

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

Also small.


The following examples show off the functionality of CartoPy. They
illustrate the kinds of things you can do with this library. For
more examples, tutorials, and guides on how to use CartoPy, see
Copy link
Member

Choose a reason for hiding this comment

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

Cartopy

@@ -1,4 +1,8 @@
__tags__ = ['Lines and polygons']
"""
Always Circular Stereo
Copy link
Member

Choose a reason for hiding this comment

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

A bit weird for a title; it's more like "Keep Boundary Circular" or something along those lines.

@@ -1,4 +1,8 @@
__tags__ = ['Miscellanea']
"""
Un Flag
Copy link
Member

Choose a reason for hiding this comment

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

UN

"""
__tags__ = ["Scalar data"]
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this works on this line.

Copy link
Author

Choose a reason for hiding this comment

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

ah you are right - I'm trying to remember how we fixed this in MPL...maybe we just removed those lines? Or it was fine w/o moving them?

@@ -0,0 +1,3 @@
netCDF4
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't there be sphinx and sphinx-gallery in here at least?

@choldgraf
Copy link
Author

working on changes now - re: the deleted __init__.py...this would mess up sphinx-gallery since it auto-runs any python files inside the examples folder. Do these examples get run as a part of the tests?

@choldgraf
Copy link
Author

regarding auto-linking the API...the trick is that you'd need to do this with autosummary and jinja templates. This isn't hard to do but we'd basically have two options:

  1. Manually list all of the objects in the autosummary (e.g., all of the coordinate classes)
  2. Do something like "automodule" which would loop through all objects in a module like crs and build the proper sphinx-gallery template. The problem here is that it'd import all the public classes / functions in the module. IIRC the modules within cartopy have all kinds of imports etc in their public namespace, right?

@QuLogic
Copy link
Member

QuLogic commented Jun 29, 2017

You can change the regex to skip __init__.py, though? You can see that Travis is failing right now without it.

I think most of Cartopy is not auto(doc/module/summary/etc.)'d because they're simply not documented enough. Partly the reason is also #203, I think.

BTW, you'll need to fill out the CLA before this is merged.

@choldgraf
Copy link
Author

yep the regex should be pretty easy to fix, it'd just be a bit hacky is all but I can make the change in the next push.

I'll sign the CLA - do you have a link to a tl;dr for what exactly it is that I'm signing away? I've never seen something like that for an open-source project before...

@dopplershift
Copy link
Contributor

dopplershift commented Jun 29, 2017

(IANAL, blah blah)

  1. You have the rights to the contribution you're making (it doesn't belong to someone else, no IP infringement)
  2. You grant the Met Office a permanent license to distribute it (i.e. You can't take it back)
  3. The Met Office can distribute this contribution under the current or future versions of the LGPL
  4. You retain the copyright of your contribution

It's more common for projects backstopped by larger organizations that have legal departments and want to try to avoid potential IP problems. The Python Software Foundation and Django (as well as my own MetPy project) are among many that do. Django has a nice FAQ on the agreements in general.

@choldgraf
Copy link
Author

ok latest push adds example auto-linking. It doesn't generate any new API pages (e.g., stub pages) but links to the autofunction/class/module section that was already built.

e.g., see: http://predictablynoisy.com/cartopy/gallery/lines_and_polygons/global_map.html#sphx-glr-gallery-lines-and-polygons-global-map-py

I'm still not sure how to get __init__.py working, since the regex is only to tell sphinx-gallery whether to run the file, not whether to render the file (e.g., it will still try to create a little gallery slot for __init__.py). I have an issue open with them to see what's up with that.

Will get to the CLA once I can figure out how to insert a digital signature (spilled coffee on my computer so I'm operating off of an old crappy macbook air)

@choldgraf
Copy link
Author

So it looks like there is no way to have sphinx gallery skip a python file entirely if it's within the examples directory. That gives us three options:

  1. Remove init.py and refactor the tests so that they don't treat the examples as a module...this seems relatively straightforward on first glance and I can put some time into doing this if folks are friendly to it.
  2. Embed an __init__.py file in each examples directory.
  3. Turn all of the testing-related examples code into separate files in the tests directory, and leave the examples as-is.

Any thoughts on any of those? The examples are going to get run by sphinx-gallery each time the docs build, so that's a good way to test which of them are still building or not. If a particular example is crucial to have the same output each time, then perhaps it can just be incorporated into a test?

LMK

@QuLogic
Copy link
Member

QuLogic commented Jun 30, 2017

Maybe if we add a doc build to Travis, then that would be sufficient to drop the examples from the tests? @pelson?

@choldgraf
Copy link
Author

FWIW you can tell sphinx-gallery to raise an error and fail if it fails to build.

@pelson
Copy link
Member

pelson commented Jul 4, 2017

Will get to the CLA once I can figure out how to insert a digital signature (spilled coffee on my computer so I'm operating off of an old crappy macbook air)

Don't need to overthink it - a scribble is perfectly fine. 😉 🖊 🐾


I didn't actually think cartopy tested any of the examples... that was a decision taken in Iris which I felt was unhealthy and vowed not to go down that path with cartopy. That said, the examples should be importable IMO (namespaces... they are a honking good idea!).

Perhaps when we build the docs we can simply blow away the __init__ (assuming that doesn't have other, unforeseen, impacts)...

FWIW you can tell sphinx-gallery to raise an error and fail if it fails to build.

Sounds like a good option for our CI testing.

@ajdawson
Copy link
Member

ajdawson commented Jul 4, 2017

I didn't actually think cartopy tested any of the examples... that was a decision taken in Iris which I felt was unhealthy and vowed not to go down that path with cartopy. That said, the examples should be importable IMO (namespaces... they are a honking good idea!).

We don't test the examples, but lib/cartopy/tests/mpl/test_caching.py imports the sample_data function from cartopy.examples.waves, thus it fails when the __init__.py file is removed from the examples package. It probably shouldn't be doing this really... Regardless, having the examples importable is useful and this should be preserved.

@choldgraf
Copy link
Author

The function that's imported from the examples into the tests is quite small, so that's easy to move to a different spot. More generally I'm curious what's the benefit of having the examples be importable. I've never seen this done in another package, so wondering what gains it provides?

it would be possible to obliterate that file temporarily as a part of the docs makefile, though that's a little bit clunky :-)

@pelson
Copy link
Member

pelson commented Jul 5, 2017

More generally I'm curious what's the benefit of having the examples be importable.

Mostly:

from cartopy.examples.hurricane_katrina import main

main()

# Celebrate

Anybody with an installation of cartopy is able to run an example easily from their python interpreter. I've long intended to advertise this feature in the example pages, but seem to have never gotten around to it... 😄

@choldgraf
Copy link
Author

That makes sense - that's cool that you can run things interactively!

I can edit the make html command so that it recursively deletes the __init__.py file from each folder, then re-creates them. Though requiring everything to be inside of a main function does limit the benefits of sphinx-gallery as it makes it harder to write examples in a narrative style. I guess at the end of the day, the question is: what is the ratio of "users looking at examples on the website" to "users running examples interactively in python".

FWIW, in the near future you'll be able to automatically generate links for mybinder.org with sphinx-gallery, which means any example will have a link that takes users to a live jupyter notebook where they can run the code. Just a thought!

@choldgraf
Copy link
Author

Also here's the CLA, does that work?

https://www.dropbox.com/s/icgc3wrwgh7i4o4/cla_form.pdf?dl=0

@choldgraf
Copy link
Author

Latest push adds a little script at the end of the docs build that inserts a noshow class to each of the gallery examples that link to the __init__.py files. That way they don't show up in the gallery and the examples can stay in there.

@choldgraf
Copy link
Author

hey all - other than the now-conflicting files, what's blocking on this PR? I'm trying to remember if this ball is in my court or in the devs court :-)

would be good to get this merged before a next release if it's happening soon (@QuLogic)

@QuLogic
Copy link
Member

QuLogic commented Aug 10, 2017

Well, the tests are failing and I think you still need to submit the CLA, at least.

@choldgraf
Copy link
Author

@QuLogic I put a link to the CLA here: #902 (comment)

@ajdawson
Copy link
Member

You need to send the CLA to the email address given in the document.

@choldgraf
Copy link
Author

gotcha, done

@pelson pelson added the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Sep 15, 2017
@choldgraf
Copy link
Author

ok I think I figured out what was going with travis, so let's see if it builds now.

I see there's a CLA checker in there...I emailed the CLA on August 10th.

@pelson pelson added the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Sep 16, 2017
@pelson pelson added the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Sep 16, 2017
@choldgraf
Copy link
Author

I'm not sure what the travis test failures are about - in one case it seems to be because there's a fortran library that's not linked properly, in another its an image comparison in one of the tests. But neither of those seem related to this PR...

Copy link
Member

@pelson pelson left a comment

Choose a reason for hiding this comment

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

This is definitely close. I'm 100% behind using sphinx-gallery. I'd like the ability to continue using the plot directive in some situations (such as inline/contextual examples that aren't destined for the gallery). I'd also like to step back from autosummary as a means for getting 100% documentation coverage - I think in the next few months there will be development effort to improve the coverage in a contexual/richer form that will be more beneficial to the user.

Other than that, I'll double check to see where the administration is at wrt. your CLA. It all looks good to me.

Cheers,

@@ -3,7 +3,7 @@
The cartopy Feature interface
=============================

The :ref:`data copyright, license and attribution <referencing_copyright>` can be blended on the map using `text annotations (mpl docs) <http://matplotlib.org/users/annotations_intro.html>`_ as shown in `feature_creation <../examples/feature_creation.html>`_.
The :ref:`data copyright, license and attribution <referencing_copyright>` can be blended on the map using `text annotations (mpl docs) <http://matplotlib.org/users/annotations_intro.html>`_ as shown in `feature_creation <../gallery/lines_and_polygons/feature_creation.html>`_.
Copy link
Member

Choose a reason for hiding this comment

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

It isn't ideal to change the URL of the examples (because of StackOverflow/Google links etc.). Any way of avoiding it?

Copy link
Author

Choose a reason for hiding this comment

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

hmmm, not without changing the directory structure of the examples. In other projects where we've implemented sphinx gallery we just kinda took a "rip the band-aid off" approach...I agree that's unfortunate :-/


------------


Example of using the Feature class with the matplotlib interface
----------------------------------------------------------------

.. literalinclude:: /examples/feature_creation.py
Copy link
Member

Choose a reason for hiding this comment

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

I definitely think we need to have a separation between what should live in the gallery, and what is useful as inline examples within the context of a chapter.

Cartopy hasn't got this completely right though - for example, there are a number of inline examples that really should be in the gallery: http://scitools.org.uk/cartopy/docs/v0.15/matplotlib/advanced_plotting.html#block-plots

Could we do the promotion of inline/contextual examples -> gallery examples as a separate PR?

Copy link
Author

Choose a reason for hiding this comment

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

I could also include the literalinclude in there as well if that's preferred, but if you don't want that in this PR I can take them out.

@@ -0,0 +1,5 @@
"""
Copy link
Member

Choose a reason for hiding this comment

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

Can we have just a blank file?
If not, then we need some copyright headers.

Copy link
Author

Choose a reason for hiding this comment

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

The problem is that sphinx-gallery will auto-detect any .py files and try to run them as examples, and if a file doesn't have a header, e.g.:

"""
My Header
---------

then it'll error. So there needs to be something like that in there I believe. I could try to get away with something like

"""
.
-

so it's as minimal as possible...

@@ -0,0 +1,5 @@
"""
Copy link
Member

Choose a reason for hiding this comment

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

Ditto (and for all other REMOVE type files)

'sphinx.ext.autodoc',
'sphinx.ext.doctest',
'sphinx.ext.intersphinx',
'sphinx.ext.coverage',
'sphinx.ext.viewcode',
'sphinx.ext.extlinks',
'sphinx.ext.autosummary',
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind removing this from the PR? My prior experience suggests that in certain situations having autogenerated docs is worse than having no docs at all - cartopy is no shining start wrt its docs, but what is documented is generally more readable than most autogenerated API summary (e.g. http://scitools.org.uk/iris/docs/latest/iris/iris.html. To be clear - I implemented that autosummary generation for iris, so I'm explicitly not bad-mouthing anybody else 😉 ). I'm expecting to be able to invest some significant effort on cartopy's manually crafted docs over the coming months, and so keeping the autosummary would mask the shortcomings.

Copy link
Author

Choose a reason for hiding this comment

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

the main reason for autosummary is that it lets you auto-generate mini-galleries for functions etc with sphinx-gallery. In conjunction with the template files in this PR, that will generate pages like this matplotlib example. Usually I don't use automodule either, but instead do it class-by-class (or function-by-function) to get the most modularity over my docs

Copy link
Member

Choose a reason for hiding this comment

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

@pelson what's the decision on this? Do you want the autosummary removed or not? IMO it should be removed; I'm not convinced enough by @choldgraf's justification for keeping it in.

# Sphinx gallery configuration
sphinx_gallery_conf = {
'examples_dirs': ['../../lib/cartopy/examples'],
'filename_pattern': '^((?!sgskip).)*$',
Copy link
Member

Choose a reason for hiding this comment

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

So we can now remove this line, right?

'numpy': 'http://docs.scipy.org/doc/numpy/reference',
'scipy': 'http://docs.scipy.org/doc/scipy/reference',
'shapely': 'http://toblerity.org/shapely',
'cartopy': None},
Copy link
Member

Choose a reason for hiding this comment

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

These two lines should be removable at this point?

(the second line is the backreferences_dir one)

Copy link
Author

Choose a reason for hiding this comment

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

you want to remove the backreferences_dir line? I'm not sure which you're referring to

@pelson pelson removed Pending CLA Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form labels Oct 17, 2017
@choldgraf
Copy link
Author

@corinnebosley thanks for giving a ping to get this going. Regarding autosummary you can take it or leave it, just trying to incorporate features that have proven useful in other projects.

TBH I'm not spending a lot more time on this PR as I've already put in a lot of effort and the iterations on feedback are going extremely slowly. I understand we've all got other stuff going on so no hard feelings, I just can't keep re-orienting myself to this PR once every month. If you (or someone with merge rights) wants to make a push on this then I can spend some time on it as well.

@pp-mo
Copy link
Member

pp-mo commented Nov 29, 2017

Replaced by #969

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants