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

Pep8ify examples #3425

Merged
merged 21 commits into from Sep 9, 2014
Merged

Pep8ify examples #3425

merged 21 commits into from Sep 9, 2014

Conversation

thisch
Copy link
Contributor

@thisch thisch commented Aug 27, 2014

This PR converts the animation, api and axes_grid examples.

(Retry of #3182/#3183)

I addressed @ianthomas23' comments from #3183

Signed-off-by: Thomas Hisch <t.hisch@gmail.com>
Signed-off-by: Thomas Hisch <t.hisch@gmail.com>
Signed-off-by: Thomas Hisch <t.hisch@gmail.com>
@@ -46,19 +46,17 @@ def derivs(state, t):
th2 = -10.0
w2 = 0.0

rad = pi/180
Copy link
Member

Choose a reason for hiding this comment

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

Why is this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unused var

@tacaswell
Copy link
Member

Left a few small comments, 👍 from me once they are made/addressed.

As a note to other reviewers, https://github.com/matplotlib/matplotlib/pull/3425/files?w=1 makes your like better by not showing you lines with pure-white space changes.

@tacaswell tacaswell added this to the v1.5.x milestone Aug 27, 2014
@thisch
Copy link
Contributor Author

thisch commented Aug 28, 2014

@tacaswell addressed your comments

@tacaswell
Copy link
Member

lgtm, but given the scope and automatic nature of this PR, I would like at least one more dev to review it.

@@ -27,10 +27,10 @@

# Make some spirals
r = np.array(range(nverts))
theta = np.array(range(nverts)) * (2*np.pi)/(nverts-1)
theta = np.array(range(nverts))*(2*np.pi)/(nverts - 1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

np.linspace(0, 2*np.pi, nverts) would probably make this a lot clearer - @tacaswell WDYT ?

Copy link
Member

Choose a reason for hiding this comment

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

Didn't a bunch of these files get changed during the SciPy sprints a few
months ago? Particularly with respect to spacing around operators? Or was
that in the codebase itself?

Also, now that these files have been PEP8-ified, I think there is some sort
of white-list or black-list in the testing directory that indicates which
files should be tested for pep8-compliance. This way the files stay pep8-ed.

On Thu, Aug 28, 2014 at 9:09 AM, Thomas Hisch notifications@github.com
wrote:

In examples/api/collections_demo.py:

@@ -27,10 +27,10 @@

Make some spirals

r = np.array(range(nverts))
-theta = np.array(range(nverts)) * (2_np.pi)/(nverts-1)
+theta = np.array(range(nverts))_(2*np.pi)/(nverts - 1)

np.linspace(0, 2*np.pi, nverts) would probably make this a lot clearer -
@tacaswell https://github.com/tacaswell WDYT ?


Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/3425/files#r16837317.

Copy link
Member

Choose a reason for hiding this comment

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

👍 to both comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@WeatherGod are you referring to the blacklist in the test_coding_standards.py file?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes. So, a quick read through that file, it seems to me that we don't
pep8 test the example files at all, I think? Should we?

On Thu, Aug 28, 2014 at 9:52 AM, Thomas Hisch notifications@github.com
wrote:

In examples/api/collections_demo.py:

@@ -27,10 +27,10 @@

Make some spirals

r = np.array(range(nverts))
-theta = np.array(range(nverts)) * (2_np.pi)/(nverts-1)
+theta = np.array(range(nverts))_(2*np.pi)/(nverts - 1)

@WeatherGod https://github.com/WeatherGod are you referring to the
blacklist in the test_coding_standards.py file?


Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/3425/files#r16839843.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I added a simple implementation in test_coding_standards.py. Let's see if it works with travis CI.

@thisch
Copy link
Contributor Author

thisch commented Aug 28, 2014

As the examples do not get installed I have to use the example from the git repo. Any idea how I can determine the local path of the workspace of the mpl repo in .travis.yml?

Edit: done

@WeatherGod
Copy link
Member

ugh... yeah, didn't think about that. Perhaps we shouldn't worry about that, then?

@thisch
Copy link
Contributor Author

thisch commented Aug 28, 2014

If there are no objections, this is ready to merge

pep8_additional_ignore=PEP8_ADDITIONAL_IGNORE):
extra_exclude_directories=None,
pep8_additional_ignore=PEP8_ADDITIONAL_IGNORE,
dirname=None, expected_bad_files=None):
Copy link
Member

Choose a reason for hiding this comment

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

only add kwargs at the end to maintain back-compatibility.

@tacaswell
Copy link
Member

As long as you are going through everything can you also remove the line continuation \ ?

@thisch
Copy link
Contributor Author

thisch commented Aug 28, 2014

@tacaswell which line continuation(s) ?

def drag_pan(self, button, key, x, y):
pass

# Now, the transforms themselves.

class HammerTransform(Transform):

Copy link
Member

Choose a reason for hiding this comment

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

spurious blank line

@WeatherGod
Copy link
Member

There are just too many files for me to go through at the moment. At this point, I am -1 on merging. I don't know what tool you used for pep8-ing, either it is buggy or something is wrong with my interpretation of pep8.

Signed-off-by: Thomas Hisch <t.hisch@gmail.com>
Signed-off-by: Thomas Hisch <t.hisch@gmail.com>
@thisch
Copy link
Contributor Author

thisch commented Aug 30, 2014

@WeatherGod I addressed all your comments

return {'custom_hammer':mspines.Spine.circular_spine(self,
(0.5, 0.5), 0.5)}
return {'custom_hammer': mspines.Spine.circular_spine(self,
(0.5, 0.5), 0.5)}
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 still under 80 characters (can't tell with the github viewer)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes: 78 chars

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, the line length of the examples gets checked in the test_coding_standards unit test

Copy link
Member

Choose a reason for hiding this comment

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

ah, good.

grid = AxesGrid(fig, 141, # similar to subplot(141)
nrows_ncols = (2, 2),
grid = AxesGrid(fig, 141, # similar to subplot(141)
nrows_ncols=(2, 2),
axes_pad = 0.05,
Copy link
Member

Choose a reason for hiding this comment

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

I guess the autopep8 tool got fatigued and gave up removing the spaces for these kwargs...

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 can't reproduce it. Probably I messed it up.

edit: looked into the wrong line. It is indeed a problem of autopep8 - created hhatto/autopep8#158

nrows_ncols=(1, 3),
axes_pad = 0.1,
add_all=True,
label_mode = "L",
Copy link
Member

Choose a reason for hiding this comment

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

still have extra spaces around the "axes_pad" and "label_mode" kwargs

@WeatherGod
Copy link
Member

Just want to see if I get the gist of the changes to test_coding_standards.py. Basically, it looks like you refactored it to get rid of the globals, rights?

@thisch
Copy link
Contributor Author

thisch commented Aug 30, 2014

@WeatherGod, yes I wanted to get rid of the globals.


# prepare the demo image
Z, extent = get_demo_image()
Z2 = np.zeros([150, 150], dtype="d")
ny, nx = Z.shape
Z2[30:30+ny, 30:30+nx] = Z
Z2[30:30 + ny, 30:30 + nx] = Z
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 personally prefer NO spaces around the arithmetic operator in this case. I'll revert this change if you are fine with that

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'm not going to change this as I found occurrences of indexing expressions in the matplotlib src where spaces around operators are both used (type1font.py) and not used (tri/triinterpolate.py).

@WeatherGod
Copy link
Member

Does anybody else want to give this a look-through? I think it is +1 at this point, though.

@thisch
Copy link
Contributor Author

thisch commented Sep 8, 2014

ping

@@ -13,7 +13,7 @@
ax.xaxis.set_major_formatter(formatter)

xs = np.logspace(1, 9, 100)
ys = (0.8 + 0.4 * np.random.uniform(size=100)) * np.log10(xs)**2
ys = (0.8 + 0.4*np.random.uniform(size=100))*np.log10(xs)**2
Copy link
Member

Choose a reason for hiding this comment

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

It seems to be an idiosyncrasy of the particular pep8 tool that it always puts spaces around plus and minus, and never allows them around asterisk and solidus. This is completely contrary to the letter and spirit of the actual PEP 8. Nevertheless, it seems we are stuck with it because the advantage of automated checking is deemed to outweigh the disadvantage of the occasional poor choice made by the automated tools. (In the line above, having spaces around the second asterisk would slightly enhance readability.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The spaces were removed by me on purpose and not by the (auto)pep8 tool. I'll readd the spaces around the second asterisk in a coming PR

Copy link
Member

Choose a reason for hiding this comment

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

@thisch I don't want to be too fussy--many things are a matter of taste, and not worth fiddling with--but if you are making extensive changes to improve style and make things more consistent with mpl guidelines, then you could also try to eliminate the use of trailing backslashes as line continuations. Methods include the use of parentheses to make a list of imports, or breaking an import up into multiple imports; use of a temporary variable for a chunk of an expression that is otherwise getting out of hand; and taking advantage of whatever form of parentheses or brackets is appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@efiring thx for your comment. I care a lot about consistency and therefore want to help making the mpl codebase more consistent. There already exists a tool to break a list of imports into multiple imports called isort (https://github.com/timothycrosley/isort - check out its Readme file). Automating fixing your other mentioned issues is probably not worth the effort even if I fully agree to your comments.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, but I think isort is probably overkill for us, and would result in unproductive code churn. I think our imports are mostly OK. (If it were to be used, my configuration choices would include "multi-line-output=0".) I sympathize with the author's explanation at the end: "I'm too lazy to spend 8 hours mindlessly performing a function, but not too lazy to spend 16 hours automating it."

efiring added a commit that referenced this pull request Sep 9, 2014
@efiring efiring merged commit f7c68b6 into matplotlib:master Sep 9, 2014
@efiring
Copy link
Member

efiring commented Sep 9, 2014

Good enough; overall an improvement; time to move on. Thanks for sticking with it.

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

5 participants