-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
In LogTransform, clip after log, not before. #9477
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
Changes from 5 commits
07a9450
def5e9a
6357245
746d466
0f9c6fb
b81b323
e3b777f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,15 +92,24 @@ class LogTransformBase(Transform): | |
|
||
def __init__(self, nonpos): | ||
Transform.__init__(self) | ||
if nonpos == 'mask': | ||
self._fill_value = np.nan | ||
else: | ||
self._fill_value = 1e-300 | ||
self._clip = {"clip": True, "mask": False}[nonpos] | ||
|
||
def transform_non_affine(self, a): | ||
with np.errstate(invalid="ignore"): | ||
a = np.where(a <= 0, self._fill_value, a) | ||
return np.divide(np.log(a, out=a), np.log(self.base), out=a) | ||
with np.errstate(divide="ignore", invalid="ignore"): | ||
out = np.log(a) | ||
out /= np.log(self.base) | ||
if self._clip: | ||
# SVG spec says that conforming viewers must support values up | ||
# to 3.4e38 (C float); however experiments suggest that Inkscape | ||
# (which uses cairo for rendering) runs into cairo's 24-bit limit | ||
# (which is apparently shared by Agg). | ||
# Ghostscript (used for pdf rendering appears to overflow even | ||
# earlier, with the max value around 2 ** 15 for the tests to pass. | ||
# On the other hand, in practice, we want to clip beyond | ||
# np.log10(np.nextafter(0, 1)) ~ -323 | ||
# so 1000 seems safe. | ||
out[a <= 0] = -1000 | ||
return out | ||
|
||
|
||
class InvertedLogTransformBase(Transform): | ||
|
@@ -220,11 +229,17 @@ def __init__(self, axis, **kwargs): | |
if axis.axis_name == 'x': | ||
base = kwargs.pop('basex', 10.0) | ||
subs = kwargs.pop('subsx', None) | ||
nonpos = kwargs.pop('nonposx', 'mask') | ||
nonpos = kwargs.pop('nonposx', 'clip') | ||
else: | ||
base = kwargs.pop('basey', 10.0) | ||
subs = kwargs.pop('subsy', None) | ||
nonpos = kwargs.pop('nonposy', 'mask') | ||
nonpos = kwargs.pop('nonposy', 'clip') | ||
|
||
if len(kwargs): | ||
raise ValueError(("provided too many kwargs, can only pass " | ||
"{'basex', 'subsx', nonposx'} or " | ||
"{'basey', 'subsy', nonposy'}. You passed ") + | ||
"{!r}".format(kwargs)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just format the entire string? no need to add, also no need for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you try to format the whole string it blows up on a key error with "'basex', 'subsx', nonposx'". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "{" -> "{{". |
||
|
||
if nonpos not in ['mask', 'clip']: | ||
raise ValueError("nonposx, nonposy kwarg must be 'mask' or 'clip'") | ||
|
@@ -432,18 +447,17 @@ class LogitTransform(Transform): | |
|
||
def __init__(self, nonpos): | ||
Transform.__init__(self) | ||
if nonpos == 'mask': | ||
self._fill_value = np.nan | ||
else: | ||
self._fill_value = 1e-300 | ||
self._nonpos = nonpos | ||
self._clip = {"clip": True, "mask": False}[nonpos] | ||
|
||
def transform_non_affine(self, a): | ||
"""logit transform (base 10), masked or clipped""" | ||
with np.errstate(invalid="ignore"): | ||
a = np.select( | ||
[a <= 0, a >= 1], [self._fill_value, 1 - self._fill_value], a) | ||
return np.log10(a / (1 - a)) | ||
with np.errstate(divide="ignore", invalid="ignore"): | ||
out = np.log10(a / (1 - a)) | ||
if self._clip: # See LogTransform for choice of clip value. | ||
out[a <= 0] = -1000 | ||
out[1 <= a] = 1000 | ||
return out | ||
|
||
def inverted(self): | ||
return LogisticTransform(self._nonpos) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why mask here instead of 'clip' like semilogx?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because typo. But actually both should be using "mask" mode if we want to keep backcompatibility (that's what we had before). Feel free to push either option (but it seems normal to return to the 2.0.1 behavior for now).