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

Support pixel-by-pixel alpha in imshow. #14889

Merged
merged 10 commits into from
Sep 10, 2019

Conversation

tillahoffmann
Copy link
Contributor

@tillahoffmann tillahoffmann commented Jul 25, 2019

PR Summary

This PR supports pixel-by-pixel alpha channels in imshow and is a replacement for #8183.

cc @jklymak

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
    • Likely too minor a change to need a dedicated example?
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
    • Not a major new feature.
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way
    • No API changes.

@story645
Copy link
Member

This is important/asked for enough that I'd consider it a major feature, so docs would be a plus.

@tillahoffmann
Copy link
Contributor Author

tillahoffmann commented Jul 25, 2019

@story645, there are some doc updates here. What sort of additional documentation do you have in mind? A further extension of the docstring or an example similar to https://github.com/matplotlib/matplotlib/blob/master/examples/images_contours_and_fields/image_masked.py? If an example, do you have a preference for adding this to an existing example or creating a dedicated one?

@story645
Copy link
Member

Adding to an existing example would be great, or as a new one in the color section of the examples.

@tillahoffmann
Copy link
Contributor Author

tillahoffmann commented Jul 25, 2019

It looks like we could update this example with the new implementation. In particular, the following no longer applies:

While imshow makes it easy to visualize a 2-D matrix as an image, it doesn't easily let you add transparency to the output.

Happy for me to rewrite while keeping the generated images the same?

@story645
Copy link
Member

that'd be perfect, thanks!

lib/matplotlib/image.py Outdated Show resolved Hide resolved
@jklymak jklymak added this to the v3.2.0 milestone Jul 25, 2019
Copy link

@ruidazeng ruidazeng left a comment

Choose a reason for hiding this comment

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

More detailed documentations will be nice. It is still a bit difficult to understand just by reading the source codes.

@tillahoffmann
Copy link
Contributor Author

@ruidazeng, what information would you like to see specifically?

@tillahoffmann
Copy link
Contributor Author

@jklymak, @story645, any additional thoughts on this?

@story645
Copy link
Member

story645 commented Aug 4, 2019

@tillahoffmann I'm happy with the docs, thanks!

@@ -281,7 +282,11 @@ def set_alpha(self, alpha):
----------
alpha : float
"""
martist.Artist.set_alpha(self, alpha)
if np.isscalar(alpha):
Copy link
Contributor

Choose a reason for hiding this comment

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

the docs of np.isscalar basically tell you to not use this function, and suggests np.ndim(alpha) == 0 instead: https://docs.scipy.org/doc/numpy/reference/generated/numpy.isscalar.html

Copy link
Contributor

Choose a reason for hiding this comment

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

also, should this unset any previously set array_alpha?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the pointer regarding np.isscalar. Yes, the _array_alpha should also be unset. Having said that, maybe the implementation proposed in tillahoffmann#1 is preferable because it also addresses @jklymak's #14889 (comment) above.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I prefer the approach of tillahoffmann#1, yes this means that the type of get_alpha() changed but that seems better than lying as to the alpha value.
OTOH, tillahoffmann#1 also has a change to composite_images and that change appears to not take array_alpha into account? (Try with two overlapping images that use array alpha, and save to pdf to check.)
It may be fine to just disable software compositing in that case if it's too complicated, and draw the two images separately.

@tacaswell tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Aug 19, 2019
The alpha blending value, between 0 (transparent) and 1 (opaque).
This parameter is ignored for RGBA input data.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have dropped This parameter is ignored for RGBA input data. because it appears to be respected by the released matplotlib==3.1.1.

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

Thanks @tillahoffmann !

@tacaswell
Copy link
Member

I restarted the appveyor CI which looks like it had a worker fail. Given that azure passed, I don't think that this is a windows related bug.

@ImportanceOfBeingErnest
Copy link
Member

Is there a reason to allow vectorized alpha only for images, and not just every ScalarMappable?

lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
self._imcache = None

def _get_scalar_alpha(self):
Copy link
Member

Choose a reason for hiding this comment

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

This appears to just return 1 for alpha-arrays. IMHO this needs documentation: What's the purpose of this function and why this kind of handling of alpha-arrays?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a description.

@tillahoffmann
Copy link
Contributor Author

tillahoffmann commented Sep 9, 2019

Is there a reason to allow vectorized alpha only for images, and not just every ScalarMappable?

Hm, yes, good point. @ImportanceOfBeingErnest, are you suggesting adding the handling of alpha arrays here?

def to_rgba(self, x, alpha=None, bytes=False, norm=True):

@ImportanceOfBeingErnest
Copy link
Member

Yes, naively I would think one could have ScalarMappable._alpha which can be a scalar or of the same shape as ScalarMappable._A. However there might be problems because alpha is usually a property of the artist, not the mixin. But if there are no problems, having every ScalarMappable being able to profit from vectorized alphas would be nice. E.g. there are many cases where you cannot use imshow so sooner or later people would ask to have it implemented for pcolor-like functions as well.

@timhoffm
Copy link
Member

timhoffm commented Sep 9, 2019

If the intention is to broaden to all ScalarMappables this should move to 3.3. I don't think we have a clear enough view and enough testing time to bring such a major change into 3.2.

Are there any possible problems with merging this as is for imshow only and think on a generalization later?

@ImportanceOfBeingErnest
Copy link
Member

On the other hand it would be a pity to delay it if it turns out that a more general solution is not desireable. I think the real question is: Does the implementation as shown here prevent a more general solution in the future (in the sense of needing to deprecate newly introduced stuff again etc.)?

@anntzer
Copy link
Contributor

anntzer commented Sep 9, 2019

We can always tag the current approach as experimental (as we did for constrained_layout).

@tillahoffmann
Copy link
Contributor Author

I believe the approach proposed by @ImportanceOfBeingErnest is certainly preferable over the implementation in this PR, but I don't think we'd have to break any interface in the future if we wanted to support vector-alpha at the level of the ScalarMappable mix-in.

@tacaswell
Copy link
Member

I am in favor of merging this now and addressing generalizing it in 3.3. It is better to get useful, but incomplete code out rather than wait for perfect general solutions.

This does not add any public API other than allowing imshow to take a greater range of inputs.

This also connects to #8738

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: color/alpha
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants