8000 Deprecate adding a (meaningless) colorbar to RGB or RGBA images · Issue #20576 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Deprecate adding a (meaningless) colorbar to RGB or RGBA images #20576

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.

Sign up for GitHub

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

Jump to bottom
Open
Zac-HD opened this issue Jul 5, 2021 · 16 comments
Open

Deprecate adding a (meaningless) colorbar to RGB or RGBA images #20576

Zac-HD opened this issue Jul 5, 2021 · 16 comments
Labels
Maintenance

Comments

@Zac-HD
Copy link
Contributor
Zac-HD commented Jul 5, 2021

Describe the issue

In #9391, I proposed to open a new issue for "colorbar should refuse to let itself be created for an RGB(A) image" because colorbars are not meaningful for images with existing color. #4369 (comment) similarly notes that this makes no sense.

(better late than never I guess!)

Proposed fix

I think that attempting to add a colorbar for an RGB or RGBA image should be an explicit error, following a period of deprecation warnings.
@jklymak
Copy link
Member
jklymak commented Jul 5, 2021

A colorbar is just an artist. I'm not aware of us being prescriptive about the use of any artist just because it is not sensible.

I suppose an argument can be made that a colormapable that does not use a norm should make an empty colorbar; we don't currently track on the mappable if it ever did a mapping or not.

@timhoffm
8000 Copy link
Member
timhoffm commented Jul 5, 2021

One could consider it a design flaw that AxesImage inherits from ScalarMappable, but that ship has sailed.

@jklymak
Copy link
Member
jklymak commented Jul 5, 2021

Well, a colorbar means something for imshow in general, just not when imshow is called with with RGBA...

@jkfindeisen
Copy link

For example, the colorbar could re-create exactly the colormap of the used image. It might still be desired to show one, for example to resemble the layout of other plots.

@timhoffm
Copy link
Member
timhoffm commented Jul 6, 2021 •
edited
Loading

For example, the colorbar could re-create exactly the colormap of the used image.

For a RGB image there is no reasonable way to map the values to a linear range (and there's also no scale). The only reasonable colorbar is an empty colorbar.

@jklymak
Copy link
Member
jklymak commented Jul 6, 2021

I don't think any of this is hard to do (check mappable is AxesImage, and don't fill the colorbar if get_array is 3- or 4-D, or emit a warning/error). But I'm still not sure why this is a guard rail we need to put up, or why putting the guardrail up would be less confusing than the status quo.

@timhoffm
Copy link
Member
timhoffm commented Jul 6, 2021 •
edited
Loading

Taking the original example code of #9391 we get:
grafik

The RGB cases are clearly wrong: There is a colorbar implying a colormapping, but obviously the mapping is not related to the image. Even worse the scale seems to be influenced by the data. IMHO this falls under the case "errors should never pass silently".

There are two options how to handle this reasonably:

  1. error out if one tries to create a colorbar for a RGB AxesImage, or
  2. draw an empty colorbar and issue a warning

I tend towards 1.

@jklymak
Copy link
Member
jklymak commented Jul 6, 2021

OK, in that case I guess we should error if mappable.get_array().dim > 2 and just say that such a mappable cannot have a colorbar. Right now if we pass a Line2D we get

AttributeError: 'Line2D' object has no attribute 'get_array'

which isn't the greatest error message either.

@jklymak
Copy link
Member
jklymak commented Jul 6, 2021

Maybe:

       if mappable is None:
            mappable = cm.ScalarMappable(norm=norm, cmap=cmap)
        elif not hasattr(mappable, 'get_array'):
            raise ValueError('colorbar mappable must be a subclass of '
                             'ScalarMappable or have a get_array method.')
        elif mappable.get_array().ndim > 2:
            raise ValueError('Mappable array has more than '
                             'two dimensions, and hence cannot be mapped '
                             'using a colorbar; perhaps because you '
                             'passed RGB(A) data to the method that '
                             'made the mappable.')

@timhoffm
Copy link
Member
timhoffm commented Jul 6, 2021 •
edited
Loading

The Line2D case is slightly different. Due to duck typing we don't thoroughly check the input types. If it's wrong it blows up somehow. This is the general rule in Python. Passing Line2D is a clear violation of the API, which states that you have to pass a ScalarMappable, so blowing up arbitrarily is ok.

The RGB AxesImage is different in that it is a ScalarMappable and formally fulfills the API contract. It's only that it cannot be mapped to colors (which as written above is a design flaw). So we need to be more comforting to the user and give a nicer error.

So, I wouldn't add the explicit hasattr(mappable, 'get_array') for simplicity of the code. But I don't object if you feel strongly about it. Other than that, I think your code is good.

@jklymak
Copy link
Member
jklymak commented Jul 6, 2021

Then I guess the remaining problem is what if someone "needs" to add an empty colorbar. We don't currently have an easy way to do that without hoisting a fake ScalarMappable.

@timhoffm
Copy link
Member
timhoffm commented Jul 6, 2021

Then I guess the remaining problem is what if someone "needs" to add an empty colorbar. We don't currently have an easy way to do that without hoisting a fake ScalarMappable.

I question the "need". I claim that an empty colorbar is not what users < 8000 em>really want, and at least nobody has asked for it so far. What is the purpose of an empty colorbar? It's just visual noise. It would almost always be better to keep an empty space instead of the colorbar (that's in general also not simple, but another topic).

If empty colorbar is really a thing, let's wait until user demand comes up. We can then always add docs how to create one or even add helper code.

@jklymak
Copy link
Member
jklymak commented Jul 6, 2021

Anyone is free to take up the above patch. I don't really feel strongly enough to add this to the code base, but if others feel strongly about it, they should go ahead....

@jkfindeisen
Copy link
jkfindeisen commented Jul 7, 2021 •
edited
Loading

For a RGB image there is no reasonable way to map the values to a linear range (and there's also no scale). The only reasonable colorbar is an empty colorbar.

I can easily create an RGB image from a data array and a colormap, then store the RGB image, terminate the program, load the RGB image again, display it and display the same colormap and it would be a reasonable output. One could even try to reverse engineer a data array from an RGB image given a colormap but that's outside of the scope here. Here, I guess, it's more about what is reasonable and meaningful and I can only say that I had such cases and displayed RGB images with a colorbar (in Matlab) and it was a meaningful outcome to me because I knew what I was doing.

I guess the idea is to prevent people from doing things that likely aren't meaningful, but it also restricts the freedom of what you can do with the software. Maybe a warning ("this may not be meaningful") would be a better compromise.

But I guess (without inside knowledge) that if one really wants, one could probably create a colorbar object not attached to any image and simply position it where the colorbar would normally be, so not allowing colorbars for RGB images isn't really limiting anyone.

@timhoffm
Copy link
Member
timhoffm commented Jul 7, 2021

Yes, you can create arbitrary colorbars that are not related to an image. In that case, you have to state which value range, and how the mapping is done (via norm and colormap). And I think this is how it should be. So we are not limiting any use case (including supporting your example), but you have to be explicit what you want.

@jkfindeisen
Copy link

Then I think that it's fine.

timhoffm added a commit to timhoffm/matplotlib that referenced this issue Jul 11, 2021
@timhoffm
Deprecate mappables without data from linking to a colormap.
814b10f 
If a mappable does not provide data for mapping via get_array(), it
makes no sense to allow linking it to a colormap.

Closes matplotlib#20576
@timhoffm timhoffm mentioned this issue Jul 11, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Maintenance
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Deprecate linking mappables without data to a colormap timhoffm/matplotlib
4 participants
@jklymak @timhoffm @Zac-HD @jkfindeisen
0