Clip RGB data to valid range for imshow #10220

Zac-HD · 2018-01-10T13:53:03Z

~~Axes.imshow can now apply the norm or vmin and vmax arguments to RGB images, allowing easy display of high-bit-depth data, or adjustments of brightness and contrast.~~

Out-of-range values in un-normalised RGB and RGBA images are now clipped to the nearest bound, rather than being wrapped. This bug often hid outliers, and could make interpretation of the plotted image entirely unreliable.

Closes #9391. Closes #5382. Related to pydata/xarray#1796.

Has Pytest style unit tests
Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature

@tacaswell, I'd appreciate any suggestions you might have about tests.

tacaswell · 2018-01-10T14:51:46Z

This is definitely not going in to 2.1.2 as this is a new feature not a bug-fix.

I am not convinced that this is a good idea, if the user is passing in an RGB(A) image, then the assumption is that it is a ready-to-go image and we should do as little processing as possible on it. Starting to do so opens up a big can of worms (for example, this uses the same norm for all of the planes, it also seems reasonable to norm each plane independently). Once you go down this road the range of extensions seems like it will get large which is why this logic should live in the application level.

The Normalize methods do not return strictly [0, 1] values either, they put in sentinels for over/under/bad which needs to be accounted for.

I think the fixes should be clarifying the docstring (which seems to have already been done) and detecting when we get high-bit-depth ints and down-scaling them to 8 bit (we output Nx8 bit colors so the down sampling is going to happen eventually), and clipping the floats.

In either case, the logic needs to be AxesImage not in imshow so it applies to data later set via im.set_data.

tacaswell

Aside from my general concerns about adding this feature, the logic needs to live in AxesImage not in imshow so it applies to data set via set_data and it needs tests.

jklymak · 2018-01-10T15:18:10Z

Yeah, this is the same argument as in the other Issues. RGB(A) is RGB(A). If the upstream app doesn't know how to form an RGB array within proper limits, its hard to get excited about helping them. I think a good argument can be made that we should clip instead of modulo values that are out of range, but I don't agree with allowing vmin/vmax.

anntzer · 2018-01-10T15:37:25Z

I don't like the idea either at all, but if it does go in in some form this should probably use a machinery similar to #8738 because it'd be silly to have one mechanism for normalizing two-channel images and another for three-channel images.

dstansby · 2018-01-10T16:51:34Z

I also agree on forcing RGB(A) to be between 0 and 1 (as far as I can think all it really requires is im = im / np.max(im) if you're original data isn't in that range). I think we could do better on the docstring though, and erroring/warning on out of range input.

jklymak · 2018-01-10T17:13:19Z

I think we can error or clip/warn/info on out-of-range for im.

I'm -1 on automatic normalization if values are greater than 1. How do we know the users data doesn't really go to >max(im) in other images that they are trying to compare.

I'm also not in favour of applying arbitrary user-supplied normalizations to each RGB(A) channel in imshow including using vmin or vmax or the non-linear Norms.

To me, and I assume whoever wrote imshow, specifying RGB(A) means the user wants that color plotted on the screen (within the limits of color gamuts etc etc).

Its clear there is a body of users who thinks of RGB as channel1/channel2/channel3 with arbitrary data in them, and that they have developed enough intuition about what those images look like to be able to quantitatively see something in those three channels. I don't think imshow should change for that body of users. On the otherhand, a new function channelrgbimage or whatever name makes the most sense to that community, could be easy enough to add. Then vmin/vmax could be specified as floats or length-3 arrays, and three normalizations (one for each channel) could be supplied to map into colorspace.

As @anntzer points out, #8738 was designed to map 2-channel data to arbitrary colors. Thats a fair bit more general than this use case, which strictly maps to RGB.

efiring · 2018-01-10T18:23:51Z

Adding to the chorus, and going a bit farther: at most, we should error out or clip with a warning. Users who want to scale the channels are free to do so outside of mpl. I don't see any reason why that functionality should be part of mpl.

jklymak · 2018-01-10T18:42:04Z

I guess what many users want is clip, but we should _log.warn so unattentive users don't get a saturated image and not realize they've lost dynamic range of their image. If they don't like the warning, they can clip/normalize their data properly (or suppress the warning via logging.)

jklymak · 2018-01-11T00:39:45Z

lib/matplotlib/image.py

@@ -13,6 +13,7 @@

 from math import ceil
 import os
+import warnings


I'd strongly suggest this is a use for logging instead of warnings.

import logging .... _log = logging.getLogger(__name__) .... _log.warn('Clipping...')

I disagree, but I'm happy to let @tacaswell or another maintainer settle it.

FWIW I did a search for each pattern and found 21 log calls (mostly for import errors or configuration state changes) and 66 warnings (mostly for invalid calls or data), so warnings seem to be the convention here (as in Numpy, Pandas, etc).

Logging was just added a couple of months ago.

Just to follow up, when we added logging, we decided to follow the guidelines at https://docs.python.org/3/howto/logging.html#logging-basic-tutorial for when to use which tool:

warnings.warn() in library code if the issue is avoidable and the client application should be
modified to eliminate the warning

logging.warning() if there is nothing the client application can do about the situation, but the
event should still be noted

We decided not go back and check that the current instances of warnings.warn follow that pattern, but agreed to fix them as they come along.

I'd drop my "strongly" above, and mildly argue that this warning is the latter case, and the particular advantage of logging is that the warnings can be turned off if the user is OK and cognizant w/ the clipping. I don't think warnings.warn can be turned off, or at least not on a per-module basis.

Ah, that makes sense! The contributing guide is actually pretty good 😄

Zac-HD · 2018-01-11T03:29:09Z

I'm seeing a strong consensus that matplotlib should not normalize or scale RGB or RGBA images, so I've dropped that part of the pull. However this still serves to close the linked issues (as "wontfix") and allows downstream libraries to support it without worrying about API compatibility.

I've moved the clipping logic into the add_data method where it belongs, but not added tests yet.

Zac-HD · 2018-01-11T13:35:12Z

Converted from warnings.warn to _log.warn, added test.

~~An image comparison test also seems like a good idea, but I can't get everything to build 😕. If this is needed, would someone else be able to supply the reference images?~~ Edit: I'm now testing this by checking the max and min of .get_array() on the image.

WeatherGod · 2018-01-11T16:54:19Z

doc/users/next_whats_new/2018_01_11_imshow-rgb-scaling-clipping.rst

+When `Axes.imshow` is passed an RGB or RGBA value with out-of-range
+values, it now issues a warning and clips them to the valid range.
+The old behaviour, wrapping back in to the range, often hid outliers
+and made interpreting RGB images unreliable.


Should we clarify what ranges are, or perhaps link to an explanation of the ranges?

The ranges are explained by the docs for Axes.imshow, and the reference to it here is turned into a link by the backticks IIRC.

Sorry to drag this out. I think this really needs to be in the API changes as well in case smeone was depending on the wrap behaviour:

Log of changes to Matplotlib that affect the outward-facing API. If updating Matplotlib breaks your scripts, this list may help you figure out what caused the breakage and how to fix it by updating your code.

WeatherGod · 2018-01-11T16:58:21Z

lib/matplotlib/image.py

+            # - otherwise casting wraps extreme values, hiding outliers and
+            # making reliable interpretation impossible.
+            high = 255 if np.issubdtype(self._A.dtype, np.integer) else 1
+            if self._A.min() < 0 or high < self._A.max():


I wonder if we should pad this by an eps in case the 0-1 values came from some numerical computation, and we probably wouldn't want to spam the user with unnecessary warnings?

We have to clip to exactly [0 .. 1], and for consistency I would prefer to always warn if clipping. If users feel spammed, they can either fix their data or filter the logging output.

I think the suggestion might have been to silently clip if (a-vmax)/(vmin - vmax) - 1 < 1e-10 or something that would just represent bit noise that would never affect the interpretation of the data.

Yes, that's the question - I just think the answer is "No, we shouldn't".

Agree that we shouldn't.

Zac-HD · 2018-01-12T03:34:40Z

The remaining test errors are because the pytest caplog fixture is unavailable on some Travis environments, which in turn is because they are running old versions of pytest (3.1.0, caplog was added in 3.3.0, latest is 3.3.2).

I can mark this with a skipif, or if you don't mind requiring a newer version of pytest I'll just leave it as-is.

anntzer · 2018-01-12T03:39:41Z

I would adjust .travis.yml (and whatever docs) to bump the minimal required pytest version.
Given the progressively wider use of logging in mpl it makes sense to require a version of pytest that has caplog...

jklymak · 2018-01-12T04:30:56Z

lib/matplotlib/image.py

+            # making reliable interpretation impossible.
+            high = 255 if np.issubdtype(self._A.dtype, np.integer) else 1
+            if self._A.min() < 0 or high < self._A.max():
+                _log.warn(


Its _log.warning, isn't it?

Yep, turns out that _log.warn "is functionally identical but deprecated" (but not programmatically deprecated).

Zac-HD · 2018-01-13T04:59:48Z

OK, I've responded to all the review comments, tests are present and passing, and Travis has been told to use a new version of pytest.

Travis isn't using a new version of pytest, but I think that can be fixed by blowing away some caches and re-running the job.

efiring

Approved subject to tests passing.

jklymak · 2018-01-14T18:57:45Z

Restarted the two failing tests. Looked like something flaky, versus an actual failure of this PR.

Zac-HD · 2018-01-16T02:27:33Z

@efiring, I've done all I can to get the test passing - there are two remaining problems:

lib/matplotlib/tests/test_rcparams.py::test_validator_invalid is failing with a null byte issue somewhere, which is nothing to do with this pull. That's the Python 3.6 job on Travis.
The Travis jobs for python 3.4 and 3.7 are still using an older (incompatible) version of pytest. If you clear the cache are restart those jobs I would expect all the tests to pass.

efiring · 2018-01-16T02:32:13Z

I confess: I know how to restart the tests, but I don't know how to clear a cache on Travis. @tacaswell knows all, and needs to approve the changes (or not) in any case, so I will leave it to him.

Zac-HD · 2018-01-16T02:35:59Z

FYI it's under the "More options" button on the upper right, then "caches" and pick the relevant pull request - or just blow away everything with "delete all repository caches".

Usually this is all automatic, but things just haven't been expiring as they should this year 🤷‍♂️

QuLogic · 2018-01-16T02:43:59Z

TBH, I'm not sure about bumping the pytest requirement. I'm not seeing 3.3 packaged in a lot of distros just yet.

Zac-HD · 2018-01-16T03:05:27Z

TBH, I'm not sure about bumping the pytest requirement. I'm not seeing 3.3 packaged in a lot of distros just yet.

Given the trouble with Travis, I'm happy to ditch the check for logging - now we just check that the array was in fact clipped to the valid range, but not that a warning was logged.

tacaswell · 2018-01-16T22:24:37Z

I am also concerned about the pytest version bump.

If the cache / version issues persist try rebasing on current master.

Zac-HD · 2018-01-17T22:17:13Z

@tacaswell - I'm no longer bumping the pytest version.

@efiring - tests all passing now 🎉

efiring · 2018-01-18T01:59:18Z

I'm happy, but @tacaswell still has an angry red X next to his review...

Zac-HD · 2018-01-21T02:58:28Z

Ping @tacaswell - if you're happy with this it can be merged; and if not I'd appreciate tips on how to improve it 😄

jklymak

Still needs API change added.
Also, I don' think you need to manually edit credits.rst

When `Axes.imshow` is passed an RGB or RGBA value with out-of-range values, it now logs a warning and clips them to the valid range. The old behaviour, wrapping back in to the range, often hid outliers and made interpreting RGB images unreliable.

Zac-HD · 2018-01-26T05:42:47Z

Ah, sorry - I'd missed that comment. Now with API changes documented; and since it's been a while I also bumped the dates and rebased on master.

The history of credits.rst suggests that the list was generated from Git metadata (by @efiring), but that it's not automatically updated. Unless someone knows how else it will be added, I'll therefore leave my name on the list 😄

(and ping @tacaswell again; I think we're just waiting for your approval now)

Zac-HD · 2018-01-29T08:12:04Z

Ping @jklymak and @tacaswell; I've made all the changes you wanted and would love to get this merged in time for v2.2 - the due date is tomorrow!

tacaswell · 2018-02-04T16:26:44Z

Thanks! Sorry I have been slow on review recently.

Zac-HD mentioned this pull request Jan 10, 2018

Support RGB[A] arrays in plot.imshow() pydata/xarray#1796

Merged

4 tasks

Zac-HD changed the title ~~Imshow rgb fixes~~ imshow fixes for RGB data: support normalisation, clip to valid range Jan 10, 2018

Zac-HD force-pushed the imshow-rgb-fixes branch from bdb5be1 to ab6e305 Compare January 10, 2018 14:13

tacaswell added this to the v2.2 milestone Jan 10, 2018

tacaswell requested changes Jan 10, 2018

View reviewed changes

anntzer mentioned this pull request Jan 10, 2018

imshow doesn't normalize the color range in RGB images #9391

Closed

Zac-HD force-pushed the imshow-rgb-fixes branch from ab6e305 to 1290594 Compare January 11, 2018 00:33

jklymak reviewed Jan 11, 2018

View reviewed changes

Zac-HD mentioned this pull request Jan 11, 2018

Normalisation for RGB imshow pydata/xarray#1819

Merged

WeatherGod reviewed Jan 11, 2018

View reviewed changes

jklymak reviewed Jan 12, 2018

View reviewed changes

Zac-HD force-pushed the imshow-rgb-fixes branch from 5059e7e to 766ce74 Compare January 13, 2018 03:21

efiring approved these changes Jan 14, 2018

View reviewed changes

Zac-HD force-pushed the imshow-rgb-fixes branch from 766ce74 to a4764c5 Compare January 15, 2018 12:51

Zac-HD force-pushed the imshow-rgb-fixes branch 2 times, most recently from 2236a2c to 068fa28 Compare January 16, 2018 08:17

jklymak requested changes Jan 25, 2018

View reviewed changes

Zac-HD changed the title ~~imshow fixes for RGB data: support normalisation, clip to valid range~~ Clip RGB data to valid range for imshow Jan 26, 2018

Zac-HD force-pushed the imshow-rgb-fixes branch from e05a632 to 7f97698 Compare January 26, 2018 05:40

jklymak approved these changes Jan 29, 2018

View reviewed changes

jklymak mentioned this pull request Feb 2, 2018

Floating point image RGB values must be in the 0..1 range #10372

Closed

jklymak added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Feb 2, 2018

Zac-HD mentioned this pull request Feb 4, 2018

Should imshow() recognise 0-255 images? pydata/xarray#1880

Closed

tacaswell merged commit 605fd3c into matplotlib:master Feb 4, 2018

QuLogic modified the milestones: needs sorting, v2.2.0 Feb 12, 2018

jklymak mentioned this pull request Mar 27, 2018

Bug causes to_rgba to fail inside cm.py #10890

Closed

jklymak mentioned this pull request Apr 9, 2018

Fix logic error in ScalarMappable.to_rgba #11002

Merged

Zac-HD deleted the imshow-rgb-fixes branch May 26, 2018 14:02

Clip RGB data to valid range for imshow #10220

Clip RGB data to valid range for imshow #10220

Conversation

Zac-HD commented Jan 10, 2018 • edited Loading

tacaswell commented Jan 10, 2018

tacaswell left a comment

Choose a reason for hiding this comment

jklymak commented Jan 10, 2018

anntzer commented Jan 10, 2018

dstansby commented Jan 10, 2018

jklymak commented Jan 10, 2018 • edited Loading

efiring commented Jan 10, 2018

jklymak commented Jan 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zac-HD Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

Zac-HD commented Jan 11, 2018 • edited Loading

Zac-HD commented Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jklymak Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zac-HD commented Jan 12, 2018

anntzer commented Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zac-HD commented Jan 13, 2018

efiring left a comment

Choose a reason for hiding this comment

jklymak commented Jan 14, 2018

Zac-HD commented Jan 16, 2018

efiring commented Jan 16, 2018

Zac-HD commented Jan 16, 2018

QuLogic commented Jan 16, 2018

Zac-HD commented Jan 16, 2018 • edited Loading

tacaswell commented Jan 16, 2018

Zac-HD commented Jan 17, 2018

efiring commented Jan 18, 2018

Zac-HD commented Jan 21, 2018

jklymak left a comment

Choose a reason for hiding this comment

Zac-HD commented Jan 26, 2018

Zac-HD commented Jan 29, 2018

tacaswell commented Feb 4, 2018

Zac-HD commented Jan 10, 2018 •

edited

Loading

jklymak commented Jan 10, 2018 •

edited

Loading

Zac-HD Jan 11, 2018 •

edited

Loading

Zac-HD commented Jan 11, 2018 •

edited

Loading

Zac-HD commented Jan 11, 2018 •

edited

Loading

jklymak Jan 16, 2018 •

edited

Loading

anntzer commented Jan 12, 2018 •

edited

Loading

Zac-HD commented Jan 16, 2018 •

edited

Loading