-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ENH: Added FuncNorm and PiecewiseNorm classes in colors #7294
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
Conversation
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 the patch! I unfortunately don't have time to do a detail review of the code so I pointed out only the elements that are not matching our contribution requirements (which we are documenting right now…).
Mostly, make sure that
(1) your code is pep8 (as far as I could tell, your code is, but the examples aren't)
(2) docstring are in numpydoc format
(3) examples should be moved to the examples folder, and if possible use the object-oriented mpl interface.
(4) You'll have to add a test at some point as well, but we can worry about this later.
Do you mind adding the figures your patch generates to give an idea of how it looks?
Thanks!
Nelle
@@ -0,0 +1,62 @@ | |||
""" |
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 examples need to be moved to the examples folder in the root directory.
Can you also make sure to follow our (very new) docstring standard for examples, with a title and a description: http://matplotlib.org/devdocs/devel/MEP/MEP12.html (see the sphinx docstring example).
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 the feedback!
Do you have any suggestions where in particular in the example folders? I knew the example folders was preferred but I did it in the doc folder because that is where similar examples (Other normalization examples) were located.
Sure, I will change all the formatting for the docstrings. Even though I read about that formatting I tried to follow the same formatting that was being used in the nearby classes, but I will change that.
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 should probably move all of those in the examples folder - else they won't be dealt with properly in the gallery.
The title + description element is a fairly recent thing - we'd like to use sphinx-gallery that requires such formatting..
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 have now added and commited the docstring to the examples, however I have not yet moved it to the examples folder, as I am not sure which subfolder is the most appropriate, or whether I should create a new subfolder.
import matplotlib.pyplot as plt | ||
import matplotlib.cm as cm | ||
|
||
x=np.linspace(0,16*np.pi,1024) |
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 code is not PEP8 compatible. Try running the tool flake8 to identify those pep8 incompatibility. Most text editor provide plugin for flake8 (SublimeText, vim, emacs…).
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.
Ok thanks! I was not sure if the PEP8 compatibility also applied to the examples. I will correct this.
figsize=(16,10) | ||
cmap=cm.gist_rainbow | ||
|
||
plt.figure(figsize=figsize) |
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 try using the object oriented interface instead of the pyplot one? It's more powerful and flexible.
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.
Do you mean, fig=plt.figure(figsize=figsize), and then ax=fig.add_subplot,.. and ax.plot , etc?
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.
Exactly!
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 default figsize should be good. The default approach should be
fig, ax = plt.subplots()
ax.pcolormesh(...)
center=.5, | ||
vmin=None, vmax=None, clip=False): | ||
""" | ||
*fpos*: |
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.
Please use numpydoc format for docstring.
[Edit: 23/10/2016] Examples used to be here. Now they are at the top. |
Can you re-implement the existing PowerNorm classes as special-cased sub-classes of these? |
|
||
if orderneg is None: | ||
orderneg = orderpos | ||
ArbitraryNorm.__init__(self, |
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 please use super()
?
Nifty! |
@tacaswell In any case unless there are some existing unit tests for the current implementation of PowerNorm, so the new implementation could be checked for back compatibility, I am not sure if I would feel confident to guarantee if the new implementations will behave in exactly the same way, exceptions an all, at least not yet. |
In general I like the idea a lot. However...
Not having to pass in the inverse (which would require a bit of reengineering of
(where the need for the inverse depends on how 1. is resolved).
(assuming you normalize yourself the range of the normalizer to [0, 1] after calling it). Basically, this is trading off API complexity in
|
@anntzer Thank you for the feedback :) Some replies below!
Essentially what a normalisation does is to describe a biyection from A->B where A is the real numbers, and B are the real numbers in the [0-1] interval. And when the data is converted to the [0,1], you want to have a linear representation of B, in both the plot and in the colorbar. The only difference comes in the ticks really, because the values assigned to the ticks still need to be the values from A. But the colorbar still needs to be linear en B, and the only way to achieve uniform spacing in B, with labels in the ticks that correspond to the values in A, is to be able to invert the function. From a mathematically point of view seems analytically impossible. I would have to see the exact implementation of the code, but my feeling is that you would achieve a colorbar that is linear in A, and that would compress the colours in the regions where numbers in A are closer together. Essentially the class would amplify the features in the figure, but giving it more colors to certain regions, but then the colorbar will not show a fair distribution of the colors. In fact, I think the colorbar essentially would look like the bottom part of this plot, with the colors not spread equally:
Essentially there are 3 use cases: B-The user wants two arbitrary transformations. For example, if the user wants to amplify values around 0, both on the positive and the negative side, then two transformations may be applied, one for values above 0, and one for values below 0. Of course, there are analytical functions that would achieve this with a single function, but independent control over the positive side and the negative side would be much more difficult. In this case two functions, and a reference point may be applied. C-The user wants again the double transformation, but wants to only applied it in a symmetric way, so, really only a function needs to be specified. Maybe the prototypes could be: A - B - C -
|
Actually, ticks positions are not computed using the inverse transform (see e.g. the plots at http://matplotlib.org/devdocs/users/colormapnorms.html#symmetric-logarithmic and http://matplotlib.org/devdocs/users/colormapnorms.html#power-law). Only a few cases are special-cased, e.g. log normalization uses the I'm not sure it makes sense to try to guess tick locations anyways (for example, in the case of a "split" linear normalization such as the one that your propose, it probably makes sense to put ticks equally spaced above the "zero" and below it, rather than uniformly everywhere; certainly you'd want to have the zero labeled). I think having some actual use cases for all the classes that you propose, and comparing how they look like using |
@anntzer I think in such non linear cases it will be very difficult to guess what the user wants. I agree that we should have a tick at 0, but in my case I would still like to have uniform ticks accross the whole colorbar, and the reason is that the level of detail that one will be able to look at should be related to the distribution of colors, that is what the normalization is for in the first place. My reasoning is that if you only plan to have 25% of your colorbar for negative numbers, is because you do not care that much about details in the negative range, hence, you do not need to be able to read the values as clearly. Honestly, I think most users using this will end up manually adding the ticks to meet the expectations of the level of detail they want to emphasize, in different regions of the colorbar. |
@NelleV I just pushed some significant upgrades to this. I would appreciate getting some feedback from you! From the contents point of view I am quite, happy. My plan is to clear the TODO list, and then after that, I am not sure how to proceed. Please, feel free to add stuff to the TODO list. Cheers :) |
One thing that needs to be clairifed is how vmin/vmax interact with the function you pass in. I think you basically always do |
@anntzer Right now this is calculated similarly to the first option: I would have to think about the implications of doing it the other way, and whether it is possible to match the intervals as easily. |
As a tangent, can this please not be named |
Perhaps ContinuousNorm (because I think continuity of the transformation is the only real requirement) or FuncNorm (consistently with FuncFormatter)? I am personally still opposed to what I see essentially as a rather complex rewrite of |
Hmm, |
My point is that this class should just take a single callable rather than a bunch of them; if the user wants a piecewise-defined normalization he should just construct such a function using |
I think it should be slightly more generic than just taking piecewise. Could it take any function (well kind of...)? Like: def f(x):
if x>=0.4:
return x**0.2
return x**0.5
FuncNorm(f) `cause I think it gives the user the flexibility they need/want out of this while also keeping the args/kwargs to a minimum. I think it ends up being more maintainable by inherently not making any assumptions about what the user wants. (As a side note, this could then possibly be used to build a CategoricalNorm depending on long it takes me to wrap up #6934) |
You probably cannot (completely) build a catergorical norm out of it because the color drawing assumes that the output of the transform countinuously covers [0, 1](after rescaling). |
shrugs I wrote one, just haven't factored it out of that PR into a standalone...but this gives another approach of sorts. Either way, I think I'm 👍 on something like |
Well I agree that np.piezewise is closely related to this, and that it should be used internally (I will include it when I have some time). However, I still think the argument to the class should not be given already as a piecewise lambda function for two reasons:
Apart from this, I think About taking any function, of course, this could be very easily implemented, because I would always apply a final linear transformation to the function to make sure that f(vmin)=0 and f(vmax)=1. This would be called Ideally, then I would make If you agree, I am happy to implement it this way. |
As it is right now, and as discussed above, the inverse is not used to find tick positions automatically. In fact I don't think there's really a smart way to do this for arbitrary transforms. For example, I'd say the choice of ticks on your example (https://cloud.githubusercontent.com/assets/12649253/19448591/5e62fb7c-949a-11e6-9ba9-f45090dbff90.png) is rather poor. If anything, I'd manually put ticks at [-1, -.5, -.25, 9, .005, .015, .1, 1, 2]. Another question that I think should be addressed is how often do people need complex normalizations that happen to be representable as piecewise functions? In other words, are we not looking at the wrong target? In any case I think I've said enough on this PR for now and should let others express their opinion for now :-) |
@anntzer Yes, as you said it is very difficult to find good tick positions for arbitrary transforms, but I do not think is impossible. The image in your link is from the very first version. It currently looks like this. It shows the important ticks (interval changes at -0.4, 1, 1.2), and then it uniformly distributes the rest, which may still give ugly numbers. Nevertheless this could be improved by rounding the numbers automatically to a digit depending on the neightboring numbers, so this should be possible to implement (EDIT: I just gave a try to this, and updated the figure), I think it works acceptably well)l. It will always be up the the user, but a good automatic guess is always appreciated. About the piecewise normalization, normally they are useful to enhance particular features that are sitting at particular value of the dynamic range. In particular around cero, and this is why |
EDIT: All is good now! The rebase suggested by @QuLogic solved it! @NelleV Edit: So I think I understand what is happening. For example if I was using "axes" in a previous revision, and then I stop using it, then starts tracking _axes.py, and tells me that I have decreased the coverage. Same happens with figure.py (I started using plt.subplots()). Is this normal? |
Next, stage 2: FuncNorm plus tests. |
@efiring Great, I will start working on it! I assume we should also include an example, right? |
Yes, an example would help. |
I have now created the next PRs related to this, implementing "FuncNorm". |
This PR is just amazing, it needs to be merged! I love the way it allows to precisely tune the colorbar (normalized at 0 or other points, centered or not, intensity amplification on some ranges, etc). Great work! PS: Also I find your examples above (#7294 (comment)) very useful and meaningful, it would be great if these would be added to the documentation alongside the PR. |
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 strongly
- oppose adding a new domain-specific language to describe functions (
root{n}
, etc). Yes, the parser has already been merged into cbook but it is not actually used right now and would prefer removing it. - believe that we should still first sort out the overlap between scales, norms, locators, and formatters that has been (much) discussed above, before adding more complex functionality.
As usual, other devs should feel free to override this review if there is consensus to do so.
Closing based on lack of comments over the rejection two weeks ago. |
Ironically, the result of a very positive comment about the PR ( thanks @lrq3000 I am glad you liked it :) ) is that it gets closed. @anntzer I completely respect the decision, however I just wish it would have been made right at the beginning. I spent many hours of my free time implementing changes to fit the opinions and criteria of different contributors and members here, just to, in the end, have the PR closed due to reasons related to the existing design of the library, and that I have not control over. I agree with the issues that you raise, but based on the same issues, the PR could have been closed right after I posted it... Anyway, maybe next time it will work out better, and please feel free to ping me if the scales/norms/locators are redesigned and I will be happy to reimplement the functionality :) |
My very first comment (#7294 (comment), made the day the PR was opened) was optimistic about the functionality, but was also already raising issues about the PR design that were never addressed, even though as you mention it is at least in part due to fundamental design issues in Matplotlib. So in my opinion the fundamental question here is whether we should accept merging more and more layers on top of an initial poor design or choose to fix the fundamental problems first. I favor the first option; it does not mean that this PR cannot be merged but for that you need to convince two committers that this is a good approach (this is the standard policy for all PRs, even from other core devs) and so far none has approved the PR. In my view there is no value on keeping the PR opened forever. But closing it does not mean that we cannot revisit it later, it is simply to remove it from the "general todo list". |
Also, AFAICT this is the kind of features that should be easily publishable as an independent package providing the custom Norm class. Doing so would allow the module get some real world usage first. |
@anntzer Honestly I regret having made my comment. I cannot comment on the implementation since I am not a matplotlib developer and I don't know the codebase, but this PR is obviously a much needed feature, as it is a very common need, which can objectively be considered as core. Colorbars are one of the basic and essential feature of most scientific graphics, providing ways to normalize is essential to allow readers an intelligible quantification (basically it is necessary almost everytime you have to plot both positive and negative values, this qualifies well for me as a core need and feature...). I hope this feature will find its way back to matplotlib at some point, else it would be IMHO a great loss... |
I think |
I think this PR should stay closed. I'd be into re-opening #7631 that allows the user to specify arbitrary norms. |
Summary [Last edit 16/12/2016]:
This has now been split into different PRs:
-Implementing _StringFuncParser (Merged)
-Implementing FuncNorm
Summary [Last edit 25/10/2016]:
FuncNorm
that can take any function and use it for the normalization.LogNorm
, orPowerNorm
, could be reimplemented using this.PiecewiseNorm
inheriting fromFuncNorm
that allows specifying a list of non-divergent functions (lambda functions or special strings) to map different ranges of the data dynamic range, into different ranges of the colorbar range. It creates internally a np.piecewise function hiding all the complexity of making a piecewise function that maps non-linearly but continuously in different intervals to [0-1].colors.PiecewiseNorm(flist=['linear', 'quadratic', 'cubic'], refpoints_cm=[0.3, 0.5], refpoints_data=[-1., 1.])
to provide the inverse functions because we know them.
MirrorPiecewiseNorm
: It is similar toPiecewiseNorm
with two intervals, with the difference that both intervals are not treated equally from a translational point of view, but symetrically. In an scenario where the user would like to amplify features around 0, for both the positive and negative sides symmetrically, but strongly for the negative side, then he would have to do:MirrorPiecewiseNorm
automatically takes care the transformation of the function in the second range internally so the user only has to provide f(x)=sqrt(x) for the positive range, and f(x)=cubicroot(x) for the negative rangeMirrorRootNorm
inheriting fromMirrorArbitraryNorm
, it would let the user, achieve the same by just specifyingcolors.MirrorRootNorm(orderpos=2, orderneg=3)
.RootNorm
inheriting fromFuncNorm
, to perform root normalization by simply specifying the ordercolors.RootNorm(order=3)
.TO DO list:
Some examples of usage and outputs:
This is an example of an array with a large dynamic range using a regular linear scale. It consist of several regions:
Example of log normalization using FuncNorm. I have set manually the minimum value to 0.01 to fix the dynamic range that will be plotted. Negative features are not shown.
or alternatively with:
Example of root normalization using FuncNorm. Of course with direct sqrt normalization, negative features cannot be shown.
Performing a symmetric amplification of the features around 0
Amplifying positive and negative features standing on 0.6
Amplifying positive and negative features standing on both -0.4 and 1.2. The bumps in the bottom area (-2<y<-1) sitting at the values -0.4 (light blue) and 1.2 (orange) are amplified, both the positive and the negative bumps. We can observe that the ticks are not exactly equispaced, to have a tick at the interval changes, and at round numbers.
Amplifying only positive features standing on all -1, -0.2 and 1.2. The bumps in the bottom area (-2<y<-1) sitting at the values -1 (black), -0.6 (turquoise) and 1.2 (yellow) are amplified, but in this case only the positive bumps.