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

Issue 1504: changed how draw handles alpha in markerfacecolor #1505

Merged
merged 5 commits into from Jan 14, 2013

Conversation

tacaswell
Copy link
Member

changed how draw handles alpha in markerfacecolor so that the set value respected.

At least in the Agg backend, the alpha value of the face color passed into draw_markers (in _backend_agg.cpp) is overridden by the value in gc_obj. It is not clear to me this in the correct behavior, but these changes fix the problem with minimal changes

@WeatherGod
Copy link
Member

I am guess you will have to rebase your branch onto the latest master. I have yet to really look over the changes, but at first glance, it makes sense.

@dmcdougall
Copy link
Member

I am in agreement with @WeatherGod here. This is a useful patch. Thanks for taking time out of your day to do this @tacaswell.

When you rebase, you'll get some conflicts you'll need to fix. Then you'll need to git push --force up to your github branch.

@tacaswell
Copy link
Member Author

rebased and forced as requested

@tacaswell
Copy link
Member Author

rebased and forced again

@@ -961,6 +965,14 @@ def _get_rgb_face(self, alt=False):
rgbFace = colorConverter.to_rgb(facecolor)
return rgbFace

def _get_rgba_face(self, alt=False):
facecolor = self._get_markerfacecolor(alt=alt)
if is_string_like(facecolor) and facecolor.lower()=='none':
Copy link
Member

Choose a reason for hiding this comment

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

PEP8 nitpick: there should be spaces around ==

@NelleV
Copy link
Member

NelleV commented Jan 14, 2013

The code looks good. I've underlined a couple of PEP8 problems

@NelleV
Copy link
Member

NelleV commented Jan 14, 2013

👍 LGTM, but I'm not a core dev, so my opinion doesn't matter :)

I haven't run the tests, but I trust travis to do it (and not crash while doing it).

@mdboom
Copy link
Member

mdboom commented Jan 14, 2013

👍

@pelson
Copy link
Member

pelson commented Jan 14, 2013

👍 LGTM, but I'm not a core dev, so my opinion doesn't matter :)

I trust you know that's not so @NelleV.

@tacaswell - this looks good to me too. I think we can merge this happily.

pelson added a commit that referenced this pull request Jan 14, 2013
NF - Transparency for markers.
@pelson pelson merged commit 09b9c04 into matplotlib:master Jan 14, 2013
@tacaswell tacaswell deleted the issue1504 branch January 14, 2013 15:57
@mdboom
Copy link
Member

mdboom commented Jan 14, 2013

@NelleV: Let me second @pelson's comment about your opinion ;) My 👍 earlier was in reference to this PR being ready, not to your opinion not mattering, just to be clear :)

@NelleV
Copy link
Member

NelleV commented Jan 15, 2013

I don't really have a sufficient knowledge of matplotlib to do good reviews (but I do consider myself as being part of the PEP8 consistency brigade :) )

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 23, 2013
(PR matplotlib#1505).

Tests added is matplotlib#1663 pass with this patch.
@tacaswell tacaswell mentioned this pull request Jan 23, 2013
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.

None yet

6 participants