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

Blank colorbars for high dynamic range LogNorm imshow plots #10051

Closed
ngoldbaum opened this issue Dec 20, 2017 · 17 comments
Closed

Blank colorbars for high dynamic range LogNorm imshow plots #10051

ngoldbaum opened this issue Dec 20, 2017 · 17 comments

Comments

@ngoldbaum
Copy link
Contributor

Bug summary

When creating a LogNorm imshow plot with more than 9 orders of magnitude of dynamic range the automatically generated colorbar from plt.colorbar() has no automatically generated tick marks or labels.

Code for reproduction

This generates a plot with colorbar tick labels

import numpy as np

from matplotlib import pyplot as plt
from matplotlib.colors import LogNorm

data = np.zeros((800, 800)) + 1
data[50, 51] = 1e9

print(np.log10(data.max()/data.min()))

image = plt.imshow(data, origin='lower', interpolation='nearest', norm=LogNorm())
plt.colorbar(image)

plt.savefig('labels.png')

labels

While this script generates a plot with no colorbar tick labels:

import numpy as np

from matplotlib import pyplot as plt
from matplotlib.colors import LogNorm

data = np.zeros((800, 800)) + 1
data[50, 51] = 2e9

print(np.log10(data.max()/data.min()))

image = plt.imshow(data, origin='lower', interpolation='nearest', norm=LogNorm())
plt.colorbar(image)

plt.savefig('no-labels.png')

no-labels

Expected outcome

I would expect the second plot to also have colorbar tick labels.

Matplotlib version

  • Operating system: Mac OS High Sierra
  • Matplotlib version: 2.1.1 (installed via pip)
  • Matplotlib backend (print(matplotlib.get_backend())): MacOSX
  • Python version: 3.6
@jklymak
Copy link
Member

jklymak commented Dec 20, 2017

#9903 fixes this:

no-labels

@ngoldbaum
Copy link
Contributor Author

ngoldbaum commented Dec 20, 2017

@jklymak what about the minor ticks? does #9903 change that as well? If so we'll need to fix that in yt for the next matplotlib release after #9903 gets merged. This behavior also changed with matplotlib 2.0 - we have some matplotlib version-specific hacks in yt already for this:

https://github.com/yt-project/yt/blob/b83691665f6574d7bf1b01c7bd69088c32e1d6a9/yt/visualization/plot_window.py#L925

It would be nice if you could make it so that the minorticks get drawn just like the current verison.

@efiring
Copy link
Member

efiring commented Dec 20, 2017

But what we really want is for minor ticks to be drawn on the colorbar as they would be on a normal axis, that is, they should be shorter than the major ticks.

@jklymak
Copy link
Member

jklymak commented Dec 20, 2017

Like this (from #9903)?

Edit: Just to be clear this is with 1e9 instead of 2e9

no-labels

@jklymak
Copy link
Member

jklymak commented Dec 20, 2017

@ngoldbaum You don't really want minor ticks when you get more than two or more decades between major ticks. They will look very funny. Thats why you were getting a blank before.

We could probably tweak the auto tick location to let there be more major ticks, but...

@ngoldbaum
Copy link
Contributor Author

If the minor ticks get drawn in when there's less dynamic range I'm fine with that. It does seem like a nice cleanup of colorbar handling - if that can get more sane I'm ok with there being some changes in behavior.

In general yt should just do whatever matplotlib does here and not worry too much about whether there are minor in where the tick labels are differences on old versions.

@jklymak
Copy link
Member

jklymak commented Dec 20, 2017

OK, great. I looked at the LogLocator code. It definitely drops the minor ticks if there is more than a decade between major ticks. I think this is perfectly reasonable. I also think its relatively straightforward for you to hard-code the minor ticks if you think you really want them. Or write a custom version of LogLocator if you want some automation. Happy to help w/ that if you guys decide you need that. (you basically just remove a check for the number of strides to be 1)

@ngoldbaum
Copy link
Contributor Author

Nah, i don't think it makes any sense to draw log minor ticks when there's more than one order of magnitude between major ticks.

#9903 looks great! It's bugged me in the past that there are all these subtle differences between the norm locators so making things more uniform is very helpful for people building plotting code on top of matplotlib. It'll also be nice that the number of ticks scales naturally with the size of the colorbar.

@jklymak
Copy link
Member

jklymak commented Dec 20, 2017

#9903 seems to work well, but it does some violence to the way that colorbars are made. (long story is that instead of the pcolor object's yaxis going from 0-1, it goes from vmin to vmax, just like any other axis. But it only does this for the default Norm and LogNorm) So it may not make it into Master. If not, then we should revisit this case. I'm sure we can make it work OK w/ the existing code.

@ngoldbaum
Copy link
Contributor Author

@jklymak was there a fix for this in the current colorbar implementation for 2.2?

@jklymak
Copy link
Member

jklymak commented Feb 20, 2018

No #9903 is not merged yet. It needs a bit of a rebase, but is reviewable as is.

@ngoldbaum
Copy link
Contributor Author

Sorry, i mean a fix for this issue separate from #9903 (as you suggested in your comment from December), since it looks like #9903 won't land in 2.2.

@ngoldbaum
Copy link
Contributor Author

No worries if the answer is that it would be a pain to fix or no one wants to, I've only seen a few reports about this behavior from yt users and I don't think it's a common issue.

@jklymak
Copy link
Member

jklymak commented Feb 20, 2018

I’ve not looked at this case in isolation. I imagine a PR would be welcome, but I’d argue #9903 obviates the need, and would hence be a bit of a waste of time. But nothing got in for 2.2 so if someone could do something quick for 2.2 that might be useful.

@ngoldbaum
Copy link
Contributor Author

@jklymak now that #9903 is merged I guess this is fixed? I haven't tested with 3.0rc1 although I guess I could tomorrow when I test yt with the rc.

@ngoldbaum
Copy link
Contributor Author

This is indeed fixed, I'm closing.

@jklymak
Copy link
Member

jklymak commented Aug 13, 2018

Phew! I was sweating for a while.... 😉 Thanks so much for testing the RC!

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

No branches or pull requests

3 participants