Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
Add power-law normalization#1204
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
travisbot commentedSep 5, 2012
lib/matplotlib/colors.py Outdated
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 not a docstring. Use""" instead.
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.
Good to know. I actually stole this code outright fromLogNorm. I'll put together a patch fixing this class as well.
bgamari commentedSep 10, 2012
How does this look now? Comments? |
WeatherGod commentedSep 10, 2012
I doubt this will make v1.2.x as the feature freeze has already happened. However, it will still be interesting to get this in for the next release. The todo list is: unit tests and new entry to "what's new". Would also be nice to add some examples to the gallery. I, too, would also like to make certain any sort of ambiguities regarding normalizations that are offset from zero. |
bgamari commentedSep 10, 2012
No worries regarding not making v1.2. I'll be sure to handle the tasks you mention in the next spin of the patch. I'm not really sure what the best way to handle offset data is. I implemented what I believe is the obvious approach in 42a8b47. Here I simply remove the offset and adjust the normalization appropriately. This gives, Does this seem reasonable? |
bgamari commentedSep 11, 2012
Seeing as there were no objections to my handling of offset data, I squashed this commit as well as another fix into b277f87. |
lib/matplotlib/colors.py Outdated
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.
PEP8: Spaces needed around the==.
dmcdougall commentedJan 18, 2013
Let's try and get this into @WeatherGod Are you happy with the handling of offsetting? |
dmcdougall commentedJan 18, 2013
This needs a test, an entry in |
bgamari commentedJan 18, 2013
Thanks for the comments! I'll try to incorporate them this weekend. |
lib/matplotlib/tests/test_colors.py Outdated
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.
TheSymLogNorm test uses theassert_array_almost_equal function. Removing this made the test fail because it couldn't find it.
bgamari commentedJan 22, 2013
I apologize for the sad state of the previous branch. It was admittedly incomplete work and I hadn't managed to get the testsuite to run. Thanks for the suggestions. |
dmcdougall commentedJan 22, 2013
@bgamari No problem! That's why I'm here :) |
bgamari commentedJan 22, 2013
@dmcdougall, how do things look now? I think this should be in good shape now although if you have any suggestions for further additions to the |
examples/api/power_norm_demo.py Outdated
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.
Matplotlib doesn't have scipy as a dependency, so we can't have this line in the example. On the plus side, it looks like this function isn't even used in the 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.
Yes, this snuck in. Killed now
bgamari commentedNov 27, 2013
@mdboom, indeed it does seem that the |
lib/matplotlib/colors.py Outdated
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 think this needs to bewarnings.warn, and is the cause of the Travis failure.
bgamari commentedNov 27, 2013
@mdboom, good catch! We'll see how this branch fares. |
lib/matplotlib/colors.py Outdated
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.
pep8 here
bgamari commentedNov 28, 2013
I've fixed the remaining PEP8 issues as well as the testcase. It seems that Travis approves. |
lib/matplotlib/tests/test_colors.py Outdated
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 testing that the inverse method is working, but there is no test to check that power norm is actually doing the right thing. For example, I believe I could change line 65 frommcolors.PowerNorm(1.5) tomcolors.Normalize() and the test will still pass.
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.
@bgamari Did this get addressed?
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, not yet.
pelson commentedJan 9, 2014
Looks like a small test extension and a rebase would see this into v1.4.0. Is there any chance you could take a look@bgamari? |
bgamari commentedJan 22, 2014
How does this look? |
tacaswell commentedJan 22, 2014
Travis produces a divide by 0 error in this test. |
lib/matplotlib/tests/test_colors.py Outdated
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 divide by zero error is explicit in the test. That does not seem right.....
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.
Yep, sorry about that; I was still waiting for my tests to finish when I pushed this branch. The1/0 should have beennp.nan.
mdboom commentedJan 27, 2014
@bagmari: It looks like this no longer merges cleanly. Would you mind rebasing against master? |
bgamari commentedJan 27, 2014
I believe I (finally) have the test issues sorted out and the branch rebased. |
tacaswell commentedFeb 26, 2014
There is still a py3k issue: It looks like something needs to be explicitly made into an numpy array. I am very confused why this also does not happen on 2.... |
bgamari commentedMar 10, 2014
Looks like we've finally converged. |
doc/users/whats_new.rst Outdated
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 probably should not be removed.
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.
damn you git rerere
bgamari commentedMar 10, 2014
@tacaswell, thanks again for catching all of these mistakes. I think I probably owe you a case of beer or two by now. |
tacaswell commentedApr 4, 2014
This should get a medal as an exceptionally long running but finally merged PR. |
Here is a first cut at introducing power-law normalization (e.g.gamma correction) to
matplotlib.colors. Do others feel this would be a useful addition to matplotlib?I should note that it's unclear if/how data which is offset from zero should be handled which gives me pause.