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

[examples] final pep8 fixes #3774

Merged
merged 26 commits into from Nov 15, 2014
Merged

[examples] final pep8 fixes #3774

merged 26 commits into from Nov 15, 2014

Conversation

thisch
Copy link
Contributor

@thisch thisch commented Nov 9, 2014

No description provided.

@tacaswell tacaswell added this to the v1.5.x milestone Nov 9, 2014
xdata = line.get_xdata()
ydata = line.get_ydata()
maxd = 0.05
d = np.sqrt((xdata-mouseevent.xdata)**2. + (ydata-mouseevent.ydata)**2.)
d = np.sqrt((xdata - mouseevent.xdata)**2. + (ydata - mouseevent.ydata)**2.)
Copy link
Member

Choose a reason for hiding this comment

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

is this under 80 chars? if not, perhaps turning those "2." into "2" would help? Or maybe just using np.hypot()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ATM we ignore the E501 (the line length error class) check anyway, as too many lines are longer than 80 chars.

labels=labels, autopct='%.0f%%',
shadow=False, radius=0.5)
labels=labels, autopct='%.0f%%',
shadow=False, radius=0.5)
# Turn off shadow for tiny plot
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 comment lines need to go above this function call

@WeatherGod
Copy link
Member

overall, this is a good set of changes.

@@ -14,7 +14,7 @@
X, Y = meshgrid(x, y)
Z1 = bivariate_normal(X, Y, 1.0, 1.0, 0.0, 0.0)
Z2 = bivariate_normal(X, Y, 1.5, 0.5, 1, 1)
Z = 10 * (Z2-Z1) # difference of Gaussians
Z = 10*(Z2 - Z1) # difference of Gaussians
Copy link
Member

Choose a reason for hiding this comment

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

This is a case where the original was better. If you are going to use spaces around the minus here (which the real PEP 8 does not require), then they are also needed around the asterisk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really. The real Pep8 says

If operators with different priorities are used, consider adding whitespace around the operators with the lowest priority(ies). Use your own judgment; however, never use more than one space, and always have the same amount of whitespace on both sides of a binary operator.

Copy link
Member

Choose a reason for hiding this comment

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

Notice the "consider", and the "use your own judgment". Also, notice that this is directed at forms without parentheses, such as a*b + c*d. There, the spaces visually support the operator precedence.

@WeatherGod
Copy link
Member

ok, I think all my comments are addressed. I'll let Travis wrap up and then merge. Fantastic work @thisch!

@thisch
Copy link
Contributor Author

thisch commented Nov 15, 2014

Travis is failing because of the recently added boxplot examples. The whitespace issues in these examples are the cause of the Travis build failure. Is it ok if I merge the master branch into this branch or should i rebase my commits on top of master?

@thisch
Copy link
Contributor Author

thisch commented Nov 15, 2014

@efiring I will address your comments about whitespace around operators in a different PR. There are many places now in the examples where expressions like a*(b+c) were converted to a*(b + c) as part of my pep8ification. I think I'll revert some of these changes.

@WeatherGod
Copy link
Member

Rebase is better. I'll hold off until you are ready.

On Sat, Nov 15, 2014 at 10:48 AM, Thomas Hisch notifications@github.com
wrote:

@efiring https://github.com/efiring I will address your comments about
whitespace around operators in a different PR. There are many places now in
the examples where expressions like a_(b+c) were converted to a_(b + c)
as part of my pep8ification. I think I'll revert some of these changes.


Reply to this email directly or view it on GitHub
#3774 (comment)
.

@thisch
Copy link
Contributor Author

thisch commented Nov 15, 2014

@WeatherGod done; travis tests pass except for python 2.6, which is probably not related to this PR

@WeatherGod
Copy link
Member

Indeed, it seems unrelated. Good work. Merging...

WeatherGod added a commit that referenced this pull request Nov 15, 2014
[examples] final pep8 fixes
@WeatherGod WeatherGod merged commit ebde05f into matplotlib:master Nov 15, 2014
@thisch thisch deleted the pep8final branch November 15, 2014 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants