Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Add a helper to copy a colormap and set its extreme colors.#14645

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
timhoffm merged 1 commit intomatplotlib:masterfromanntzer:with_extremes
Oct 22, 2020

Conversation

anntzer
Copy link
Contributor

See changelog. See also the number of explicit copies in the examples,
which suggest that the old setter-based API is a bit of a footgun (as
forgetting to copy seems easy).

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code isFlake 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

@phobson
Copy link
Member

I like this addition and would find it very useful.

I wonder ifget_cmap should also return a copy by default. I can imagine that would sufficiently API-breaking that we need to think hard about it. But aget_cmap function that accepts these params as well would be quite nice.

@ImportanceOfBeingErnest
Copy link
Member

Alternatively one could argue thatget_cmap() gets you an existing colormap, and e.g. something likenew_colormap() gets you a copy of an existing one. This would allow the notably useful API to be incorporated without backwards compatibility issues,

 plt.cm.new_colormap("viridis", bad="white")
phobson reacted with thumbs up emoji

@anntzer
Copy link
ContributorAuthor

Initially I thought that changingget_cmap to return a copy would be too disruptive, but let's look at the various behavriors ofget_cmap:

  • If it is called with a second argumentN, it's already returning a new (resampled) colormap anyways; so we don't even need to bother with a copy.
  • If it is called with a string argument (and noN), it is returning a reference to one of the builtin colormaps. I would argue that these are intended to be immutable and that modifying them should be considered a bug. (Although I would readily admit that one can get away with modifying them in single use scripts.)
  • If it is called with a colormap argument, it returns that colormap (without ever resampling it). If the call-site controls the argument, one may wonder why they additionally pass it throughget_cmap; if the argument comes from "outside", then it should likely be considered immutable as well.

Thus, I think modifyingget_cmap to return a copy is more likely to fix accidental modifications of builtin or custom cmaps rather than be disruptive.

On the other hand, I would say thatget_cmap(...).with_extremes(bad=...) (ornew_cmap(...).with_extremes(...) -- throughout this paragraph, read whatever spelling you prefer) is not that worse thanget_cmap(..., bad=...), and thatwith_extremes has the advantage of also working with non-builtin cmaps (and having the functionality both inget_cmap and inwith_extremes means having to duplicate the code and the docs, yada yada). Finally, having the parameters inget_cmap also modify a non-builtin-colormap passed as argument would be inconsistent with theN parameter ofget_cmap, which, for the better or (mostly?) the worst, does not resample non-builtin cmaps.

@timhoffm
Copy link
Member

As you've pointed outget_cmap has quite some odd behavior. I think it should just return a registered colormap and that should be immutable. No passing through ofColormap inputs, no resampling etc. everything else should be done by creating new colormaps from existing colormap objects.

SinceColormaps are currently not immutable,get_cmap should return copies of the internal colormaps. It then makes sense to haveColormap.resampled() andColormap.with_extremes(). Maybe even better to have just one functionColormap.copy(N=None, bad=None, under=None, over=None) - naming is open for better suggestions.

Side remark: I'm also not sure if the segmentationN should be a is a fundamental aspect of all colormaps. E.g. one could imagine colormapping by functions. OTOH maybe there is just no need for this and you can always approximate the function onN segments.

@tacaswelltacaswell added this to thev3.3.0 milestoneAug 24, 2019
@QuLogicQuLogic modified the milestones:v3.3.0,v3.4.0May 2, 2020
See changelog.  See also the number of explicit copies in the examples,which suggest that the old setter-based API is a bit of a footgun (asforgetting to copy seems easy).
@tacaswell
Copy link
Member

I really like this pattern.

@jklymak
Copy link
Member

This does seem useful. Wasn't sure how it fit in w/@timhoffm plans for the colormap registry...

@timhoffm
Copy link
Member

This does not interfere with my plans for the colormap registry (#18503). The registry only conerns registered colormaps. You can still have separate colormap objects and create new ones. In fact,with_extremes() would still be the canonical way to use custom extremes on a registered colormap, e.g. (API not finalized):

my_cmap = mpl.colormaps['jet'].with_extremes(...)

@@ -0,0 +1,13 @@
`.Colormap.set_extremes` and `.Colormap.with_extremes`
Copy link
Member

Choose a reason for hiding this comment

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

I think we've decided not to put links in section titles for aestethic reasons.

But I'm going to merge as is. I'll anyway do a small colormap-related cleanup PR and will update there. While that's a bit of a messy way of working it saves us an extra review roundtrip here.

@timhoffmtimhoffm merged commite89114e intomatplotlib:masterOct 22, 2020
@timhoffmtimhoffm mentioned this pull requestOct 22, 2020
@anntzeranntzer deleted the with_extremes branchOctober 22, 2020 22:00
@timhoffmtimhoffm mentioned this pull requestJun 28, 2024
3 tasks
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@timhoffmtimhoffmtimhoffm approved these changes

@tacaswelltacaswelltacaswell approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.4.0
Development

Successfully merging this pull request may close these issues.

7 participants
@anntzer@phobson@ImportanceOfBeingErnest@timhoffm@tacaswell@jklymak@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp