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

Merged
merged 5 commits into from Jan 14, 2013

Projects

None yet

6 participants

@tacaswell
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
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
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
Member

rebased and forced as requested

@tacaswell
Member

rebased and forced again

@NelleV NelleV commented on an outdated diff Jan 14, 2013
lib/matplotlib/lines.py
@@ -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':
@NelleV
NelleV Jan 14, 2013 Contributor

PEP8 nitpick: there should be spaces around ==

@NelleV NelleV commented on the diff Jan 14, 2013
lib/matplotlib/tests/test_axes.py
@@ -1005,6 +1006,19 @@ def test_mollweide_inverse_forward_closure():
# compare
np.testing.assert_array_almost_equal(xy, xy2, 3)
+
@NelleV
NelleV Jan 14, 2013 Contributor

PEP8 nitpick: there should 2 blank lines and not 3 here

@NelleV NelleV commented on an outdated diff Jan 14, 2013
lib/matplotlib/tests/test_axes.py
@@ -1005,6 +1006,19 @@ def test_mollweide_inverse_forward_closure():
# compare
np.testing.assert_array_almost_equal(xy, xy2, 3)
+
+
+@image_comparison(baseline_images=['translucent_markers'], remove_text=True)
+def test_translucent_markers():
+ np.random.seed(0)
+ data = np.random.random(50)
+
+ fig = plt.figure()
+ ax = fig.add_subplot(111)
+ ax.plot(data, 'D', mfc=[1,0,0,.5], markersize=100)
+
+
@NelleV
NelleV Jan 14, 2013 Contributor

Same here :)

@NelleV
NelleV Jan 14, 2013 Contributor

(2 blank lines except of 3)

@NelleV NelleV commented on an outdated diff Jan 14, 2013
lib/matplotlib/tests/test_axes.py
@@ -1005,6 +1006,19 @@ def test_mollweide_inverse_forward_closure():
# compare
np.testing.assert_array_almost_equal(xy, xy2, 3)
+
+
+@image_comparison(baseline_images=['translucent_markers'], remove_text=True)
+def test_translucent_markers():
+ np.random.seed(0)
+ data = np.random.random(50)
+
+ fig = plt.figure()
+ ax = fig.add_subplot(111)
+ ax.plot(data, 'D', mfc=[1,0,0,.5], markersize=100)
@NelleV
NelleV Jan 14, 2013 Contributor

PEP8 nitpick: there should be spaces after ,

@NelleV
Contributor
NelleV commented Jan 14, 2013

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

Thomas A Caswell pep8 fixes dcdbc5a
@NelleV
Contributor
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
Member
mdboom commented Jan 14, 2013

๐Ÿ‘

@pelson
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 pelson merged commit 09b9c04 into matplotlib:master Jan 14, 2013
@tacaswell tacaswell deleted the tacaswell:issue1504 branch Jan 14, 2013
@mdboom
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
Contributor
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 tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 23, 2013
@tacaswell tacaswell Fixed code added in 0c20b5c
(PR #1505).

Tests added is #1663 pass with this patch.
17fa943
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment