PEP8 fixes on backend_bases.py #1345

Merged
merged 2 commits into from Oct 11, 2012

Projects

None yet

6 participants

@NelleV
Contributor
NelleV commented Oct 9, 2012

Pep8 fixes on the module backend_bases

@efiring efiring commented on an outdated diff Oct 9, 2012
lib/matplotlib/backend_bases.py
return width, height, descent
flags = self._text2path._get_hinting_flag()
font = self._text2path._get_font(prop)
size = prop.get_size_in_points()
font.set_size(size, dpi)
- font.set_text(s, 0.0, flags=flags) # the width and height of unrotated string
+ font.set_text(
+ s, 0.0, flags=flags) # the width and height of unrotated string
@efiring
efiring Oct 9, 2012 Member

I don't think it aids readability for a comment to cause an otherwise short statement to be split like this.
Alternatives: put the comment on a separate line (below, indented), or shorten the comment, e.g., "width, height of unrotated string".

@efiring efiring commented on an outdated diff Oct 9, 2012
lib/matplotlib/backend_bases.py
self.inaxes = axes_list[0]
try:
- xdata, ydata = self.inaxes.transData.inverted().transform_point((x, y))
+ xdata, ydata = self.inaxes.transData.inverted(
+ ).transform_point((x, y))
@efiring
efiring Oct 9, 2012 Member

Ack! If this line is too long (is it actually > 79 characters?), then the only decent way I see to shorten it is to use a temporary variable:

invtrans = self.inaxes.transData.inverted()
xdata, ydata = invtrans.transform_point((x, y))
@efiring efiring commented on an outdated diff Oct 9, 2012
lib/matplotlib/backend_bases.py
@@ -1299,7 +1316,8 @@ def _update_enter_leave(self):
class MouseEvent(LocationEvent):
"""
- A mouse event ('button_press_event', 'button_release_event', 'scroll_event',
+ A mouse event (
+ 'button_press_event', 'button_release_event', 'scroll_event',
@efiring
efiring Oct 9, 2012 Member

This is a bad break. Just move 'scroll_event' to the next line in the original.

@efiring efiring commented on an outdated diff Oct 9, 2012
lib/matplotlib/backend_bases.py
inaxes = None # the Axes instance if mouse us over axes
- xdata = None # x coord of mouse in data coords
- ydata = None # y coord of mouse in data coords
- step = None # scroll steps for scroll events
+ xdata = None # x coord of mouse in data coords
+ ydata = None # y coord of mouse in data coords
+ step = None # scroll steps for scroll events
@efiring
efiring Oct 9, 2012 Member

Very picky, and I won't insist; but leaving the hash marks aligned really would look better in this case.

@efiring efiring and 1 other commented on an outdated diff Oct 9, 2012
lib/matplotlib/backend_bases.py
# Try deleting that artist, or its parent if you
# can't delete the artist
while h:
- print("Removing",h)
+ print("Removing", h)
@efiring
efiring Oct 9, 2012 Member

This is not a PEP 8 question, but this line looks like a debugging statement that got left in by mistake. I'm pretty sure it should be commented out.

@NelleV
NelleV Oct 9, 2012 Contributor

I'm deleting it.

@efiring efiring and 1 other commented on an outdated diff Oct 9, 2012
lib/matplotlib/backend_bases.py
'raw': 'Raw RGBA bitmap',
'rgba': 'Raw RGBA bitmap',
'svg': 'Scalable Vector Graphics',
'svgz': 'Scalable Vector Graphics'
- }
+ }
@efiring
efiring Oct 9, 2012 Member

This indentation of the closing brace looks wrong to my eye; is there actually a PEP 8 edict about it? If not, I think the original is much better, giving a better picture of the logical structure.

@NelleV
NelleV Oct 9, 2012 Contributor

Not that I know of: usually, you don't align } with the text above: there are several ways to do that. I'll change this indentation.

@efiring efiring commented on an outdated diff Oct 9, 2012
lib/matplotlib/backend_bases.py
svg = self.switch_backends(FigureCanvasSVG)
return svg.print_svgz(*args, **kwargs)
if _has_pil:
- filetypes['jpg'] = filetypes['jpeg'] = 'Joint Photographic Experts Group'
+ filetypes['jpg'] = filetypes[
+ 'jpeg'] = 'Joint Photographic Experts Group'
@efiring
efiring Oct 9, 2012 Member

This is a bad break. Make it two separate lines.

filetypes['jpeg'] = 'Joint Photographic Experts Group'
filetypes['jpg'] = filetypes['jpeg']

Or use a temporary variable to hold the string.

@efiring efiring commented on an outdated diff Oct 9, 2012
lib/matplotlib/backend_bases.py
@@ -2055,16 +2084,20 @@ def print_figure(self, filename, dpi=None, facecolor='w', edgecolor='w',
bbox_extra_artists = kwargs.pop("bbox_extra_artists", None)
if bbox_extra_artists is None:
- bbox_extra_artists = self.figure.get_default_bbox_extra_artists()
+ bbox_extra_artists = \
@efiring
efiring Oct 9, 2012 Member

Please, no trailing backslash continuations. One way out here would be to use the temporary variable, "fig = self.figure". That may be the best that can be done short of more substantial refactoring and renaming. Long names are good--but only up to a point.

@efiring efiring commented on an outdated diff Oct 9, 2012
lib/matplotlib/backend_bases.py
@@ -2551,7 +2589,7 @@ class NavigationToolbar2(object):
(None, None, None, None),
('Subplots', 'Configure subplots', 'subplots', 'configure_subplots'),
('Save', 'Save the figure', 'filesave', 'save_figure'),
- )
+ )
@efiring
efiring Oct 9, 2012 Member

Original indentation was better.

@NelleV
Contributor
NelleV commented Oct 9, 2012

Updated with a new patch.

@dmcdougall dmcdougall and 1 other commented on an outdated diff Oct 9, 2012
lib/matplotlib/backend_bases.py
@@ -364,7 +369,8 @@ def _iter_collection(self, gc, master_transform, all_transforms,
xo, yo = toffsets[i % Noffsets]
if offset_position == 'data':
if Ntransforms:
- transform = all_transforms[i % Ntransforms] + master_transform
+ transform = all_transforms[
+ i % Ntransforms] + master_transform
@dmcdougall
dmcdougall Oct 9, 2012 Member

I think you should break this line at the +.

@efiring
efiring Oct 10, 2012 Member

@NelleV I agree with @dmcdougall that this is a bad break; with parentheses you could follow his suggestion.

@dmcdougall dmcdougall commented on an outdated diff Oct 9, 2012
lib/matplotlib/backend_bases.py
if self.flipy():
- transform = Affine2D().scale(fontsize/text2path.FONT_SCALE,
- fontsize/text2path.FONT_SCALE).\
- rotate(angle).translate(x, self.height-y)
+ transform = Affine2D().scale(fontsize / text2path.FONT_SCALE,
+ fontsize / text2path.FONT_SCALE).\
+ rotate(angle).translate(
+ x, self.height - y)
else:
@dmcdougall
dmcdougall Oct 9, 2012 Member

As @efiring pointed out, trailing backslashes can be alleviated by being a little more proactive. Perhaps create a temporary variable to hold the value of fontsize/text2path.FONT_SCALE?

@dmcdougall dmcdougall commented on an outdated diff Oct 9, 2012
lib/matplotlib/backend_bases.py
else:
- transform = Affine2D().scale(fontsize/text2path.FONT_SCALE,
- fontsize/text2path.FONT_SCALE).\
- rotate(angle).translate(x, y)
+ transform = Affine2D().scale(fontsize / text2path.FONT_SCALE,
+ fontsize / text2path.FONT_SCALE).\
+ rotate(angle).translate(x, y)
@dmcdougall
dmcdougall Oct 9, 2012 Member

Ditto here.

@dmcdougall dmcdougall commented on an outdated diff Oct 9, 2012
lib/matplotlib/backend_bases.py
if ismath:
width, height, descent, glyphs, rects = \
- self._text2path.mathtext_parser.parse(s, dpi, prop)
+ self._text2path.mathtext_parser.parse(s, dpi, prop)
@dmcdougall
dmcdougall Oct 9, 2012 Member

Here I don't think the backslash looks too bad. I think the only way to deal with it is to do the following:

dims = self._text2path.mathtext_parser.parse(s, dpi, prop)
return dims[0:3]  # return width, height, descent
@dmcdougall dmcdougall commented on the diff Oct 9, 2012
lib/matplotlib/backend_bases.py
return width, height, descent
flags = self._text2path._get_hinting_flag()
font = self._text2path._get_font(prop)
size = prop.get_size_in_points()
font.set_size(size, dpi)
- font.set_text(s, 0.0, flags=flags) # the width and height of unrotated string
+ # the width and height of unrotated string
+ font.set_text(s, 0.0, flags=flags)
@dmcdougall
dmcdougall Oct 9, 2012 Member

Good. Much better.

@dmcdougall dmcdougall commented on an outdated diff Oct 9, 2012
lib/matplotlib/backend_bases.py
inaxes = None # the Axes instance if mouse us over axes
- xdata = None # x coord of mouse in data coords
- ydata = None # y coord of mouse in data coords
+ xdata = None # x coord of mouse in data coords
+ ydata = None # y coord of mouse in data coords
@dmcdougall
dmcdougall Oct 9, 2012 Member

Either make these comments line up, or shift them all to the left. I don't really have a preference.

@dmcdougall dmcdougall commented on an outdated diff Oct 9, 2012
lib/matplotlib/backend_bases.py
@@ -1245,28 +1260,30 @@ def __init__(self, name, canvas, x, y,guiEvent=None):
# Find all axes containing the mouse
if self.canvas.mouse_grabber is None:
- axes_list = [a for a in self.canvas.figure.get_axes() if a.in_axes(self)]
+ axes_list = [
+ a for a in self.canvas.figure.get_axes() if a.in_axes(self)]
@dmcdougall
dmcdougall Oct 9, 2012 Member

This break is a little untidy. Perhaps a temporary variable to store the output of self.canvas.figure.get_axes()?

@dmcdougall dmcdougall and 1 other commented on an outdated diff Oct 9, 2012
lib/matplotlib/backend_bases.py
self.inaxes = axes_list[0]
try:
- xdata, ydata = self.inaxes.transData.inverted().transform_point((x, y))
+ xdata, ydata = self.inaxes.transData.inverted().transform_point(
+ (x, y))
@dmcdougall
dmcdougall Oct 9, 2012 Member

Perhaps another temporary variable to store the output of self.inaxes.transData.inverted()?

@efiring
efiring Oct 10, 2012 Member

@NelleV This is another important suggestion by @dmcdougall.

@dmcdougall dmcdougall commented on the diff Oct 9, 2012
lib/matplotlib/backend_bases.py
- x = None # x position - pixels from left of canvas
- y = None # y position - pixels from right of canvas
- button = None # button pressed None, 1, 2, 3
- dblclick = None # whether or not the event is the result of a double click
- inaxes = None # the Axes instance if mouse us over axes
- xdata = None # x coord of mouse in data coords
- ydata = None # y coord of mouse in data coords
- step = None # scroll steps for scroll events
+ x = None # x position - pixels from left of canvas
+ y = None # y position - pixels from right of canvas
+ button = None # button pressed None, 1, 2, 3
+ dblclick = None # whether or not the event is the result of a double click
+ inaxes = None # the Axes instance if mouse us over axes
+ xdata = None # x coord of mouse in data coords
+ ydata = None # y coord of mouse in data coords
+ step = None # scroll steps for scroll events
@dmcdougall dmcdougall commented on the diff Oct 9, 2012
lib/matplotlib/backend_bases.py
# Try deleting that artist, or its parent if you
# can't delete the artist
while h:
- print("Removing",h)
@dmcdougall dmcdougall commented on the diff Oct 9, 2012
lib/matplotlib/backend_bases.py
svg = self.switch_backends(FigureCanvasSVG)
return svg.print_svgz(*args, **kwargs)
if _has_pil:
- filetypes['jpg'] = filetypes['jpeg'] = 'Joint Photographic Experts Group'
+ filetypes['jpg'] = 'Joint Photographic Experts Group'
+ filetypes['jpeg'] = filetypes['jpg']
+
@dmcdougall
dmcdougall Oct 9, 2012 Member

Much better. Thanks.

@dmcdougall dmcdougall commented on an outdated diff Oct 9, 2012
lib/matplotlib/backend_bases.py
- bbox_filtered = [b for b in bb if b.width!=0 or b.height!=0]
+ bbox_filtered = [
+ b for b in bb if b.width != 0 or b.height != 0]
@dmcdougall
dmcdougall Oct 9, 2012 Member

This break is a little untidy, perhaps replicate the breaking on the new lines 2088--2089?

@dmcdougall dmcdougall and 2 others commented on an outdated diff Oct 9, 2012
lib/matplotlib/backend_bases.py
- bbox_filtered = [b for b in bb if b.width!=0 or b.height!=0]
+ bbox_filtered = [b
+ for b
+ in bb
+ if b.width != 0 or b.height != 0]
@dmcdougall
dmcdougall Oct 9, 2012 Member

Oh dear. I don't think this is better either.

@efiring
efiring Oct 9, 2012 Member

It would be quite readable as two lines if the second line started with "if".

@NelleV
NelleV Oct 9, 2012 Contributor

This is fixed.

@dmcdougall dmcdougall and 2 others commented on an outdated diff Oct 9, 2012
lib/matplotlib/backend_bases.py
@@ -1245,28 +1258,32 @@ def __init__(self, name, canvas, x, y,guiEvent=None):
# Find all axes containing the mouse
if self.canvas.mouse_grabber is None:
- axes_list = [a for a in self.canvas.figure.get_axes() if a.in_axes(self)]
+ axes_list = [a
+ for a
+ in self.canvas.figure.get_axes()
+ if a.in_axes(self)]
@dmcdougall
dmcdougall Oct 9, 2012 Member

This one as well.

@efiring
efiring Oct 9, 2012 Member

Again, two lines, start second with "if".

@NelleV
NelleV Oct 9, 2012 Contributor

This should be fixed as well.

@dmcdougall
Member

I'll have a think about those list comprehensions, but other than that I'm giving this my +1.

@dmcdougall
Member

Excellent! Thanks @NelleV.

@efiring efiring commented on an outdated diff Oct 9, 2012
lib/matplotlib/backend_bases.py
if bbox_filtered:
_bbox = Bbox.union(bbox_filtered)
- bbox_inches1 = TransformedBbox(_bbox,
- Affine2D().scale(1./self.figure.dpi))
+ bbox_inches1 = TransformedBbox(
+ _bbox,
+ Affine2D().scale(1. / self.figure.dpi))
bbox_inches = Bbox.union([bbox_inches, bbox_inches1])
@efiring
efiring Oct 9, 2012 Member

Suggestion:

trans = Affine2D().scale(1.0 / self.figure.dpi)
bbox_inches1 = TransformedBbox(_bbox, trans)
@dmcdougall
Member

@efiring Thanks for your input on this. When you're happy, make it known.

@dmcdougall
Member

@NelleV When you've implemented @efiring's suggestion, could you squash all these to a single commit?

@NelleV
Contributor
NelleV commented Oct 10, 2012

It's done

@dmcdougall
Member

@NelleV Cheers. The Hawaiians are sleeping at the moment, so we'll wait a little bit to get Eric's approval.

@efiring efiring commented on an outdated diff Oct 10, 2012
lib/matplotlib/backend_bases.py
path, transform = path_id
- transform = transforms.Affine2D(transform.get_matrix()).translate(xo, yo)
+ transform = transforms.Affine2D(
+ transform.get_matrix()).translate(xo, yo)
@efiring
efiring Oct 10, 2012 Member

In all the examples like this, I would favor using more indentation for the second line, perhaps aligning it with the start of the RHS of the line above. This would make it more visually obvious that it is a continuation of the line above, not an independent line in a block.

@WeatherGod WeatherGod commented on an outdated diff Oct 10, 2012
lib/matplotlib/backend_bases.py
@@ -499,24 +505,25 @@ def _get_text_path_transform(self, x, y, s, prop, angle, ismath):
fontsize = self.points_to_pixels(prop.get_size_in_points())
if ismath == "TeX":
- verts, codes = text2path.get_text_path(prop, s, ismath=False, usetex=True)
+ verts, codes = text2path.get_text_path(
+ prop, s, ismath=False, usetex=True)
@WeatherGod
WeatherGod Oct 10, 2012 Member

PEP8 says: "When using a hanging indent the following considerations should be applied; there should be no arguments on the first line and further indentation should be used to clearly distinguish itself as a continuation line." While I guess it shouldn't be required here, I just simply put myself in the habit of using two tabs in such cases.

@WeatherGod WeatherGod commented on the diff Oct 10, 2012
lib/matplotlib/backend_bases.py
'raw': 'Raw RGBA bitmap',
'rgba': 'Raw RGBA bitmap',
'svg': 'Scalable Vector Graphics',
- 'svgz': 'Scalable Vector Graphics'
- }
+ 'svgz': 'Scalable Vector Graphics'}
@WeatherGod
WeatherGod Oct 10, 2012 Member

Actually, I have found it to be better to end lists and dictionaries like this with the last element having a comma and putting the closing brace or bracket on its own line. This makes for fewer merge conflicts as it is easier for patch to handle any edits to such a list of things. Of course, that is merely my own opinion.

@dmcdougall
dmcdougall Oct 11, 2012 Member

That's sensible.

@WeatherGod WeatherGod commented on the diff Oct 10, 2012
lib/matplotlib/backend_bases.py
@@ -2057,16 +2083,15 @@ def print_figure(self, filename, dpi=None, facecolor='w', edgecolor='w',
if bbox_extra_artists is None:
bbox_extra_artists = self.figure.get_default_bbox_extra_artists()
- bb = [a.get_window_extent(renderer) for a in bbox_extra_artists]
+ bb = [a.get_window_extent(renderer)
+ for a in bbox_extra_artists]
@WeatherGod
WeatherGod Oct 10, 2012 Member

bad binary operator break here

@NelleV
NelleV Oct 10, 2012 Contributor

I'm not sure I understand what you mean here. this is not a binary operator

@efiring
efiring Oct 10, 2012 Member

@WeatherGod Do you mean that you would put the "for" on the first line? Regardless of what PEP 8 says or doesn't say, that would look wrong to me; "for...artists" forms a phrase that should not be split.

@WeatherGod
WeatherGod Oct 10, 2012 Member

I have always interpreted that line in PEP8 to mean any operator like "if", "for", "else", in addition to operators like "+", '/", etc.

@NelleV
NelleV Oct 10, 2012 Contributor

PEP8 does not say anything about list comprehension. What @WeatherGod mentions is that a break should occur after a binary operator (and/or)::

if (something_very_long() and
   something_else_ver_long():)

and not before::

if (something_very_long()
     and something_else_very_long())
@dmcdougall
dmcdougall Oct 10, 2012 Member

I think the way @NelleV has done it here is fine. The fact these operators are English words means they feel like phrases. Breaking after a for or if would disrupt the natural flow.

At the end of the day it doesn't really matter, as long as it's consistent.

@WeatherGod
WeatherGod Oct 11, 2012 Member

Given the lack of definitiveness of this issue in PEP8, I decided to go looking elsewhere in the python documentation to see how it is done. Indeed, it is never actually stated anywhere how to properly break up the comprehension, but it appears that others also put the break before the operator. I take back my comments and give in to the crowd.

@WeatherGod WeatherGod commented on the diff Oct 10, 2012
lib/matplotlib/backend_bases.py
- bbox_filtered = [b for b in bb if b.width!=0 or b.height!=0]
+ bbox_filtered = [b for b in bb
+ if b.width != 0 or b.height != 0]
@efiring
efiring Oct 10, 2012 Member

@WeatherGod Same here: The way it is now looks to me like the right way to break it. Think of how you would read it out loud; what are the natural chunks? Would you pause after the "if"? I don't mind letting the binary arithmetic operator stay on the first line (though I don't see any good reason for that rule), but I think "if" is different, and "for" is even more different.

@efiring efiring merged commit 74c74d5 into matplotlib:master Oct 11, 2012
@pwuertz

This change triggers the problem in #1443

@minrk

This change caused #1492

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment