X Tutup
Skip to content

Set hexbin default linecolor to 'face'#7500

Merged
NelleV merged 3 commits intomatplotlib:masterfrom
dstansby:hexbin-edgecolors
Nov 23, 2016
Merged

Set hexbin default linecolor to 'face'#7500
NelleV merged 3 commits intomatplotlib:masterfrom
dstansby:hexbin-edgecolors

Conversation

@dstansby
Copy link
Copy Markdown
Member

@dstansby dstansby commented Nov 22, 2016

Fixes #7185

NelleV
NelleV requested changes Nov 22, 2016
View reviewed changes
Copy link
Copy Markdown
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.

Hi @dstansby
The patch looks good overall, but there are pep8 incompatible changes added. Also, can you document those changes in the default change section
(doc/users/dflt_style_changes.rst) and what's new section (doc/users/whats_new/README.rst).
Thanks

lib/matplotlib/axes/_axes.py Outdated
If 'none', draws the edges in the same color as the fill color.
This is the default, as it avoids unsightly unpainted pixels
between the hexagons.
edgecolors : {'face', 'none', *None*} or mpl color, optional, default is 'face'
Copy link
Copy Markdown
Member

@NelleV NelleV Nov 22, 2016

Choose a reason for hiding this comment

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

This line is too long for pep8. It should be 79 char or less.

Copy link
Copy Markdown
Member Author

@dstansby dstansby Nov 23, 2016

Choose a reason for hiding this comment

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

I'm currently struggling to find a way to split it on to two lines without the 2nd line just becoming part of the paragraph underneath... Any ideas?

Copy link
Copy Markdown
Member

@tacaswell tacaswell Nov 23, 2016

Choose a reason for hiding this comment

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

This is an acceptable place to use \ to break the doc line.

Copy link
Copy Markdown
Member

@NelleV NelleV Nov 23, 2016

Choose a reason for hiding this comment

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

something like
edgecolors : {"face", "none", None, color}, optional, default: "face" would also have been acceptable.

@NelleV NelleV changed the title Set hexbin default linecolor to 'face' [MRG] Set hexbin default linecolor to 'face' Nov 22, 2016
@tacaswell tacaswell added this to the 2.0 (style change major release) milestone Nov 23, 2016
@tacaswell tacaswell changed the title [MRG] Set hexbin default linecolor to 'face' [MRG+1] Set hexbin default linecolor to 'face' Nov 23, 2016
tacaswell
tacaswell reviewed Nov 23, 2016
View reviewed changes
doc/users/dflt_style_changes.rst

The default value of the ``linecolor`` kwarg for `~matplotlib.Axes.hexbin` has
changed from ``'none'`` to ``'face'``. If 'none' is now supplied, no line edges
are drawn around the hexagons.
Copy link
Copy Markdown
Member

@tacaswell tacaswell Nov 23, 2016

Choose a reason for hiding this comment

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

This should note that the default behavior has not changed (just the meaning of 'none').

@efiring
Copy link
Copy Markdown
Member

efiring commented Nov 23, 2016

On 2016/11/23 10:34 AM, Thomas A Caswell wrote:

@tacaswell commented on this pull request.

In lib/matplotlib/axes/_axes.py
#7500:

@@ -4163,10 +4163,12 @@ def hexbin(self, x, y, C=None, gridsize=100, bins=None,
linewidths : scalar, optional, default is None
If None, defaults to 1.0.

  •    edgecolors : {'none'} or mpl color, optional, default is 'none'

  •        If 'none', draws the edges in the same color as the fill color.

  •        This is the default, as it avoids unsightly unpainted pixels

  •        between the hexagons.

  •    edgecolors : {'face', 'none', _None_} or mpl color, optional, default is 'face'

This is an acceptable place to use || to break the doc line.

Or just leave out the "optional"? After all, specifying the default
only makes sense if it is optional.
Alternatively, a reasonable convention would be to say that if the
default is not specified on an optional parameter, the default is the
first entry in the list of possibilities.

@QuLogic
Copy link
Copy Markdown
Member

QuLogic commented Nov 23, 2016

the default is the first entry in the list of possibilities.

That's how Numpydoc already defines arguments that take a restricted set of inputs.

@NelleV NelleV merged commit adfeab7 into matplotlib:master Nov 23, 2016
@NelleV
Copy link
Copy Markdown
Member

NelleV commented Nov 23, 2016

Thanks for the patch @dstansby !

@QuLogic QuLogic changed the title [MRG+1] Set hexbin default linecolor to 'face' Set hexbin default linecolor to 'face' Nov 23, 2016
@dstansby dstansby deleted the hexbin-edgecolors branch November 24, 2016 11:06
@jklymak jklymak mentioned this pull request Oct 26, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@NelleV NelleV NelleV requested changes

@tacaswell tacaswell tacaswell approved these changes

Assignees

No one assigned

Labels

API: changes

Projects

None yet

Milestone

v2.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@dstansby @efiring @QuLogic @NelleV @tacaswell
X Tutup