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

changes for MEP12/sphinx-gallery compliance #8209

Closed
wants to merge 4 commits into from

Conversation

yinleon
Copy link
Contributor

@yinleon yinleon commented Mar 7, 2017

Regarding #7206

@tacaswell tacaswell added this to the 2.0.1 (next bug fix release) milestone Mar 7, 2017
@@ -1,4 +1,8 @@
"""
===========
Poly Editor
==========
Copy link
Member

Choose a reason for hiding this comment

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

missing '='

# A class that will downsample the data and recompute when zoomed.
class DataDisplayDownsampler(object):
def __init__(self, xdata, ydata):
self.origYData = ydata
self.origXData = xdata
self.ratio = 5
self.ratio = 50
Copy link
Member

Choose a reason for hiding this comment

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

This seems a bit too aggressive.

@tacaswell
Copy link
Member

Left 2 small comments, but otherwise looks good!

@tacaswell
Copy link
Member

In the future, it is better to make PRs against some branch other than your master branch.

@yinleon
Copy link
Contributor Author

yinleon commented Mar 7, 2017

@tacaswell will do! My first commit to an OSS project, so apologies.

ax.axis[direction].set_visible(True)

for direction in ["left", "right", "bottom", "top"]:
# hides boarders
Copy link
Member

Choose a reason for hiding this comment

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

boarders -> borders


A matplotlib based game of Pong illustrating one way to write interactive animation which are easily ported to multiple backends pipong.py was written by Paul Ivanov <http://pirsquared.org>
Copy link
Member

Choose a reason for hiding this comment

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

matplotlib -> Matplotlib
animation -> animations
Add period at end of sentence.

Also, please wrap this line at 79 characters.

Resampling Data
===============

Downsampling lowers the sample rate or sample size of a signal. In this tutorial, the signal is downsampled when the plot is adjusted through dragging and zooming.
Copy link
Member

Choose a reason for hiding this comment

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

Wrap this line.

Demonstrate the mixing of 2d and 3d subplots.
============================================
Demonstrate the mixing of 2d and 3d subplots
============================================
Copy link
Member

Choose a reason for hiding this comment

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

Missing blank line after, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I resolved this?

=================

This is an example of creating a stacked bar plot with error bars using `plt.bar`.
Note the parameters `yerr` used for error bars, and `bottom` to stack the women's bars on top of the men's bars.
Copy link
Member

@QuLogic QuLogic Mar 7, 2017

Choose a reason for hiding this comment

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

Lines need to be wrapped.

@@ -1,3 +1,11 @@
"""
=======
Dophins
Copy link
Member

Choose a reason for hiding this comment

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

Dophins -> Dolphins

Dophins
=======

This example shows how to draw, and manipulate shapes given verticies and nodes using the patches, path and transforms classes.
Copy link
Member

Choose a reason for hiding this comment

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

Wrap line.

Filling the area between lines
==============================

This example shows how to use fill_between() to color between lines based on user-defined logic.
Copy link
Member

Choose a reason for hiding this comment

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

Wrap line.

======================

This shows 4 possible projections using subplot.
Matplotlib also supports the <a href='http://matplotlib.org/basemap/'>Basemaps Toolkit</a> for geographic projections.
Copy link
Member

Choose a reason for hiding this comment

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

Wrap line. Probably should mention Cartopy as well.

MRI
===

This example illustrates how to read an image (of an MRI) into a numpy array, and display it in greyscale using `ax.imshow`.
Copy link
Member

Choose a reason for hiding this comment

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

numpy -> NumPy

Wrap line.

ax.axis[direction].set_visible(True)

for direction in ["left", "right", "bottom", "top"]:
# hides boarders
Copy link
Contributor

Choose a reason for hiding this comment

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

borders

# pipong.py was written by Paul Ivanov <http://pirsquared.org>
"""
====
PONG
Copy link
Contributor

Choose a reason for hiding this comment

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

"Pong" (I don't think it should be all caps).

@@ -1,3 +1,11 @@
"""
=======
Dophins
Copy link
Contributor

Choose a reason for hiding this comment

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

dolphins

@@ -1,3 +1,10 @@
"""
================
Axis Line Styles
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like most already converted examples only capitalize the first word (which I prefer too), so let's stay consistent (applies to this and all files below).

Demonstrate the mixing of 2d and 3d subplots.
============================================
Demonstrate the mixing of 2d and 3d subplots
============================================
Copy link
Contributor

Choose a reason for hiding this comment

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

blank line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I resolved this?


A matplotlib based game of Pong illustrating one way to write interactive animation which are easily ported to multiple backends pipong.py was written by Paul Ivanov <http://pirsquared.org>
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and below, wrap your lines at 79 characters (most editors should provide some option to help that).

Copy link
Contributor

Choose a reason for hiding this comment

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

Here and below, use the rst syntax for hyperlinks (http://www.sphinx-doc.org/en/stable/rest.html#hyperlinks).

Filling the area between lines
==============================

This example shows how to use fill_between() to color between lines based on user-defined logic.
Copy link
Contributor

Choose a reason for hiding this comment

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

`fill_between`

(with backquotes) -- ultimately this should create a link to the API docs.

Dophins
=======

This example shows how to draw, and manipulate shapes given verticies and nodes using the patches, path and transforms classes.
Copy link
Contributor

Choose a reason for hiding this comment

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

"vertices".

Please spell-check your docstrings.

MRI
===

This example illustrates how to read an image (of an MRI) into a numpy array, and display it in greyscale using `ax.imshow`.
Copy link
Contributor

Choose a reason for hiding this comment

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

just imshow

@anntzer
Copy link
Contributor

anntzer commented Mar 7, 2017

Apparently I reviewed this at the same time as @QuLogic :-)

@tacaswell
Copy link
Member

Apparently other reviewer can spell better than I can..

@yinleon
Copy link
Contributor Author

yinleon commented Mar 7, 2017

thanks @anntzer and @QuLogic, you two are equally thorough :)


A Matplotlib based game of Pong illustrating one way to write interactive
animations which are easily ported to multiple backends pipong.py was written
by <a href='http://pirsquared.org'>Paul Ivanov</a>.
Copy link
Member

Choose a reason for hiding this comment

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

I am somewhat not surprised by the original author of that example…
ping @ivanov :D

@yinleon
Copy link
Contributor Author

yinleon commented Mar 7, 2017

should I be alarmed about the Travis CLI failure? Now sure I understand what I'm seeing under Details.

Pong
====

A Matplotlib based game of Pong illustrating one way to write interactive
Copy link
Contributor

Choose a reason for hiding this comment

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

Beware of the trailing whitespace (remove it for PEP8-compliance)

====

A Matplotlib based game of Pong illustrating one way to write interactive
animations which are easily ported to multiple backends pipong.py was written
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, just remove the trailing whitespace for PEP8-compliance

@afvincent
Copy link
Contributor

afvincent commented Mar 7, 2017

The Travis failure is related to PEP8 (more precisely to two trailing whitespaces). More details here ;).

Edit: typo

@QuLogic
Copy link
Member

QuLogic commented Mar 7, 2017

If you have changes to multiple files, editing on GitHub is not a good choice. It creates way too many single commits of one or two minor changes. Please rebase and squash this on your local copy if you can.

@NelleV
Copy link
Member

NelleV commented Mar 8, 2017

Can we activate the squash and merge option? This would allow us to squash and merge before merging instead of asking contributors to rebase themselves.

ping @tacaswell

@QuLogic
Copy link
Member

QuLogic commented Mar 8, 2017

TBH, as a contributor, I don't really like squash&merge as it decouples your fork and upstream's (I don't like rebase&merge for the same reason.)

@yinleon
Copy link
Contributor Author

yinleon commented Mar 8, 2017

Hi, I squashed all 20 commits. Did I do it correctly? Is it reflected in this PR, or should I make a new one?

@NelleV
Copy link
Member

NelleV commented Mar 8, 2017

Yep, it is done and it works! Thanks!

@NelleV NelleV changed the title changes for MEP12/sphinx-gallery compliance [MRG+] changes for MEP12/sphinx-gallery compliance Mar 8, 2017
@NelleV NelleV changed the title [MRG+] changes for MEP12/sphinx-gallery compliance [MRG+1] changes for MEP12/sphinx-gallery compliance Mar 8, 2017
Copy link
Member

@NelleV NelleV left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@QuLogic
Copy link
Member

QuLogic commented Mar 8, 2017

Hi, I squashed all 20 commits. Did I do it correctly? Is it reflected in this PR, or should I make a new one?

No, it's not quite right. After rebasing, you need to force push the result. If you don't force, git will complain and suggest you pull everything, but that will restore all the previous commits locally and add another merge commit.

So, please do the rebase, and then do git push --force origin master (btw, it's safer to use a feature branch instead of master) so that you can replace all the old commits.

@NelleV
Copy link
Member

NelleV commented Mar 8, 2017

@yinleon Are you still around Berkeley? I was mistaken and something didn't quite happen right with the rebase. I am happy to help you out with this tomorrow (I should be at BIDS all afternoon if you are around).

@yinleon
Copy link
Contributor Author

yinleon commented Mar 8, 2017

Sure thing @QuLogic.
@NelleV yes-- I will be at BIDS tomorrow.
Sorry for the confusion all.

ax.axis[direction].set_axisline_style("-|>")

Copy link
Member

Choose a reason for hiding this comment

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

There is some whitespace here causing the pep8 failure.

@NelleV
Copy link
Member

NelleV commented Mar 10, 2017

Hi @yinleon
There was a small blank line issue, that I fixed.

This patch needs an additional review.

@NelleV NelleV added the MEP: MEP12 gallery and examples improvements label Mar 11, 2017
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.

LGTM, but can you amend this last typo, @NelleV?

matplotlib event handling to interact with objects on the canvas
===========
Poly Editor
==========
Copy link
Member

Choose a reason for hiding this comment

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

One missing =.

@NelleV NelleV changed the title [MRG+1] changes for MEP12/sphinx-gallery compliance [MRG+2] changes for MEP12/sphinx-gallery compliance Mar 12, 2017
@NelleV
Copy link
Member

NelleV commented Mar 16, 2017

I think this is ready to be merged. @anntzer can you have a look at this?

====

A Matplotlib based game of Pong illustrating one way to write interactive
animations which are easily ported to multiple backends pipong.py was written
Copy link
Contributor

Choose a reason for hiding this comment

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

dot after "multiple backends"


A Matplotlib based game of Pong illustrating one way to write interactive
animations which are easily ported to multiple backends pipong.py was written
by <a href='http://pirsquared.org'>Paul Ivanov</a>.
Copy link
Contributor

Choose a reason for hiding this comment

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


This is an example of plotting Edward Lorenz's 1963 "Deterministic
Nonperiodic Flow" in a 3-dimensional space using mplot3d.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest using an explicit hyperlink target (http://docutils.sourceforge.net/docs/ref/rst/restructuredtext.html#hyperlink-targets).

=================

This is an example of creating a stacked bar plot with error bars using
`plt.bar`. Note the parameters `yerr` used for error bars, and `bottom`
Copy link
Contributor

Choose a reason for hiding this comment

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

yerr and bottom should use *foo*, not backticks (they are not references to linkable elements).

========

This example shows how to draw, and manipulate shapes given vertices and nodes
using the `patches`, `path` and `transforms` classes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Patch, Path, and Transform (references to actual python names).


This shows 4 possible projections using subplot.
Matplotlib also supports
<a href='http://matplotlib.org/basemap/'>Basemaps Toolkit</a> and
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above regarding hyperlink syntax (either way works).

@dstansby dstansby self-assigned this Apr 12, 2017
@dstansby
Copy link
Member

This is going to take a bit of a rebase now sphinx-gallery is merged. I've assigned this to myself and will try to do it in the next day or so.

@choldgraf
Copy link
Contributor

hey @yinleon , a lot has changed with MPL docs in the last few weeks :)

This means it'll be a bit more complicated to get this merged. Do you have time to do a live skype where you and I can spot-check these issues and get it back to a mergeable state?

@dstansby
Copy link
Member

dstansby commented May 1, 2017

So I actually had a go at rebasing this last night, and gave up after a while. Quite a lot of the changes have already been made (titles), and there's only a couple of things that are now new unfortunately.

I think the easiest thing to do here would be to manually compare the PR diff and the current files, and just copy across anything new.

@choldgraf
Copy link
Contributor

@phobson I think that's a good idea - let's see if @yinleon is interested in finishing this up and then we can make a plan from there

@story645 story645 added this to sphinx-gallery in Documentation overhaul May 1, 2017
@QuLogic QuLogic modified the milestones: 2.0.1 (next bug fix release), 2.0.2 (next bug fix release) May 3, 2017
@tacaswell tacaswell mentioned this pull request Jul 10, 2017
@tacaswell
Copy link
Member

Closing in favor of #8860

@tacaswell tacaswell closed this Jul 10, 2017
@QuLogic QuLogic modified the milestones: 2.1 (next point release), 2.0.3 (next bug fix release) Jul 15, 2017
@QuLogic QuLogic changed the title [MRG+2] changes for MEP12/sphinx-gallery compliance changes for MEP12/sphinx-gallery compliance Jul 15, 2017
@dstansby dstansby removed their assignment Jul 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation MEP: MEP12 gallery and examples improvements
Projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants