matplotlib-devel

[matplotlib-devel] faulty design for Lasso widget?

[Benjamin R. <ben...@ou...>]

Hello all,

I tracked down an annoying problem in one of applications to the Lasso
widget I was using.  The widget constructor lets you specify a function to
call when the lasso operation is complete.  So, when I create a Lasso, I
set the canvas's widget lock to the new lasso, and the release function
will unlock it when it is done.  What would occassionally happen is that
the canvas wouldn't get unlocked and I wouldn't be able to use any other
widget tools.

It turns out that the release function is not called if the number of
vertices collected is not more than 2.  So, accidental clicks that activate
the lasso never get cleaned up.  Because of this design, it would be
impossible to guarantee a proper cleanup.  One could add another
button_release callback to clean up if the canvas is still locked, but
there is no guarantee that that callback is not called before the lasso's
callback, thereby creating a race condition.

The only solution I see is to guarantee that the release callback will be
called regardless of the length of the vertices array.  Does anybody see a
problem with that?

Cheers!
Ben Root

Re: [matplotlib-devel] faulty design for Lasso widget?

[Ryan M. <rm...@gm...>]

On Fri, Feb 17, 2012 at 10:14 AM, Benjamin Root <ben...@ou...> wrote:
> Hello all,
>
> I tracked down an annoying problem in one of applications to the Lasso
> widget I was using.  The widget constructor lets you specify a function to
> call when the lasso operation is complete.  So, when I create a Lasso, I set
> the canvas's widget lock to the new lasso, and the release function will
> unlock it when it is done.  What would occassionally happen is that the
> canvas wouldn't get unlocked and I wouldn't be able to use any other widget
> tools.
>
> It turns out that the release function is not called if the number of
> vertices collected is not more than 2.  So, accidental clicks that activate
> the lasso never get cleaned up.  Because of this design, it would be
> impossible to guarantee a proper cleanup.  One could add another
> button_release callback to clean up if the canvas is still locked, but there
> is no guarantee that that callback is not called before the lasso's
> callback, thereby creating a race condition.
>
> The only solution I see is to guarantee that the release callback will be
> called regardless of the length of the vertices array.  Does anybody see a
> problem with that?

Not having looked at the Lasso code, wouldn't it be possible to use
one internal callback for the button_release event, and have this
callback call the users' callbacks if points > 2 and always handle the
unlocking of the canvas?

Ryan

-- 
Ryan May
Graduate Research Assistant
School of Meteorology
University of Oklahoma

Re: [matplotlib-devel] faulty design for Lasso widget?

[Benjamin R. <ben...@ou...>]

On Fri, Feb 17, 2012 at 11:06 AM, Ryan May <rm...@gm...> wrote:

> On Fri, Feb 17, 2012 at 10:14 AM, Benjamin Root <ben...@ou...> wrote:
> > Hello all,
> >
> > I tracked down an annoying problem in one of applications to the Lasso
> > widget I was using.  The widget constructor lets you specify a function
> to
> > call when the lasso operation is complete.  So, when I create a Lasso, I
> set
> > the canvas's widget lock to the new lasso, and the release function will
> > unlock it when it is done.  What would occassionally happen is that the
> > canvas wouldn't get unlocked and I wouldn't be able to use any other
> widget
> > tools.
> >
> > It turns out that the release function is not called if the number of
> > vertices collected is not more than 2.  So, accidental clicks that
> activate
> > the lasso never get cleaned up.  Because of this design, it would be
> > impossible to guarantee a proper cleanup.  One could add another
> > button_release callback to clean up if the canvas is still locked, but
> there
> > is no guarantee that that callback is not called before the lasso's
> > callback, thereby creating a race condition.
> >
> > The only solution I see is to guarantee that the release callback will be
> > called regardless of the length of the vertices array.  Does anybody see
> a
> > problem with that?
>
> Not having looked at the Lasso code, wouldn't it be possible to use
> one internal callback for the button_release event, and have this
> callback call the users' callbacks if points > 2 and always handle the
> unlocking of the canvas?
>
> Ryan
>
>
The problem is that the constructor does not establish the lock.  It is the
user's responsibility to establish a lock and release the locks for these
widgets.  Plus, if the user's callback has cleanup code (such as mine did),
not guaranteeing that the callback is done can leave behind a mess.

Now, if we were to change the paradigm so that the Widget class establishes
and releases the lock, and that the user should never handle that, then
that might be a partial solution, but still leaves unsolved the user's
cleanup needs.

Ben Root

Re: [matplotlib-devel] faulty design for Lasso widget?

[Tony Yu <ts...@gm...>]

On Fri, Feb 17, 2012 at 12:17 PM, Benjamin Root <ben...@ou...> wrote:

>
>
> On Fri, Feb 17, 2012 at 11:06 AM, Ryan May <rm...@gm...> wrote:
>
>> On Fri, Feb 17, 2012 at 10:14 AM, Benjamin Root <ben...@ou...> wrote:
>> > Hello all,
>> >
>> > I tracked down an annoying problem in one of applications to the Lasso
>> > widget I was using.  The widget constructor lets you specify a function
>> to
>> > call when the lasso operation is complete.  So, when I create a Lasso,
>> I set
>> > the canvas's widget lock to the new lasso, and the release function will
>> > unlock it when it is done.  What would occassionally happen is that the
>> > canvas wouldn't get unlocked and I wouldn't be able to use any other
>> widget
>> > tools.
>> >
>> > It turns out that the release function is not called if the number of
>> > vertices collected is not more than 2.  So, accidental clicks that
>> activate
>> > the lasso never get cleaned up.  Because of this design, it would be
>> > impossible to guarantee a proper cleanup.  One could add another
>> > button_release callback to clean up if the canvas is still locked, but
>> there
>> > is no guarantee that that callback is not called before the lasso's
>> > callback, thereby creating a race condition.
>> >
>> > The only solution I see is to guarantee that the release callback will
>> be
>> > called regardless of the length of the vertices array.  Does anybody
>> see a
>> > problem with that?
>>
>> Not having looked at the Lasso code, wouldn't it be possible to use
>> one internal callback for the button_release event, and have this
>> callback call the users' callbacks if points > 2 and always handle the
>> unlocking of the canvas?
>>
>> Ryan
>>
>>
> The problem is that the constructor does not establish the lock.  It is
> the user's responsibility to establish a lock and release the locks for
> these widgets.  Plus, if the user's callback has cleanup code (such as mine
> did), not guaranteeing that the callback is done can leave behind a mess.
>
> Now, if we were to change the paradigm so that the Widget class
> establishes and releases the lock, and that the user should never handle
> that, then that might be a partial solution, but still leaves unsolved the
> user's cleanup needs.
>
> Ben Root
>
> I just started looking at the Lasso widget for some other changes, and I
agree: the `points > 2` check should be removed.

As for the design of Lasso, I don't quite understand why it disconnects the
callbacks after a single call. If the onpress event from the lasso
demo<http://matplotlib.sourceforge.net/examples/event_handling/lasso_demo.html>was
made a method of Lasso, then you could reuse the Lasso widget instead
of having to create a new one for each selection. Of course, this might
complicate the widget lock used in the demo, but that's true for all other
widgets, right?

-Tony

Re: [matplotlib-devel] faulty design for Lasso widget?

[Benjamin R. <ben...@ou...>]

On Mon, Feb 27, 2012 at 9:45 PM, Tony Yu <ts...@gm...> wrote:

>
>
> On Fri, Feb 17, 2012 at 12:17 PM, Benjamin Root <ben...@ou...> wrote:
>
>>
>>
>> On Fri, Feb 17, 2012 at 11:06 AM, Ryan May <rm...@gm...> wrote:
>>
>>> On Fri, Feb 17, 2012 at 10:14 AM, Benjamin Root <ben...@ou...> wrote:
>>> > Hello all,
>>> >
>>> > I tracked down an annoying problem in one of applications to the Lasso
>>> > widget I was using.  The widget constructor lets you specify a
>>> function to
>>> > call when the lasso operation is complete.  So, when I create a Lasso,
>>> I set
>>> > the canvas's widget lock to the new lasso, and the release function
>>> will
>>> > unlock it when it is done.  What would occassionally happen is that the
>>> > canvas wouldn't get unlocked and I wouldn't be able to use any other
>>> widget
>>> > tools.
>>> >
>>> > It turns out that the release function is not called if the number of
>>> > vertices collected is not more than 2.  So, accidental clicks that
>>> activate
>>> > the lasso never get cleaned up.  Because of this design, it would be
>>> > impossible to guarantee a proper cleanup.  One could add another
>>> > button_release callback to clean up if the canvas is still locked, but
>>> there
>>> > is no guarantee that that callback is not called before the lasso's
>>> > callback, thereby creating a race condition.
>>> >
>>> > The only solution I see is to guarantee that the release callback will
>>> be
>>> > called regardless of the length of the vertices array.  Does anybody
>>> see a
>>> > problem with that?
>>>
>>> Not having looked at the Lasso code, wouldn't it be possible to use
>>> one internal callback for the button_release event, and have this
>>> callback call the users' callbacks if points > 2 and always handle the
>>> unlocking of the canvas?
>>>
>>> Ryan
>>>
>>>
>> The problem is that the constructor does not establish the lock.  It is
>> the user's responsibility to establish a lock and release the locks for
>> these widgets.  Plus, if the user's callback has cleanup code (such as mine
>> did), not guaranteeing that the callback is done can leave behind a mess.
>>
>> Now, if we were to change the paradigm so that the Widget class
>> establishes and releases the lock, and that the user should never handle
>> that, then that might be a partial solution, but still leaves unsolved the
>> user's cleanup needs.
>>
>> Ben Root
>>
>> I just started looking at the Lasso widget for some other changes, and I
> agree: the `points > 2` check should be removed.
>
> As for the design of Lasso, I don't quite understand why it disconnects
> the callbacks after a single call. If the onpress event from the lasso
> demo<http://matplotlib.sourceforge.net/examples/event_handling/lasso_demo.html>was made a method of Lasso, then you could reuse the Lasso widget instead
> of having to create a new one for each selection. Of course, this might
> complicate the widget lock used in the demo, but that's true for all other
> widgets, right?
>
> -Tony
>
>
The Lasso disconnects itself after the button_release event because that's
what indicates that you are done.  The user gets back a single Line2D
object that is assumed to represent a single path with no breaks.  Reusing
the Lasso widget would be a situation that would require a different idiom
for user interaction.  I wouldn't be against a "MultiLasso" widget that
works differently from Lasso, but I really wouldn't want to make changes to
existing user widgets.  It is iffy enough about whether to remove the
point-count-check.

Ben Root

Re: [matplotlib-devel] faulty design for Lasso widget?

[John H. <jd...@gm...>]

On Tue, Feb 28, 2012 at 10:44 AM, Benjamin Root <ben...@ou...> wrote:

>
> The Lasso disconnects itself after the button_release event because that's
> what indicates that you are done.  The user gets back a single Line2D
> object that is assumed to represent a single path with no breaks.  Reusing
> the Lasso widget would be a situation that would require a different idiom
> for user interaction.  I wouldn't be against a "MultiLasso" widget that
> works differently from Lasso, but I really wouldn't want to make changes to
> existing user widgets.  It is iffy enough about whether to remove the
> point-count-check.
>
>
I added the point count check according to git blame and I don't remember
why but I agree is doesn't look like it makes sense to me.  I think this
lasso is lightly used based on the volume of questions we get about it.
 One reason it may be lightly used is because it is hard to work with.  If
we can improve it, even changing functionality,  I would be in favor.  Eg,
allowing the same Lasso to handle repeated selections makes sense to me.
 If the change looks too onerous, we could call it Lasso2 and deprecate the
former.

Re: [matplotlib-devel] faulty design for Lasso widget?

[Benjamin R. <ben...@ou...>]

On Tue, Feb 28, 2012 at 10:58 AM, John Hunter <jd...@gm...> wrote:

>
>
> On Tue, Feb 28, 2012 at 10:44 AM, Benjamin Root <ben...@ou...> wrote:
>
>>
>> The Lasso disconnects itself after the button_release event because
>> that's what indicates that you are done.  The user gets back a single
>> Line2D object that is assumed to represent a single path with no breaks.
>> Reusing the Lasso widget would be a situation that would require a
>> different idiom for user interaction.  I wouldn't be against a "MultiLasso"
>> widget that works differently from Lasso, but I really wouldn't want to
>> make changes to existing user widgets.  It is iffy enough about whether to
>> remove the point-count-check.
>>
>>
> I added the point count check according to git blame and I don't remember
> why but I agree is doesn't look like it makes sense to me.  I think this
> lasso is lightly used based on the volume of questions we get about it.
>  One reason it may be lightly used is because it is hard to work with.  If
> we can improve it, even changing functionality,  I would be in favor.  Eg,
> allowing the same Lasso to handle repeated selections makes sense to me.
>  If the change looks too onerous, we could call it Lasso2 and deprecate the
> former.
>

I think that logic is slightly faulty.  Another reason why we don't get a
lot of questions about it could be because it does (almost) exactly what
users want.  Currently, I use the Lasso widget in my track_maker project to
select storm cells in a movie of radar images.  Because I have multiple
modes to the program, a persistent Lasso object makes little sense. I also
want the Lasso's line to disappear after I am done selecting it so that I
can put in a polygon patch of my coloring and hatching.

I am skeptical of any redesign of Lasso.  I am willing to see what others
have in mind, but I still think that it might better belong in a new class
"MultiLasso".  I see no reason to deprecate the current Lasso (only to fix
it).

Cheers!
Ben Root

Re: [matplotlib-devel] faulty design for Lasso widget?

[Benjamin R. <ben...@ou...>]

On Tue, Feb 28, 2012 at 1:00 PM, Tony Yu <ts...@gm...> wrote:

>
>
> On Tue, Feb 28, 2012 at 11:44 AM, Benjamin Root <ben...@ou...> wrote:
>
>> On Mon, Feb 27, 2012 at 9:45 PM, Tony Yu <ts...@gm...> wrote:
>>
>>>
>>>
>>> On Fri, Feb 17, 2012 at 12:17 PM, Benjamin Root <ben...@ou...> wrote:
>>>
>>>>
>>>>
>>>> On Fri, Feb 17, 2012 at 11:06 AM, Ryan May <rm...@gm...> wrote:
>>>>
>>>>> On Fri, Feb 17, 2012 at 10:14 AM, Benjamin Root <ben...@ou...>
>>>>> wrote:
>>>>> > Hello all,
>>>>> >
>>>>> > I tracked down an annoying problem in one of applications to the
>>>>> Lasso
>>>>> > widget I was using.  The widget constructor lets you specify a
>>>>> function to
>>>>> > call when the lasso operation is complete.  So, when I create a
>>>>> Lasso, I set
>>>>> > the canvas's widget lock to the new lasso, and the release function
>>>>> will
>>>>> > unlock it when it is done.  What would occassionally happen is that
>>>>> the
>>>>> > canvas wouldn't get unlocked and I wouldn't be able to use any other
>>>>> widget
>>>>> > tools.
>>>>> >
>>>>> > It turns out that the release function is not called if the number of
>>>>> > vertices collected is not more than 2.  So, accidental clicks that
>>>>> activate
>>>>> > the lasso never get cleaned up.  Because of this design, it would be
>>>>> > impossible to guarantee a proper cleanup.  One could add another
>>>>> > button_release callback to clean up if the canvas is still locked,
>>>>> but there
>>>>> > is no guarantee that that callback is not called before the lasso's
>>>>> > callback, thereby creating a race condition.
>>>>> >
>>>>> > The only solution I see is to guarantee that the release callback
>>>>> will be
>>>>> > called regardless of the length of the vertices array.  Does anybody
>>>>> see a
>>>>> > problem with that?
>>>>>
>>>>> Not having looked at the Lasso code, wouldn't it be possible to use
>>>>> one internal callback for the button_release event, and have this
>>>>> callback call the users' callbacks if points > 2 and always handle the
>>>>> unlocking of the canvas?
>>>>>
>>>>> Ryan
>>>>>
>>>>>
>>>> The problem is that the constructor does not establish the lock.  It is
>>>> the user's responsibility to establish a lock and release the locks for
>>>> these widgets.  Plus, if the user's callback has cleanup code (such as mine
>>>> did), not guaranteeing that the callback is done can leave behind a mess.
>>>>
>>>> Now, if we were to change the paradigm so that the Widget class
>>>> establishes and releases the lock, and that the user should never handle
>>>> that, then that might be a partial solution, but still leaves unsolved the
>>>> user's cleanup needs.
>>>>
>>>> Ben Root
>>>>
>>>> I just started looking at the Lasso widget for some other changes, and
>>> I agree: the `points > 2` check should be removed.
>>>
>>> As for the design of Lasso, I don't quite understand why it disconnects
>>> the callbacks after a single call. If the onpress event from the lasso
>>> demo<http://matplotlib.sourceforge.net/examples/event_handling/lasso_demo.html>was made a method of Lasso, then you could reuse the Lasso widget instead
>>> of having to create a new one for each selection. Of course, this might
>>> complicate the widget lock used in the demo, but that's true for all other
>>> widgets, right?
>>>
>>> -Tony
>>>
>>>
>> The Lasso disconnects itself after the button_release event because
>> that's what indicates that you are done.  The user gets back a single
>> Line2D object that is assumed to represent a single path with no breaks.
>> Reusing the Lasso widget would be a situation that would require a
>> different idiom for user interaction.  I wouldn't be against a "MultiLasso"
>> widget that works differently from Lasso, but I really wouldn't want to
>> make changes to existing user widgets.  It is iffy enough about whether to
>> remove the point-count-check.
>>
>> Ben Root
>>
>>
> Wouldn't this argument suggest that `RectangleSelector` and `SpanSelector`
> should disconnect as well? I think their functionalities and their
> behaviors are (or should be) pretty similar.
>

Hadn't thought about that, and I don't use them, so I hadn't noticed.


>
> In any case, I agree that changing this behavior of `Lasso` is not
> desirable for compatibility reasons. I'll write up something that looks
> more like `RectangleSelector` and `SpanSelector`. Maybe `LassoSelector`?
>

Actually, I like that idea.  It gives a consistent naming scheme.

Ben Root

Re: [matplotlib-devel] faulty design for Lasso widget?

[Tony Yu <ts...@gm...>]

On Tue, Feb 28, 2012 at 2:04 PM, Benjamin Root <ben...@ou...> wrote:

>
>
> On Tue, Feb 28, 2012 at 1:00 PM, Tony Yu <ts...@gm...> wrote:
>
>>
>>
>> On Tue, Feb 28, 2012 at 11:44 AM, Benjamin Root <ben...@ou...> wrote:
>>
>>> On Mon, Feb 27, 2012 at 9:45 PM, Tony Yu <ts...@gm...> wrote:
>>>
>>>>
>>>>
>>>> On Fri, Feb 17, 2012 at 12:17 PM, Benjamin Root <ben...@ou...>wrote:
>>>>
>>>>>
>>>>>
>>>>> On Fri, Feb 17, 2012 at 11:06 AM, Ryan May <rm...@gm...> wrote:
>>>>>
>>>>>> On Fri, Feb 17, 2012 at 10:14 AM, Benjamin Root <ben...@ou...>
>>>>>> wrote:
>>>>>> > Hello all,
>>>>>> >
>>>>>> > I tracked down an annoying problem in one of applications to the
>>>>>> Lasso
>>>>>> > widget I was using.  The widget constructor lets you specify a
>>>>>> function to
>>>>>> > call when the lasso operation is complete.  So, when I create a
>>>>>> Lasso, I set
>>>>>> > the canvas's widget lock to the new lasso, and the release function
>>>>>> will
>>>>>> > unlock it when it is done.  What would occassionally happen is that
>>>>>> the
>>>>>> > canvas wouldn't get unlocked and I wouldn't be able to use any
>>>>>> other widget
>>>>>> > tools.
>>>>>> >
>>>>>> > It turns out that the release function is not called if the number
>>>>>> of
>>>>>> > vertices collected is not more than 2.  So, accidental clicks that
>>>>>> activate
>>>>>> > the lasso never get cleaned up.  Because of this design, it would be
>>>>>> > impossible to guarantee a proper cleanup.  One could add another
>>>>>> > button_release callback to clean up if the canvas is still locked,
>>>>>> but there
>>>>>> > is no guarantee that that callback is not called before the lasso's
>>>>>> > callback, thereby creating a race condition.
>>>>>> >
>>>>>> > The only solution I see is to guarantee that the release callback
>>>>>> will be
>>>>>> > called regardless of the length of the vertices array.  Does
>>>>>> anybody see a
>>>>>> > problem with that?
>>>>>>
>>>>>> Not having looked at the Lasso code, wouldn't it be possible to use
>>>>>> one internal callback for the button_release event, and have this
>>>>>> callback call the users' callbacks if points > 2 and always handle the
>>>>>> unlocking of the canvas?
>>>>>>
>>>>>> Ryan
>>>>>>
>>>>>>
>>>>> The problem is that the constructor does not establish the lock.  It
>>>>> is the user's responsibility to establish a lock and release the locks for
>>>>> these widgets.  Plus, if the user's callback has cleanup code (such as mine
>>>>> did), not guaranteeing that the callback is done can leave behind a mess.
>>>>>
>>>>> Now, if we were to change the paradigm so that the Widget class
>>>>> establishes and releases the lock, and that the user should never handle
>>>>> that, then that might be a partial solution, but still leaves unsolved the
>>>>> user's cleanup needs.
>>>>>
>>>>> Ben Root
>>>>>
>>>>> I just started looking at the Lasso widget for some other changes, and
>>>> I agree: the `points > 2` check should be removed.
>>>>
>>>> As for the design of Lasso, I don't quite understand why it disconnects
>>>> the callbacks after a single call. If the onpress event from the lasso
>>>> demo<http://matplotlib.sourceforge.net/examples/event_handling/lasso_demo.html>was made a method of Lasso, then you could reuse the Lasso widget instead
>>>> of having to create a new one for each selection. Of course, this might
>>>> complicate the widget lock used in the demo, but that's true for all other
>>>> widgets, right?
>>>>
>>>> -Tony
>>>>
>>>>
>>> The Lasso disconnects itself after the button_release event because
>>> that's what indicates that you are done.  The user gets back a single
>>> Line2D object that is assumed to represent a single path with no breaks.
>>> Reusing the Lasso widget would be a situation that would require a
>>> different idiom for user interaction.  I wouldn't be against a "MultiLasso"
>>> widget that works differently from Lasso, but I really wouldn't want to
>>> make changes to existing user widgets.  It is iffy enough about whether to
>>> remove the point-count-check.
>>>
>>> Ben Root
>>>
>>>
>> Wouldn't this argument suggest that `RectangleSelector` and
>> `SpanSelector` should disconnect as well? I think their functionalities and
>> their behaviors are (or should be) pretty similar.
>>
>
> Hadn't thought about that, and I don't use them, so I hadn't noticed.
>
>
>>
>> In any case, I agree that changing this behavior of `Lasso` is not
>> desirable for compatibility reasons. I'll write up something that looks
>> more like `RectangleSelector` and `SpanSelector`. Maybe `LassoSelector`?
>>
>
> Actually, I like that idea.  It gives a consistent naming scheme.
>
> Ben Root
>
>
I just added a PR for
LassoSelector<https://github.com/matplotlib/matplotlib/pull/730>
.

As I mention in the PR notes, only the last commit adds the LassoSelector
widget<https://github.com/tonysyu/matplotlib/commit/8345d4c6afcd8afa9d646d554440de27fcc7d49c>.
The rest of the commits in the PR comes from branching off PR
724<https://github.com/matplotlib/matplotlib/pull/724>
.

Ben: I don't think you asked for justification of this functionality, but
as I was testing out this code, I decided it would be fun to demonstrate my
use case.

My example use case:
======================
I have some code to track particles flowing near a wall, but sometimes the
detected trajectories are wrong. One way to verify the trajectories is to
plot the velocity components (horizontal and vertical), and valid
velocities should cluster together (plot on left in linked images).
Nevertheless, the picture this presents isn't definitive, so it'd be nice
to pick data with specific values of velocity components and then display
the particle trajectories (plot on right in linked images).

Here are images from an interactive data explorer that uses LassoSelector:
http://imgur.com/a/OnGPo

-Tony