Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

RangeSlider handle set_val bugfix#22711

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

Merged
QuLogic merged 2 commits intomatplotlib:mainfromandrew-fennell:RangeSlider-bugfix
Mar 30, 2022

Conversation

andrew-fennell
Copy link
Contributor

@andrew-fennellandrew-fennell commentedMar 27, 2022•
edited
Loading

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 npimport matplotlib.pyplot as pltfrom matplotlib.widgets import RangeSlider# generate a fake imagenp.random.seed(19680801)N = 128img = 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 RangeSliderslider_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 histogramlower_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:
original graph

Result after my updates:
image

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 (andpytest passes).
  • IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

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

Copy link

@github-actionsgithub-actionsbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 uson gitter for real-time discussion.

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

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

Copy link
Contributor

@ianhiianhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks for working on this@andrew-fennell !

First thing before going further the handle moving code needs to be moved into theif 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 theactive_slider mechanism

@andrew-fennell
Copy link
ContributorAuthor

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

This gif shows that theactive_slider functionality still works:
RangeSlider_Horizontal

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:
the issue with vertical RangeSliders prior to the latest commit

Here is the same example after this bug fix:
RangeSlider_Vertical

Comment on lines 918 to 921
hbottom_pos = self._handles[0].get_ydata()
htop_pos = self._handles[1].get_ydata()
if hbottom_pos != val[0]:
hbottom_pos[0] = val[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Contributor

@ianhiianhiMar 27, 2022•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

AlsoI 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]

becauseget_{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

Copy link
ContributorAuthor

@andrew-fennellandrew-fennellMar 27, 2022•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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
Copy link
Contributor

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

You're doing great :)

andrew-fennell reacted with laugh emoji

@andrew-fennellandrew-fennellforce-pushed theRangeSlider-bugfix branch 2 times, most recently fromaf3639b to3c5a6e9CompareMarch 27, 2022 13:33
@ianhiianhi added the status: needs workflow approvalFor PRs from new contributors, from which GitHub blocks workflows by default. labelMar 27, 2022
@andrew-fennell
Copy link
ContributorAuthor

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

@ianhi
Copy link
Contributor

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:

deftest_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:

defclick_and_drag(tool,start,end,key=None):

@andrew-fennell
Copy link
ContributorAuthor

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 (likehpositions).

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

Copy link
Contributor

@ianhiianhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

👍 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.

Comment on lines 1131 to 1135
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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

If you want to simplify a bit you can do this

Suggested change
hpositions= [h.get_ydata()[0]forhinslider._handles]
assert_allclose(hpositions, (0,1))
else:
hpositions= [h.get_xdata()[0]forhinslider._handles]
assert_allclose(hpositions, (0,1))
hpositions= [h.get_ydata()[0]forhinslider._handles]
else:
hpositions= [h.get_xdata()[0]forhinslider._handles]
assert_allclose(hpositions, (0,1))

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Alright, I simplified the tests in the latest commit.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 theclick_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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM, only some minor style suggestions.

@andrew-fennell
Copy link
ContributorAuthor

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.

timhoffm reacted with thumbs up emoji

@QuLogicQuLogic added this to thev3.5.2 milestoneMar 30, 2022
@QuLogicQuLogic merged commitd7a4751 intomatplotlib:mainMar 30, 2022
@QuLogic
Copy link
Member

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

andrew-fennell reacted with hooray emoji

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestMar 30, 2022
QuLogic added a commit that referenced this pull requestMar 30, 2022
…711-on-v3.5.xBackport PR#22711 on branch v3.5.x (RangeSlider handle set_val bugfix)
@astromancer
Copy link
Contributor

This gif shows that theactive_slider functionality still works:RangeSlider_Horizontal

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

importnumpyasnpfromscrawl.imageimportImageDisplayim=ImageDisplay(np.random.randn(100,100))

tacaswell reacted with rocket emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ianhiianhiianhi approved these changes

@github-actionsgithub-actions[bot]github-actions[bot] left review comments

@QuLogicQuLogicQuLogic approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Labels
status: needs workflow approvalFor PRs from new contributors, from which GitHub blocks workflows by default.topic: widgets/UI
Projects
None yet
Milestone
v3.5.2
Development

Successfully merging this pull request may close these issues.

[Bug]: cannot give init value for RangeSlider widget
5 participants
@andrew-fennell@ianhi@QuLogic@astromancer@timhoffm
[8]ページ先頭
©2009-2025 Movatter.jp