Faster legend with location 'best' #8126
Conversation
Also clarify the meaning of the 'filled' argument. This differs from the old implementation, but the only place where filled=False is used is in legend.py, and in that context the old behavior was incorrect anyway.
src/_path.h
Outdated
| @@ -872,6 +872,64 @@ bool path_intersects_path(PathIterator1 &p1, PathIterator2 &p2) | |||
| return false; | |||
| } | |||
|
|
|||
| // returns whether the segment from (x1,y1) to (x2,y2) | |||
| // intersects the rectangle centered at (cx,cy) with size (w,h) | |||
| inline bool segment_intersects_rectangle(double x1, double y1, | |||
There was a problem hiding this comment.
Maybe worth adding a reference to how the conditions below were derived?
There was a problem hiding this comment.
It's difficult to explain with text alone. Is there a place where I can put an SVG or something to illustrate it?
There was a problem hiding this comment.
Not sure what's best, I'll let @tacaswell handle this :)
There was a problem hiding this comment.
(Absent further opinions, I think a reasonable option is to create a src/doc folder and put it there. It only needs to be accessible from a source checkout anyways.)
There was a problem hiding this comment.
Adding a section to https://github.com/matplotlib/matplotlib/blob/master/doc/users/path_tutorial.rst is probably the best path here.
There was a problem hiding this comment.
Are you sure? This is a low-level internal implementation detail, the function isn't even user-accessible. I'm documenting the mathematical derivation of the equations that are used internally.
I've placed the file in src/doc/ for now so you can take a look.
lib/matplotlib/path.py
Outdated
| BboxTransformTo(bbox)) | ||
| result = self.intersects_path(rectangle, filled) | ||
| return result | ||
| return _path.path_intersects_rectangle(self, bbox.x0, bbox.y0, bbox.x1, bbox.y1, filled) |
There was a problem hiding this comment.
Line too long, see https://travis-ci.org/matplotlib/matplotlib/jobs/204040250#L1684.
| That is, if the path completely encloses the bounding box, | ||
| :meth:`intersects_bbox` will return True. | ||
|
|
||
| The bounding box is always considered filled. |
There was a problem hiding this comment.
Is this a change in behavior or clarification of current behavior?
There was a problem hiding this comment.
It's a change in behaviour when filled=False, however this mode was only used in legend.py, and in that context the old behaviour was incorrect: the bounding box (legend) is always filled even when the plot lines are not filled.
There was a problem hiding this comment.
Is there a way to not change this behavior? This is a public method on a public so we must be careful about changing API. You will be surprised what behavior users have found and rely on 😈 .
On the other hand, the docstring was clearly a copy-paste from the method above and not quite right and I think it is reasonable to interpret 'intersects bbox' as 'is in or crosses boundary of bbox'. It that case this needs to be documented as an API change in https://github.com/matplotlib/matplotlib/tree/master/doc/api/api_changes
There was a problem hiding this comment.
The name 'bounding box' already implies that the box represents the boundary of some more complex object which is somewhere inside the bounding box, so I think the new behavior makes more sense.
In order to retain the old behavior, it would be necessary to add an additional option, because we would actually need three use cases:
- Filled path intersects filled bbox (filled=True, unchanged, needed in many places).
- Unfilled path intersects unfilled bbox (filled=False, old behavior).
- Unfilled path intersects filled bbox (filled=False, new behavior, needed in legend).
So it looks like that would also result in an API change, unless the filled argument accepts more than two possible values.
If you agree, I will keep the behavior as I've implemented it now and update api_changes.
|
@MaartenBaert the failure on Appveyor is real, you probably want to check how to make |
| """ | ||
| from .transforms import BboxTransformTo | ||
| rectangle = self.unit_rectangle().transformed( |
There was a problem hiding this comment.
Did this used to deal with non-affine transformations?
There was a problem hiding this comment.
No, I checked that. This is just a somewhat confusing way of getting a rectangle that represents the bounding box.
|
On 2017/02/26 1:15 PM, Maarten Baert wrote:
The name 'bounding box' already implies that the box represents the
boundary of some more complex object which is somewhere inside the
bounding box, so I think the new behavior makes more sense.
Agreed!
|
src/_path.h
Outdated
| inline bool segment_intersects_rectangle(double x1, double y1, | ||
| double x2, double y2, | ||
| double cx, double cy, | ||
| double w, double h) { |
There was a problem hiding this comment.
looks like (from surrounding code) opening brace of function is usually on a newline?
src/_path.h
Outdated
| double x1, y1, x2, y2; | ||
|
|
||
| curve.vertex(&x1, &y1); | ||
| if(2.0 * fabs(x1 - cx) <= w && 2.0 * fabs(y1 - cy) <= h) { |
|
Squashed version here: PR #8224. |
|
(you could have force-pushed as well) |
See issue #8108. This doesn't solve the problem yet, but it's a step in the right direction.