Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

migrate examples to sphinx-gallery #8472

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

Merged
NelleV merged 3 commits into matplotlib:master from choldgraf:sg_event_handling
Apr 17, 2017

Conversation

Copy link
Contributor

@choldgraf choldgraf commented Apr 13, 2017

Now that sphinx gallery is active on the dev docs, I think we should try to use the momentum to get the rest of the examples building with SG.

Since there are many, many examples, I've taken a stab at semi-automatic the conversion process so that the current examples will build with SG. This PR does this for one folder of the examples so we can see what it looks like.

Basically, the only thing we need for SG to build an example is a title header at the top of each example. Ideally we'd also like to have a comment below it, though a blank line is fine if there is no comment. To that extent here's roughly what the automation does:

  1. pull the file name, split it by _, capitalize the first letter of each word. That’s the title.
  2. Look for any top-level comments (e.g., """, or # ) and add the title above them, turning the comment into the proper sphinx-gallery comment.
  3. If there is no comment, just add the title and create an empty line below.
  4. Run each python file, and for any that don't run (e.g. because they only run on windows), append _sgskip to the filename.
  5. Add a rule to the sphinx-gallery pattern match so that it only matches files that don't have _sgskip in them.

This seems to do a pretty good job of converting the examples and generating descriptions when possible. Basically, this will give us >= whatever the current non-SG examples setup is doing.

If folks (maybe @anntzer and @dopplershift ?) could give some thoughts on this approach, and if we think this is a good shot at doing conversion, then I can do it for the rest of the examples and we can have a fully functioning sphinx gallery!

@@ -1,3 +1,9 @@
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

two _sgskip suffix in filename

Copy link
Contributor Author

@choldgraf choldgraf Apr 13, 2017

Choose a reason for hiding this comment

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

good catch - ran my script one too many times :)

(I'll fix these and will overwrite the commit so that we don't have tons and tons of changes)

Copy link
Contributor

anntzer commented Apr 13, 2017

If you have this automated, I would suggest switching everything as you suggested, send to travis+appveyor to see what needs to be renamed, rename the minimal set, and then for later PRs have a very liberal policy of docstring fixes and removal of old examples.

@dstansby dstansby added this to the 2.1 (next point release) milestone Apr 13, 2017
Copy link
Contributor Author

choldgraf commented Apr 13, 2017 •
edited
Loading

@anntzer sounds good - I'll take a stab at this and we can look at how well it does the conversion. Seems to be working reasonably well (famous last words)

@@ -1,63 +0,0 @@
# -*- noplot -*-
Copy link
Member

@tacaswell tacaswell Apr 14, 2017

Choose a reason for hiding this comment

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

Why delete this?

Copy link
Contributor Author

@choldgraf choldgraf Apr 14, 2017

Choose a reason for hiding this comment

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

accidental delete in this push - it'll be back in the next one. However if the examples are ever built entirely from sphinx gallery, I don't believe these lines will be necessary anymore

Copy link
Contributor Author

@choldgraf choldgraf Apr 14, 2017

Choose a reason for hiding this comment

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

oops just realized you were talking about the whole file and not just the noplot line...I just meant noplot won't be needed with sphinx gallery

@anntzer anntzer dismissed their stale review April 14, 2017 06:38

issues were addressed

Copy link
Contributor Author

ok that took a bit more work than I expected :)

BUT, I think that the examples should be more or less good to go now. I'm sure there are some things that I've missed, so let's see if they build properly and I can spot check some fixes if needed tomorrow.

Copy link
Contributor

anntzer commented Apr 14, 2017

If this is not too much work (hehe...), could you consider rewriting the history so that this is easier to review?

Specifically

  • one commit with all the automated changes -- either put the script in the commit message if it's short enough, or just describe what it did. This way I can simply skim through the diff and trust that it is the automated output.
  • one or more commits with the manual changes that actually need to be reviewed.

git rebase -i may be helpful :-) Alternatively, you can copy the entire repo in its current state, go back to master, apply the automated changes, commit, overwrite the repo with the copy that also has the manual changes, and commit again.

@choldgraf choldgraf changed the title (削除) SG event handling (削除ここまで) (追記) migrate examples to sphinx-gallery (追記ここまで) Apr 14, 2017
Copy link
Contributor Author

@anntzer fair point - this is a lot of files to change :)

I just went through and rebased onto master + squashed some commits. So now there are 3 commits: 1 that just changes the regex match, 1 that contains most of the auto stuff, 1 that has more manual tweaks, renaming, ensuring links work, etc.

The docs build on my machine, so hopefully the same will be true for travis.
phobson reacted with thumbs up emoji

@@ -36,19 +36,27 @@

The syntax of set is

.. code::
Copy link
Contributor

@anntzer anntzer Apr 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

You could use slightly more lightweight markup: https://thomas-cokelaer.info/tutorials/sphinx/rest_syntax.html#how-to-include-simple-code

Same comment applies throughout the PR.

Copy link
Contributor Author

@choldgraf choldgraf Apr 14, 2017

Choose a reason for hiding this comment

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

yeah there were examples where just a tab was being used, and it seemed to keep causing errors. so adding the .. code:: just ensured that it worked.

Copy link
Contributor Author

@choldgraf choldgraf Apr 14, 2017

Choose a reason for hiding this comment

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

ahh I see what I was getting wrong before - I wasn't adding the :: at the end of paragraphs. will switch this back, agree it's cleaner

10000 3.30 1.31 0.53
50000 19.30 6.53 1.98

.. code::
Copy link
Contributor

Choose a reason for hiding this comment

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

choldgraf reacted with thumbs up emoji
Patch / PatchCollection 1
Line2D / LineCollection 2
Text 3
.. code::
Copy link
Contributor

Choose a reason for hiding this comment

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

a properly set table would be nice here too

@@ -23,7 +23,8 @@
import matplotlib
# Make sure that we are using QT5
matplotlib.use('Qt5Agg')
from PyQt5 import QtCore, QtWidgets
# Uncomment this line before running, it breaks sphinx-gallery builds
Copy link
Contributor

Choose a reason for hiding this comment

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

sgskip is not enough? should this be reported as an issue with s-g?

Copy link
Contributor Author

@choldgraf choldgraf Apr 14, 2017

Choose a reason for hiding this comment

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

already did this ;) sphinx-gallery/sphinx-gallery#224

Copy link
Contributor Author

choldgraf commented Apr 14, 2017 •
edited
Loading

Also just a quick rehash of the scope of this PR

I would suggest switching everything as you suggested, send to travis+appveyor to see what needs to be renamed, rename the minimal set, and then for later PRs have a very liberal policy of docstring fixes and removal of old examples.

I think the goal here is to get all the examples working / displaying in the gallery (and potentially to deprecate the examples page, since at this point they will be copies of one another, I think...).

For formatting improvements in docstrings, etc I think we should have an issue to keep track of these things that can be tackled in future PRs (unless there aren't too many in which case I can fix them)
phobson reacted with thumbs up emoji

Copy link
Contributor

anntzer commented Apr 14, 2017

Sure, that's why I'm 1. not blocking on these changes, and 2. only suggesting a few improvements to the manually edited parts of the markup.

Copy link
Contributor Author

Cool! Just making sure we're all on the same page :) I think it was a good idea to squash into one auto commit and one manual commit.

Copy link
Contributor Author

latest push removes the .. code:: blocks and formats the tables you pointed out

Copy link
Contributor Author

It looks like the builds are passing on travis (!!!)

Shall we take care of small things in subsequent PRs?

Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

conditional on appveyor passing

Copy link
Contributor Author

Woot, tests passing. Anyone wanna take a look? @phobson or @dopplershift? I think we are in good shape with this given that there will be some subsequent prs to clean things up.

Copy link
Member

Sorry, I broke this by merging a different PR :(

Copy link
Contributor Author

just pushed a rebase - I think the problem was in the file getting renamed, should be fixed now

Copy link
Member

phobson commented Apr 17, 2017

@choldgraf Looks like we've still got merge conflict. I should be able to take a good long look at this tomorrow night.

Copy link
Contributor Author

OK just rebased again.

I think that was actually another new file that got merged after I had rebased the last time :P I think this is going to keep happening since there are so many files changed in this PR.

@phobson another option is to get this merged and then I can spend a little time tomorrow on converting the remaining examples to mpl_examples etc and we can do a review on that. LMK what you think is best.

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.

Stuff to fix; probably not 100% comprehensive. Can be done here or in another PR.

@@ -5,6 +5,8 @@
=============================

A pure OO (look Ma, no pylab!) example using the agg backend


Copy link
Member

Choose a reason for hiding this comment

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

Extra line.

@@ -8,6 +8,8 @@
ttf file for a font instance, you can do so using the
font_manager.FontProperties fname argument (for a more flexible
solution, see the font_family_rc.py and fonts_demo.py examples).


Copy link
Member

Choose a reason for hiding this comment

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

Extra line.

@@ -1,3 +1,9 @@
"""
=============
Demo Axes Rgb
Copy link
Member

Choose a reason for hiding this comment

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

Capitalize RGB.

Pipong
======

A matplotlib based game of Pong illustrating one way to write interactive
Copy link
Member

Choose a reason for hiding this comment

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

Matplotlib


For detailed comments on animation and the techniques used here, see
the wiki entry
http://www.scipy.org/wikis/topical_software/MatplotlibAnimation
Copy link
Member

Choose a reason for hiding this comment

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

Link is dead.

@@ -1,3 +1,9 @@
"""
====================
Lineprops Dialog Gtk
Copy link
Member

Choose a reason for hiding this comment

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

Capitalize GTK.

@@ -1,4 +1,8 @@
"""
==============
Pylab With Gtk
Copy link
Member

Choose a reason for hiding this comment

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

Capitalize GTK.

@@ -1,4 +1,8 @@
"""
=============
Svg Histogram
Copy link
Member

Choose a reason for hiding this comment

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

Capitalize SVG.

SVG tooltip example
===================
===========
Svg Tooltip
Copy link
Member

Choose a reason for hiding this comment

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

Capitalize SVG.

.. _user_interfaces-toolmanager:

===========
Toolmanager
Copy link
Member

Choose a reason for hiding this comment

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

ToolManager

Copy link
Contributor Author

ok...I'll spot check those points from @QuLogic ...after that it would be great if we could get this PR merged because I don't want to keep having to rebase every 30 minutes.

Copy link
Contributor Author

ok the changes have been made per @QuLogic 's suggestions

Copy link
Contributor Author

I'm pretty sure that one Travis failure is not related to these changes. Since these were all tiny changes, and since appveyor was passing on the last iteration, can we assume it will follow suit?

Copy link
Contributor Author

looks like tests are all passing...
phobson reacted with hooray emoji

@NelleV NelleV merged commit d9b1468 into matplotlib:master Apr 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic left review comments

@tacaswell tacaswell tacaswell approved these changes

@anntzer anntzer anntzer approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v2.1
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /