Skip to content

Adding headers for examples/units for MEP12/sphinx-gallery compliance #8254

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
merged 1 commit into from
Mar 25, 2017

Conversation

yinleon
Copy link
Contributor

@yinleon yinleon commented Mar 10, 2017

Changes in accordance to #7206

Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

Just some small changes to make, otherwise this all looks good 👍

============
Artist tests
============

Test unit support with each of the matplotlib primitive artist types
Copy link
Member

Choose a reason for hiding this comment

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

Could you put a full stop at the end of this sentence?

Copy link
Member

Choose a reason for hiding this comment

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

Also capitalize Matplotlib if you change this line.

============
Artist tests
============

Test unit support with each of the matplotlib primitive artist types

The axes handles unit conversions and the artists keep a pointer to
Copy link
Member

Choose a reason for hiding this comment

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

axes --> axis

============
Artist tests
============

Test unit support with each of the matplotlib primitive artist types

The axes handles unit conversions and the artists keep a pointer to
their axes parent, so you must init the artists with the axes instance
Copy link
Member

Choose a reason for hiding this comment

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

axes --> axis

Copy link
Member

Choose a reason for hiding this comment

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

I would change this 'init' to the full 'initialize'...

===================
Bar demo with units
===================

plot using a variety of cm vs inches conversions. The example shows
Copy link
Member

Choose a reason for hiding this comment

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

plot --> Plot

Copy link
Member

Choose a reason for hiding this comment

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

Also, spell out centimetre (inches is not abbreviated).

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

This is the same example as
<a href='http://matplotlib.org/examples/api/barchart_demo.html'>
Copy link
Member

Choose a reason for hiding this comment

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

I think there is probably a better way of linking between examples instead of using a hyperlink, but I can't think of how to at the moment.

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. Once we'll finish the migration to sphinx gallery, rst markup will work.

============
Radian ticks
============

Plot with radians from the basic_units mockup example package
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a full stop and a newline at the end of the first sentence?

@tacaswell tacaswell added this to the 2.0.1 (next bug fix release) milestone Mar 10, 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.

In line with @dstansby's comments, these are not exactly comments on what you've done, but general improvements, really. Depends on how much work you want to do...

============
Artist tests
============

Test unit support with each of the matplotlib primitive artist types
Copy link
Member

Choose a reason for hiding this comment

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

Also capitalize Matplotlib if you change this line.

============
Artist tests
============

Test unit support with each of the matplotlib primitive artist types

The axes handles unit conversions and the artists keep a pointer to
their axes parent, so you must init the artists with the axes instance
Copy link
Member

Choose a reason for hiding this comment

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

I would change this 'init' to the full 'initialize'...

============
Artist tests
============

Test unit support with each of the matplotlib primitive artist types

The axes handles unit conversions and the artists keep a pointer to
their axes parent, so you must init the artists with the axes instance
if you want to initialize them with unit data, or else they will not
Copy link
Member

Choose a reason for hiding this comment

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

... and 'initialize them with unit data' to 'use them with unit data'.

@@ -34,13 +38,15 @@
ax.add_collection(lc)

# test a plain-ol-line
line = lines.Line2D([0*cm, 1.5*cm], [0*cm, 2.5*cm], lw=2, color='black', axes=ax)
line = lines.Line2D(
[0*cm, 1.5*cm], [0*cm, 2.5*cm], lw=2, color='black', axes=ax)
Copy link
Member

Choose a reason for hiding this comment

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

If this is too long, I'd break after the positional arguments instead; this doesn't look that great.

ax.add_line(line)

if 0:
# test a patch
# Not supported at present.
rect = patches.Rectangle((1*cm, 1*cm), width=5*cm, height=2*cm, alpha=0.2, axes=ax)
rect = patches.Rectangle(
(1*cm, 1*cm), width=5*cm, height=2*cm, alpha=0.2, axes=ax)
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

===================
Bar demo with units
===================

plot using a variety of cm vs inches conversions. The example shows
Copy link
Member

Choose a reason for hiding this comment

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

Also, spell out centimetre (inches is not abbreviated).

===================
Bar demo with units
===================

plot using a variety of cm vs inches conversions. The example shows
how default unit instrospection works (ax1), how various keywords can
Copy link
Member

Choose a reason for hiding this comment

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

instrospection -> introspection

@@ -1,8 +1,13 @@
"""
==========
Evans test
Copy link
Member

Choose a reason for hiding this comment

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

Who or what is "Evans"? Does anyone know? @tacaswell? I think this needs a different name.

Copy link
Member

Choose a reason for hiding this comment

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

This all predates me. I assume someone from JPL

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

@NelleV NelleV changed the title Adding headers for examples/units for MEP12/sphinx-gallery compliance [MRG+1] Adding headers for examples/units for MEP12/sphinx-gallery compliance Mar 11, 2017
@dstansby
Copy link
Member

Cool, going to merge this in to get the titles in ready for sphinx-gallery, and open a new PR with the suggested cleanups. Thanks for the PR @yinleon !

@dstansby dstansby changed the title [MRG+1] Adding headers for examples/units for MEP12/sphinx-gallery compliance Adding headers for examples/units for MEP12/sphinx-gallery compliance Mar 25, 2017
@dstansby dstansby merged commit 98ce210 into matplotlib:master Mar 25, 2017
@dstansby dstansby mentioned this pull request Mar 25, 2017
@NelleV
Copy link
Member

NelleV commented Mar 25, 2017

wohoo! This section did not appear in our gallery. I'm going to try to add to the sphinx-gallery one.

dstansby added a commit that referenced this pull request Mar 25, 2017
Adding headers for examples/units for MEP12/sphinx-gallery compliance
@dstansby
Copy link
Member

Backported to 2.0.x via. 932d469

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
None yet
Development

Successfully merging this pull request may close these issues.

5 participants