Skip to content

[MNT]: Deprecate onselect/onmove_callback of selectors #21929

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
ericpre opened this issue Dec 11, 2021 · 5 comments · Fixed by #26000
Closed

[MNT]: Deprecate onselect/onmove_callback of selectors #21929

ericpre opened this issue Dec 11, 2021 · 5 comments · Fixed by #26000
Labels
Maintenance topic: widgets/UI

Comments

@ericpre
Copy link
Member

ericpre commented Dec 11, 2021

Summary

If I remember correctly, at some point, @anntzer suggested to deprecate onselect argument for selectors. Now, that I understand better what he meant at the time, I think it makes a lot of sense, because the behaviour of the "builtin" selector callback is contentious and inconsistent between selectors.

Proposed fix

The alternative would be something along the lines of:

import matplotlib.pyplot as plt
from matplotlib.widgets import RectangleSelector
import functools

_, ax = plt.subplots()
ax.plot([1, 2, 3])


def dummy_callback(*args):
    pass


def onselect_callback(event, selector):
    print('## onselect callback ##')
    if not selector.ignore(event):
        print('   ', selector.extents)
        return
    print('   event ignored')


def onmove_callback(event, selector):
    print('## onmove callback ##')
    if not selector.ignore(event) and selector._eventpress:
        print('   ', selector.extents)
        return
    print('   event ignored')


tool = RectangleSelector(ax, dummy_callback, interactive=True,
                         drag_from_anywhere=True,)
tool.extents = (1.2, 2.0, 1.0, 1.6)

# alternative to onselect
rectangle_onselect_callback = functools.partial(onselect_callback, selector=tool)
tool.connect_event('button_release_event', rectangle_onselect_callback)

# alternative to onmove_callback (SpanSelector only)
rectangle_onmove_callback = functools.partial(onmove_callback, selector=tool)
tool.connect_event('motion_notify_event', rectangle_onmove_callback)

In my opinion, the reasons to deprecate are:

Proposed fix:

  • Deprecate the onselect/onmove arguments of the selectors, as per example above
  • improve the API to ignore events (the example above uses a private API to reproduce the onmove_callback of the SpanSelector)
  • update example, improve documentation on the use of callback registry.
@ericpre
Copy link
Member Author

ericpre commented Dec 11, 2021

cc @dstansby, @dhomeier

@dstansby
Copy link
Member

I think at least making the onselect argument a keyword argument, and defaulting to a no-op functions would be great, since it's often common to just want a selector without attaching a callback to it.

As for deprecating, I'm not too sure what the motivation is?

@anntzer
Copy link
Contributor

anntzer commented Dec 12, 2021

I think you may be referring to #20603, specifically #20603 (comment)? There, the point was to deprecate the on* methods, not the kwargs (which I think are fine, although defaulting to noop seems good too?).

@ericpre
Copy link
Member Author

ericpre commented Dec 12, 2021

I think you may be referring to #20603, specifically #20603 (comment)? There, the point was to deprecate the on* methods, not the kwargs (which I think are fine, although defaulting to noop seems good too?).

Yes, this is what I was referring to and indeed this is different discussion - sorry, I should have checked it up before mentioning it here!

As for deprecating, I'm not too sure what the motivation is?

I have list some reasons above:

In my opinion, the reasons to deprecate are:

To clarify the inconsistency with signature of the arguments (onselect and onmove_callback), here are the correspoding docstrings:

SpanSelector:

    onselect : callable
        A callback function that is called after a release event and the
        selection is created, changed or removed.
        It must have the signature::

            def on_select(min: float, max: float) -> Any

    onmove_callback : func(min, max), min/max are floats, default: None
        Called on mouse move while the span is being selected.

RectangleSelector:

    onselect : function
        A callback function that is called after a release event and the
        selection is created, changed or removed.
        It must have the signature::

            def onselect(eclick: MouseEvent, erelease: MouseEvent)

        where *eclick* and *erelease* are the mouse click and release
        `.MouseEvent`\s that start and complete the selection.

LassoSelector:

    onselect : function
        Whenever the lasso is released, the *onselect* function is called and
        passed the vertices of the selected path.

PolygonSelector:

    onselect : function
        When a polygon is completed or modified after completion,
        the *onselect* function is called and passed a list of the vertices as
        ``(xdata, ydata)`` tuples.

@thiagoluisbecker thiagoluisbecker mentioned this issue May 29, 2023
Merged
2 tasks
@QuLogic
Copy link
Member

QuLogic commented Sep 20, 2024

#26000 only made these optional; do we want to do the deprecation as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Maintenance topic: widgets/UI
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

making onselect a keyword argument on selectors thiagoluisbecker/matplotlib
4 participants
@QuLogic @anntzer @dstansby @ericpre