Make AxesImage.contains account for transforms #12052
Conversation
Corrects contains for AxesImages with transforms applied See. matplotlib#12042
lib/matplotlib/image.py
Outdated
| bbox = Bbox(np.array([[xmin, ymin], [xmax, ymax]])) | ||
| transformed_bbox = TransformedBbox(bbox, trans) | ||
|
|
||
| inside = transformed_bbox.contains(mouseevent.x,mouseevent.y) |
There was a problem hiding this comment.
Thanks a lot @ilopata1! No criticism to you because I think you talked this through w/ @tacaswell, but wrt #12031 (excessive transforms being created) I think this is the kind of code that'll do that. I think this transformed_bbox should be saved so that a new bbox is not created on every call to contains.
There was a problem hiding this comment.
@jklymak Sounds very reasonable. Understanding the class well enough to find places at which the extents and transforms are modified is more than I have the capacity to take on at present, so regretfully, much as I would like to help further I am going to have to leave this hanging out there and hope that others might have the time and interest to carry to completion.
There was a problem hiding this comment.
@ilopata1 This solution may be the best we can do for now. But it was a bit of an a-ha moment for why so many transforms get created even if a plot is not changing.
Does contains get called all the time, or just after a mouse click? If just after a mouse click, its probably not at all a performance problem for this change to be put in.
There was a problem hiding this comment.
I can't speak to when/whether it may get called internally, but from an application perspective it is most likely to get called on mouse events. This may include mouse moves, which can be generated in high volume.
I just ran a test and on an Intel i5 processor, over 18 executions of this code fragment the average execution time was 0.00022 seconds. To put that another way, contains would need to be called 4,500 times on my machine for the execution time to be one second.
There was a problem hiding this comment.
This doesn't actually work because TransformedBbox.contains is basically broken for non-axis-aligned rectangles (it inherits contains from Bbox, which checks x and y separately).
This can be seen by using a 45° rotation instead -- with this PR, the whole axis-aligned rectangle that contains the transformed AxesImage will be considered as "contained".
I think(?) you could apply the inverse transform to the event coordinates and check whether that's in the untransformed bbox? That'll work around the issue above, and avoid the cost of creating a TransformedBbox.
|
@anntzer Good catch. I have tested your hypothesis and using the reverse transform on the event cordinates works. Being a noob, I don't know if I am supposed to update this PR or create a new one. Can you give me a pointer? |
|
Go ahead and update this one since we know the current approach isn't quite
right. The only time I would open a new PR is in situations where we want
to evaluate and compare multiple approaches at the same time.
…On Fri, Sep 7, 2018 at 10:54 AM ilopata1 ***@***.***> wrote:
@anntzer <https://github.com/anntzer> Good catch. I have tested your
hypothesis and using the reverse transform on the event cordinates works.
Being a noob, I don't know if I am supposed to update this PR or create a
new one. Can you give me a pointer?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#12052 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARy-JB1TBhTSqJZvqPxH6EG0eUY5-9Fks5uYoiegaJpZM4Wd2kH>
.
|
Changed approach to apply inverse transform to event coordinates. This a) addresses an issue with TransformedBbox only working with coordinate aligned axes; and b) avoids creating a new transform.
|
When editing on GitHub, please try to give your commits descriptive messages. To whomever merges, please squash-merge and update the commit message. |
|
Someone give me a pointer on what I need to do, if anything, to resolve the codecov failure? Thanks |
|
We can prob let the codecov slide, but I think you can fake a mouse event and add a test for this. Ideally you could grep for mouse event in the test subdirectory, and then make a test. |
|
@ilopata1 where is this one at? Are you done? Be sure to ping us if you need a review.... |
|
@jklymak I spent a long time trying to write a test case that would validate the code but had much more trouble writing a standalone test that worked than I did preparing the original patch. I can say that the patch fixes my case in my application, but I am hesitant to recommend the patch without being able to confirm it more authoritatively. So to answer your question more directly, I am stalled. |
|
Thanks a lot for the contribution. I'm going to close this as having had no action for quite a while. OTOH, if there is still a community that wants this, please feel free to re-open, with my apologies! |
Corrects contains() method for AxesImages with transforms applied See. #12042
PR Summary
As described in #12042
Proposed change creates a bounding box from the image extents, applies the current transform to the bounding box and then tests whether the mouse event occurred within the transformed bounding box.