Skip to content

scatter: fix marker kwarg bug. Closes #4073, #3895. - #4079

Merged
tacaswell merged 2 commits into
matplotlib:masterfrom
efiring:scattercleanup
Feb 19, 2015
Merged

tacaswell merged 2 commits into
matplotlib:masterfrom
efiring:scattercleanup

Conversation

@efiring

@efiring efiring commented Feb 8, 2015

Copy link
Copy Markdown
Member

I also tried to clean up some of the scatter code and documentation,
together with related documentation in collections. I added a test.

@efiring

efiring commented Feb 8, 2015

Copy link
Copy Markdown
Member Author

This also raises a question: does anyone know why scatter still has the undocumented "verts" kwarg? We should either remove it, with deprecation if necessary, or include it in the docstring.

@tacaswell tacaswell added this to the 1.5.0 milestone Feb 8, 2015
@tacaswell

Copy link
Copy Markdown
Member

I am fine with removing it as it looks like it is one more way to specify the marker shape. Probably do need deprecation cycle though.

marker can also be array like as MarkerStyle can take arrays as input.

I also tried to clean up some of the scatter code and documentation,
together with related documentation in collections.  I added a test.
@mdboom

mdboom commented Feb 9, 2015

Copy link
Copy Markdown
Member

Wow. Good find guys.

@tacaswell

Copy link
Copy Markdown
Member

Merged this locally so that I could add a commit (f200421) to document the code removal on top.

Merged to master as 86e5e65

philippjfr added a commit to holoviz/holoviews that referenced this pull request Mar 25, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Coming in late to the discussion but I have just spent a few minutes trying to determine what I did wrong on my scatter graphs...

I thought c stood as an acronym for color as fc stands for facecolor etcetera... however this does not apply here, specifying color='green' instead of c='green' means you set the edge-colour to green instead of the face-colour...

Should I open an issue/PR about this? Or can people live with this inconsistency?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As far as I understand it, 'c' should be an alias for 'color'. Furthermore,
'color' should specify both edge and face colors, unless explicitly
overriden by 'facecolor' and such.

On Wed, Apr 29, 2015 at 12:45 PM, OceanWolf notifications@github.com
wrote:

In lib/matplotlib/axes/_axes.py
#4079 (comment):

  •    if ec is not None:
    
  •        edgecolors = ec
    
  •    fc = kwargs.pop('facecolor', None)
    
  •    if fc is not None:
    
  •        facecolors = fc
    
  •    fc = kwargs.pop('facecolors', None)
    
  •    if fc is not None:
    
  •        facecolors = fc
    
  •    # 'color' should be deprecated in scatter, or clearly defined;
    
  •    # since it isn't, I am giving it low priority.
    
  •    co = kwargs.pop('color', None)
    
  •    if co is not None:
    
  •        if edgecolors is None:
    
  •            edgecolors = co
    
  •        if facecolors is None:
    
  •            facecolors = co
    

Coming in late to the discussion but I have just spent a few minutes
trying to determine what I did wrong on my scatter graphs...

I thought c stood as an acronym for color as fc stands for facecolor
etcetera... however this does not apply here, specifying color='green'
instead of c='green' means you set the edge-colour to green instead of
the face-colour...

Should I open an issue/PR about this? Or can people live with this
inconsistency?

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is a confusing mess for historical reasons. It looks like there might be a need for "facecolors=None" in the arg list, along with "edgecolors=None". I suspect I had a reason for not including this in the PR, but I don't remember; maybe it is worth a try.

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.

5 participants