Skip to content

pep8ify examples/ part 3#3183

Closed
twmr wants to merge 4 commits intomatplotlib:masterfrom
twmr:pep8examplesapiv3
Closed

pep8ify examples/ part 3#3183
twmr wants to merge 4 commits intomatplotlib:masterfrom
twmr:pep8examplesapiv3

Conversation

@twmr
Copy link
Copy Markdown
Contributor

@twmr twmr commented Jul 4, 2014

examples/pylab_examples, examples/units and examples/use_interface remain to be done.

twmr
twmr reviewed Jul 4, 2014
View reviewed changes
Comment thread examples/event_handling/path_editor.py Outdated
Copy link
Copy Markdown
Contributor Author

@twmr twmr Jul 4, 2014

Choose a reason for hiding this comment

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

I did this in several places - hope you are fine with that.

I just discovered that the sqrt version is the fastest compared to the complex version and np.hypot.

In [5]: import numpy as np

In [6]: a = np.random.rand(int(1e6))

In [7]: b = np.random.rand(int(1e6))

In [10]: %timeit np.abs(a + 1j * b)
10 loops, best of 3: 40.4 ms per loop

In [11]: %timeit np.hypot(a, b)
10 loops, best of 3: 23.4 ms per loop

In [12]: %timeit np.sqrt(a ** 2 + b ** 2)
100 loops, best of 3: 8.13 ms per loop

Probably I'll change it back, WDYT ?

Copy link
Copy Markdown
Contributor Author

@twmr twmr Jul 4, 2014

Choose a reason for hiding this comment

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

I changed it to np.hypot

@ianthomas23
Copy link
Copy Markdown
Member

ianthomas23 commented Jul 4, 2014

I don't approve of the blanket use of whitespace around all binary operators. In many cases this renders the code less readable than before, and is contrary to PEP8, which says:

If operators with different priorities are used, consider adding whitespace around the operators with the lowest priority(ies). Use your own judgment; however.

PEP8 encourages

x = x*2 - 1
hypot2 = x*x + y*y
c = (a+b) * (a-b)

and discourages

x = x * 2 - 1
hypot2 = x * x + y * y
c = (a + b) * (a - b)

I'll indicate some of the changes that use excessive whitespace regardless of operator priority, contrary to PEP8.

If we are going to use an automated tool to correct PEP8 compliance, it must be configured to be less aggressive than this.

ianthomas23
ianthomas23 reviewed Jul 4, 2014
View reviewed changes
Comment thread examples/event_handling/idle_and_timeout.py
Copy link
Copy Markdown
Member

@ianthomas23 ianthomas23 Jul 4, 2014

Choose a reason for hiding this comment

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

Excessive whitespace.

@twmr
Copy link
Copy Markdown
Contributor Author

twmr commented Jul 4, 2014

@ianthomas23 I absolutely agree (see PyCQA/pycodestyle#292 (comment)).

@twmr
Copy link
Copy Markdown
Contributor Author

twmr commented Jul 4, 2014

@ianthomas23 I think it would be the best to ignore the space issues for the moment and fix them in a different PR

@tacaswell
Copy link
Copy Markdown
Member

tacaswell commented Jul 4, 2014

I want to hold off on these until after 1.4. Touching this many files makes me nervous.

@tacaswell tacaswell added this to the v1.4.x milestone Jul 4, 2014
@ianthomas23 ianthomas23 mentioned this pull request Jul 8, 2014
Closed
@twmr
Copy link
Copy Markdown
Contributor Author

twmr commented Aug 8, 2014

I'll recreate a PR once 1.4 is released.

@twmr twmr closed this Aug 8, 2014
@twmr twmr mentioned this pull request Aug 27, 2014
Merged
@twmr twmr deleted the pep8examplesapiv3 branch October 10, 2014 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v1.4.x

Development

Successfully merging this pull request may close these issues.

5 participants

@twmr @ianthomas23 @tacaswell @efiring @WeatherGod