Fix InvertedLog10Transform.inverted() #10242
Conversation
|
Fixng a RecursionError seems release-critical... |
There was a problem hiding this comment.
Thanks for the PR! We try to conform to the PEP8 coding guidelines to make our code neat, and it looks like there's a couple of exceptions: https://travis-ci.org/matplotlib/matplotlib/jobs/328459556#L2105
I think the things that need changing are:
- Lines need to be 79 characters or shorter
- There needs to be two blank lines between classes/methods that are at the lowest indent level
| @@ -93,7 +93,7 @@ class LogTransformBase(Transform): | |||
| is_separable = True | |||
| has_inverse = True | |||
|
|
|||
| def __init__(self, nonpos): | |||
| def __init__(self, nonpos='mask'): | |||
There was a problem hiding this comment.
This should probably default to 'clip' as this is the default else where.
There was a problem hiding this comment.
You are right, LogScale uses clip as default and so should LogTransform. Fixed in the PR.
|
@dstansby: I made the changes to conform to PEP8 and all checks pass now. |
| @@ -448,7 +455,7 @@ class LogitTransform(Transform): | |||
| is_separable = True | |||
| has_inverse = True | |||
|
|
|||
| def __init__(self, nonpos): | |||
| def __init__(self, nonpos='mask'): | |||
There was a problem hiding this comment.
I think this needs changing to 'clip' instead of 'mask'
There was a problem hiding this comment.
For consistency, I used the same default for LogitTransform (and LogisticTransform) as for LogitScale, which already uses 'mask'.
lib/matplotlib/tests/test_scale.py
Outdated
| def test_logscale_transform_repr(): | ||
| fig, ax = plt.subplots() | ||
| ax.set_yscale('log') | ||
| s = repr(ax.transData) |
There was a problem hiding this comment.
ping @fkloosterman - can this test do more than just make sure it runs?
There was a problem hiding this comment.
you mean something like:
s = repr(matplotlib.scale.Log10Transform(nonpos='clip'))
assert s=="Log10Transform('clip')"
There was a problem hiding this comment.
@fkloosterman yes something like that. Note, the pep8 checker will complain about the last line unless you put spaces around the == operator:
assert s == "Log10Transform('clip')"
lib/matplotlib/tests/test_scale.py
Outdated
| def test_logscale_transform_repr(): | ||
| fig, ax = plt.subplots() | ||
| ax.set_yscale('log') | ||
| s = repr(ax.transData) |
There was a problem hiding this comment.
@fkloosterman yes something like that. Note, the pep8 checker will complain about the last line unless you put spaces around the == operator:
assert s == "Log10Transform('clip')"
|
@phobson is this one OK by you now? |
|
@jklymak Looks good to me. Travis failure seems unrelated, yeah? |
|
Yeah, I think sometimes Travis has a minor problem with tex... |
|
Thanks a lot @fkloosterman! |
PR Summary
Closes issue #10202
InvertedLog10Transform.inverted() method fails because position argument
nonposis missing. To fix this, a default value for thenonposargument is added in constructor ofLogTransformBasebase class (and to be consistent, also toLogitTransformandLogisticTransformconstructors). While testing it was noted that repr of Log/Logit/LogisticTransform objects results in max recursion error, because theTransformbase class implements__repr__with a call tostr(self)and no__str__methods are implemented for the Log/Logit/LogisticTransform classes. This was fixed as well.PR Checklist