FIX markerfacecolor / mfc not in rcparams by kalagau · Pull Request #8479 · matplotlib/matplotlib · GitHub

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

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.

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

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

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

👍

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 ?

Done!

On consideration, this should have a test.

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.

@tacaswell will these do?

Don't need cleanup anymore on master :)

Assuming they pass, yes 👍

Great 👍

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?

Trim dashes to text length.

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.

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

Same as above.

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

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

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

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?

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.

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

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

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

I am not sure why Travis CI failed?

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.

@jklymak I just refreshed my review.

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

@jklymak is there anything else we are waiting for?

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

Thanks a lot for your patience and PR @kalagau!

Which matplotlib version contains/will contain this?

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 done

anntzer mentioned this pull request Sep 26, 2019

markerfacecolor / mfc not in rcparams #3293

Closed

                           linestyle = rcParams['lines.linestyle']
                       if marker is None:
                           marker = rcParams['lines.marker']
+                      if markerfacecolor is None and not rcParams['lines.markerfacecolor'] == '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':

                       leg = ax.legend()
                       assert getattr(leg.legendPatch, get_func)() == target
+              @cleanup

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

+              * '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.

FIX markerfacecolor / mfc not in rcparams by kalagau · Pull Request #8479 · matplotlib/matplotlib · GitHub

Repository navigation

FIX markerfacecolor / mfc not in rcparams - #8479

FIX markerfacecolor / mfc not in rcparams#8479jklymak merged 1 commit intomatplotlib:mastermatplotlib/matplotlib:masterfrom kalagau:TeamDoItTomorrow3923kalagau/matplotlib:TeamDoItTomorrow3923Copy head branch name to clipboard

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!

Reviewers

FIX markerfacecolor / mfc not in rcparams#8479
jklymak merged 1 commit into
matplotlib:masterfrom
kalagau:TeamDoItTomorrow3923

kalagau commented Apr 13, 2017 •
edited
Loading

QuLogic Apr 18, 2017 •
edited
Loading

kalagau commented Apr 18, 2017 •
edited
Loading