Skip to content

PowerNorm: do not clip negative values #10234

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

Merged
merged 4 commits into from
Jul 9, 2018

Conversation

aparamon
Copy link
Contributor

PR Summary

Do not clip negative values in PowerNorm, consistently with LogNorm.
PowerNorm can now be used with offset data.

@jklymak
Copy link
Member

jklymak commented Jan 11, 2018

Ummm....

I don't know who wrote PowerNorm, but its not mathematically clear to me. (a**2 - vmin**2)/(vmax**2 - vmin**2) .neq. ((a-vmin)/(vmax-vmin))**2

I'd expect 0.5 to map to vmin**2 + 0.5*(vmax**2 - vmin**2). i.e. linearly in squared space. This does it linearly in linear space. But I'm happy to be corrected - I didn't think about it for very long.

@tacaswell tacaswell added this to the v3.0 milestone Jan 11, 2018
@efiring
Copy link
Member

efiring commented Jan 11, 2018

It was added in #1204, with a lot of discussion over a long period--including discussion of what to do about negative numbers.

This is related to two old open PRs that need attention and decisions: #7294 and #7631.

@jklymak
Copy link
Member

jklymak commented Jan 11, 2018

In #7631: They do (f(a)-f(vmin))/(f(vmax) - f(vmin)). This is also how LogNorm works. SymLogNorm does whatever the heck SymLogNorm does, but I think is consistent with this across zero...

Unless I'm being stupid, PowerNorm does f(a - vmin)/f(vmax - vmin) which is very different. I'm not used to thinking about plotting things as a power, but if you did this in a logaritmic scale, you'd get very weird results.

I don't see that anyone looking at #1204 caught this or this was discussed, but I may have missed it.

@efiring
Copy link
Member

efiring commented Jan 11, 2018

Yes, it is equivalent to ((a - vmin) / (vmax - vmin))**gamma, so a plain Normalize is the case of gamma=1. This seems reasonable to me; we are not dealing with logs. It is consistent with the description of Gamma correction: https://en.wikipedia.org/wiki/Gamma_correction. First the data are linearly scaled to the 0-1 range based on specified vmin and vmax, and second, that 0-1 range is mapped to a new 0-1 range by raising it to a power. The puzzle to me is why the clipping was put there in the first place; it looks like confusion between the need for the second stage to be in the 0-1 range and the implementation, which put part of the clipping in the first stage--where it doesn't belong. I think this PR, removing that odd restriction of the original data to positive values, is doing the right thing.

@jklymak
Copy link
Member

jklymak commented Jan 11, 2018

Of course @anntzer picked up on it in #7294 (comment)

@jklymak
Copy link
Member

jklymak commented Jan 11, 2018

OK, but thats saying the scale should be the same if vmin=0, vmax=1, or if vmin = 10000 and vmax = 10001. A parabola x**2 has a very different slope at those two values. If the colorbar ticks go 0-1 I'd be fine with that, but they say 10000 to 10001, with the ticks quadratically spaced as if it were from 0 to 1. Thats seems pretty un-numerate to me. If the user wants to normalize between 0 and 1, they should do so and make the colorbar plotted as such.

Not sure I get the analogy to gamma correction - thats mapping 0-1 to 0-1 isn't it?

But hey, I never use PowerNorm so I'll stop complaining.

@jklymak
Copy link
Member

jklymak commented Jan 11, 2018

Sorry, parting shot: If I had two axes that were side-by-side and each was power-norm 2, and one went from 0-10, and the other from 5-10, with the second one half as long as the first, I'd expect the spacing between 7 and 8 to be the same in each axis.

@aparamon
Copy link
Contributor Author

@jklymak Indeed, (a**2 - vmin**2)/(vmax**2 - vmin**2) .neq. ((a-vmin)/(vmax-vmin))**2. Only the latter is translation invariant.

The code

import matplotlib.colors as colors
print(colors.PowerNorm(vmin = 0, vmax = 1, gamma = 3)([0, 0.5, 1]))
print(colors.PowerNorm(vmin = -2, vmax = -1, gamma = 3)([-2, -1.5, -1]))

expectedly outputs

[0.    0.125 1.   ]
[0.    0.125 1.   ]

after the fix.

The value of 0 is in no way special for the rest PowerNorm code, so the clipping by it is pretty artificial. LogNorm doesn't do it.

@jklymak
Copy link
Member

jklymak commented Jan 12, 2018

My point is that PowerNorm should not be linearly invariant, any more than LogNorm is. Nothing to stop a user from doing a linear transform before sending to the norm, but the transform should be in non-linear space (thats the hard part).

@efiring
Copy link
Member

efiring commented Jan 12, 2018

@jklymak you are talking about a different sort of transform than what is implemented in PowerNorm. What is implemented is not going to be changed, with the likely exception of this PR. If you see a compelling use case for the version you are describing, then it will need a different PR and a different name.

@jklymak
Copy link
Member

jklymak commented Jan 12, 2018

Fair enough. Suggest docs change to make the order of operations explicit: Linearly map a given value to the [0, 1] interval and then apply a power-law normalization over that interval.

@aparamon
Copy link
Contributor Author

The value of 0 is in no way special for the rest PowerNorm code, so the clipping by it is pretty artificial. LogNorm doesn't do it.

Sorry for inaccuracy: LogNorm actually does clip negative values. In fact, PowerNorm appears special/different from LogNorm with regard to translation invariance. Not sure if it's good or bad, but I support changing the doc-string for PowerNorm as proposed by @jklymak .

@efiring
Copy link
Member

efiring commented Jan 13, 2018

By all means, making it clear in the docstring exactly what PowerNorm does would be an important addition to this PR.

@jklymak
Copy link
Member

jklymak commented May 25, 2018

ping! Is this still active? I think this normalization is mathematically wrong-headed, but at least lets document it properly!

@aparamon
Copy link
Contributor Author

Are there any show-stoppers left to get the pull request accepted? (are any additional changes expected from my part?)

@efiring
Copy link
Member

efiring commented Jun 18, 2018

It needs to pass the tests.

@jklymak jklymak requested a review from efiring July 9, 2018 19:51
@efiring efiring merged commit 6ceac0d into matplotlib:master Jul 9, 2018
@tacaswell
Copy link
Member

I am confused by this, it looks like it does still clip negative values to 0...

@jklymak
Copy link
Member

jklymak commented Jul 9, 2018

Yes, after the norm has been applied. Thats desirable (vmin maps to zero)

@aparamon aparamon deleted the PowerNorm-offset-fix branch July 9, 2018 21:57
@efiring
Copy link
Member

efiring commented Jul 10, 2018

I was wondering about that also--instead of clipping, should the values be "bad" (out of range?) so they would be colored as such?

@jklymak
Copy link
Member

jklymak commented Jul 10, 2018

@efiring I always forget how the out of range stuff works. What gets put in if data is out of range? I didn't quickly find an answer (ahem Documentation issue). Note that I don't think LogNorm puts data out of range, it just clips....

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

Successfully merging this pull request may close these issues.

4 participants