Cleanup scales

[anntzer]

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

[NelleV]

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

[anntzer]

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

[efiring]

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

[NelleV]

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

[anntzer]

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

[NelleV]

Why do you add one here?

[anntzer]

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

[NelleV]

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

[efiring]

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?

[NelleV]

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

[efiring]

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

[efiring]

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

[anntzer]

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?

[matthew-brett]

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.

[efiring]

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)?

[Kojoley]

Original code came from #3753 with this comment https://github.com/matplotlib/matplotlib/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.

[anntzer]

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

[efiring]

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.

[Kojoley]

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

[NelleV]

I am attempting to cancel my approval :(

[anntzer]

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

[tacaswell]

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

[efiring]

@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?

[NelleV]

It's way to old.

[efiring]

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

[anntzer]

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.

[efiring]

@anntzer, #7410 is merged.

[anntzer]

done.

[Kojoley]

You could rewrite this like:

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

[anntzer]

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

[Kojoley]

Ah, yes. You are right.

[NelleV]

Thanks @anntzer

[phobson]

much improved! thanks for this.

[Kojoley]

Backported to v2.x as dc9e7ec

[Kojoley]

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