Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix containment test with nonlinear transforms.#7844
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Fix containment test with nonlinear transforms.#7844
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecov-io commentedJan 16, 2017
Current coverage is 62.10% (diff: 60.00%)@@ 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
|
Can you add a regression test? |
Done. |
NelleV left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It looks good.
Can you move the import at the top of module?
lib/matplotlib/path.py Outdated
| result = _path.point_in_path(point[0], point[1], radius, self, | ||
| transform) | ||
| return result | ||
| from .transforms import Affine2D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Any reason to have this here and not at the top of the module as python convention require?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Probably dates back to when I implemented this as a quickfix. Moved up.
652371f toac133e8Compareac133e8 to7d67293CompareActually 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. |
Test failure is probably related to pytest migration... we can wait until it is done, as that seems close(?). |
Let's wait until the pytest migration is over. But, can we fix the circular import in a more robust way? |
This would likely imply a small performance cost as |
I would feel much more comfortable with fixing the circular import. |
There's also and would have to become something like which is worse than a circular import IMO. |
I don't think this should be merge with a circular import.
What is you preferred solution other than the circular import, then? As mentioned above I can work around it by obfuscating the implementation of |
lib/matplotlib/path.py Outdated
| # 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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can't this beif not transform.is_affine: ? That seems way better thatisinstance anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good catch, done.
7d67293 toffd60aaCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM 👍
lib/matplotlib/tests/test_path.py Outdated
| polygon = ax.axvspan(1, 10) | ||
| assert polygon.get_path().contains_point( | ||
| ax.transData.transform_point((5, .5)), ax.transData) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
May also be worth checking that the points (0.5, 0.5) and (20, 0.5) are not insidepolygon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
| # 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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is there a reason not to just do the transformation here even if it is an affine transformation? (thus removing theif block)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Iguess 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.
ffd60aa to76173e1CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 Will merge if/when tests pass
Uh oh!
There was an error while loading.Please reload this page.
Fixes#3540; see in particular#3540 (comment).
Edit: Alsofixes#7655.
@mdboom Would it be reasonable to remove the definition of
Transform.__array__and only keepAffine2D.__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)?