Cleanup scales by anntzer · Pull Request #7144 · matplotlib/matplotlib · GitHub

Fixes #7143, together with some cleanups (in a separate commit).
There's a test failure (RMS=0.010) I can reproduce locally but I don't visually see any differences in the plots...

By the way the 1-1e-300 in the clipping of logit transforms is not very useful...

nitpick: a = np.log(a) is much more common and readable IMO

There was a comment about returning a itself from _mask_non_positives, i.e. saving a copy. I make an extra copy in where but get it back here. I don't mind just writing this as return np.log(a) / np.log(self.base) + 1 either -- it's probably a premature optimization anyways...

Recommendation: np.log(a, out=a) keeps the memory and speed efficiency and improves the readability.

I personally tend to favor readability over memory and speed efficiency, but OK…

I think the new version is not too bad:
return np.divide(np.log(a, out=a), np.log(self.base), out=a)

Why do you add one here?

... because the previous version did so (by multiplying by base before taking the log, which comes down to the same)... I didn't figure out why so I just left it, but apparently that's not needed. Removed.

Eh… That was an odd choice from previous developers, and somehow very confusing considering the name of the functions. Thanks for clarifying.

It looks like this practice of multiplying by the base in the forward transform and dividing by it in the inverse dates back to 8327bd4. I don't understand it, but it seems like it would have a substantial effect on log plots. @mdboom, do you remember why this was being done? Is there something that depends on it?

Why do you remove 1 here? (It is obviously linked to the +1 up there…)

It looks like we need tests to ensure that the inverse actually reverses the action of the transform for any transform with an inverse.

@NelleV, can you revoke your "approval"? It is misleading, given that you pointed out a major bug.

I'm not sure what is the "major bug" you're talking about? I think as long as we either keep the +/-1 on both sides or get rid of it on both sides we're good?

Tests of the inverses sound like a good idea.

I must say I am lost with the +1 -1 issue, I can't see that in the diff any more.

Sorry, the problem is that the review comments on this page are showing an outdated diff. The current proposed modification looks fine to me--but I still think we need to understand the consequences, if any, of the change in output that these modifications make. Doesn't it make a substantive difference somewhere in mpl when the log transform returns log10(x) instead of 1 + log10(x)?

Original code came from #3753 with this comment https://github.com//pull/3753#issuecomment-62970292 related to the +1/-1 > Ok, the problem that get_minpos is kind of specially crafted for log axes stays, but I fixed the logit tick locator with a hack for now. In case limits for the axis are outside ]0, 1[, I assume 1 - get_minpos() as axis limit. That defaults to 1 - 1e-7 and should be good for most cases.

@Kojoley I don't see how this is related?

I'm wondering whether the +1 -1 business is actually a bug that has been present for nearly a decade, but that has mostly subtle effects. I'm also wondering whether it is related to #6915.

@anntzer Nevermind. @efiring https://en.wikipedia.org/wiki/Logit

I am attempting to cancel my approval :(

Could this be milestoned to 2.0? This gets rid of a spurious warning while maintaining full backcompatibility.

milestoned an 2.0.1 so it will be backported, but will not block 2.0

@anntzer I'm inclined to just merge this ASAP to be sure it gets into 2.0. It looks like the test failure is a very slight difference in one image. Would you replace the test image, please, so we can get the tests passing and proceed?

It's way to old.

@anntzer It might be best to get #7410 merged and propagated up to master before re-checking the tests on this one; it's possible that with #7410, this one might change additional baseline images.

Ping me when #7410 is merged.

I'm pretty sure the diff is due to some floating point imprecision. For example, if I subtract 1 from the return value of LogTransformBase.transform_non_affine and add 1 back to the return value of InvertedLogTransformBase.transform_non_affine (what the previous version used to do), the RMS error slightly increases to 0.010.

@anntzer, #7410 is merged.

done.

You could rewrite this like:

class Log10Transform(LogTransformBase):
    base = 10.0
    inverted = InvertedLog10Transform

No, because InvertedLog10Transform is not defined yet at that point. (InvertedLog10Transform could define inverted as a reference to the Log10Transform class but I decided to keep definitions symmetric.)

Ah, yes. You are right.

Thanks @anntzer

much improved! thanks for this.

Cleanup scales

Backported to v2.x as dc9e7ec

I have broken master and v2.x by merging this.
At the point when the builds were made there was no test_log_margins test (it was restored by #7435).

NelleV reviewed Sep 20, 2016

View reviewed changes

Cleanup of style.<>Transforms; catch nan warning.

f8b1a9a

anntzer force-pushed the cleanup-scales branch from 63af7a7 to f8b1a9a Compare September 20, 2016 17:33

NelleV approved these changes Sep 20, 2016

View reviewed changes

tacaswell added this to the 2.1 (next point release) milestone Sep 20, 2016

NelleV previously requested changes Sep 21, 2016

View reviewed changes

tacaswell modified the milestones: 2.0 (style change major release), 2.1 (next point release), 2.0.1 (next bug fix release) Sep 27, 2016

Fix slightly imprecise test image.

7e0811d

Kojoley reviewed Nov 8, 2016

View reviewed changes

NelleV approved these changes Nov 10, 2016

View reviewed changes

NelleV changed the title Cleanup scales [MRG+1] Cleanup scales Nov 10, 2016

phobson approved these changes Nov 10, 2016

View reviewed changes

phobson changed the title [MRG+1] Cleanup scales [MRG+2] Cleanup scales Nov 10, 2016

Kojoley approved these changes Nov 13, 2016

View reviewed changes

Kojoley merged commit 540fc5a into matplotlib:master Nov 13, 2016

Kojoley changed the title [MRG+2] Cleanup scales Cleanup scales Nov 13, 2016

Kojoley added a commit that referenced this pull request Nov 13, 2016

Merge pull request #7144 from anntzer/cleanup-scales

dc9e7ec

Cleanup scales

Kojoley mentioned this pull request Nov 13, 2016

test_log_margins test failure #7452

Closed

anntzer deleted the cleanup-scales branch November 13, 2016 18:50

QuLogic modified the milestones: 2.0.1 (next bug fix release), 2.0 (style change major release) Dec 7, 2016

QuLogic mentioned this pull request Jan 6, 2017

Setting logarithmic yscale in Radar chart broken #7751

Closed

-                      return InvertedLog10Transform()
+                      with np.errstate(invalid="ignore"):
+                          a = np.where(a <= 0, self._fill_value, a)
+                      np.log(a, a)

+                          a = np.where(a <= 0, self._fill_value, a)
+                      np.log(a, a)
+                      a /= np.log(self.base)
+                      a += 1

                   def transform_non_affine(self, a):
-                      return ma.power(10.0, a) / 10.0
+                      return ma.power(self.base, a - 1)

Cleanup scales by anntzer · Pull Request #7144 · matplotlib/matplotlib · GitHub

Repository navigation

Cleanup scales - #7144

Cleanup scales#7144Kojoley merged 2 commits intomatplotlib:mastermatplotlib/matplotlib:masterfrom anntzer:cleanup-scalesanntzer/matplotlib:cleanup-scalesCopy head branch name to clipboard

Conversation

anntzer commented Sep 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NelleV Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiring commented Sep 20, 2016

Uh oh!

anntzer commented Sep 20, 2016

Uh oh!

matthew-brett commented Sep 20, 2016

Uh oh!

efiring commented Sep 20, 2016

Uh oh!

Kojoley commented Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anntzer commented Sep 20, 2016

Uh oh!

efiring commented Sep 20, 2016

Uh oh!

Kojoley commented Sep 20, 2016

Uh oh!

NelleV left a comment

Choose a reason for hiding this comment

Uh oh!

anntzer commented Sep 26, 2016

Uh oh!

tacaswell commented Sep 27, 2016

Uh oh!

efiring commented Nov 5, 2016

Uh oh!

efiring commented Nov 5, 2016

Uh oh!

anntzer commented Nov 5, 2016

Uh oh!

efiring commented Nov 5, 2016

Uh oh!

anntzer commented Nov 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NelleV left a comment

Choose a reason for hiding this comment

Uh oh!

phobson left a comment

Choose a reason for hiding this comment

Uh oh!

Kojoley commented Nov 13, 2016

Uh oh!

Kojoley commented Nov 13, 2016

Cleanup scales#7144
Kojoley merged 2 commits into
matplotlib:masterfrom
anntzer:cleanup-scales

NelleV Sep 20, 2016 •
edited
Loading

Kojoley commented Sep 20, 2016 •
edited
Loading