RangeSlider handle set_val bugfix #22711

andrew-fennell · 2022-03-27T00:34:37Z

PR Summary

This bug fix is in response to issue #22706.

There was an issue with the handles not updating when set_val was called on a RangeSlider object.

I updated set_val to adjust the appropriate handles when set_val is called. There are still other issues with handles in the RangeSlider class, but this pull request should address the issue highlighted.

Here is an example test that I used to reproduce the original issue:

import numpy as np
import matplotlib.pyplot as plt
from matplotlib.widgets import RangeSlider

# generate a fake image
np.random.seed(19680801)
N = 128
img = np.random.randn(N, N)

fig, axs = plt.subplots(1, 2, figsize=(10, 5))
fig.subplots_adjust(bottom=0.25)

im = axs[0].imshow(img)
axs[1].hist(img.flatten(), bins='auto')
axs[1].set_title('Histogram of pixel intensities')

# Create the RangeSlider
slider_ax = fig.add_axes([0.20, 0.1, 0.60, 0.03])
slider = RangeSlider(slider_ax, "Threshold", img.min(), img.max(),valinit=[0.0,1])

# Create the Vertical lines on the histogram
lower_limit_line = axs[1].axvline(slider.val[0], color='k')
upper_limit_line = axs[1].axvline(slider.val[1], color='k')


def update(val):
    # The val passed to a callback by the RangeSlider will
    # be a tuple of (min, max)

    # Update the image's colormap
    im.norm.vmin = val[0]
    im.norm.vmax = val[1]

    # Update the position of the vertical lines
    lower_limit_line.set_xdata([val[0], val[0]])
    upper_limit_line.set_xdata([val[1], val[1]])

    # Redraw the figure to ensure it updates
    fig.canvas.draw_idle()

slider.set_val([-1,5])
slider.on_changed(update)
plt.show()

This was the original result:

Result after my updates:

This is my first PR to a major repository. If I am misunderstanding something, please let me know.

Closes #22686

PR Checklist

Tests and Styling

Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

New features are documented, with examples if plot related.
New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
Documentation is sphinx and numpydoc compliant (the docs should build without error).

github-actions

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

ianhi

Thanks for working on this @andrew-fennell !

First thing before going further the handle moving code needs to be moved into the if else block for orientation otherwise this will break for vertical sliders.

Otherwise I think this is on the right track! Only other thing to consider if this will have any weird interactions with the active_slider mechanism

andrew-fennell · 2022-03-27T04:05:42Z

I moved the horizontal RangeSlider handle section into the if ... else ... block.

This gif shows that the active_slider functionality still works:

I went ahead and fixed the vertical slider handles as well, since it was discussed in issue #22706 as well.

Here is an example of the vertical RangeSlider handles before:

Here is the same example after this bug fix:

ianhi · 2022-03-27T13:14:27Z

lib/matplotlib/widgets.py

+            hbottom_pos = self._handles[0].get_ydata()
+            htop_pos = self._handles[1].get_ydata()
+            if hbottom_pos != val[0]:
+                hbottom_pos[0] = val[0]


are all these checks necessary or can we we just go ahead and set them?

Also I think that the more technically correct way to get the position as a scalar (which is how you're using it here)
would be with:

hbottom_pos = self._handles[0].get_ydata()[0]

because get_{x,y}data returns an array (length 1 in this case) and then you're comparing an array to scalar adn asking if theyre equal

The RangleSlider._update_val_from_pos requires an array and not just a scalar.

With that said, I am no longer doing the comparison, so I was able to remove that line completely and just use the value passed into the set_val function.

I re-tested my two examples and everything is looking great!

ianhi · 2022-03-27T13:15:09Z

This is my first PR to a major repository. If I am misunderstanding something, please let me know.

You're doing great :)

andrew-fennell · 2022-03-27T14:39:25Z

Should the codecov tests be passing? Will those checks prevent this PR from being approved?

ianhi · 2022-03-27T14:58:53Z

Should the codecov tests be passing? Will those checks prevent this PR from being approved?

My understanding is that it's not strictly required but strongly preferable. My next ask was actually going to be to have you add some tests.

Can you at the very least add checks of the handle position to this test:

matplotlib/lib/matplotlib/tests/test_widgets.py

Line 1090 in 2e921df

def test_range_slider(orientation):

better still would be to also add new tests that simulate mouse clicks which can be simulated in testing using this method:

matplotlib/lib/matplotlib/testing/widgets.py

Line 92 in 2e921df

def click_and_drag(tool, start, end, key=None):

andrew-fennell · 2022-03-27T15:41:49Z

The tests that I wrote should suffice. I just followed what was already there for checking slider values.

I don't like my usage of redundant variables, but in trying to keep everything readable (and to pass the flake8 line length restrictions), I added a few (like hpositions).

Let me if there is anything else I should to do improve this.

ianhi

👍 from me. Nice work @andrew-fennell

The only final improvement I would suggest would be to add a test of clicking and dragging that captures the switching behavior between the two handles. But I don't think this fix needs to be held up on that.

Next steps for this PR: You need two approvals from people with write access to the repo (and they may also have comments). This is a primarily volunteer project so it may take a few days. But if no one reviews after a few days feel free to drop a message to ping people.

ianhi · 2022-03-28T02:34:05Z

lib/matplotlib/tests/test_widgets.py

+        hpositions = [h.get_ydata()[0] for h in slider._handles]
+        assert_allclose(hpositions, (0, 1))
+    else:
+        hpositions = [h.get_xdata()[0] for h in slider._handles]
+        assert_allclose(hpositions, (0, 1))


If you want to simplify a bit you can do this

Suggested change

hpositions = [h.get_ydata()[0] for h in slider._handles]

assert_allclose(hpositions, (0, 1))

else:

hpositions = [h.get_xdata()[0] for h in slider._handles]

assert_allclose(hpositions, (0, 1))

hpositions = [h.get_ydata()[0] for h in slider._handles]

else:

hpositions = [h.get_xdata()[0] for h in slider._handles]

assert_allclose(hpositions, (0, 1))

Alright, I simplified the tests in the latest commit.

I wanted to add click_and_drag testing, but I think it may be a bit difficult with the RangeSlider because of how the click event is registered. I'll keep looking into it, but I'm not sure that I'll have it ready for this PR.

There are other widgets that look like they could also benefit from click_and_drag testing, so I may make another PR in the future addressing that.

I appreciate all of the help, Ian.

ah indeed I see the problem with using that. I'd suggest opening a new issue about that where we can discuss how to further improve and generalize the click_and_drag I think it will boil down to actually sending events through the canvas like I do in mpl-playback: https://github.com/ianhi/mpl-playback/blob/2c0896205b94e2c3c1ee9e8582291cc2e94aab99/mpl_playback/playback.py#L219

I wanted to add click_and_drag testing, but I think it may be a bit difficult with the RangeSlider because of how the click event is registered. I'll keep looking into it, but I'm not sure that I'll have it ready for this PR.

I opened the issue #22720 to address the issues with using mouse testing with some widgets.

timhoffm

LGTM, only some minor style suggestions.

lib/matplotlib/tests/test_widgets.py

lib/matplotlib/widgets.py

andrew-fennell · 2022-03-30T00:28:33Z

Just to reiterate here in a standalone comment:
The only lines that are being added and not covered by the added tests are the mouse interactions. That's why the the one codecov test is failing.

I opened the issue #22720 to address this because it is out of scope of this PR.

QuLogic · 2022-03-30T06:20:17Z

Thanks @andrew-fennell! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again.

…711-on-v3.5.x Backport PR #22711 on branch v3.5.x (RangeSlider handle set_val bugfix)

astromancer · 2022-11-07T10:07:43Z

This gif shows that the active_slider functionality still works:

Been working on something similar in case anyone might be interested. Code available here. Docs etc still WIP.

import numpy as np
from scrawl.image import ImageDisplay

im = ImageDisplay(np.random.randn(100, 100))

github-actions bot reviewed Mar 27, 2022

View reviewed changes

ianhi suggested changes Mar 27, 2022

View reviewed changes

andrew-fennell force-pushed the RangeSlider-bugfix branch from b14fcc4 to 4bb9274 Compare March 27, 2022 04:01

andrew-fennell requested a review from ianhi March 27, 2022 04:07

andrew-fennell force-pushed the RangeSlider-bugfix branch from 4bb9274 to 79252d5 Compare March 27, 2022 04:11

ianhi reviewed Mar 27, 2022

View reviewed changes

andrew-fennell force-pushed the RangeSlider-bugfix branch 2 times, most recently from af3639b to 3c5a6e9 Compare March 27, 2022 13:33

ianhi added the status: needs workflow approval For PRs from new contributors, from which GitHub blocks workflows by default. label Mar 27, 2022

andrew-fennell requested a review from ianhi March 27, 2022 13:39

ianhi added the topic: widgets/UI label Mar 27, 2022

RangeSlider handle set_val bugfix

2566eaa

andrew-fennell force-pushed the RangeSlider-bugfix branch from 3c5a6e9 to 2566eaa Compare March 27, 2022 13:41

nik1097 mentioned this pull request Mar 27, 2022

[Bug]: cannot give init value for RangeSlider widget #22686

Closed

andrew-fennell force-pushed the RangeSlider-bugfix branch from bbabbea to 110ee1f Compare March 27, 2022 15:37

ianhi approved these changes Mar 28, 2022

View reviewed changes

andrew-fennell force-pushed the RangeSlider-bugfix branch from 110ee1f to 787db12 Compare March 28, 2022 15:00

This was referenced Mar 28, 2022

[MNT]: Generalize widget mouse testing #22720

Open

[Bug]: RangeSlider.set_val does not move the slider (only poly and value) #22706

Closed

timhoffm reviewed Mar 29, 2022

View reviewed changes

lib/matplotlib/tests/test_widgets.py Outdated Show resolved Hide resolved

lib/matplotlib/widgets.py Outdated Show resolved Hide resolved

Added handle position checks to test_widgets.py

0c6ccf6

andrew-fennell force-pushed the RangeSlider-bugfix branch from 787db12 to 0c6ccf6 Compare March 29, 2022 22:16

timhoffm approved these changes Mar 30, 2022

View reviewed changes

QuLogic added this to the v3.5.2 milestone Mar 30, 2022

QuLogic approved these changes Mar 30, 2022

View reviewed changes

QuLogic merged commit d7a4751 into matplotlib:main Mar 30, 2022

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Mar 30, 2022

Backport PR matplotlib#22711: RangeSlider handle set_val bugfix

4dfa216

meeseeksmachine mentioned this pull request Mar 30, 2022

Backport PR #22711 on branch v3.5.x (RangeSlider handle set_val bugfix) #22729

Merged

QuLogic added a commit that referenced this pull request Mar 30, 2022

Merge pull request #22729 from meeseeksmachine/auto-backport-of-pr-22…

7f6b0a1

…711-on-v3.5.x Backport PR #22711 on branch v3.5.x (RangeSlider handle set_val bugfix)

Uh oh!

RangeSlider handle set_val bugfix #22711

RangeSlider handle set_val bugfix #22711

Uh oh!

Conversation

andrew-fennell commented Mar 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

ianhi left a comment

Choose a reason for hiding this comment

Uh oh!

andrew-fennell commented Mar 27, 2022

Uh oh!

ianhi Mar 27, 2022

Choose a reason for hiding this comment

Uh oh!

ianhi Mar 27, 2022 • 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!

andrew-fennell Mar 27, 2022 • 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!

ianhi commented Mar 27, 2022

Uh oh!

andrew-fennell commented Mar 27, 2022

Uh oh!

ianhi commented Mar 27, 2022

Uh oh!

andrew-fennell commented Mar 27, 2022

Uh oh!

ianhi left a comment

Choose a reason for hiding this comment

Uh oh!

ianhi Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

andrew-fennell Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

andrew-fennell Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

ianhi Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

andrew-fennell Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

timhoffm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

andrew-fennell commented Mar 30, 2022

Uh oh!

QuLogic commented Mar 30, 2022

Uh oh!

astromancer commented Nov 7, 2022

Uh oh!

Uh oh!

andrew-fennell commented Mar 27, 2022 •

edited

Loading

ianhi Mar 27, 2022 •

edited

Loading

andrew-fennell Mar 27, 2022 •

edited

Loading