Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default boxplot style rebase #6481

Merged
merged 13 commits into from May 30, 2016

Conversation

jenshnielsen
Copy link
Member

Rebase of #5523 on master hopefully fixing the travis issues.

@jenshnielsen jenshnielsen added this to the 2.0 (style change major release) milestone May 26, 2016
@jenshnielsen
Copy link
Member Author

@phobson I hope you are ok with me making a few changes on top of your PR so we can get it merged before we cut the 2.x beta. I have tried to do a few modifications as possible to restore old default behaviours with matplotlib 1.5.x classic style and fix the tests. Let me know if you can spot any issues.
Otherwise I think this is ready for review. I will need to squash all image changes together before merging but prefer that to wait until this has been reviewed.

@phobson
Copy link
Member

phobson commented May 26, 2016

@jenshnielsen absolutely! sorry i was lagging behind

@@ -941,33 +941,37 @@ def validate_animation_writer_path(p):
'boxplot.showfliers': [True, validate_bool],
'boxplot.meanline': [False, validate_bool],

'boxplot.flierprops.color': ['C0', validate_color],
Copy link
Member

Choose a reason for hiding this comment

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

All of these changes need to be reflected into the template.

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I can see they already do

@tacaswell
Copy link
Member

Something is not right here, the output from http://matplotlib.org/examples/pylab_examples/boxplot_demo.html does not seem to respect the color (either on this branch or v1.5.1) but does work on master http://matplotlib.org/devdocs/examples/pylab_examples/boxplot_demo.html

@phobson
Copy link
Member

phobson commented May 28, 2016

@tacaswell that is weird. I'm not sure what's going on.

@jenshnielsen I'm not sure why the medians aren't showing up in this example either: https://github.com/matplotlib/matplotlib/pull/6481/files#diff-b5878226d9a3230ab74d34de5a0f994f

Going for the Spinal Tap approach.

Q: How much blacker could the boxplots be?
A: A little, actually. I left some color in the
median line.
It was just "blue". Now it's [0] to pick out the
first color from the existing color cycle,
whatever that may be.
@jenshnielsen
Copy link
Member Author

@tacaswell I fixed the color issue (The flier color was set by the marker argument but not the face and edgecolor which was set to default values)

@jenshnielsen
Copy link
Member Author

The effect of the color can also be seen in the changes to boxplot_mod_artists_after_plotting.png
The fliers used to have their color set from

bp = ax.boxplot(x, sym="o")
    for key in bp:
        for obj in bp[key]:
            obj.set_color('green')

This is no longer the case because edge and facecolor now takes effect

@jenshnielsen
Copy link
Member Author

Running out of time to look more at this at the moment. I will try to get back to it later today.

This will need some test image updated after the fix above.

There seems to be something wrong with test_bxp_custompatchartist
the linestyle is not correctly reflected in the test image (dotted)

@jenshnielsen
Copy link
Member Author

@phobson Sorry for being stupid but can you point me exactly to which example is missing the median?
I can only see bxp_patchartist.png which simply has the same patch color as the plotted under the median making it hard to see

@phobson
Copy link
Member

phobson commented May 30, 2016

@jenshnielsen
Copy link
Member Author

So we are talkign about bxp_patchartist.png?
In that test we are plotting a median which has the color C0 on top of a patch with a color C0 so naturally it's not really visible.

We should probably either change the default median color or change the default patch color to use some thing else than patch.facecolor

@phobson
Copy link
Member

phobson commented May 30, 2016

Oh yeah. That makes sense. This is tricky. My preference is that if we're filling in the box with a patch artist, the the median should be the same color as the border of the patch artist. 

Sorry if this was unintelligible. I'm on my phone.

On Mon, May 30, 2016 at 7:48 AM -0700, "Jens Hedegaard Nielsen" notifications@github.com wrote:

So we are talkign about bxp_patchartist.png?

In that test we are plotting a median which has the color C0 on top of a patch with a color C0 so naturally it's not really visible.

We should probably either change the default median color or change the default patch color to use some thing else than patch.facecolor


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@jenshnielsen
Copy link
Member Author

Thats not really easy to do unfortunately. To do that I think we would need to add a different rc param for the median color when using patch. I changed the defaults mean and median to C1 and C2 which are different from the patch color. I squashed the image changing commits together

@tacaswell tacaswell merged commit 1bf3377 into matplotlib:master May 30, 2016
tacaswell added a commit that referenced this pull request May 31, 2016
@tacaswell
Copy link
Member

backported to v2.x as ff98856

@phobson
Copy link
Member

phobson commented May 31, 2016

thanks @tacaswell and @jenshnielsen !!

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

Successfully merging this pull request may close these issues.

None yet

3 participants