Add sym-log normalization. #1355

Merged
merged 2 commits into from Nov 13, 2012

Conversation

Projects
None yet
5 participants
@Tillsten
Contributor

Tillsten commented Oct 10, 2012

Added a symmetric log-normalization. Fixed a small bug in LogNorm while on it.

@Tillsten

This comment has been minimized.

Show comment Hide comment
@Tillsten

Tillsten Oct 10, 2012

Contributor

closes #632

Contributor

Tillsten commented Oct 10, 2012

closes #632

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Oct 10, 2012

Member

Thanks for this @Tillsten . Do you have a small test case for this? Would you be willing to add it as a image test?
Additionally, do you have a test case where LogNorm was failing?

Thanks,

Member

pelson commented Oct 10, 2012

Thanks for this @Tillsten . Do you have a small test case for this? Would you be willing to add it as a image test?
Additionally, do you have a test case where LogNorm was failing?

Thanks,

@NelleV

View changes

lib/matplotlib/colors.py
+ Calculates vmin and vmax in the transformed system.
+ """
+ vmin, vmax = self.vmin, self.vmax
+ if abs(vmin) > self.linthresh:#

This comment has been minimized.

Show comment Hide comment
@NelleV

NelleV Oct 10, 2012

Contributor

There's a # sign, which, I think, is not supposed to me there here.

@NelleV

NelleV Oct 10, 2012

Contributor

There's a # sign, which, I think, is not supposed to me there here.

@dmcdougall

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Oct 10, 2012

Member

That LogNorm bug fix would be nice to get into v1.2.x, which won't happen if it's bundled with this new SymLogNorm feature.

@Tillsten Would you be willing to separate them, creating a new PR for the LogNorm fix? Perhaps I'm missing something and there's a better way of doing this.

Member

dmcdougall commented Oct 10, 2012

That LogNorm bug fix would be nice to get into v1.2.x, which won't happen if it's bundled with this new SymLogNorm feature.

@Tillsten Would you be willing to separate them, creating a new PR for the LogNorm fix? Perhaps I'm missing something and there's a better way of doing this.

@Tillsten

This comment has been minimized.

Show comment Hide comment
@Tillsten

Tillsten Oct 10, 2012

Contributor

@dmcdougall done in #1362.

Contributor

Tillsten commented Oct 10, 2012

@dmcdougall done in #1362.

@Tillsten

This comment has been minimized.

Show comment Hide comment
@Tillsten

Tillsten Oct 11, 2012

Contributor

@pelson Will try to make some test tomorrow. Btw they are almost no test for colors.

Contributor

Tillsten commented Oct 11, 2012

@pelson Will try to make some test tomorrow. Btw they are almost no test for colors.

@Tillsten

This comment has been minimized.

Show comment Hide comment
@Tillsten

Tillsten Oct 15, 2012

Contributor

Anything else to do? By the way, right now there is a lot of the duplication of functionality in colors and scales, which could be combined.

Contributor

Tillsten commented Oct 15, 2012

Anything else to do? By the way, right now there is a lot of the duplication of functionality in colors and scales, which could be combined.

@dmcdougall

View changes

lib/matplotlib/tests/test_colors.py
@@ -4,7 +4,7 @@
from __future__ import print_function
import numpy as np
-from numpy.testing.utils import assert_array_equal
+from numpy.testing.utils import assert_array_equal, assert_array_approx_equal

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Oct 15, 2012

Member

This line is making the test_colors script fail.

@dmcdougall

dmcdougall Oct 15, 2012

Member

This line is making the test_colors script fail.

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Oct 15, 2012

Member

Aha! Typo. Wow that took much longer than I expected.

It should be from numpy.testing.utils import assert_array_equal, assert_array_almost_equal. You will also need to change the call to this method in the symlog test.

@dmcdougall

dmcdougall Oct 15, 2012

Member

Aha! Typo. Wow that took much longer than I expected.

It should be from numpy.testing.utils import assert_array_equal, assert_array_almost_equal. You will also need to change the call to this method in the symlog test.

@dmcdougall

View changes

lib/matplotlib/tests/test_colors.py
+ norm = mcolors.SymLogNorm(3, vmax=5)
+ vals =[-15, -1, 2, 6]
+ normed_vals=norm(vals)
+ assert_array_approx_equal(normed_vals, [ 0., 0.55241723, 0.79512015, 1.04424997])

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Oct 15, 2012

Member

This line is greater than 79 characters. Can you wrap it?

@dmcdougall

dmcdougall Oct 15, 2012

Member

This line is greater than 79 characters. Can you wrap it?

@dmcdougall

View changes

lib/matplotlib/colors.py
+ """
+ Normalize a given value to the 0-1 range on a symlog scale.
+ """
+ def __init__(self, linthresh, linscale=1.0, vmin=None, vmax=None, clip=False):

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Oct 15, 2012

Member

This is > 79 chars.

@dmcdougall

dmcdougall Oct 15, 2012

Member

This is > 79 chars.

@dmcdougall

View changes

lib/matplotlib/colors.py
+ np.asarray(a, dtype='f')
+ sign = np.sign(a)
+ masked = abs(a)> self.linthresh
+ log = sign[masked] * self.linthresh * (1 + np.log(np.abs(a[masked]) / self.linthresh))

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Oct 15, 2012

Member

This is > 79 chars.

@dmcdougall

dmcdougall Oct 15, 2012

Member

This is > 79 chars.

@dmcdougall

View changes

lib/matplotlib/colors.py
+ np.asarray(a, dtype='f')
+ sign = np.sign(a)
+ masked = abs(a)> self.linthresh
+ exp = sign[masked] * self.linthresh * np.exp(sign[masked] * a[masked] / self.linthresh - 1)

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Oct 15, 2012

Member

This is > 79 chars.

@dmcdougall

dmcdougall Oct 15, 2012

Member

This is > 79 chars.

@Tillsten

This comment has been minimized.

Show comment Hide comment
@Tillsten

Tillsten Oct 16, 2012

Contributor

The issued mentioned are fixed, but i'll also add an functionality to set the ration between the linear- and the logpart, similar to the scale. But that will happen later this week.

Contributor

Tillsten commented Oct 16, 2012

The issued mentioned are fixed, but i'll also add an functionality to set the ration between the linear- and the logpart, similar to the scale. But that will happen later this week.

@dmcdougall

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Oct 16, 2012

Member

@Tillsten Could you push the fixes? It'd be nice to see the CI server run the tests just as a sanity check.

Member

dmcdougall commented Oct 16, 2012

@Tillsten Could you push the fixes? It'd be nice to see the CI server run the tests just as a sanity check.

@dmcdougall

View changes

lib/matplotlib/colors.py
+ masked = np.abs(a)> self.linthresh
+ sign = np.sign(a[masked])
+ log = sign * self.linthresh \
+ * (1 + np.log(np.abs(a[masked]) / self.linthresh))

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Nov 7, 2012

Member

Use of backslash to continue a line is discouraged, as it is not PEP8 compliant. Could you use parentheses to continue the line, or a temporary variable to make the line shorter?

@dmcdougall

dmcdougall Nov 7, 2012

Member

Use of backslash to continue a line is discouraged, as it is not PEP8 compliant. Could you use parentheses to continue the line, or a temporary variable to make the line shorter?

@dmcdougall

View changes

lib/matplotlib/colors.py
+ Inplace transformation.
+ """
+ np.asarray(a, dtype='f')
+ masked = np.abs(a)> self.linthresh

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Nov 7, 2012

Member

Could you put a space before >?

@dmcdougall

dmcdougall Nov 7, 2012

Member

Could you put a space before >?

@dmcdougall

View changes

lib/matplotlib/colors.py
+ masked = np.abs(a)> self.linthresh
+ sign = np.sign(a[masked])
+ exp = sign * self.linthresh \
+ * np.exp(sign * a[masked] / self.linthresh - 1)

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Nov 7, 2012

Member

Another trailing backslash. Could you modify this as per my comment above?

@dmcdougall

dmcdougall Nov 7, 2012

Member

Another trailing backslash. Could you modify this as per my comment above?

@dmcdougall

View changes

lib/matplotlib/colors.py
+ raise ValueError("Not invertible until scaled")
+ vmin, vmax = self.vmin, self.vmax
+ val = ma.asarray(value)
+ val = val*(self._upper-self._lower)+self._lower

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Nov 7, 2012

Member

Could you put spaces around the binary operators: *, - and +?

@dmcdougall

dmcdougall Nov 7, 2012

Member

Could you put spaces around the binary operators: *, - and +?

@dmcdougall

View changes

lib/matplotlib/colors.py
+ self._transform_vmin_vmax()
+
+ def autoscale_None(self, A):
+ ' autoscale only None-valued vmin or vmax'

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Nov 7, 2012

Member

Docstrings should be encased by triple double-quotes, as you have done in autoscale. Could you either use encasing """, or perhaps use # and make it a comment?

@dmcdougall

dmcdougall Nov 7, 2012

Member

Docstrings should be encased by triple double-quotes, as you have done in autoscale. Could you either use encasing """, or perhaps use # and make it a comment?

@dmcdougall

View changes

lib/matplotlib/tests/test_colors.py
+ norm = mcolors.SymLogNorm(3, vmax=5)
+ vals = [-15, -1, 2, 6]
+ normed_vals = norm(vals)
+ expected = [ 0., 0.55241723, 0.79512015, 1.04424997]

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Nov 7, 2012

Member

Could you remove the the space character between [ and 0?

@dmcdougall

dmcdougall Nov 7, 2012

Member

Could you remove the the space character between [ and 0?

@dmcdougall

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Nov 7, 2012

Member

@Tillsten Could you also add a section to doc/users/whats_new.rst giving yourself credit for your new creation?

Thanks for your hard work.

Member

dmcdougall commented Nov 7, 2012

@Tillsten Could you also add a section to doc/users/whats_new.rst giving yourself credit for your new creation?

Thanks for your hard work.

@Tillsten

This comment has been minimized.

Show comment Hide comment
@Tillsten

Tillsten Nov 7, 2012

Contributor

Oh, thanks for reminding me to finish the PR. Had to do lab stuff and forgot about it :(, sorry

Contributor

Tillsten commented Nov 7, 2012

Oh, thanks for reminding me to finish the PR. Had to do lab stuff and forgot about it :(, sorry

@Tillsten

This comment has been minimized.

Show comment Hide comment
@Tillsten

Tillsten Nov 9, 2012

Contributor

Gna, the handling of scalars is broken i think.

Contributor

Tillsten commented Nov 9, 2012

Gna, the handling of scalars is broken i think.

@dmcdougall

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Nov 12, 2012

Member

Perhaps write a test for the scalar case, too?

Member

dmcdougall commented Nov 12, 2012

Perhaps write a test for the scalar case, too?

@Tillsten

This comment has been minimized.

Show comment Hide comment
@Tillsten

Tillsten Nov 12, 2012

Contributor

i think it is now ready to merge, but maybe there are some style issues left.

Contributor

Tillsten commented Nov 12, 2012

i think it is now ready to merge, but maybe there are some style issues left.

@WeatherGod

View changes

lib/matplotlib/colors.py
+ if not self.scaled():
+ raise ValueError("Not invertible until scaled")
+ val = ma.asarray(value)
+ val = val*(self._upper-self._lower)+self._lower

This comment has been minimized.

Show comment Hide comment
@WeatherGod

WeatherGod Nov 12, 2012

Member

Need spaces around mathematical operators

@WeatherGod

WeatherGod Nov 12, 2012

Member

Need spaces around mathematical operators

@WeatherGod

View changes

lib/matplotlib/colors.py
+ """
+ Inverse inplace Transformation.
+ """
+ masked = np.abs(a) > self.linthresh * self._linscale_adj

This comment has been minimized.

Show comment Hide comment
@WeatherGod

WeatherGod Nov 12, 2012

Member

It would be more readable if there were parens around the multiplication part here.

@WeatherGod

WeatherGod Nov 12, 2012

Member

It would be more readable if there were parens around the multiplication part here.

@WeatherGod

View changes

lib/matplotlib/colors.py
+ log = (self._linscale_adj + np.log(np.abs(a[masked]) / self.linthresh))
+ log *= sign * self.linthresh
+ a[masked] = log
+ a[~masked]*= self._linscale_adj

This comment has been minimized.

Show comment Hide comment
@WeatherGod

WeatherGod Nov 12, 2012

Member

space before the "*=" operator

@WeatherGod

WeatherGod Nov 12, 2012

Member

space before the "*=" operator

@Tillsten

This comment has been minimized.

Show comment Hide comment
@Tillsten

Tillsten Nov 12, 2012

Contributor

I really need a pep8 checker for my editor 👍

Contributor

Tillsten commented Nov 12, 2012

I really need a pep8 checker for my editor 👍

@dmcdougall

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Nov 13, 2012

Member

@Tillsten Thanks. Looks like @WeatherGod had spotted some PEP8 issues I had missed. Thanks for that.

Would you squash all this down to a single commit?

Member

dmcdougall commented Nov 13, 2012

@Tillsten Thanks. Looks like @WeatherGod had spotted some PEP8 issues I had missed. Thanks for that.

Would you squash all this down to a single commit?

@Tillsten

This comment has been minimized.

Show comment Hide comment
@Tillsten

Tillsten Nov 13, 2012

Contributor

Sorry, still learning the git-basics, i think i have done something wrong.

Contributor

Tillsten commented Nov 13, 2012

Sorry, still learning the git-basics, i think i have done something wrong.

@Tillsten

This comment has been minimized.

Show comment Hide comment
@Tillsten

Tillsten Nov 13, 2012

Contributor

Ok, now it looks fine.

Contributor

Tillsten commented Nov 13, 2012

Ok, now it looks fine.

@dmcdougall

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Nov 13, 2012

Member

You should add an entry in doc/users/whats_new.rst giving yourself credit for your baby.

Member

dmcdougall commented Nov 13, 2012

You should add an entry in doc/users/whats_new.rst giving yourself credit for your baby.

@Tillsten

This comment has been minimized.

Show comment Hide comment
@Tillsten

Tillsten Nov 13, 2012

Contributor

Can i just put it into the changelog? I don't think this commit is big enough for the what's new section.

Contributor

Tillsten commented Nov 13, 2012

Can i just put it into the changelog? I don't think this commit is big enough for the what's new section.

@dmcdougall

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Nov 13, 2012

Member

Sure.

Member

dmcdougall commented Nov 13, 2012

Sure.

@dmcdougall

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Nov 13, 2012

Member

How do you feel about adding an example to the gallery?

Member

dmcdougall commented Nov 13, 2012

How do you feel about adding an example to the gallery?

@Tillsten

This comment has been minimized.

Show comment Hide comment
@Tillsten

Tillsten Nov 13, 2012

Contributor

@dmcdougall That would only make sense in comparison to other norms with addition of an explanation. Maybe i will
such an example after i contributed a symetric norm which i have laying around.

Contributor

Tillsten commented Nov 13, 2012

@dmcdougall That would only make sense in comparison to other norms with addition of an explanation. Maybe i will
such an example after i contributed a symetric norm which i have laying around.

@dmcdougall

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Nov 13, 2012

Member

Is there anything else you wish to add?

Member

dmcdougall commented Nov 13, 2012

Is there anything else you wish to add?

@Tillsten

This comment has been minimized.

Show comment Hide comment
@Tillsten

Tillsten Nov 13, 2012

Contributor

Not for this PR.

Contributor

Tillsten commented Nov 13, 2012

Not for this PR.

dmcdougall added a commit that referenced this pull request Nov 13, 2012

@dmcdougall dmcdougall merged commit c62a6cc into matplotlib:master Nov 13, 2012

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment