Fix containment test with nonlinear transforms. by anntzer · Pull Request #7844 · matplotlib/matplotlib

anntzer · 2017-01-16T19:51:47Z

Fixes #3540; see in particular #3540 (comment).

Edit: Also fixes #7655.

@mdboom Would it be reasonable to remove the definition of Transform.__array__ and only keep Affine2D.__array__, thus forcing call sites to explicitly only pass in affine transforms to the C++ code, rather than sometimes silently dropping the nonlinear part (as in this case)?

codecov-io · 2017-01-16T23:44:46Z

Current coverage is 62.10% (diff: 60.00%)

Merging #7844 into master will increase coverage by <.01%

@@             master      #7844   diff @@
==========================================
  Files           174        174          
  Lines         56051      56054     +3   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          34808      34811     +3   
  Misses        21243      21243          
  Partials          0          0

Powered by Codecov. Last update 3b9a92d...050b20c

NelleV · 2017-01-28T19:46:42Z

Can you add a regression test?

anntzer · 2017-01-28T21:37:58Z

Done.

NelleV

It looks good.
Can you move the import at the top of module?

NelleV · 2017-01-28T21:40:52Z

lib/matplotlib/path.py

-        result = _path.point_in_path(point[0], point[1], radius, self,
-                                     transform)
-        return result
+        from .transforms import Affine2D


Any reason to have this here and not at the top of the module as python convention require?

Probably dates back to when I implemented this as a quickfix. Moved up.

anntzer · 2017-01-28T22:08:33Z

Actually there is a circular import between transforms.py and path.py, so I left the Affine2D import inside the function, with a note to that effect.

anntzer · 2017-01-28T22:43:08Z

Test failure is probably related to pytest migration... we can wait until it is done, as that seems close(?).

NelleV · 2017-01-29T19:14:12Z

Let's wait until the pytest migration is over.

But, can we fix the circular import in a more robust way?

anntzer · 2017-01-29T20:12:25Z

This would likely imply a small performance cost as transforms relies on passing a Path to update_path_extent to compute not only extremal points (which is just as cheap in numpy) but also minpos (and x[x > 0].min() is very likely slower than a C loop). (Fixing #7413 would probably help.)

NelleV · 2017-01-29T20:33:42Z

I would feel much more comfortable with fixing the circular import.

anntzer · 2017-01-29T20:36:55Z

There's also transform_path_affine, which currently reads

    def transform_path_affine(self, path):
        return Path(self.transform_affine(path.vertices),
                    path.codes, path._interpolation_steps)

and would have to become something like

    def transform_path_affine(self, path):
        return type(path)(self.transform_affine(path.vertices),
                    path.codes, path._interpolation_steps)

which is worse than a circular import IMO.

I don't think this should be merge with a circular import.

anntzer · 2017-02-21T00:52:48Z

What is you preferred solution other than the circular import, then? As mentioned above I can work around it by obfuscating the implementation of transform_path_affine to sneakily get a handle to the Path class without a circular import, but that's not really better IMO.

tacaswell · 2017-02-21T01:31:58Z

lib/matplotlib/path.py

+        # We need to import Affine2D here to prevent a circular import between
+        # the .path and .transforms modules.
+        from .transforms import Affine2D
+        if transform and not isinstance(transform, Affine2D):


Can't this be if not transform.is_affine: ? That seems way better that isinstance anyway.

Good catch, done.

NelleV

LGTM 👍

dstansby · 2017-03-19T19:02:42Z

lib/matplotlib/tests/test_path.py

+    polygon = ax.axvspan(1, 10)
+    assert polygon.get_path().contains_point(
+        ax.transData.transform_point((5, .5)), ax.transData)
+


May also be worth checking that the points (0.5, 0.5) and (20, 0.5) are not inside polygon

dstansby · 2017-03-19T19:06:26Z

lib/matplotlib/path.py

+        # transform the path ourselves.  If `transform` is affine, letting
+        # `point_in_path` handle the transform avoids allocating an extra
+        # buffer.
+        if transform and not transform.is_affine:


Is there a reason not to just do the transformation here even if it is an affine transformation? (thus removing the if block)

I guess you may save the creation of a large temporary (if the c++ code does the transform one point at a time), which would be relevant for very large paths.

To be honest I don't know but decided not to take any risks.

dstansby

👍 Will merge if/when tests pass

anntzer mentioned this pull request Jan 17, 2017

Event picking does not seem to work on polar bar plots #7655

Closed

NelleV previously approved these changes Jan 28, 2017

View reviewed changes

anntzer force-pushed the nonlinear-transformed-path-containment branch from 652371f to ac133e8 Compare January 28, 2017 21:45

anntzer mentioned this pull request Jan 28, 2017

Deprecate Transform.__array__ in favor of Transform.get_affine and Affine2D.__array__ #7975

Open

anntzer force-pushed the nonlinear-transformed-path-containment branch from ac133e8 to 7d67293 Compare January 28, 2017 22:07

NelleV changed the title ~~Fix containment test with nonlinear transforms.~~ [MRG+1] Fix containment test with nonlinear transforms. Jan 29, 2017

NelleV changed the title ~~[MRG+1] Fix containment test with nonlinear transforms.~~ Fix containment test with nonlinear transforms. Jan 29, 2017

tacaswell reviewed Feb 21, 2017

View reviewed changes

Fix containment test with nonlinear transforms.

15eb7e7

anntzer force-pushed the nonlinear-transformed-path-containment branch from 7d67293 to ffd60aa Compare February 21, 2017 01:46

anntzer mentioned this pull request Feb 22, 2017

Default legend location 'best' is extremely slow #8108

Open

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

anntzer changed the title ~~Fix containment test with nonlinear transforms.~~ [MRG] Fix containment test with nonlinear transforms. Mar 15, 2017

NelleV approved these changes Mar 19, 2017

View reviewed changes

NelleV changed the title ~~[MRG] Fix containment test with nonlinear transforms.~~ [MRG+"] Fix containment test with nonlinear transforms. Mar 19, 2017

NelleV changed the title ~~[MRG+"] Fix containment test with nonlinear transforms.~~ [MRG+1] Fix containment test with nonlinear transforms. Mar 19, 2017

dstansby reviewed Mar 19, 2017

View reviewed changes

Test path containment w/ nonlin. transforms; group containment tests.

76173e1

anntzer force-pushed the nonlinear-transformed-path-containment branch from ffd60aa to 76173e1 Compare March 19, 2017 19:11

dstansby approved these changes Mar 19, 2017

View reviewed changes

dstansby self-assigned this Mar 19, 2017

dstansby merged commit 0611b61 into matplotlib:master Mar 19, 2017

dstansby changed the title ~~[MRG+1] Fix containment test with nonlinear transforms.~~ Fix containment test with nonlinear transforms. Mar 19, 2017

anntzer deleted the nonlinear-transformed-path-containment branch March 19, 2017 20:33

dstansby removed their assignment Apr 10, 2017

anntzer mentioned this pull request Aug 2, 2017

Patches cannot be picked when y-axis has logarithmic scaling #8979

Closed

Uh oh!

Conversation

anntzer commented Jan 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jan 16, 2017

Current coverage is 62.10% (diff: 60.00%)

Uh oh!

NelleV commented Jan 28, 2017

Uh oh!

anntzer commented Jan 28, 2017

Uh oh!

NelleV left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Jan 28, 2017

Uh oh!

anntzer commented Jan 28, 2017

Uh oh!

NelleV commented Jan 29, 2017

Uh oh!

anntzer commented Jan 29, 2017

Uh oh!

NelleV commented Jan 29, 2017

Uh oh!

anntzer commented Jan 29, 2017

Uh oh!

anntzer commented Feb 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NelleV 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dstansby left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

anntzer commented Jan 16, 2017 •

edited

Loading

NelleV left a comment •

edited

Loading