Skip to content
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

Add power-law normalization #1204

Merged
merged 5 commits into from Apr 4, 2014
Merged

Conversation

bgamari
Copy link
Contributor

@bgamari bgamari commented Sep 5, 2012

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.

@travisbot
Copy link

This pull request fails (merged aa31ca7d into 0f9f85f).

return vmax**gamma * pow(val, -gamma)

def autoscale(self, A):
'''
Copy link
Member

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.

Copy link
Contributor Author

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 from LogNorm. I'll put together a patch fixing this class as well.

@bgamari
Copy link
Contributor Author

bgamari commented Sep 10, 2012

How does this look now? Comments?

@WeatherGod
Copy link
Member

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
Copy link
Contributor Author

bgamari commented Sep 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,

 >>> from matplotlib.colors import PowerNorm
 >>> n = PowerNorm(1.0, -100, 100)
 >>> n(-100)
 0.0
 >>> n(0)
 0.5
 >>> n(100)
 1.0

Does this seem reasonable?

@bgamari
Copy link
Contributor Author

bgamari commented Sep 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.

@ghost ghost assigned dmcdougall Jan 18, 2013
vmin, vmax = self.vmin, self.vmax
if vmin > vmax:
raise ValueError("minvalue must be less than or equal to maxvalue")
elif vmin==vmax:
Copy link
Member

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
Copy link
Member

Let's try and get this into v1.3.

@WeatherGod Are you happy with the handling of offsetting?

@dmcdougall
Copy link
Member

This needs a test, an entry in CHANGELOG and whats_new, an example, and the issues outlined above need to be addressed as well.

@bgamari
Copy link
Contributor Author

bgamari commented Jan 18, 2013

Thanks for the comments! I'll try to incorporate them this weekend.

@@ -4,7 +4,7 @@

from __future__ import print_function
import numpy as np
from numpy.testing.utils import assert_array_equal, assert_array_almost_equal
Copy link
Member

Choose a reason for hiding this comment

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

The SymLogNorm test uses the assert_array_almost_equal function. Removing this made the test fail because it couldn't find it.

@bgamari
Copy link
Contributor Author

bgamari commented Jan 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
Copy link
Member

@bgamari No problem! That's why I'm here :)

@bgamari
Copy link
Contributor Author

bgamari commented Jan 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 whats_new contribution, I'd be happy to hear.

@@ -0,0 +1,27 @@
#!/usr/bin/python

from scipy.stats import norm
Copy link
Member

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.

Copy link
Contributor Author

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
Copy link
Contributor Author

bgamari commented Nov 27, 2013

@mdboom, indeed it does seem that the PowerNorm test fails on Travis. I'm really not sure why, though. The test succeeds for me and Travis doesn't seem to provide any additional diagnostics

self.vmin = ma.min(A)
if self.vmin < 0:
self.vmin = 0
warn("Power-law scaling on negative values is ill-defined, clamping to 0.")
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs to be warnings.warn, and is the cause of the Travis failure.

@bgamari
Copy link
Contributor Author

bgamari commented Nov 27, 2013

@mdboom, good catch! We'll see how this branch fares.

self.vmin = ma.min(A)
if self.vmin < 0:
self.vmin = 0
warnings.warn("Power-law scaling on negative values is ill-defined, clamping to 0.")
Copy link
Member

Choose a reason for hiding this comment

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

pep8 here

@bgamari
Copy link
Contributor Author

bgamari commented Nov 28, 2013

I've fixed the remaining PEP8 issues as well as the testcase. It seems that Travis approves.


a = [-0.5, 0, 0.5, 1, 1.5]
pnorm = mcolors.PowerNorm(1.5)
assert_array_almost_equal(a, pnorm.inverse(pnorm(a)))
Copy link
Member

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 from mcolors.PowerNorm(1.5) to mcolors.Normalize() and the test will still pass.

Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, not yet.

@pelson
Copy link
Member

pelson commented Jan 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
Copy link
Contributor Author

bgamari commented Jan 22, 2014

How does this look?

@tacaswell
Copy link
Member

Travis produces a divide by 0 error in this test.

assert_array_almost_equal(norm(a), pnorm(a))

a = [0.5, 0, 2, 4, 8]
expected = [1/0, 0, 1./16, 1./4, 1]
Copy link
Member

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.....

Copy link
Contributor Author

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. The 1/0 should have been np.nan.

@mdboom
Copy link
Member

mdboom commented Jan 27, 2014

@bagmari: It looks like this no longer merges cleanly. Would you mind rebasing against master?

@bgamari
Copy link
Contributor Author

bgamari commented Jan 27, 2014

I believe I (finally) have the test issues sorted out and the branch rebased.

@tacaswell
Copy link
Member

There is still a py3k issue:

ERROR: matplotlib.tests.test_colors.test_PowerNorm

----------------------------------------------------------------------

Traceback (most recent call last):

File "/home/travis/virtualenv/python3.3/lib/python3.3/site-packages/nose/case.py", line 198, in runTest

self.test(*self.arg)

File "/home/travis/virtualenv/python3.3/lib/python3.3/site-packages/matplotlib-1.4.x-py3.3-linux-x86_64.egg/matplotlib/tests/test_colors.py", line 62, in test_PowerNorm

assert_array_almost_equal(norm(a), pnorm(a))

File "/home/travis/virtualenv/python3.3/lib/python3.3/site-packages/matplotlib-1.4.x-py3.3-linux-x86_64.egg/matplotlib/colors.py", line 1184, in __call__

result[value < 0] = 0

TypeError: unorderable types: list() < int()

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
Copy link
Contributor Author

bgamari commented Mar 10, 2014

Looks like we've finally converged.

@@ -181,19 +189,6 @@ added. Furthermore, the the subplottool is now implemented as a modal
dialog. It was previously a QMainWindow, leaving the SPT open if one closed the
plotwindow.

Cairo backends
Copy link
Member

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.

Copy link
Contributor Author

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
Copy link
Contributor Author

bgamari commented Mar 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 added a commit that referenced this pull request Apr 4, 2014
@tacaswell tacaswell merged commit d0250b4 into matplotlib:master Apr 4, 2014
@tacaswell
Copy link
Member

This should get a medal as an exceptionally long running but finally merged PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants