GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — 2D Normalization & Colormapping for ScalerMappables by patniharshit · Pull Request #8738 · matplotlib/matplotlib

Conversation

@patniharshit

@patniharshit patniharshit commented Jun 9, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

PR Summary

This PR is in referance to GSoC project '2D colormaps'.
(Project proposal is here)

  • The BivariateColormap class subclasses Colormap class. After generation it is flattened to 1d look up table.

  • The implementation of BivariateColormap is not perfect yet and just something has been generated to show proof of concept.

  • The bivariate data is identified by ndim == 3 and if an instance of either Bivariate Colormap or Norm has been passed in. This data is normalized and mapped to univariate integer index so as to directly index flattened BivariateColormap.

  • Plotting some air temperature and surface pressure data:
    bivariate

Feel free to review.

@story645 @tacaswell

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/whats_new.rst if major new feature
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
story645 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
story645
story645 reviewed Jun 9, 2017
View reviewed changes
Comment thread lib/matplotlib/colors.py Outdated
def inverse(self, value):
return value

class Norm2d:

@story645 story645 Jun 9, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

"BivariateNorm" just so that it can follow the mpl convention of "Xnorm"?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
patniharshit reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread lib/matplotlib/colors.py Outdated
"""
Normalize a list of two values corresponding to two 1D normalizers
"""
def __init__(self, norm_instances=None):

@story645 story645 Jun 9, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

is there are a reason norm_instances = none and is not:

def __init__(self, norm1 = None, norm2 = None):

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@patniharshit patniharshit Jun 9, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

No reason. I think what you are proposing is more convenient from user's perspective.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/matplotlib/colors.py Outdated
norm_instances :
A list of length two having instances of 1D normalizers
"""
if norm_instances is None:

@story645 story645 Jun 9, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

How do you handle the use case [BoundaryNorm(), None] or [None, BoundaryNorm]?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@patniharshit patniharshit Jun 9, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yes this should be handled.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@patniharshit patniharshit Jun 9, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is being handled now.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@anntzer

anntzer commented Jun 10, 2017

Copy link
Copy Markdown
Contributor

How would this relate to actual plotting functions?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@patniharshit

patniharshit commented Jun 10, 2017

Copy link
Copy Markdown
Contributor Author

@anntzer Currently, functions like imshow, pcolormesh, scalar mappable etc expects 1D normalizer. To extend their support for bivariate colormaps there is a need for bivariate norm also.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anntzer

anntzer commented Jun 10, 2017

Copy link
Copy Markdown
Contributor

I understand, but right now all I see this PR implements is some trivial way of combining to normalizers. If there's no bigger example usage, it's hard to judge whether that's sufficient, or too simple, or too complex.
For example, would it be better to just have a generic n-dimensional normalizer?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@story645

story645 commented Jun 11, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This should stay in its own PR for reviewing ease, but what about postponing the decision on whether to merge this until the whole set of 2d coloring PRs come in?

The issue with an N-color normalizer (I like it in theory/kinda agree on creating it and then sub classing to this) is that past 3 variables it's not really implementable.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anntzer

anntzer commented Jun 11, 2017

Copy link
Copy Markdown
Contributor

I agree with keeping the PR open for now.

Not sure why you cannot have more three dimensions, at the end we're just writing a mapping from R^n -> {colors}.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@story645

story645 commented Jun 11, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Yes, you can technically have N dimensions, but:
1 dim = colorbar
2 dim - color wheel/ color square
3 dim - color triangle
1 free dim - alph
More dims - open research problem
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anntzer

anntzer commented Jun 11, 2017

Copy link
Copy Markdown
Contributor

Well a color triangle really is only two dimensional (the third variable is determined by the first two).
Again, as this PR does not discuss how to plot the "colorbar" at all it's hard to judge...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anntzer
anntzer requested changes Jun 11, 2017
View reviewed changes

@anntzer anntzer left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Further PRs on nonscalar normalizers need to come in before we can judge whether this design makes sense.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@story645 story645 changed the title 2D normalizer [WIP] 2D normalizer Jun 11, 2017
@patniharshit
patniharshit force-pushed the norm2d branch 2 times, most recently from 1a3d16b to 8e93d20 Compare July 7, 2017 12:51
@patniharshit
patniharshit force-pushed the norm2d branch 2 times, most recently from 13627d3 to 2f8b5bf Compare July 23, 2017 17:22
anntzer
anntzer reviewed Jul 23, 2017
View reviewed changes
Comment thread lib/matplotlib/axes/_axes.py Outdated

if len(args) == 1:
C = np.asanyarray(args[0])
isBivari = (isinstance(norm, mcolors.BivariateNorm) or

@anntzer anntzer Jul 23, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

try to stick to pep8 (is_bivar)
also this can br written as isinstance(norm, (mcolors.BivariateNorm, mcolors.BivariateColormap))
(similarly below)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@patniharshit patniharshit Jul 27, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Updated.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@patniharshit patniharshit Jul 29, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The previous change was reverted because two different variables norm and cmap are being checked for their respective instances.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
anntzer
anntzer reviewed Jul 23, 2017
View reviewed changes
Comment thread lib/matplotlib/colorbar.py
xlabel='',
ylabel='',
):
#: The axes that this colorbar lives in.

@anntzer anntzer Jul 23, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Unless there is a strong reason to expose them, attributes should be private (start with an underscore) by default. Otherwise, they all become part of the public API, which is very difficult to change while maintaining back-compatibility.
(Same comment throughout the PR.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@patniharshit patniharshit Jul 27, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Any specific attribute you are pointing to? I tried to keep it similar to ColorbarBase class.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@tacaswell tacaswell Aug 20, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

ax is probably fine to stay public (it is public on every Artist), but the xvalues, xboundaries, etc can probably be made private.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
story645
story645 reviewed Jul 24, 2017
View reviewed changes
Comment thread lib/matplotlib/axes/_axes.py Outdated
isNorm = isinstance(norm, (mcolors.Normalize, mcolors.BivariateNorm))
if norm is not None and not isNorm:
msg = "'norm' must be an instance of 'mcolors.Normalize' " \
"or 'mcolors.BivariateNorm'"

@story645 story645 Jul 24, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

shouldn't BivariateNorm subclass Normalize? 'specially since now it's normalizing down to a 1d space?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@patniharshit patniharshit Jul 27, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

BivariateNorm does not inherit anything from Normalize so I thought it should not subclass it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@story645 story645 Jul 27, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

With the new design, BivariateNorm should havel all the same classes as Normalize...and I think subclassing it so it's registered generically as a Norm is preferable to having to treat it as a special case when it doesn't need to be.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@tacaswell tacaswell Jul 27, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This can be done with an ABC https://docs.python.org/3/library/abc.html#abc.ABCMeta.register which both Normalize and BivariateNorm register with.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
patniharshit reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@story645 story645 Jul 27, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sure, but I don't see a reason why BivariateNorm shouldn't be subclassing Norm as it has the same architecture as any other norm...take data (scaler or vector)->map to 1D lut value ->get color, and color ->lut->scaler/vector.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@tacaswell tacaswell Jul 27, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

If the code paths are completely different, why force the sub-class? In general, sub-classing is only worth it when you can share significant amounts of implementation details, otherwise duck-typing is better.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@story645 story645 Jul 28, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

My point is mostly that the code paths shouldn't end up being all that different and more to the point in mpl land there's probably a ton of code that checks if things are norms and if this code works in those instances it doesn't make sense to change all that code to cover both cases when this is still fundamentally a norm. But I think I'm on the same page as you on the solution maybe being a "Norm" metaclass.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/matplotlib/colors.py Outdated
temp[0] = temp[0] * (256)
temp[1] = temp[1] * (256)
temp = temp.astype(int)
return temp[0] + temp[1] * 256

@story645 story645 Jul 24, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

can you guarantee that this scheme always yields uniqueness?
(I'm confused about what's going on here and sort of have been thinking about a more general mapping between nd arrays and numbers wherein you get a sorted unique list of the nd tuples in the dataset so that the lookup/map is just the index of the value in the sorted list.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@patniharshit patniharshit Jul 27, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Since after normalization every variable ends up between 0 to 1. So here I am changing it to index flattened 1d lut of BivariateColormap by mapping 0-1 to 1 to 256*256. Here I am currently hardcoding 256 as number of rows and columns in lut of 2d colormap. But this should be changed somehow to use N attribute of BivariateColormap.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@tacaswell tacaswell Jul 28, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is column major indexing (row * num_cols + col is c / numpy style, col * num_rows + row).

I am more concerned that the descritization is happening here rather than in the color map (which is done https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/colors.py#L483) which prevents the size of the colormap from having to be known by the norm.

Why can't this return a Nd array? I think the most important places the norm and color map are called is https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/cm.py#L209 and
https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/image.py#L365 . In the first case it can be blindly passed through and in the second that resampling needs to happen over each plane.

In the case of the color bar, that already needs to be heavily special cased.

20:11 $ git grep '\.norm[ (]' | grep -v linalg
lib/matplotlib/cm.py:        self.norm = norm
lib/matplotlib/cm.py:            x = self.norm(x)
lib/matplotlib/cm.py:        self.norm = norm
lib/matplotlib/collections.py:        self.norm = other.norm
lib/matplotlib/colorbar.py:            y = self.norm(self._boundaries.copy())
lib/matplotlib/colorbar.py:            b = self.norm(self._boundaries, clip=False).filled()
lib/matplotlib/colorbar.py:            xn = self.norm(x, clip=False).filled()
lib/matplotlib/colorbar.py:        self.norm = mappable.norm
lib/matplotlib/colorbar.py:                                       facecolor=self.cmap(self.norm(val)),
lib/matplotlib/image.py:                A = self.norm(A)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@tacaswell tacaswell Aug 20, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I am still a bit concerned that the descritization is being done here rather than in the color map. Won't this break for any colormap which is not 256 by 256?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@story645 story645 changed the title [WIP] 2D normalizer [WIP] 2D Normalization & Colormapping for ScalerMappables Jul 24, 2017
tacaswell
tacaswell reviewed Jul 27, 2017
View reviewed changes
Comment thread lib/matplotlib/axes/_axes.py Outdated
isNorm = isinstance(norm, (mcolors.Normalize, mcolors.BivariateNorm))
if norm is not None and not isNorm:
msg = "'norm' must be an instance of 'mcolors.Normalize' " \
"or 'mcolors.BivariateNorm'"

@tacaswell tacaswell Jul 27, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This can be done with an ABC https://docs.python.org/3/library/abc.html#abc.ABCMeta.register which both Normalize and BivariateNorm register with.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
patniharshit reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread lib/matplotlib/axes/_axes.py Outdated
temp = np.asarray(X)
if temp.ndim == 3 and isinstance(norm, mcolors.BivariateNorm):
temp = norm(temp)
X = cmap(temp, alpha=self.get_alpha(), bytes=True)

@tacaswell tacaswell Jul 27, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Doing this here means you can not change anything about the color map or norm later.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@patniharshit patniharshit Aug 5, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Removed this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/matplotlib/axes/_axes.py Outdated

if len(args) == 1:
C = np.asanyarray(args[0])
isBivari = isinstance(norm, (mcolors.BivariateNorm,

@tacaswell tacaswell Jul 27, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why would the norm ever be a colormap?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@patniharshit patniharshit Jul 29, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks for pointing this out. Fixed now.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/matplotlib/axes/_axes.py Outdated
cmap = mcolors.BivariateColormap()
if norm is None:
norm = mcolors.BivariateNorm()
C = norm(C)

@tacaswell tacaswell Jul 27, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why do this up-front? defering all of this to _make_image means the colormap and norm and be updated.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@patniharshit patniharshit Aug 16, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Check now.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
tacaswell
tacaswell reviewed Jul 28, 2017
View reviewed changes
Comment thread lib/matplotlib/colorbar.py Outdated
linthresh=norm.linthresh,
base=10)
else:
if mpl.rcParams['_internal.classic_mode']:

@tacaswell tacaswell Jul 27, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Don't need any of the classic shims as there is no back-compatibility to maintain.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@patniharshit patniharshit Jul 29, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Removed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/matplotlib/colorbar.py Outdated
or len(self._x) >= self.n_rasterize):
self.solids.set_rasterized(True)

def _ticker(self, norm):

@tacaswell tacaswell Jul 27, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Probably better to factor this as def _make_ticker(norm, values, boundaries, locator, formatter): as a local function in update_ticks.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@patniharshit patniharshit Jul 29, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You mean local/private function of class or nested function?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@tacaswell tacaswell Aug 1, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

nested function

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@patniharshit patniharshit Aug 2, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/matplotlib/colors.py Outdated
temp[0] = temp[0] * (256)
temp[1] = temp[1] * (256)
temp = temp.astype(int)
return temp[0] + temp[1] * 256

@tacaswell tacaswell Jul 28, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is column major indexing (row * num_cols + col is c / numpy style, col * num_rows + row).

I am more concerned that the descritization is happening here rather than in the color map (which is done https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/colors.py#L483) which prevents the size of the colormap from having to be known by the norm.

Why can't this return a Nd array? I think the most important places the norm and color map are called is https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/cm.py#L209 and
https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/image.py#L365 . In the first case it can be blindly passed through and in the second that resampling needs to happen over each plane.

In the case of the color bar, that already needs to be heavily special cased.

20:11 $ git grep '\.norm[ (]' | grep -v linalg
lib/matplotlib/cm.py:        self.norm = norm
lib/matplotlib/cm.py:            x = self.norm(x)
lib/matplotlib/cm.py:        self.norm = norm
lib/matplotlib/collections.py:        self.norm = other.norm
lib/matplotlib/colorbar.py:            y = self.norm(self._boundaries.copy())
lib/matplotlib/colorbar.py:            b = self.norm(self._boundaries, clip=False).filled()
lib/matplotlib/colorbar.py:            xn = self.norm(x, clip=False).filled()
lib/matplotlib/colorbar.py:        self.norm = mappable.norm
lib/matplotlib/colorbar.py:                                       facecolor=self.cmap(self.norm(val)),
lib/matplotlib/image.py:                A = self.norm(A)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/matplotlib/colorbar.py
self.ax.set_xlabel(self._xlabel, **self._labelkw)
self.stale = True

def set_label(self, xlabel, ylabel, **kw):

@tacaswell tacaswell Jul 28, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The different signature from the ColorBar base class is a bit problematic, but not sure what to do about it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@anntzer anntzer Aug 20, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

perhaps set_labels(self, xylabel) (where xylabel=(xlabel, ylabel)); set_xlabel; set_ylabel?
let set_label raise an error
this likely would play better with traitlets

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@patniharshit
patniharshit force-pushed the norm2d branch from ca3cf70 to a5cd938 Compare August 5, 2017 13:49
@patniharshit
patniharshit force-pushed the norm2d branch from b43e6c7 to 23b1ffa Compare August 15, 2017 19:08
@story645 story645 changed the title [WIP] 2D Normalization & Colormapping for ScalerMappables 2D Normalization & Colormapping for ScalerMappables Aug 20, 2017
@patniharshit

patniharshit commented Aug 20, 2017

Copy link
Copy Markdown
Contributor Author

@anntzer can you have a look at this PR again? Some things have changed since your last review.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tacaswell
tacaswell reviewed Aug 20, 2017
View reviewed changes
Comment thread lib/matplotlib/axes/_axes.py Outdated
C = C.ravel()
# convert to one dimensional arrays if univariate
if isinstance(norm, mcolors.BivariateNorm):
C = np.asarray([C[0].ravel(), C[1].ravel()])

@tacaswell tacaswell Aug 20, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This may be better done as np.asarray([c.ravel() for c in C]) to be a bit future proof.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
patniharshit reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
tacaswell
tacaswell reviewed Aug 20, 2017
View reviewed changes
Comment thread lib/matplotlib/axes/_axes.py Outdated
msg = "'norm' must be an instance of 'mcolors.Normalize'"

if norm is not None and not isinstance(norm, mcolors.Norms):
msg = "'norm' must be an instance of 'mcolors.Normalize' " \

@tacaswell tacaswell Aug 20, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We have a style preference for using () for continuation rather than \ so

msg = ('...' +
       '...')

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@anntzer anntzer Aug 20, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The + is not needed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@patniharshit patniharshit Aug 21, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
anntzer
anntzer reviewed Aug 20, 2017
View reviewed changes
Comment thread lib/matplotlib/axes/_axes.py Outdated
if len(args) == 1:
C = np.asanyarray(args[0])
numRows, numCols = C.shape
isBivari = (isinstance(norm, mcolors.BivariateNorm)

@anntzer anntzer Aug 20, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

use_underscores_notCapsForVariables

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
patniharshit reacted with laugh emoji
All reactions
  • 😄 1 reaction

@patniharshit patniharshit Aug 21, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Changed isBivari to is_bivari.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
anntzer
anntzer reviewed Aug 20, 2017
View reviewed changes
Comment thread lib/matplotlib/collections.py
return
if self._A.ndim > 1:
if (self._A.ndim > 1 and
not isinstance(self.norm, mcolors.BivariateNorm)):

@anntzer anntzer Aug 20, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

should check ndim == 2 in this case?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
anntzer
anntzer reviewed Aug 20, 2017
View reviewed changes
Comment thread lib/matplotlib/colorbar.py
"""
Label the axes of the colorbar
"""
self._xlabel = '%s' % (xlabel, )

@anntzer anntzer Aug 20, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

just self._xlabel = xlabel should be enough (unless we explicitly want to support any object via conversion to string, and even then str(xlabel) is clearer.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@patniharshit patniharshit Aug 21, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Just imitating how it is done in ColorbarBase.set_label.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
anntzer
anntzer reviewed Aug 20, 2017
View reviewed changes
Comment thread lib/matplotlib/colors.py Outdated
"""
Abstract Base Class to group `Normalize` and `BivariateNorm`
"""
__metaclass__ = ABCMeta

@anntzer anntzer Aug 20, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

definitely wrong on Py3
use six
should have been caught by pycodestyle perhaps?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@patniharshit patniharshit Aug 21, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed now.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@tacaswell

tacaswell commented Aug 27, 2017

Copy link
Copy Markdown
Member

This is looking good 👍
patniharshit reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tacaswell

tacaswell commented Aug 27, 2017

Copy link
Copy Markdown
Member

Can you build the docs locally?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
patniharshit added 2 commits August 28, 2017 00:49
@patniharshit
Add line continuation in docstrings
75a289f
@patniharshit
Debug doc build fail
7803974
@patniharshit
patniharshit force-pushed the norm2d branch from 33cd323 to 84ff764 Compare August 28, 2017 18:14
@patniharshit
Add missing symbols in docs
e40c87b
@patniharshit
patniharshit force-pushed the norm2d branch from 84ff764 to e40c87b Compare August 28, 2017 18:39
@patniharshit

patniharshit commented Aug 28, 2017

Copy link
Copy Markdown
Contributor Author

@tacaswell doc build is passing now.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tacaswell

tacaswell commented Sep 2, 2017

Copy link
Copy Markdown
Member

@patniharshit I broke your branch via #8966 this needs to be rebased (sorry).

I think it is ok to either just update the shim around the resampling code or to move it all into a loop (I think clearer now) to resample each plane.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@patniharshit

patniharshit commented Sep 3, 2017

Copy link
Copy Markdown
Contributor Author

I don't have access to my laptop for two days. Will resolve the conflicts by Wednesday.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
shoyer
shoyer reviewed Sep 18, 2017
View reviewed changes
Comment thread lib/matplotlib/axes/_axes.py
- MxN -- univariate values to be mapped (float or int)
- MxNx3 -- RGB (float or uint8)
- MxNx4 -- RGBA (float or uint8)
- 2xMxN -- bivariate values to be mapped (float or int)

@shoyer shoyer Sep 18, 2017

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

What happens when the input is shape (2, 3, 4)?

This sort of ambiguity should be avoided (and indicates that this API is probably not the right design choice).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@anntzer anntzer mentioned this pull request Jan 10, 2018
Clip RGB data to valid range for imshow #10220
Merged
5 tasks done
@QuLogic QuLogic mentioned this pull request May 8, 2019
Feature request: Bivariate colormapping #14168
Closed
jklymak
jklymak requested changes May 8, 2019
View reviewed changes

@jklymak jklymak left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This PR came up in #14168.

I'm not sure the status of this, but in case someone comes back to it...

This looks technically well done. However, I'm not a fan of this API (see @shoyer comment as well). Its very complex from a documentation point of view. There is no way from the dosctrings that someone who didn't know what this PR did would understand what is meant by "2xMxN -- bivariate values to be mapped (float or int)". Further propagating support for two norms across all the 2-D plotting functions is going to be a maintenance nightmare. I'd far prefer this just got its own methods, i.e.: ax.bivariate_pcolormesh(x, y, Z1, Z1, norm1, norm2) etc.

I'm also very concerned about adding a whole new colorbar implementation. Maybe its unavoidable, but given that its so different, I wonder if it should also get its own method...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@shoyer

shoyer commented May 8, 2019

Copy link
Copy Markdown

I agree with putting this into a separate API. imshow is already pretty overloaded.

In shaping this API, I think it would be helping to identify clear use-cases / critical user journeys and make sure they are well supported and documented. For example, one good use case is visualizing complex-valued functions with luminosity and (perceptually uniform?) hue.

My concern is that in general this isn't a terribly effective visualization technique. That's not to say matplotlib shouldn't have this functionality, but let's try to limit its impact on the rest of the API.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jklymak

jklymak commented May 8, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

My concern is that in general this isn't a terribly effective visualization technique. That's not to say matplotlib shouldn't have this functionality, but let's try to limit its impact on the rest of the API.

My use case is shading a quantity based on gradient strength... If you'll forgive the sacrilege of linking to mathworks. The API for this was just:

shadedpcolor(x,z,U,(dU),[-1 1]/4,[-1 1]/300,0.7,cmp,0); 
axis ij; 
shadedcolorbar('v',[-1 1]/4,0.7,cmp);

https://www.mathworks.com/matlabcentral/fileexchange/14157-shaded-pseudo-color
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tacaswell tacaswell mentioned this pull request Sep 10, 2019
Support pixel-by-pixel alpha in imshow. #14889
Merged
4 of 6 tasks
@tacaswell
tacaswell marked this pull request as draft August 17, 2020 19:22
@tacaswell tacaswell mentioned this pull request Oct 26, 2020
colormap generalization #18809
Closed
@tacaswell tacaswell mentioned this pull request Aug 29, 2022
imshow doesn't normalize the color range in RGB images #9391
Closed
@github-actions github-actions Bot added the status: needs rebase label Oct 18, 2022
@github-actions

github-actions Bot commented Apr 13, 2023

Copy link
Copy Markdown

Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the status: inactive Marked by the “Stale” Github Action label Apr 13, 2023
@story645 story645 mentioned this pull request Aug 24, 2023
[ENH]: 2D colormaps #26590
Closed
@timhoffm

timhoffm commented Dec 13, 2024

Copy link
Copy Markdown
Member

This is superseeded. Tracked in #14168, and worked on in #28658, #28454, #29221.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@timhoffm timhoffm closed this Dec 13, 2024
@trygvrad trygvrad added this to Bivariate and Multivariate Colormapping Jul 3, 2025
@trygvrad trygvrad moved this to Todo in Bivariate and Multivariate Colormapping Jul 3, 2025
@trygvrad trygvrad moved this from Todo to Superseded in Bivariate and Multivariate Colormapping Jul 3, 2025
@trygvrad trygvrad removed this from Bivariate and Multivariate Colormapping Jul 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tacaswell tacaswell tacaswell left review comments
@story645 story645 story645 left review comments
@anntzer anntzer anntzer requested changes
@jklymak jklymak jklymak requested changes
+1 more reviewer
@shoyer shoyer shoyer left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

status: inactive Marked by the “Stale” Github Action status: needs rebase

Projects

None yet

Milestone

future releases

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@patniharshit @anntzer @story645 @tacaswell @shoyer @jklymak @timhoffm
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.