FIX markerfacecolor / mfc not in rcparams #8479

kalagau · Apr 13, 2017

PR for issue #3293 and #8109 as well as suggested by @anntzer

tacaswell · Apr 14, 2017

The test failures look real, can you reproduce them locally?

https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/tests/baseline_images/test_artist/default_edges.png that is the test that is changed.

anntzer · Apr 15, 2017

There's also markeredgecolor (#8109). Could you also take care of it?

kalagau · Apr 15, 2017

Sure, I am just trying to figure out the reason behind the tests failing before making anymore changes

kalagau · Apr 15, 2017

@tacaswell the fixes seem to pass both travis and appveyor now

tacaswell · Apr 16, 2017

👍

Can you add a note in https://github.com/matplotlib/matplotlib/tree/master/doc/users/whats_new and add these values to https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/mpl-data/stylelib/classic.mplstyle ?

kalagau · Apr 16, 2017

Done!

On consideration, this should have a test.

tacaswell · Apr 16, 2017

Sorry, one more thing, could you add a test? Something like

def test_mfc_rcparams():

    rcParams['line.markerfacecolor'] = 'red'
    ln = Line2D([1, 2], [1, 2])
    assert ln.get_markerfacecolor() == matplotlib.colors.to_rgba('red')

and so-on for the two rcparams added here.

I suggest putting this test in test_lines or test_rcparams.

kalagau · Apr 17, 2017

@tacaswell will these do?

tacaswell · Apr 17, 2017

lib/matplotlib/tests/test_rcparams.py

@@ -210,6 +210,19 @@ def test_legend_colors(color_type, param_dict, target):
        leg = ax.legend()
        assert getattr(leg.legendPatch, get_func)() == target

+@cleanup


Don't need cleanup anymore on master :)

tacaswell · Apr 17, 2017

Assuming they pass, yes 👍

kalagau · Apr 17, 2017

Great 👍

QuLogic

I'm a bit confused about the semantics. Usually, when we use 'none' it means no colour, but it appears that this is treating that as default colour. Have I understood this incorrectly?

Also, when this is complete, can you squash this?

QuLogic · Apr 18, 2017

doc/users/whats_new/new_rcparams_added.rst

@@ -0,0 +1,14 @@
+New rcParams added
+----------------------------------------------------------------------


Trim dashes to text length.

QuLogic · Apr 18, 2017

doc/users/whats_new/new_rcparams_added.rst

+
+* 'lines.markerfacecolor' allows users to set the default markerfacecolor for line plots
+* 'lines.markeredgecolor' allows users to set the default markeredgecolor for line plots
+The Default color is set to none, and these rcParams are only used if the line does not have any value specifed for these params.


Please wrap at 80 here as well. This line looks like it should not be part of the previous bullet point, so add a new line above it.

QuLogic · Apr 18, 2017

lib/matplotlib/lines.py

@@ -339,6 +339,10 @@ def __init__(self, xdata, ydata,
            linestyle = rcParams['lines.linestyle']
        if marker is None:
            marker = rcParams['lines.marker']
+        if markerfacecolor is None and not rcParams['lines.markerfacecolor'] == 'none':


rcParams['lines.markerfacecolor'] != 'none'

QuLogic · Apr 18, 2017

lib/matplotlib/lines.py

@@ -339,6 +339,10 @@ def __init__(self, xdata, ydata,
            linestyle = rcParams['lines.linestyle']
        if marker is None:
            marker = rcParams['lines.marker']
+        if markerfacecolor is None and not rcParams['lines.markerfacecolor'] == 'none':
+            markerfacecolor = rcParams['lines.markerfacecolor']
+        if markeredgecolor is None and not rcParams['lines.markeredgecolor'] == 'none':


Same as above.

kalagau · Apr 18, 2017

I used no color as a default to be able to tell if the user has specified a value for the rcParam value.
Previously I had a check like:
if markerfacecolor is None
markerfacecolor = rcParams['lines.markerfacecolor']

with the default rcParams['lines.markerfacecolor'] set to 'b'

But this would interfere with the tests. because it would replace the markerfacecolor with the default value, which I had initially set to blue

kalagau · Apr 19, 2017

@QuLogic your suggestions were implemented. I think this should be good now unless anyone else has some suggestions?

QuLogic · Apr 29, 2017

It seems okay, except I'm still confused about the semantics, though maybe I'm just mistaken about how rcParam defaults work. @tacaswell?

kalagau · Apr 30, 2017

By initially setting the rcParam to 'none', what essentially ends up happening is that the line gets drawn the same way when the user does not specify the color.

I think what you were perhaps implying before is that 'none' is a color option where no color is filled and by selecting that as a default I perhaps take away the user option to use that?

tacaswell · Aug 10, 2017

I think what you were perhaps implying before is that 'none' is a color option where no color is filled and by selecting that as a default I perhaps take away the user option to use that?

Yes 'none' is a valid color meaning "no color" (which is different than None). The default values should be the current default value.

tacaswell

The default value should not be special cased where used, it should instead be set to the current default value.

tacaswell · Aug 10, 2017

@kalagau Sorry this sat so long! Are you still interested in working on this?

kalagau · Mar 12, 2018

@tacaswell I have updated the PR. Assuming all tests pass, I feel that this should be fine now?

kalagau · Mar 12, 2018

I am not sure why Travis CI failed?

jklymak · Mar 14, 2018

We had some test failures with OS X. The CI is clean.

I'm not merging yet because @phobson 's review is quite stale. It'd be nice to have a more recent review, and marking as such. @tacaswell is quite busy, so may dismiss his review if there is other support for this.

phobson · Mar 19, 2018

@jklymak I just refreshed my review.

Based on my reading of the latest changes, the comments that @tacaswell provided have been addressed.

kalagau · Mar 26, 2018

@jklymak is there anything else we are waiting for?

I think the contributor has addressed concerns, and this has sat a long time.

jklymak · Mar 26, 2018

Thanks a lot for your patience and PR @kalagau!

carstenbauer · May 18, 2018

Which matplotlib version contains/will contain this?

jklymak · May 18, 2018

3.0

tacaswell added this to the 2.1 (next point release) milestone Apr 14, 2017

tacaswell previously approved these changes Apr 16, 2017

View reviewed changes

tacaswell reviewed Apr 17, 2017

View reviewed changes

phobson approved these changes Apr 17, 2017

View reviewed changes

QuLogic reviewed Apr 18, 2017

View reviewed changes

kalagau force-pushed the TeamDoItTomorrow3923 branch 2 times, most recently from 6b3f521 to 708c44f Compare April 18, 2017 22:21

kalagau changed the title ~~FIX markerfacecolor / mfc not in rcparams~~ [MRG+1] FIX markerfacecolor / mfc not in rcparams Apr 23, 2017

dstansby added the topic: rcparams label Apr 26, 2017

mwaskom mentioned this pull request May 31, 2017

seaborn styles make "+", "x" markers invisible; proposed workaround for shipped styles #8351

Closed

tacaswell previously requested changes Aug 10, 2017

View reviewed changes

tacaswell modified the milestones: 2.2 (next next feature release), 2.1 (next point release) Aug 10, 2017

afvincent mentioned this pull request Feb 7, 2018

matplotlibrc: new entry for placing y-axis tick label on Right or Left hand side. #10267

Closed

kalagau force-pushed the TeamDoItTomorrow3923 branch 4 times, most recently from d1ca0d4 to 9b4dcf5 Compare March 12, 2018 09:17

Add markerfacecolor and markeredgecolor to the rcParams for lines

1d9880c

kalagau force-pushed the TeamDoItTomorrow3923 branch from 9b4dcf5 to 1d9880c Compare March 12, 2018 09:40

jklymak approved these changes Mar 14, 2018

View reviewed changes

jklymak added the status: needs review label Mar 14, 2018

jklymak mentioned this pull request Mar 19, 2018

Cannot set the markeredgecolor by default #8109

Closed

phobson approved these changes Mar 19, 2018

View reviewed changes

jklymak merged commit 912f9b6 into matplotlib:master Mar 26, 2018

QuLogic modified the milestones: needs sorting, v3.0 Mar 26, 2018

QuLogic changed the title ~~[MRG+1] FIX markerfacecolor / mfc not in rcparams~~ FIX markerfacecolor / mfc not in rcparams Mar 26, 2018

QuLogic removed the status: needs review label May 1, 2018

OriolAbril mentioned this pull request Jul 16, 2019

Add titlecolor in rcParams #14707

Merged

6 tasks

anntzer mentioned this pull request Sep 26, 2019

markerfacecolor / mfc not in rcparams #3293

Closed

Search code, repositories, users, issues, pull requests...

Uh oh!

FIX markerfacecolor / mfc not in rcparams #8479

FIX markerfacecolor / mfc not in rcparams #8479

Uh oh!

Conversation

kalagau commented Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tacaswell commented Apr 14, 2017

Uh oh!

anntzer commented Apr 15, 2017

Uh oh!

kalagau commented Apr 15, 2017

Uh oh!

kalagau commented Apr 15, 2017

Uh oh!

tacaswell commented Apr 16, 2017

Uh oh!

kalagau commented Apr 16, 2017

Uh oh!

tacaswell commented Apr 16, 2017

Uh oh!

kalagau commented Apr 17, 2017

Uh oh!

tacaswell Apr 17, 2017

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Apr 17, 2017

Uh oh!

kalagau commented Apr 17, 2017

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

QuLogic Apr 18, 2017

Choose a reason for hiding this comment

Uh oh!

QuLogic Apr 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic Apr 18, 2017

Choose a reason for hiding this comment

Uh oh!

QuLogic Apr 18, 2017

Choose a reason for hiding this comment

Uh oh!

kalagau commented Apr 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kalagau commented Apr 19, 2017

Uh oh!

QuLogic commented Apr 29, 2017

Uh oh!

kalagau commented Apr 30, 2017

Uh oh!

tacaswell commented Aug 10, 2017

Uh oh!

tacaswell left a comment

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Aug 10, 2017

Uh oh!

kalagau commented Mar 12, 2018

Uh oh!

kalagau commented Mar 12, 2018

Uh oh!

jklymak commented Mar 14, 2018

Uh oh!

phobson commented Mar 19, 2018

Uh oh!

kalagau commented Mar 26, 2018

Uh oh!

jklymak commented Mar 26, 2018

Uh oh!

carstenbauer commented May 18, 2018

Uh oh!

jklymak commented May 18, 2018

Uh oh!

kalagau commented Apr 13, 2017 •

edited

Loading

QuLogic Apr 18, 2017 •

edited

Loading

kalagau commented Apr 18, 2017 •

edited

Loading