Skip to content

Make singular colorbars consistent with single-value mappables. #26307

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jul 14, 2023

If vmin == vmax, Norm maps values to 0 (not to 0.5). Make colorbars match that behavior by having them only expand singular norms the the high side.

Closes #15566 -- not in the way I had argued in #15566 (comment), but I guess the change here is the least disruptive.

PR summary

PR checklist

vmin, vmax = sorted([self.norm.vmin, self.norm.vmax])
self.norm.vmin = vmin
_, self.norm.vmax = mtransforms.nonsingular(vmin, vmax, expander=0.1)
Copy link
Contributor

Choose a reason for hiding this comment

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

This forces self.norm.vmin <= self.norm.vmax now I think. Would a user care if they had defined their norm reversed and we are now swapping this for them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous version (just calling nonsingular) already performed that ordering; I'm not changing that behavior (which I agree we can question), just doing it explicitly first to make it easier to just expand on the top side.

@anntzer anntzer added this to the v3.8.0 milestone Aug 11, 2023
@anntzer
Copy link
Contributor Author

anntzer commented Aug 11, 2023

Not a very big deal, but this does fix a bug relatively easily so I'll nudge for a review to get this included in 3.8.

@timhoffm
Copy link
Member

Please hold up merging. I have some reservations. Will explain soon.

@timhoffm
Copy link
Member

timhoffm commented Aug 11, 2023

In the general case, no point in the colormap is special; and thus it's an arbitrary choice to which color you render singular values.

However, for divergent colormaps, the center is special, and typically associated with 0, and both sides define positive/negative deviations. I assume that no-deviations (= all zero values) is more likely than an all constant deviation - at least from my experience. The nice feature of the current collapsing to the center is that there is a continuous transition from small deviations (mapped to approximately 0.5) to zero deviations (mapped to 0.5). That continuous transition would break if we collapse the zero-deviations case to an edge.

It's not that this is an absolute argument against the proposed change, but the current behavior was a nice feature.

@anntzer
Copy link
Contributor Author

anntzer commented Aug 11, 2023

Sure, your point is well taken; the issue is that right now, this "map to the centerpoint" only happens if you add a colorbar to the image:

from pylab import *
fig, axs = subplots(1, 2)
axs[0].imshow([[0]], cmap="bwr")
im = axs[1].imshow([[0]], cmap="bwr"); fig.colorbar(im)
show()

test

The proposed fix here is to never bother mapping to the centerpoint, but the alternative is the fix I had sketched at #15566 (comment) (always map to the centerpoint, or more specifically rely on the underlying scale's autoexpander). The alternative is just a bit less backcompatible (IMO), but if there's consensus that we prefer it, I can certainly close this PR.

@greglucas
Copy link
Contributor

I think this is a useful consistency PR and a follow-up auto-expanding version of this pushed up to the Normalize class, which I don't think would conflict with one another? This PR is all happening within the colorbar code and is only expanding the norm there to be consistent with imshow() behavior. I think we should consider deprecating singular norms altogether and auto-expand with the associated scale expander (which would happen before this call to colorbar) as you mention in that comment. We have changed colorbar to be more axis-like with the ticking and we auto-expand the axis for users with a warning saying we are doing that if it happens. I think it would be useful from a consistency point of view to do something similar with norms as well.

Is there a great usecase for vmin == vmax? I can't think of one off the top of my head, I think it would generally just result from the norm.autoscale() being called with an array of all the same values.

@anntzer
Copy link
Contributor Author

anntzer commented Aug 12, 2023

Deprecating singular norms seems OK too, as long as we don't warn on imshow(single-value-array) (i.e. autoscale(single-value-array) should just silently autoexpand): not warning is consistent with plot(single-value) also silently autoexpanding (which I think is more similar to imshow(single-value-array) -- we do warn on e.g. xlim(1, 1) leading to autoexpansion but I think that's a different case).

@ksunden ksunden modified the milestones: v3.8.0, v3.9.0 Aug 23, 2023
@tacaswell
Copy link
Member

Talked about this on the call this week.

This seems like the least-bad way to fix the problem that auto-range (and hence color) depends on if you add a colorbar.

We may want to also adjust the auto limit behavior to target the center instead of the bottom, but if we do that we are going to have to pay the API breakage cost anyway so it is worth making these consistent now.

@QuLogic
Copy link
Member

QuLogic commented Mar 15, 2024

I think this just needs a rebase to fix the CI.

If vmin == vmax, Norm maps values to 0 (not to 0.5).  Make colorbars
match that behavior by having them only expand singular norms the the
high side.
@anntzer
Copy link
Contributor Author

anntzer commented Mar 15, 2024

rebased.

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

I'm not convinced and would like to have more feedback. - Sorry I missed the dev call discussion.

Yes, we have an inconsistency, but is it really that bad? What does a mappable without a colorbar even mean? The colors are not defined, so that practical use of that case should be very limited. But if we strive for consistency, would it not be better to make the adjustment to the "ambiguous" case of no colorbar. Rather than breaking the valid use case with colorbar?

A standard case for all-same values are image comparisons. For inputs A, B, you plot B - A and if that's all 0 there is no difference. It'd be quite awkward if the color of these changes.

@tacaswell
Copy link
Member

Looking at mappables with out a colorbar is still useful from a qualitative point of view or in cases where the values are in arbitrary units.

I take the point that this change is going to break people who were accidentally getting the better behavior by adding a colorbar, I don't think that came up in our discussion. Lets put this on the agenda for next week.

@timhoffm
Copy link
Member

timhoffm commented Mar 15, 2024

Looking at mappables with out a colorbar is still useful from a qualitative point of view or in cases where the values are in arbitrary units.

Agreed. My particular point on that is: If the values are in arbitrary units (and all are the same), we can choose an arbitrary color. So a change here would be semantically less breaking.

@tacaswell
Copy link
Member

I think the expectation for what to do with singular data here also depends on the sort of data you are looking at. If you are looking at ~ symmetric data (like image differences) and are using a diverging color map then the singular case expanding symmetrically around zero (as is the case in the example in #26307 (comment)) feels more natural (and is currently what you get if you add a colorbar). On the other hand, if you are primarily looking at sparse / low count images with values always >=0 than the "expand up" solution to the singular (likely all-zeros) case (which is what happens without a colorbar) feels more natural. I'm not sure off the top of my head when "expand down" would be right, but I'm sure that there is someone who has a use case for that too.

The options are:

  1. do nothing and just live with this weird "add a colorbar and change the image" behavior
  2. make adding a colorbar not change the visual appearance of a mappable when added (this PR)
  3. make the autolimit in mapabble always expand (maybe with a parameter to say {up, down, both})

My judgement is coming down to a guess of how many people will see this change and how much work it is to make them consistent. I think that this PR will hit the least number of people and is the simplest set of changes. I'm open to being wrong about that judgement and that even if it is fewer people it is a worse change so we should prefer 1 or 3.

@QuLogic QuLogic modified the milestones: v3.9.0, v3.10.0 Apr 2, 2024
@tacaswell
Copy link
Member

Talked about this on the call.

  • inconsistency is not bad, no rush on this
  • can we make colorbars singular?
  • should we have colorbar explode instead? [NO]
  • keep in mind balance between amount of code and perfection

the right way forward is not trivial, all need to think about this a bit more.

@ebarcelosf
Copy link

hi, can i solve this issue?

@timhoffm
Copy link
Member

@ebarcelosf This is first and foremost a difficult design decision. See the discussion above. Do you have anything to add?

@timhoffm
Copy link
Member

timhoffm commented Feb 10, 2025

I thought about this some more, inspired by #29522 (comment).

Fundamentally there are two possible approaches:

  • Do not allow singular norms, i.e. vmin!=vmax. This suggests (i) raise if vmin, vmax are explicitly given and equal and (ii) auto-expand when auto-scaling. IMHO we need the latter, otherwise we would raise on constant-data images, but that seems excessive. We should be able to visualize constant-data images somehow.

    The auto-expansion is somewhat arbitrary and creates a lot of "noise" on the colorbar. The colorbar scale and all the ticks except the center one are irrelevant.
    image

  • Fully embrace singular data, i.e. allow singular norms, and create a special visualization for the colorbar of singular mappings. Some possible colorbar visualizations:

    • only create a single tick that corresponds to the singular value
    • only create a single tick that corresponds to the singular value, and only color a small band corresponding to that color
    • create two identical ticks with the same label at the extrema of the colorbar
      only create a single tick that corresponds to the singular value

    grafik

    This is a bit of extra work on the colorbar, but gets rid of the obscure singular value expansion. IMHO the special case of singular values deserves a special and identifyable viszualization.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 11, 2025

re: arbitrary auto-expansion of singular norms: In my view, which is essentially that norms are essentially scales-on-colorbars (hence make_norm_from_scale) this is not more (or less) arbitrary than plot(0, 0) autoexpanding the x and y axes around 0; in fact ideally this autoexpansion should reuse Scale.limit_range_for_scale.
However, I do agree that adding some special-casing to draw e.g. only a single tick at the midpoint of the colorbar may be nice (in fact, a bit like we could choose to draw a single tick at zero when doing plot(0, 0)... although we still need to define a real autoexpansion amount e.g. for transData or for mouseover coordinates)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent plot with and without colorbar
7 participants