Cleanups: np.clip and np.ptp are awesome #7488
Conversation
| x, lastx = max(min(x, lastx), x1), min(max(x, lastx), x2) | ||
| y, lasty = max(min(y, lasty), y1), min(max(y, lasty), y2) | ||
|
|
||
| (x1, y1), (x2, y2) = np.clip( |
There was a problem hiding this comment.
This is on a hot-path (mouse move event callback), might be worth micro-benchmarking this.
There was a problem hiding this comment.
I couldn't see any slow down when trying the different backends manually (because the drawing of the canvas is probably way slower than this, even though it's obvious the new version will be a bit slower). I can roll back this patch if you prefer...
There was a problem hiding this comment.
I still did not have time to do a thourough check on the “draw_rubberband does not require its arguments to be ordered” thing. But I have another comment that may be worth to be read before I actually manage to find the time to finish the review (if nobody else does it before), so…
lib/matplotlib/axes/_axes.py
Outdated
| @@ -3675,8 +3675,7 @@ def dopatch(xs, ys, **kwargs): | |||
|
|
|||
| # width | |||
| if widths is None: | |||
| distance = max(positions) - min(positions) | |||
| widths = [min(0.15 * max(distance, 1.0), 0.5)] * N | |||
| widths = [np.clip(np.ptp(positions), 0.15, 0.5)] * N | |||
There was a problem hiding this comment.
Maybe I am missing something, but this does not seem to be equivalent to me. For example, with positions = [1.1, 0], i.e. (distance = 1.1) then [min(0.15 * max(distance, 1.0), 0.5)] * N => [0.165] * N but [np.clip(np.ptp(positions), 0.15, 0.5)] * N => [0.5] * N.
[np.clip(0.15*np.ptp(positions), 0.15, 0.5)] * N should be equivalent to the old approach.
There was a problem hiding this comment.
Good catch. Fixed, and updated the docstring to reflect the actual behavior (which was misdocumented).
|
This needs some small conflict resolution. |
|
Fixed. |
|
smuggled in an additional (unrelated, but tiny) cleanup (duplicated line, see last commit) |
|
Conflicts (from your other PR?) |
|
rebased |
All's in the title.
Also clarified that
draw_rubberbanddoes not require its arguments to be ordered... tested on all backends except on gtk, which seems broken as of master for me, and macos, which I cannot test and for which I would appreciate a confirmation (see https://gitter.im/matplotlib/matplotlib?at=5831157a238757566ccb3580).