| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
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
Sorry, something went wrong.
| @@ -0,0 +1,62 @@ | |||
| """ | |||
There was a problem hiding this comment.
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).
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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..
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| 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.
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…).
Sorry, something went wrong.
There was a problem hiding this comment.
Ok thanks! I was not sure if the PEP8 compatibility also applied to the examples. I will correct this.
Sorry, something went wrong.
| figsize=(16,10) | ||
| cmap=cm.gist_rainbow | ||
|
|
||
| plt.figure(figsize=figsize) |
There was a problem hiding this comment.
Can you try using the object oriented interface instead of the pyplot one? It's more powerful and flexible.
Sorry, something went wrong.
There was a problem hiding this comment.
Do you mean, fig=plt.figure(figsize=figsize), and then ax=fig.add_subplot,.. and ax.plot , etc?
Sorry, something went wrong.
There was a problem hiding this comment.
Exactly!
Sorry, something went wrong.
There was a problem hiding this comment.
The default figsize should be good. The default approach should be
fig, ax = plt.subplots() ax.pcolormesh(...)
Sorry, something went wrong.
| center=.5, | ||
| vmin=None, vmax=None, clip=False): | ||
| """ | ||
| *fpos*: |
There was a problem hiding this comment.
Please use numpydoc format for docstring.
Sorry, something went wrong.
|
[Edit: 23/10/2016] Examples used to be here. Now they are at the top. |
Sorry, something went wrong.
|
Can you re-implement the existing PowerNorm classes as special-cased sub-classes of these? |
Sorry, something went wrong.
|
|
||
| if orderneg is None: | ||
| orderneg = orderpos | ||
| ArbitraryNorm.__init__(self, |
There was a problem hiding this comment.
Can you please use super()?
Sorry, something went wrong.
|
Nifty! |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
In general I like the idea a lot. However...
Not having to pass in the inverse (which would require a bit of reengineering of ColorbarBase, for sure) would seem to be a big improvement to the API.
ArbitraryNorm(normalizer, [inv_normalizer]) (where the need for the inverse depends on how 1. is resolved). np.piecewise directly allows forcing the value at zero (and this should be documented, of course). Your example could be written as (something like) ArbitraryNorm(
lambda x: np.piecewise(x, [x < .4], [-(.4 - x) ** .5, (x - .4) ** .2])
)
(assuming you normalize yourself the range of the normalizer to [0, 1] after calling it). Basically, this is trading off API complexity in ArbitraryNorm vs. whether you already know np.piecewise or not. If you didn't know np.piecewise then in both cases you have to learn an extra API, but if you already did, then it's a clear gain.
|
Sorry, something went wrong.
|
@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 - ArbitraryNorm(normalizer[1], [inv_normalizer[1]]) B - ArbitraryNorm(normalizer[2], [inv_normalizer[2]],center=[ReferencePoint,ReferencePointCB]) C - ArbitraryNorm(normalizer[1], [inv_normalizer[1]],center=[ReferencePoint,ReferencePointCB])
|
Sorry, something went wrong.
|
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 LogLocator class (see matplotlib.colorbar.ColorbarBase._ticker). 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 np.piecewise, is the way to decide on 2. |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
@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 :) |
Sorry, something went wrong.
|
One thing that needs to be clairifed is how vmin/vmax interact with the function you pass in. I think you basically always do f((x-vmin)/(vmax-vmin)). A quick check suggests that this is what the current PowerNorm does too, but this is actually rather counterintuitive for me: I'd more expect (f(x)-f(vmin))/(f(vmax)-f(vmin)), which is what the log norm does, for example. (From a physical point of view: x may not be defined in a linear scale so linearly rescaling it immediately doesn't make much sense; instead, f is used to linearize the scale.) |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
As a tangent, can this please not be named ArbitraryNorm? I think that name is just broad enough to be unhelpful, 'specially since there are other types of Norms (like a categorically aware one) that wouldn't fit in. How about NonLinearNorm? or FunctionalNorm or something else just please more precise. |
Sorry, something went wrong.
|
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 np.piecewise, though. |
Sorry, something went wrong.
|
Hmm, PiecewiseNorm? Is there a current way to get a norm out of np.piecewise? |
Sorry, something went wrong.
|
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 np.piecewise (and there should be an example of doing this in the docs). |
Sorry, something went wrong.
|
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) |
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
|
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 FuncNorm |
Sorry, something went wrong.
|
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 PiecewiseNorm would be a great name for that class instead of ArbitraryNorm. 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 FuncNorm. Ideally, then I would make PiecewiseNorm inherit from FuncNorm. I would buield the piecewise function in the constructor of PiecewiseNorm, and feed it to the constructor of FuncNorm. There would be some caveats regarding vmin and vmax being able to change postplotting with clim, but I think I already thought of a way this can be done. If you agree, I am happy to implement it this way. |
Sorry, something went wrong.
|
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 :-) |
Sorry, something went wrong.
|
@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 SymLog and SymPower, which are essentially piecewise functions with 3 and 2 intervals already existed. Also, the updated example, shows how this can very easily used to enhance features sitting at even more than one value of the dynamic range. |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
Next, stage 2: FuncNorm plus tests. |
Sorry, something went wrong.
|
@efiring Great, I will start working on it! I assume we should also include an example, right? |
Sorry, something went wrong.
|
Yes, an example would help. |
Sorry, something went wrong.
|
I have now created the next PRs related to this, implementing "FuncNorm". |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
There was a problem hiding this comment.
I strongly
As usual, other devs should feel free to override this review if there is consensus to do so.
Sorry, something went wrong.
Sorry, something went wrong.
|
Closing based on lack of comments over the rejection two weeks ago. |
Sorry, something went wrong.
|
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 :) |
Sorry, something went wrong.
|
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". |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
@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... |
Sorry, something went wrong.
|
I think FuncNorm And PiecewiseNorm would be great too, but the string parsing/language-defining DSL is a non-starter from my perspective. If was limited to callable's I'd approve the PR. |
Sorry, something went wrong.
|
I think this PR should stay closed. I'd be into re-opening #7631 that allows the user to specify arbitrary norms. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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]:
to provide the inverse functions because we know them.
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.