Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
33efc48 to0f89c30CompareWhen clipping before taking the log, the clip value in the output arraycannot be beyond log(epsilon) ~ -300. When clipping after taking thelog, the clip value can be much further (see comments regarding choiceof value).This allows setting the default nonpos mode back to "clip".Some test images had to be updated as this effectively moves theposition of the "clip point", slightly changing the slope of the linethat goes to that point and thus the rasterization (by a tiny amount).
Set the default in `LogScale.__init__`, not in `Axes.set_{x,y}scale`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.
Added some tests + doc changes.
lib/matplotlib/axes/_axes.py Outdated
| @@ -1673,6 +1674,7 @@ def semilogy(self, *args, **kwargs): | |||
| self.cla() | |||
| d = {'basey': kwargs.pop('basey', 10), | |||
| 'subsy': kwargs.pop('subsy', None), | |||
| 'nonposy': kwargs.pop('nonposy', 'mask'), | |||
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).
I am in favor of putting this in for 2.1.1. It is an API change, but it is reverting an API change in 2.1.0 that was more disruptive than we expected. The change to errorbars is a very nice perk. This will also fix#9149 |
| 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 comment
The 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!r asrepr(dict) == format(dict, "")
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.
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 comment
The reason will be displayed to describe this comment to others.Learn more.
"{" -> "{{".
If you think it's not an improvement, then at least replace "{!r}.format(...)" by repr(...), no? seems a bit perverse otherwise :)
- bug reports frommatplotlib#9288 - bug report frommatplotlib#9457 - demo of errorbars going negative
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.
👍 on the change for 2.1.1 because it closes some very real, confusing behavior.
I'll let@tacaswell and@anntzer sort out the error message formatting. 😁
it's fine if you want to leave the error formatting as it is too... i'll just clean it some day. |
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.
I'm happy with the API reversion.
Backport PR#9477 on branch v2.1.x
Uh oh!
There was an error while loading.Please reload this page.
In LogTransform, clip after log, not before.
When clipping before taking the log, the clip value in the output array
cannot be beyond log(epsilon) ~ -300. When clipping after taking the
log, the clip value can be much further (see comments regarding choice
of value).
This allows setting the default nonpos mode back to "clip".
Some test images had to be updated as this effectively moves the
position of the "clip point", slightly changing the slope of the line
that goes to that point and thus the rasterization (by a tiny amount).
I think this is a better fix than#9305.
PR Summary
PR Checklist