Skip to content

FIX: make add_lines work with new colorbar #12461

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
Merged
merged 3 commits into from
Oct 14, 2018
Merged

FIX: make add_lines work with new colorbar #12461

merged 3 commits into from
Oct 14, 2018

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Oct 9, 2018 •
edited
Loading

PR Summary

Closes #12458

See below for a less pathological example...

cbar.add_lines was not operational after new colorbar changes in 3.0.0.

import matplotlib.pyplot as plt
import matplotlib as mpl
import numpy as np

fig = plt.figure()
ax = fig.add_axes([0.05, 0.8, 0.9, 0.15])
cmap = mpl.cm.cool
norm = mpl.colors.Normalize(vmin=-4, vmax=4)
levels = (-1.0, 1.0, 2.0, 3.0)

cb = mpl.colorbar.ColorbarBase(ax, cmap=cmap, norm=norm)
colors_bg = np.tile(list((1.0, 1.0, 1.0, 1.0)), (len(levels), 1))
cb.add_lines(levels=levels, colors=colors_bg, linewidths=1)

Before

cbarold

After

cbarnew

import numpy as np
import matplotlib.pyplot as plt

fig, ax = plt.subplots()
X = np.random.rand(10, 10)*10000
pcm = ax.pcolormesh(X)
# add 1000 to make colors visible...
cont = ax.contour(X+1000)
cb = fig.colorbar(pcm)
cb.add_lines(cont)
plt.show()

Before

before

After

new

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 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 (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@jklymak jklymak changed the title FIX: make add_lines wortk with new colorbar FIX: make add_lines work with new colorbar Oct 9, 2018
@jklymak jklymak added this to the v3.0.x milestone Oct 9, 2018
@jklymak jklymak mentioned this pull request Oct 9, 2018
Closed
@dstansby
Copy link
Member

dstansby commented Oct 9, 2018

Huh, and here I am manually adding lines to the cbar axes with axhline! You learn a new bit of the API every day...

@jklymak
Copy link
Member Author

jklymak commented Oct 9, 2018

Well, except the subclass Colorbar actually has a very different add_lines, so you need to call the cb.super().add_lines() to get this API. Hence, I doubt its tested, and this PR needs tests.

We could extend the API of cb.add_lines to check if the first argument is a list of levels instead of a ContourSet, but that entails a bunch of extra kwargs to pass down.

You must have been bit by the 3.0 change, i.e. the colorbar no longer has native co-ordinates from 0 to 1, but rather from vmin to vmax....

@jklymak jklymak mentioned this pull request Oct 9, 2018
Closed
@jklymak
Copy link
Member Author

jklymak commented Oct 9, 2018

Grrr, test failures are real, with the lines slightly extended a bit further into the lhs of the axes. I don't think either version is "wrong", and the PDFs are the same, so I'll upload the new images for this one.

@jklymak
Copy link
Member Author

jklymak commented Oct 9, 2018

Added new test image to test that the lines are added properly to a colorbar.

@jklymak jklymak added Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. and removed status: work in progress labels Oct 9, 2018
@anntzer
Copy link
Contributor

anntzer commented Oct 9, 2018

Based on @dstansby's comment, I guess (especially after #12380 gets merged :p) we could just rewrite the thing to use vlines()/hlines() instead?

@jklymak
Copy link
Member Author

jklymak commented Oct 9, 2018

@anntzer are you proposing we do that here? I was just going for the minimal change to fix the code. If we want to make a 3.1 upgrade, I'd be all for that. Would appending the collection to self.lines be the same as appending all the hlines? If not, it'd also be an API change...

@anntzer
Copy link
Contributor

anntzer commented Oct 9, 2018 •
edited
Loading

I think this can go in as is for now (well I didn't review it carefully, but looks fine).
After #12380 hlines() will create a LineCollection just like add_lines currently does so no API break then.

@jklymak
Copy link
Member Author

jklymak commented Oct 9, 2018

?? #12380 is for stem. OTOH, I see thet ax.hlines indeed returns a line collection, so maybe just as well to use that? But for now, I still think we should do that as a new PR for 3.1...

@QuLogic
Copy link
Member

QuLogic commented Oct 9, 2018

I don't think either version is "wrong", and the PDFs are the same, so I'll upload the new images for this one.

I think the old one is wrong, actually. The LHS shows some mixing of the line and the colorbar below, whereas the new one goes right to the edge line.

@jklymak
Copy link
Member Author

jklymak commented Oct 9, 2018

I don't think either version is "wrong", and the PDFs are the same, so I'll upload the new images for this one.

I think the old one is wrong, actually. The LHS shows some mixing of the line and the colorbar below, whereas the new one goes right to the edge line.

Yeah, I don't quite understand from the code why that is the case, however... Before the xaxis went from [0, 1], now it goes from [vmin, vmax], and this PR just redraws across those limits for both cases. Maybe something funky about axes that go from [0, 1]?

@anntzer
Copy link
Contributor

anntzer commented Oct 9, 2018

Oh sorry yes I got PRs mixed up. I guess yes you may as well use hlines/vlines here then...

@dstansby dstansby merged commit fbd0fba into matplotlib:master Oct 14, 2018
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Oct 14, 2018
@dstansby
Backport PR matplotlib#12461: FIX: make add_lines work with new colorbar
3dde05b
@meeseeksmachine meeseeksmachine mentioned this pull request Oct 14, 2018
Merged
QuLogic added a commit that referenced this pull request Oct 14, 2018
@QuLogic
Merge pull request #12527 from meeseeksmachine/auto-backport-of-pr-12…
b8e2e1b 
…461-on-v3.0.x

Backport PR #12461 on branch v3.0.x (FIX: make add_lines work with new colorbar)
@jklymak jklymak deleted the fix-cbar-addlines branch October 22, 2018 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timhoffm timhoffm timhoffm approved these changes

@dstansby dstansby dstansby approved these changes

Assignees
No one assigned
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Milestone
v3.0.1
Development

Successfully merging this pull request may close these issues.

add_lines misses lines for matplotlib.colorbar.ColorbarBase
5 participants
@jklymak @dstansby @anntzer @QuLogic @timhoffm