Add alpha-array support to _rgb_to_rgba#26520
Add alpha-array support to _rgb_to_rgba#26520AALAM98mod100 wants to merge 6 commits intomatplotlib:mainfrom
Conversation
|
Hi @AALAM98mod100 - just leaving a note that if you want this to be reviewed make sure you mark it as "Ready for review". Cheers! |
|
Thanks for the PR! As far as I can tell, it looks good! However, I am a bit surprised to see that it doesn't work for SVGs? Especially since there is an embedded PNG in the SVG. This may be a another issue though... But do you have any comments on that? Maybe we should wait with adding SVG and PDF test images (although they are quite small, so not sure if it really is a problem). Right now we are in the final stages of releasing 3.8, so it may take some time to get additional feedback. |
| @image_comparison(['image_alpha_array'], remove_text=True) | ||
| def test_image_alpha_array(): | ||
| _, ax = plt.subplots() | ||
|
|
||
| # create a 2x2 grid. If alpha is applying correctly, | ||
| # the top half should not match the 0.5 valued pixel | ||
| # in the bottom half. | ||
| arr = np.array([[.5, .5], [.75, .5]]) | ||
| cmap = plt.get_cmap('gray') | ||
| norm = colors.Normalize() | ||
| arr_rgb = cmap(norm(arr))[:, :, :3] | ||
| alpha = np.ones_like(arr) | ||
| alpha[:1] = 0.2 | ||
| ax.imshow(arr, alpha=alpha, cmap='gray', interpolation='none') |
There was a problem hiding this comment.
This can be better written as a check_figures_equal test. Compare a (n, m, 3) array plus additional alpha to the same plot with a (n, m, 4) array.
There was a problem hiding this comment.
It is also concerning that the png and svg images are different....
| extension. | ||
| """ | ||
| rgba = np.zeros((A.shape[0], A.shape[1], 4), dtype=A.dtype) | ||
| if alpha is None or np.ndim(alpha) == 0: |
There was a problem hiding this comment.
what input is np.ndim(alpha) == 0 addressing?
There was a problem hiding this comment.
Scalar alpha input should remaing unaffected by new code. This ensures just that
|
Just as a FYI, in the original issue thread, some incorrect result was observed using |
| elif A.shape[2] == 4: | ||
| array_alpha = self.get_alpha() | ||
| if array_alpha is not None and np.ndim(array_alpha) != 0: | ||
| A[:, :, 3] *= array_alpha # blend alphas of image and param |
There was a problem hiding this comment.
This modifies A in place where as in the other branches we get a copy of A back. Because it modifies in in place and we cache A higher up in the call stack it will apply the alpha everytime it draws.
tacaswell
left a comment
There was a problem hiding this comment.
The tests should use check_figures_equal and the multiple application of alpha needs to be fixed.
|
Thank you for your work on this @AALAM98mod100. The RGB case has now been fixed by #28437, which also made a refactor causing the conflicts we now see here. Given those conflicts and the fact that this PR was anyway stalled for some time, I think it's sensible to close this one in favour of #30523, which addresses the RGBA case. |
PR summary
PR checklist
Plotting demo:
Code