FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Improve docs on contourf extend by timhoffm · Pull Request #13047 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Improve docs on contourf extend - #13047

Merged
tacaswell merged 1 commit into
matplotlib:masterfrom
timhoffm:doc-contourf-extend-cmap
Jan 14, 2019
ConversationCommits (1)ChecksFiles changed
Merged

Improve docs on contourf extend#13047
tacaswell merged 1 commit into
matplotlib:masterfrom
timhoffm:doc-contourf-extend-cmap

Conversation

timhoffm commented Dec 25, 2018

Copy link
Copy Markdown
Member

PR Summary

Based on #13038 I've rewritten the docstring for contourf extend to better explain how to use it.

timhoffm added this to the v3.0.3 milestone Dec 25, 2018

jklymak commented Dec 25, 2018

Copy link
Copy Markdown
Member

This seems fine, but isn’t the underlying issue that the contour is not being updated automatically?

timhoffm commented Dec 25, 2018

Copy link
Copy Markdown
Member Author

This seems fine, but isn’t the underlying issue that the contour is not being updated automatically?

I don‘t know if that‘s possible. Does the cmap know about the associated scalar mappable? Otherwise there‘s no way that it can notify the QuadContourSet that it needs to update.

tacaswell reviewed Dec 25, 2018
View reviewed changes
Comment thread lib/matplotlib/contour.py

cs = plt.contourf(h, levels=[10, 30, 50],
colors=['#808080', '#A0A0A0', '#C0C0C0'], extend='both')
cs.cmap.set_over('red')

tacaswell Dec 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It is probably better to show updating the color map before creating the contour and then passing that in to contourf. I think that is a bit for idiomatic and does not expose this updating edge-case in the examples.

tacaswell Dec 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ah, the point is we are creating the colormap on-the-fly so can not do that.

We have not noticed the lack of update on images because we unconditionally consult the colormap every time we draw.

timhoffm Dec 25, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Updating a QuadContourSet on each draw (to get automatic updates and the same behavior as with images) would essentially mean that we have to run the full code of changed(). Not quite sure if we want to do this.

I'm just being defensive here and clearly document the current behavior.

tacaswell Dec 29, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I agree this is the conservative path here.

jklymak commented Dec 29, 2018

Copy link
Copy Markdown
Member

I don‘t know if that‘s possible. Does the cmap know about the associated scalar mappable? Otherwise there‘s no way that it can notify the QuadContourSet that it needs to update.

I guess not. I wonder if what is actually needed is a cs.set_over, cs.set_under method if users really need to do this after the fact?

timhoffm commented Dec 29, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

I guess not. cs.set_under and similar seem counter-intuitive, when there‘s already a cs.cmap.set_under with a different behavior. It‘s not obvious that one can use one but not the other. That would be my least favorite solution if we‘re going to change the API.

Other possible API changes: It‘s actually not reasonable to define the over/under colors at another time compared to the regular colors. Therefore one could:

  • add additional kwargs for over/under to contourf, or
  • Make it reasonably simple to create a cmap and norm for a discrete set of colors and boundaries. These could then be supplied to contourf.

timhoffm force-pushed the doc-contourf-extend-cmap branch from 5d5eabd to 8ebdfb1 Compare January 6, 2019 19:14
timhoffm force-pushed the doc-contourf-extend-cmap branch from 8ebdfb1 to bb4b272 Compare January 6, 2019 22:55

timhoffm commented Jan 10, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

For now, I'd leave this as is and just clearly document the current behavior. This is what this PR is about. Everything else is beyond the scope of the PR (and not a battle I want to fight right now).

So, by me, the PR is ready to go in.

Note: Travis failure is due to #13137 and unrelated to this PR.

tacaswell merged commit 6b3c015 into matplotlib:master Jan 14, 2019
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Jan 14, 2019
Backport PR matplotlib#13047: Improve docs on contourf extend
0724082
meeseeksmachine mentioned this pull request Jan 14, 2019
Merged
timhoffm deleted the doc-contourf-extend-cmap branch January 15, 2019 20:24
dstansby added a commit that referenced this pull request Jan 20, 2019
Merge pull request #13176 from meeseeksmachine/auto-backport-of-pr-13…
5b18170 
…047-on-v3.0.x

Backport PR #13047 on branch v3.0.x (Improve docs on contourf extend)
rcomer mentioned this pull request Jun 3, 2023
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tacaswell tacaswell left review comments

Assignees

No one assigned

Labels

Documentation

Projects

None yet

Milestone

v3.0.3

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL