Skip to content

Force clipped-log when drawing log-histograms. #9305

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

Closed
wants to merge 1 commit into from

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Oct 7, 2017

Fixes #9288 & #9457 .
Note that because the clipping occurs only at draw time, the autolimits are simply picked using the nonzero values (at the time the autolimits are computed, clipping is not in effect) (so the autolimits are going to be a little bit different but I am happy with relying on the margins system to handle them).
If we go this route we may ultimately want to strip out the special casing of log-scale plots on bar().
Also need to decide whether we want to allow force-clipping both x and y.
I would prefer not making this a public API for now :/

Edit: I think #9477 is a better fix.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@tacaswell tacaswell added this to the 2.1.1 (next bug fix release) milestone Oct 7, 2017
@tacaswell
Copy link
Member

Interesting. Could we push this up to the Artist level?

Tagged an 2.1.1, but I suspect that the safest thing will be to revert the default and hold something like this for 2.2.

@anntzer
Copy link
Contributor Author

anntzer commented Oct 7, 2017

As wrote in the comment I was really hoping to put this at the level of BarContainer but that guy has no control over drawing :-( so I put it at the lowest possible level.
I'd rather have the patching of the scale happen around the smallest possible of code (e.g. here it's a single statement), which is tricky to do at the draw artist level (because each artist will have its own way of interacting with the transform). So you'd effectively need to go through the draw() implementation for each Artist subclass, which I'm too lazy to do now.

Edit: But I guess the same mechanism can be helpful for log-scale errorbars that go below zero (not that they're particularly physically meaningful but I'm sure we're going to get a bug report about that...).

Edit^2: Now also implemented for log-scale errorbars and fill_between, and added tests.

@anntzer anntzer force-pushed the bar-force-clipped-log branch 2 times, most recently from b493791 to ffc2186 Compare October 8, 2017 04:06
@anntzer anntzer force-pushed the bar-force-clipped-log branch from ffc2186 to 957a5a1 Compare October 19, 2017 02:40
@anntzer
Copy link
Contributor Author

anntzer commented Oct 19, 2017

Looking at this again, I feel that the correct behavior is indeed to clip by default. However, the clipping should not be done by the scale machinery. Instead, it should be done at the renderer stage: when then renderer gets a point at (x, inf) (fully transformed, in pixel coordinates), it should plots it at (x, some-large-number) (note that right now renderers already need to implement this because otherwise this will overflow the C-level variables used by the renderer, e.g. https://github.com/matplotlib/matplotlib/blob/master/src/_backend_agg.h#L615). So you get clipping, but without the issue raised in #8045 and #8783: the clipping always occurs at some large number in pixel coordinates (thus outside of the image) rather than in value coordinates.

If it were only up to me, I would even suggest doing this independently of the scales machinery, so that even in linear scales, something like plt.plot([1, 1], [-np.inf, np.inf]) draws a vertical line at x=1. Wouldn't that behavior actually make sense? (Yes, I know, back-compatibility, yada yada.)

See also #9477 which implements something similar, which I think works better than the strategy of this PR.

@tacaswell
Copy link
Member

I also like #9477 better.

👍 for supporting plt.plot([1, 1], [-np.inf, np.inf]) for 2.2

@anntzer
Copy link
Contributor Author

anntzer commented Oct 21, 2017

You realize that would be a major change in behavior, right? 😉

@tacaswell
Copy link
Member

There is a question of how wrong the slope will be for ax.plot([1, 2, 3], [1, np.inf, 1]).

@tacaswell
Copy link
Member

Closing in favor of #9477

@tacaswell tacaswell closed this Oct 21, 2017
@tacaswell tacaswell modified the milestones: v2.1.1, unassigned Oct 21, 2017
@anntzer
Copy link
Contributor Author

anntzer commented Oct 21, 2017

For ax.plot([1, 2, 3], [1, np.inf, 1]) you'd get a vertical line starting at (1, 1) going upwards to infinity and another one going downwards from infinity to (3, 1) (I think that's pretty clear?).

The main question is what to do with (inf, inf). Realistically, this can either be treated as nan, or as a point infinitely away at an angle of 45°. I think I mildly favor the second posibility, as this allows e.g. filling [(0, 0), (0, inf), (inf, inf), (inf, 0)] to mean the positive quadrant, but I could easily be convinced otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Histograms disappear with logarithmic y-axis
2 participants