Faster legend with location 'best' by MaartenBaert · Pull Request #8126 · matplotlib/matplotlib

MaartenBaert · 2017-02-22T01:40:13Z

See issue #8108. This doesn't solve the problem yet, but it's a step in the right direction.

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.

anntzer · 2017-02-22T02:07:54Z


+// 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,


Maybe worth adding a reference to how the conditions below were derived?

It's difficult to explain with text alone. Is there a place where I can put an SVG or something to illustrate it?

Not sure what's best, I'll let @tacaswell handle this :)

(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.)

Adding a section to https://github.com/matplotlib/matplotlib/blob/master/doc/users/path_tutorial.rst is probably the best path here.

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.

anntzer · 2017-02-22T02:09:48Z

-            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)


Line too long, see https://travis-ci.org/matplotlib/matplotlib/jobs/204040250#L1684.

tacaswell · 2017-02-22T04:53:16Z

+        That is, if the path completely encloses the bounding box,
+        :meth:`intersects_bbox` will return True.
+
+        The bounding box is always considered filled.


Is this a change in behavior or clarification of current behavior?

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.

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

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.

anntzer · 2017-02-25T20:53:46Z

@MaartenBaert the failure on Appveyor is real, you probably want to check how to make fmin and fmax visible to MSVC.

tacaswell · 2017-02-26T17:19:01Z

+        The bounding box is always considered filled.
        """
-        from .transforms import BboxTransformTo
-        rectangle = self.unit_rectangle().transformed(


Did this used to deal with non-affine transformations?

No, I checked that. This is just a somewhat confusing way of getting a rectangle that represents the bounding box.

efiring · 2017-02-27T01:02:58Z

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!

anntzer

Very minor style nitpicks, otherwise looks good to me (so I'll approve but please fix them). You may or may not want to squash the style/typo related commits out of your history as well.

anntzer · 2017-03-05T23:20:05Z

+inline bool segment_intersects_rectangle(double x1, double y1,
+                                         double x2, double y2,
+                                         double cx, double cy,
+                                         double w, double h) {


looks like (from surrounding code) opening brace of function is usually on a newline?

anntzer · 2017-03-05T23:20:14Z

+    double x1, y1, x2, y2;
+
+    curve.vertex(&x1, &y1);
+    if(2.0 * fabs(x1 - cx) <= w && 2.0 * fabs(y1 - cy) <= h) {


space after if

MaartenBaert · 2017-03-07T15:35:15Z

Squashed version here: PR #8224.

anntzer · 2017-03-07T17:15:41Z

(you could have force-pushed as well)

MaartenBaert added 2 commits February 22, 2017 02:33

Add faster path_intersects_rectangle to _path.h.

fefce87

Make Path.intersects_bbox use path_intersects_rectangle.

94c4d9f

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.

anntzer reviewed Feb 22, 2017

View reviewed changes

tacaswell added this to the 2.1 (next point release) milestone Feb 22, 2017

tacaswell reviewed Feb 22, 2017

View reviewed changes

Fix maximum line length

8c05491

Avoid fmin/fmax for compatibility with MSVC++.

d9e9f29

tacaswell reviewed Feb 26, 2017

View reviewed changes

MaartenBaert added 3 commits February 26, 2017 23:42

Add documentation for segment_intersects_rectangle.

fe87068

Fix typo.

a6c8d57

Add API change for Path.intersects_bbox.

cd6877d

anntzer approved these changes Mar 5, 2017

View reviewed changes

Fix formatting of _path.h

7ac1844

MaartenBaert mentioned this pull request Mar 7, 2017

Implement Path.intersects_bbox in C++ to speed up legend positioning. #8224

Merged

MaartenBaert closed this Mar 7, 2017

dstansby added the status: duplicate label Mar 7, 2017

Uh oh!

Conversation

MaartenBaert commented Feb 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anntzer commented Feb 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiring commented Feb 27, 2017 via email

Uh oh!

anntzer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaartenBaert commented Mar 7, 2017

Uh oh!

anntzer commented Mar 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants