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

BUG: respect alpha in RGBA markeredgecolor; closes #4580 #4586

Merged
merged 4 commits into from Jul 5, 2015

Conversation

efiring
Copy link
Member

@efiring efiring commented Jul 5, 2015

No description provided.

@tacaswell
Copy link
Member

One question, which we may want to put off, is do we want the alpha to stack? That is if the artist alpha is .5 and the color is (1, 1, 1, .5), the color we set to the gc is (1, 1, 1, .25)?

rgbaFace is not None):
gc.set_alpha(rgbaFace[3])
else:
gc.set_alpha(self.get_alpha())
Copy link
Contributor

Choose a reason for hiding this comment

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

Not too sure about this line. If we have set an alpha, then this will replace the alpha on the marker edge colour. This feels wrong as a more specific value should take precedence over a more general value, right?

As I see it we should multiply alpha values here, so we do something like:

edgecolor[3] *= self.get_alpha()

and the same for the other colours...

@OceanWolf
Copy link
Contributor

@tacaswell Yes, see my line comment above, I see no other logical way to handle it.

@tacaswell
Copy link
Member

Right, but the issue is that we have not been handling it that way. If
we do the multiplication here, then we should do it for all of the other
places where set colors, which would be a back compatibility break (if I am
recalling things correctly).

On Sun, Jul 5, 2015 at 1:34 PM OceanWolf notifications@github.com wrote:

@tacaswell https://github.com/tacaswell Yes, see my line comment above,
I see no other logical way to handle it.


Reply to this email directly or view it on GitHub
#4586 (comment)
.

@efiring
Copy link
Member Author

efiring commented Jul 5, 2015

@tacaswell, instituting stacking of alpha handling would be a big API break, so no, we don't want to do it now, if at all. All I want to do now is restore behavior consistent with our long-established practice. I think it was broken a couple years ago, and no one noticed until now--but I haven't been able to track down the exact history, and don't care to spend any more time on that. What matters is getting behavior consistent with the present policy, which is that explicitly-set artist alpha overrides RGBA, but otherwise any explicit RGBA should be respected. The present PR passes the test_alpha test (which I think was associated with smoothing out a color inheritance wrinkle while accidentally breaking RGBA in mec--but I'm not positive), together with a new test added to check the mec handling.

@efiring
Copy link
Member Author

efiring commented Jul 5, 2015

It is not obvious that stacking is the right policy in any case; it makes it hard to simply set alpha to a known value, like unity.

@OceanWolf
Copy link
Contributor

If we view alpha as working then yes, a compatibility break in the way we handle colour... one might call it a colour-overhaul 😉.

If we view alpha as universally broken than no.

@tacaswell
Copy link
Member

@efiring Right, agree 100%.

I am checking that http://matplotlib.org/examples/pylab_examples/filledmarker_demo.html that sort thing behaves correctly then I will merge.

@efiring
Copy link
Member Author

efiring commented Jul 5, 2015

@OceanWolf, Alpha is working; it is not "universally broken". It is working in one of the two possible ways, a choice that was made long ago. Revisiting that decision can be done in the context of a potential major API break and overhaul of the machinery for handling colors; but not quickly. This is not the right time for it. Probably the right way to have alpha stacking via kwarg would be with a new kwarg. In any case, rather than patching in yet more complex handling of combinations of arguments, it would make sense to re-engineer the handling of colors and sets of colors from top to bottom. Right now it is extra-confusing because color policy is scattered all over the place, at several levels, and there is a proliferation of color-related kwargs and aliases.

@OceanWolf
Copy link
Contributor

@efiring Why would we want to hard set alpha like that? Imagine for example we want to change the alpha of an artist during run-time (something we do in 3d-plots for instance where alpha represents distance from user), I find multiplication/stacking makes more sense as we retain core visual information.

Try this example, you see that neither the marker colour nor the marker edge colour gets respected.

import matplotlib
import matplotlib.pyplot as plt
fc = (1,0,0,0.5)
mec = (0,0,0,0.1)
lines = plt.plot(range(10),'-o', color=fc, markeredgecolor=mec,
                 markersize=100, markeredgewidth=10, alpha=1.)
plt.show()

@tacaswell
Copy link
Member

@efiring I have a PR against your branch which fixes that filled marker demo (which I can no longer find on master!).

@OceanWolf
Copy link
Contributor

@efiring Okay, I agree, if we have time to change it then 2.0 would make a good time to do it, otherwise a color-overhaul-pt-2 for 3.0.

It possibly makes sense to have a flatten_alpha command at that time, but not convinced we would actually need it.

@efiring
Copy link
Member Author

efiring commented Jul 5, 2015

@OceanWolf Yes, in your example the alpha kwarg overrides the A in RGBA. That's just the way it is designed, like it or not. If you want the A to be respected, leave out the alpha kwarg, or set it to None which is the default. I can understand that there are situations where stacking behavior would be desired; but that's an additional behavior, not a matter of "fixing" the present behavior.

FIX: respect alpha on alt face color
@tacaswell
Copy link
Member

For completeness, see #2788 for why the example is gone.

rgbaFaceAlt is not None):
gc.set_alpha(rgbaFaceAlt[3])
else:
gc.set_alpha(self.get_alpha())
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this go in a separate function for maintainability? Virtually the same code as above, just with rgbaFace replaced with rgbaFaceAlt.

Copy link
Member Author

Choose a reason for hiding this comment

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

No. It's just 4 lines. To avoid passing in lots of things it would have to be a local function, defined inside the draw method. You would end up with less readability. If almost all of the marker "draw" functionality could be encapsulated in such a way that the same function could be used for the "regular" part and then called again for the "alt" part, that might be a good refactoring. But even that is probably not worth fiddling with now, and certainly not in the scope of this PR.

tacaswell added a commit that referenced this pull request Jul 5, 2015
FIX: respect alpha in RGBA markeredgecolor

closes #4580
@tacaswell tacaswell merged commit d58a2a5 into matplotlib:master Jul 5, 2015
@efiring efiring deleted the mec_alpha branch June 23, 2016 18:16
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

3 participants